fix old bugs/wrong behavior in applyIfNeedsToApply & related logic

kiprasmel · kiprasmel · commit 9dc430c7917d · 2023-04-04T13:31:21.000+03:00
previously, in `gitStackedRebase`, there were 3 invocations of
`applyIfNeedsToApply`. there are still 3, but what differs is:

previously, the 3rd would be invoked if and only if the
`config.autoApplyIfNeeded` was true, i.e., it was NOT mandatory.

however, the 1st invocation, which also is NOT mandatory, did not have
such guardrail, and, if the `config.autoApplyIfNeeded` was false, would
prompt the user with the common message:
`need to --apply before continuing. proceed? [Y/n/(a)lways] `

this is wrong, because this (1st) invocation is not mandatory, meanwhile
the message makes it look like it is, thus confusing the user &amp;
their longer term understanding of when an --apply is actually needed.

to further add to the confusion, the 1st invocation worked correctly
apart from the message - it (1st invoc) was not made mandatory, i.e.
would not stop the program execution if user did not allow.

---

now, there's a clear distinction of mandatory vs not,
and `applyIfNeedsToApply` works accordingly to it.
related parts were also cleaned up.

also, created a test for this -- yay!

Signed-off-by: Kipras Melnikovas &lt;kipras@kipras.org&gt;
diff --git a/apply.ts b/apply.ts
@@ -6,6 +6,7 @@ import { combineRewrittenLists } from "./git-reconcile-rewritten-list/combineRew
 
 import { AskQuestion, question, Questions } from "./util/createQuestion";
 import { isDirEmptySync } from "./util/fs";
+import { Termination } from "./util/error";
 
 import { filenames } from "./filenames";
 import { configKeys } from "./config";
