From 42946d1cf5e6f1cb37b69c08cf690d52d1468fb1 Mon Sep 17 00:00:00 2001 From: Kipras Melnikovas Date: Sun, 2 Apr 2023 16:22:05 +0300 Subject: [PATCH] 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 --- apply.ts | 93 +++++++++++++++++++------------ git-stacked-rebase.ts | 124 +++++++++++++++++++++--------------------- internal.ts | 4 ++ test/apply.spec.ts | 73 +++++++++++++++++++++++++ test/run.ts | 7 ++- 5 files changed, 203 insertions(+), 98 deletions(-) create mode 100644 test/apply.spec.ts diff --git a/apply.ts b/apply.ts index 2d48f5fe..2c6596a9 100644 --- a/apply.ts +++ b/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 { +}): Promise { 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 => { diff --git a/git-stacked-rebase.ts b/git-stacked-rebase.ts index 9b2f10c4..8a4a15c7 100755 --- a/git-stacked-rebase.ts +++ b/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, + }); } /** diff --git a/internal.ts b/internal.ts index c9f5c5d8..f361f2be 100644 --- a/internal.ts +++ b/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 */ diff --git a/test/apply.spec.ts b/test/apply.spec.ts new file mode 100644 index 00000000..56c041fc --- /dev/null +++ b/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 { + try { + await fn(); + return false; + } catch (_e) { + return true; + } +} diff --git a/test/run.ts b/test/run.ts index 4449ba38..bf8c7be6 100644 --- a/test/run.ts +++ b/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);