fix old bugs/wrong behavior in applyIfNeedsToApply & related logic

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 &
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 <[email protected]>

apply.ts

@@ -6,6 +6,7 @@ import { combineRewrittenLists } from "./git-reconcile-rewritten-list/combineRew
 
 import { question } from "./util/createQuestion";
 import { isDirEmptySync } from "./util/fs";
+import { Termination } from "./util/error";
 
 import { filenames } from "./filenames";
 import { configKeys } from "./config";
@@ -63,57 +64,79 @@ 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,
 	...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;
-}): Promise<ReturnOfApplyIfNeedsToApply> {
+}): Promise<void> {
 	const needsToApply: boolean = doesNeedToApply(pathToStackedRebaseDirInsideDotGit);
-
 	if (!needsToApply) {
-		return {
-			neededToApply: false,
-		};
+		return;
 	}
 
-	const allowedToApply = autoApplyIfNeeded || (await askIfCanApply(config));
-	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));
+
+		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): Promise<boolean> => {

git-stacked-rebase.ts

@@ -119,6 +119,7 @@ export async function gitStackedRebase(
 			 * so that the partial branches do not get out of sync.
 			 */
 			await applyIfNeedsToApply({
+				isMandatoryIfMarkedAsNeeded: false,
 				repo,
 				pathToStackedRebaseTodoFile,
 				pathToStackedRebaseDirInsideDotGit, //
@@ -133,7 +134,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, //
@@ -145,10 +156,6 @@ export async function gitStackedRebase(
 			currentBranch,
 		});
 
-		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");
@@ -645,65 +652,58 @@ 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,
-				});
-			}
+		if (canAndShouldBeApplying) {
+			await applyIfNeedsToApply({
+				isMandatoryIfMarkedAsNeeded: false,
+				repo,
+				pathToStackedRebaseTodoFile,
+				pathToStackedRebaseDirInsideDotGit, //
+				rootLevelCommandName: "--apply",
+				gitCmd: options.gitCmd,
+				autoApplyIfNeeded: options.autoApplyIfNeeded,
+				config,
+				initialBranch,
+				currentBranch,
+			});
 		}
 
 		/**

internal.ts

@@ -5,6 +5,10 @@ import Git from "nodegit";
 export const editor__internal = Symbol("editor__internal");
 export const getGitConfig__internal = Symbol("getGitConfig__internal");
 
+export const noEditor = {
+	[editor__internal]: () => void 0,
+};
+
 /**
  * meant to NOT be exported to the end user of the library
  */

test/apply.spec.ts

@@ -0,0 +1,73 @@
+import assert from "assert";
+
+import Git from "nodegit";
+
+import { configKeys } from "../config";
+import { gitStackedRebase } from "../git-stacked-rebase";
+import { humanOpChangeCommandOfNthCommitInto } from "../humanOp";
+import { editor__internal, noEditor } from "../internal";
+
+import { setupRepo } from "./util/setupRepo";
+
+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 } = 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: assert that the "needs to apply" mark is 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,
+			/**
+			 * TODO: allow handling question prompts, and respond with `n` to disallow.
+			 */
+		})
+	);
+
+	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;
+	}
+}

test/run.ts

@@ -4,6 +4,7 @@ import { testCase } from "./experiment.spec";
 import reducePathTC from "../git-reconcile-rewritten-list/reducePath.spec";
 import { parseNewGoodCommandsSpec } from "../parse-todo-of-stacked-rebase/parseNewGoodCommands.spec";
 import autoCheckoutRemotePartialBranchesTC from "./auto-checkout-remote-partial-branches.spec";
+import { applyTC } from "./apply.spec";
 
 import { sequentialResolve } from "../util/sequentialResolve";
 import { cleanupTmpRepos } from "./util/tmpdir";
@@ -16,9 +17,13 @@ function main() {
 		async () => reducePathTC(),
 		parseNewGoodCommandsSpec,
 		autoCheckoutRemotePartialBranchesTC,
+		applyTC,
 	])
 		.then(cleanupTmpRepos)
-		.then(() => process.stdout.write("\nsuccess\n\n"))
+		.then(() => {
+			process.stdout.write("\nsuccess\n\n");
+			process.exit(0);
+		})
 		.catch((e) => {
 			process.stderr.write("\nfailure: " + e + "\n\n");
 			process.exit(1);