@@ -63,59 +64,81 @@ const defaultApplyAction: ActionInsideEachCheckedOutBranch = async ({
 export const getBackupPathOfPreviousStackedRebase = (pathToStackedRebaseDirInsideDotGit: string): string =>
 	pathToStackedRebaseDirInsideDotGit + ".previous";
 
-export type ReturnOfApplyIfNeedsToApply =
-	| {
-			neededToApply: false;
-			userAllowedToApplyAndWeApplied?: never;
-	  }
-	| {
-			neededToApply: true;
-			userAllowedToApplyAndWeApplied: false;
-	  }
-	| {
-			neededToApply: true;
-			userAllowedToApplyAndWeApplied: true;
-	  };
 export async function applyIfNeedsToApply({
 	repo,
 	pathToStackedRebaseTodoFile,
 	pathToStackedRebaseDirInsideDotGit, //
 	autoApplyIfNeeded,
+	isMandatoryIfMarkedAsNeeded,
 	config,
 	askQuestion = question,
 	...rest
 }: BranchSequencerArgsBase & {
+	/**
+	 * i.e., sometimes a) we need the `--apply` to go thru,
+	 * and sometimes b) it's "resumable" on the next run,
+	 * i.e. we'd prefer to apply right now,
+	 * but it's fine if user does not apply now,
+	 * because `--apply` is resumable[1],
+	 * and on the next run of stacked rebase, user will be forced to apply anyway.
+	 *
+	 * [1] resumable, unless user runs into some edge case where it no longer is.
+	 * TODO: work out when this happens & handle better.
+	 */
+	isMandatoryIfMarkedAsNeeded: boolean;
+
 	autoApplyIfNeeded: boolean; //
 	config: Git.Config;
 	askQuestion: AskQuestion;
-}): Promise<ReturnOfApplyIfNeedsToApply> {
+}): Promise<void> {
 	const needsToApply: boolean = doesNeedToApply(pathToStackedRebaseDirInsideDotGit);
-
 	if (!needsToApply) {
-		return {
-			neededToApply: false,
-		};
+		return;
 	}
 
-	const allowedToApply = autoApplyIfNeeded || (await askIfCanApply(config, askQuestion));
-	if (!allowedToApply) {
-		return {
-			neededToApply: true,
-			userAllowedToApplyAndWeApplied: false,
-		};
-	}
+	if (isMandatoryIfMarkedAsNeeded) {
+		/**
+		 * is marked as needed to apply,
+		 * and is mandatory -- try to get a confirmation that it is ok to apply.
+		 */
+		const userAllowedToApply = autoApplyIfNeeded || (await askIfCanApply(config, askQuestion));
+
+		if (!userAllowedToApply) {
+			const msg = "\ncannot continue without mandatory --apply. Exiting.\n";
+			throw new Termination(msg);
+		} else {
+			await apply({
+				repo,
+				pathToStackedRebaseTodoFile,
+				pathToStackedRebaseDirInsideDotGit, //
+				...rest,
+			});
+
+			return;
+		}
+	} else {
+		/**
+		 * is marked as needed to apply,
+		 * but performing the apply is NOT mandatory.
+		 *
+		 * thus, do NOT ask user if should apply -- only infer from config.
+		 */
 
-	await apply({
-		repo,
-		pathToStackedRebaseTodoFile,
-		pathToStackedRebaseDirInsideDotGit, //
-		...rest,
-	});
+		if (autoApplyIfNeeded) {
+			await apply({
+				repo,
+				pathToStackedRebaseTodoFile,
+				pathToStackedRebaseDirInsideDotGit, //
+				...rest,
+			});
 
-	return {
-		neededToApply: true,
-		userAllowedToApplyAndWeApplied: true, //
-	};
+			return;
+		} else {
+			/**
+			 * not mandatory, thus do nothing.
+			 */
+		}
+	}
 }
 
 const askIfCanApply = async (config: Git.Config, askQuestion: AskQuestion = question): Promise<boolean> => {
@@ -159,6 +182,11 @@ export const markThatNeedsToApply = (
 		)
 	)[0];
 
+export const isMarkedThatNeedsToApply = (pathToStackedRebaseDirInsideDotGit: string): boolean => {
+	const pathToMark: string = getPaths(pathToStackedRebaseDirInsideDotGit).needsToApplyPath;
+	return fs.existsSync(pathToMark);
+};
+
 export const markThatApplied = (pathToStackedRebaseDirInsideDotGit: string): void =>
 	[getPaths(pathToStackedRebaseDirInsideDotGit)].map(
 		({ rewrittenListPath, needsToApplyPath, gitRebaseTodoPath }) => (
diff --git a/git-stacked-rebase.ts b/git-stacked-rebase.ts
@@ -122,6 +122,7 @@ export async function gitStackedRebase(
 			 * so that the partial branches do not get out of sync.
 			 */
 			await applyIfNeedsToApply({
+				isMandatoryIfMarkedAsNeeded: false,
 				repo,
 				pathToStackedRebaseTodoFile,
 				pathToStackedRebaseDirInsideDotGit, //
@@ -137,7 +138,17 @@ export async function gitStackedRebase(
 			return;
 		}
 
-		const { neededToApply, userAllowedToApplyAndWeApplied } = await applyIfNeedsToApply({
+		await applyIfNeedsToApply({
+			/**
+			 * at this point, if an `--apply` has been marked as needed, we must perform it.
+			 * 
+			 * we either a) already know that the user allows it, via options.autoApplyIfNeeded,
+			 * or b) if not -- we must ask the user directly if the apply can be performed.
+			 * 
+			 * if user does not allow us to perform the apply -- we cannot continue, and will terminate.
+			 */
+			isMandatoryIfMarkedAsNeeded: true,
+
 			repo,
 			pathToStackedRebaseTodoFile,
 			pathToStackedRebaseDirInsideDotGit, //
@@ -150,10 +161,6 @@ export async function gitStackedRebase(
 			askQuestion,
 		});
 
-		if (neededToApply && !userAllowedToApplyAndWeApplied) {
-			return;
-		}
-
 		if (options.push) {
 			if (!options.forcePush) {
 				throw new Termination("\npush without --force will fail (since git rebase overrides history).\n\n");
@@ -650,66 +657,59 @@ mv -f "${preparedRegularRebaseTodoFile}" "${pathToRegularRebaseTodoFile}"
 		}
 
 		/**
-		 * TODO might need to always enable,
-		 * but before more testing,
-		 * optional is good, since we ask anyway
-		 * before proceeding w/ other commands way above.
+		 * since we're invoking `git rebase --continue` directly (above),
+		 * we do not have the control over it.
+		 *
+		 * meaning that in case it's the very first rebase,
+		 * the `rewritten-list` in `.git/rebase-merge/`
+		 * (the actual git-rebase, not ours)
+		 * file is not generated yet,
+		 *
+		 * and since we depend on the `git rebase --continue` (the regular rebase)
+		 * to generate the `rewritten-list` file,
+		 * we explode trying to read the file if we try to --apply below.
+		 *
+		 * ---
+		 *
+		 * edit: oh wait nvm, it's potentially any rebase that has
+		 * `break` or `edit` or similar right??
+		 *
+		 * because if the git-rebase-todo file has `break` or `edit`
+		 * or similar commands that make `git rebase --continue` exit
+		 * before it's fully completed, (my theory now is that) our code here proceeds
+		 * and tries to --apply, but again the rewritten-list file
+		 * doesn't exist yet, so we blow up.
+		 *
+		 * ---
+		 *
+		 * let's try to account for only the 1st scenario first.
+		 * TODO implement directly in `--apply`
+		 * (e.g. if user calls `gitStackedRebase` again, while still in a rebase)
+		 *
+		 * upd: ok let's also do the 2nd one because it's useless otherwise
+		 *
 		 */
-		if (options.autoApplyIfNeeded) {
-			/**
-			 * since we're invoking `git rebase --continue` directly (above),
-			 * we do not have the control over it.
-			 *
-			 * meaning that in case it's the very first rebase,
-			 * the `rewritten-list` in `.git/rebase-merge/`
-			 * (the actual git-rebase, not ours)
-			 * file is not generated yet,
-			 *
-			 * and since we depend on the `git rebase --continue` (the regular rebase)
-			 * to generate the `rewritten-list` file,
-			 * we explode trying to read the file if we try to --apply below.
-			 *
-			 * ---
-			 *
-			 * edit: oh wait nvm, it's potentially any rebase that has
-			 * `break` or `edit` or similar right??
-			 *
-			 * because if the git-rebase-todo file has `break` or `edit`
-			 * or similar commands that make `git rebase --continue` exit
-			 * before it's fully completed, (my theory now is that) our code here proceeds
-			 * and tries to --apply, but again the rewritten-list file
-			 * doesn't exist yet, so we blow up.
-			 *
-			 * ---
-			 *
-			 * let's try to account for only the 1st scenario first.
-			 * TODO implement directly in `--apply`
-			 * (e.g. if user calls `gitStackedRebase` again, while still in a rebase)
-			 *
-			 * upd: ok let's also do the 2nd one because it's useless otherwise
-			 *
-			 */
-			const canAndShouldBeApplying: boolean =
-				/** part 1 */ fs.existsSync(path.join(pathToStackedRebaseDirInsideDotGit, filenames.rewrittenList)) &&
-				/** part 2 (incomplete?) */ !fs.existsSync(pathToRegularRebaseDirInsideDotGit) &&
-				/** part 2 (complete?) (is this even needed?) */ goodCommands.every(
-					(cmd) => !namesOfRebaseCommandsThatMakeRebaseExitToPause.includes(cmd.commandName)
-				);
+		const canAndShouldBeApplying: boolean =
+			/** part 1 */ fs.existsSync(path.join(pathToStackedRebaseDirInsideDotGit, filenames.rewrittenList)) &&
+			/** part 2 (incomplete?) */ !fs.existsSync(pathToRegularRebaseDirInsideDotGit) &&
+			/** part 2 (complete?) (is this even needed?) */ goodCommands.every(
+				(cmd) => !namesOfRebaseCommandsThatMakeRebaseExitToPause.includes(cmd.commandName)
+			);
 
-			if (canAndShouldBeApplying) {
-				await applyIfNeedsToApply({
-					repo,
-					pathToStackedRebaseTodoFile,
-					pathToStackedRebaseDirInsideDotGit, //
-					rootLevelCommandName: "--apply",
-					gitCmd: options.gitCmd,
-					autoApplyIfNeeded: options.autoApplyIfNeeded,
-					config,
-					initialBranch,
-					currentBranch,
-					askQuestion,
-				});
-			}
+		if (canAndShouldBeApplying) {
+			await applyIfNeedsToApply({
+				isMandatoryIfMarkedAsNeeded: false,
+				repo,
+				pathToStackedRebaseTodoFile,
+				pathToStackedRebaseDirInsideDotGit, //
+				rootLevelCommandName: "--apply",
+				gitCmd: options.gitCmd,
+				autoApplyIfNeeded: options.autoApplyIfNeeded,
+				config,
+				initialBranch,
+				currentBranch,
+				askQuestion,
+			});
 		}
 
 		/**
diff --git a/internal.ts b/internal.ts
@@ -7,6 +7,10 @@ import { AskQuestion } from "./util/createQuestion";
 export const editor__internal = Symbol("editor__internal");
 export const getGitConfig__internal = Symbol("getGitConfig__internal");
 
+export const noEditor = {
+	[editor__internal]: () => void 0,
+};
+
 export const askQuestion__internal = Symbol("askQuestion__internal");
 
 /**
diff --git a/test/apply.spec.ts b/test/apply.spec.ts
@@ -0,0 +1,87 @@
+import assert from "assert";
+import path from "path";
+
+import Git from "nodegit";
+
+import { configKeys } from "../config";
+import { gitStackedRebase } from "../git-stacked-rebase";
+import { humanOpChangeCommandOfNthCommitInto } from "../humanOp";
+import { askQuestion__internal, editor__internal, noEditor } from "../internal";
+
+import { setupRepo } from "./util/setupRepo";
+import { question, Questions } from "../util/createQuestion";
+import { isMarkedThatNeedsToApply } from "../apply";
+
+export async function applyTC() {
+	await integration__git_stacked_rebase_exits_if_apply_was_needed_but_user_disallowed();
+}
+
+/**
+ * create a scenario where an apply is needed, and disallow it - GSR should exit.
+ */
+async function integration__git_stacked_rebase_exits_if_apply_was_needed_but_user_disallowed() {
+	const { initialBranch, common, commitsInLatest, config, repo } = await setupRepo();
+
+	/**
+	 * ensure autoApplyIfNeeded is disabled
+	 */
+	config.setBool(configKeys.autoApplyIfNeeded, Git.Config.MAP.FALSE);
+
+	/**
+	 * force modify history, so that an apply will be needed
+	 */
+	await gitStackedRebase(initialBranch, {
+		...common,
+		[editor__internal]: ({ filePath }) => {
+			humanOpChangeCommandOfNthCommitInto("drop", {
+				filePath, //
+				commitSHA: commitsInLatest[2],
+			});
+		},
+	});
+
+	// TODO take from `gitStackedRebase`:
+	const dotGitDirPath: string = repo.path();
+	const pathToStackedRebaseDirInsideDotGit: string = path.join(dotGitDirPath, "stacked-rebase");
+	assert.deepStrictEqual(
+		isMarkedThatNeedsToApply(pathToStackedRebaseDirInsideDotGit), //
+		true,
+		`expected a "needs-to-apply" mark to be set.`
+	);
+
+	console.log("performing 2nd rebase, expecting it to throw.");
+
+	const threw: boolean = await didThrow(() =>
+		/**
+		 * perform the rebase again - now that an apply is marked as needed,
+		 * and autoApplyIfNeeded is disabled,
+		 * we should get prompted to allow the apply.
+		 */
+		gitStackedRebase(initialBranch, {
+			...common,
+			...noEditor,
+			[askQuestion__internal]: (q, ...rest) => {
+				if (q === Questions.need_to_apply_before_continuing) {
+					return "n";
+				}
+
+				return question(q, ...rest);
+			},
+		})
+	);
+
+	assert.deepStrictEqual(
+		threw,
+		true,
+		`expected 2nd invocation of rebase to throw, because user did not allow to perform a mandatory --apply.\nbut threw = ${threw} (expected true).`
+	);
+}
+
+export async function didThrow(fn: Function): Promise<boolean> {
+	try {
+		await fn();
+		return false;
+	} catch (_e) {
+		return true;
+	}
+}
diff --git a/test/run.ts b/test/run.ts
