From e756d0ac85179ace73589c2e171ac08db10ed3e9 Mon Sep 17 00:00:00 2001 From: Kipras Melnikovas Date: Wed, 22 Dec 2021 02:51:56 +0200 Subject: [PATCH] fix: (part 1?) only auto-apply in the end if it's possible w/o exploding (regression introduced in 0657970dd2303f264036606e0cdc06b6854a0104) currently `canAndShouldBeApplying` is only in that part of the logic. to be fair, the problem arises only when we try to apply before the rebase was finished. but, it very well could be that the user (even tho unlikely) that user runs `git-stacked-rebase` while a regular `git-rebase` is in progress, thus causing the same issue. TODO so we need to handle this everywhere, and the approach might be different as well. Signed-off-by: Kipras Melnikovas --- apply.ts | 9 ++- branchSequencer.ts | 4 +- forcePush.ts | 1 + git-stacked-rebase.ts | 61 ++++++++++++--- .../parseNewGoodCommands.ts | 7 +- parse-todo-of-stacked-rebase/validator.ts | 76 +++++++++++++++---- 6 files changed, 129 insertions(+), 29 deletions(-) diff --git a/apply.ts b/apply.ts index 344adc28..c7565e1b 100644 --- a/apply.ts +++ b/apply.ts @@ -106,8 +106,13 @@ const getPathOfFilenameOfNeedsToApply = (pathToStackedRebaseDirInsideDotGit: str export const unmarkThatNeedsToApply = ( pathToStackedRebaseDirInsideDotGit: string, - mark = getPathOfFilenameOfNeedsToApply(pathToStackedRebaseDirInsideDotGit) -): void => (fs.existsSync(mark) ? fs.unlinkSync(mark) : void 0); + mark = getPathOfFilenameOfNeedsToApply(pathToStackedRebaseDirInsideDotGit), + rewrittenListFile: string = path.join(pathToStackedRebaseDirInsideDotGit, "rewritten-list") +): void => ( + fs.existsSync(mark) && fs.unlinkSync(mark), + fs.existsSync(rewrittenListFile) && fs.renameSync(rewrittenListFile, rewrittenListFile + ".applied"), + void 0 +); export async function applyIfNeedsToApply({ repo, diff --git a/branchSequencer.ts b/branchSequencer.ts index 2ecc1489..176d0ca8 100644 --- a/branchSequencer.ts +++ b/branchSequencer.ts @@ -52,6 +52,7 @@ export type BranchSequencerArgs = BranchSequencerArgsBase & { actionInsideEachCheckedOutBranch: ActionInsideEachCheckedOutBranch; delayMsBetweenCheckouts?: number; callbackAfterDone?: CallbackAfterDone; + rewrittenListFile?: "rewritten-list" | "rewritten-list.applied"; }; export type BranchSequencerBase = (args: BranchSequencerArgsBase) => Promise; @@ -67,12 +68,13 @@ export const branchSequencer: BranchSequencer = async ({ // callbackBeforeBegin, actionInsideEachCheckedOutBranch, callbackAfterDone = (): void => {}, + rewrittenListFile = "rewritten-list", }) => { if (!fs.existsSync(pathToStackedRebaseDirInsideDotGit)) { return fail(`\n\nno stacked-rebase in progress? (nothing to ${rootLevelCommandName})\n\n`); } - const [exit, stackedRebaseCommandsNew] = parseNewGoodCommands(repo, pathToStackedRebaseTodoFile); + const [exit, stackedRebaseCommandsNew] = parseNewGoodCommands(repo, pathToStackedRebaseTodoFile, rewrittenListFile); if (!stackedRebaseCommandsNew) return fail(exit); // const remotes: Git.Remote[] = await repo.getRemotes(); diff --git a/forcePush.ts b/forcePush.ts index a7c0befa..171e3720 100644 --- a/forcePush.ts +++ b/forcePush.ts @@ -38,4 +38,5 @@ export const forcePush: BranchSequencerBase = (argsBase) => execSyncInRepo(`${argsBase.gitCmd} push --force`); }, delayMsBetweenCheckouts: 0, + rewrittenListFile: "rewritten-list.applied", }); diff --git a/git-stacked-rebase.ts b/git-stacked-rebase.ts index 3bcf2d6f..ee8ed66b 100755 --- a/git-stacked-rebase.ts +++ b/git-stacked-rebase.ts @@ -17,6 +17,7 @@ import { createExecSyncInRepo } from "./util/execSyncInRepo"; import { noop } from "./util/noop"; import { parseTodoOfStackedRebase } from "./parse-todo-of-stacked-rebase/parseTodoOfStackedRebase"; import { processWriteAndOrExit, fail, EitherExitFinal } from "./util/Exitable"; +import { namesOfRebaseCommandsThatMakeRebaseExitToPause } from "./parse-todo-of-stacked-rebase/validator"; // console.log = () => {}; @@ -514,15 +515,57 @@ cat "$REWRITTEN_LIST_FILE_PATH" > "$REWRITTEN_LIST_BACKUP_FILE_PATH" * before proceeding w/ other commands way above. */ if (configValues.autoApplyIfNeeded) { - await applyIfNeedsToApply({ - repo, - pathToStackedRebaseTodoFile, - pathToStackedRebaseDirInsideDotGit, // - rootLevelCommandName: "--apply", - gitCmd: options.gitCmd, - autoApplyIfNeeded: configValues.autoApplyIfNeeded, - config, - }); + /** + * 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, "rewritten-list")) && + /** 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: configValues.autoApplyIfNeeded, + config, + }); + } } return; diff --git a/parse-todo-of-stacked-rebase/parseNewGoodCommands.ts b/parse-todo-of-stacked-rebase/parseNewGoodCommands.ts index 328fecea..1ce588ae 100644 --- a/parse-todo-of-stacked-rebase/parseNewGoodCommands.ts +++ b/parse-todo-of-stacked-rebase/parseNewGoodCommands.ts @@ -14,14 +14,15 @@ import { GoodCommand, stackedRebaseCommands } from "./validator"; export function parseNewGoodCommands( repo: Git.Repository, - pathToStackedRebaseTodoFile: string // + pathToStackedRebaseTodoFile: string, // + rewrittenListFile: "rewritten-list" | "rewritten-list.applied" ): EitherExit { const [exit, goodCommands] = parseTodoOfStackedRebase(pathToStackedRebaseTodoFile); if (!goodCommands) return fail(exit); logGoodCmds(goodCommands); - const pathOfRewrittenList: string = path.join(repo.workdir(), ".git", "stacked-rebase", "rewritten-list"); + const pathOfRewrittenList: string = path.join(repo.workdir(), ".git", "stacked-rebase", rewrittenListFile); const rewrittenList: string = fs.readFileSync(pathOfRewrittenList, { encoding: "utf-8" }); const rewrittenListLines: string[] = rewrittenList.split("\n").filter((line) => !!line); @@ -36,7 +37,7 @@ export function parseNewGoodCommands( const fromToSHA = line.split(" "); assert( fromToSHA.length === 2, - "from and to SHAs, coming from rewritten-list, are written properly (1 space total)." + `from and to SHAs, coming from ${rewrittenListFile}, are written properly (1 space total).` ); const [oldSHA, newSHA] = fromToSHA; diff --git a/parse-todo-of-stacked-rebase/validator.ts b/parse-todo-of-stacked-rebase/validator.ts index b7270e30..929d587d 100644 --- a/parse-todo-of-stacked-rebase/validator.ts +++ b/parse-todo-of-stacked-rebase/validator.ts @@ -23,13 +23,16 @@ type Command = { maxUseCount: number; isRestValid: Validator; // alwaysHasSha: boolean; // TODO: parseTarget (everyone has except `break`) - nameOrAlias: EitherRebaseEitherCommandOrAlias; + nameButNeverAlias: EitherRebaseCommand; parseTargets: ParseTargets; + + makesGitRebaseExitToPause: boolean; }; const createCommand = ( - nameOrAlias: string, + nameButNeverAlias: string, { + makesGitRebaseExitToPause, parseTargets = ({ split }) => { assert( split.length >= 2, @@ -41,29 +44,48 @@ const createCommand = ( maxUseCount = Infinity, isRestValid = () => true, }: { + /** + * such exit is usually when a user wants to do manual action, + * i.e. something that needs to be followed up by + * `git rebase --continue`, + * e.g. `break`, `edit`, etc.?, but not `pick`, etc. + * + * TODO think if cases such as dropping a commit implicitly + * (removing the line instead of using the `drop` command) + * would have impact for us here, etc. + * + */ + makesGitRebaseExitToPause: boolean; + // nameOrAlias: EitherRebaseEitherCommandOrAlias, // parseTargets?: Command["parseTargets"]; maxUseCount?: number; isRestValid?: Validator; - } = {} + } ): Command => ({ maxUseCount, isRestValid, - // nameOrAlias: nameOrAlias, - nameOrAlias: nameOrAlias as EitherRebaseEitherCommandOrAlias, // TODO: TS + nameButNeverAlias: nameButNeverAlias as EitherRebaseCommand, // TODO: TS parseTargets, + makesGitRebaseExitToPause, }); export const regularRebaseCommands = { - pick: createCommand("pick"), + pick: createCommand("pick", { makesGitRebaseExitToPause: false }), // p: standardCommand, - reword: createCommand("reword"), + reword: createCommand("reword", { + makesGitRebaseExitToPause: false /** opens editor & then continues, w/o exiting in between */, + }), // r: standardCommand, - edit: createCommand("edit"), + edit: createCommand("edit", { makesGitRebaseExitToPause: true }), // e: standardCommand, - squash: createCommand("squash"), + squash: createCommand("squash", { + makesGitRebaseExitToPause: false /** opens editor & then continues, w/o exiting in between */, + }), // s: standardCommand, fixup: createCommand("fixup", { + makesGitRebaseExitToPause: false /** opens editor & then continues, w/o exiting in between */, + parseTargets: ({ split }) => { /** * TODO: add metadata about -C|-c @@ -78,17 +100,23 @@ export const regularRebaseCommands = { }, }), // f: standardCommand, - exec: createCommand("exec", { parseTargets: ({ rest }) => [rest] }), + exec: createCommand("exec", { + makesGitRebaseExitToPause: false, // + + parseTargets: ({ rest }) => [rest], + }), // x: standardCommand, - break: createCommand("break", { parseTargets: () => null }), + break: createCommand("break", { makesGitRebaseExitToPause: true, parseTargets: () => null }), // b: standardCommand, - drop: createCommand("drop"), + drop: createCommand("drop", { makesGitRebaseExitToPause: false }), // d: standardCommand, - label: createCommand("label"), + label: createCommand("label", { makesGitRebaseExitToPause: false /** TODO VERIFY */ }), // l: standardCommand, - reset: createCommand("reset"), + reset: createCommand("reset", { makesGitRebaseExitToPause: false /** TODO VERIFY */ }), // t: standardCommand, merge: createCommand("merge", { + makesGitRebaseExitToPause: false /** TODO VERIFY */, + parseTargets: ({ split }) => { if (["-C", "-c"].includes(split[1])) { assert(split.length >= 4 /** not sure if 5 */); @@ -156,23 +184,35 @@ const branchValidator: Validator = ({ rest, reasonsIfBad: reasonsWhyInvalid }) = return !(reasonsWhyInvalid.length - origLen); }; +/** + * we'll never (?) have the `makesGitRebaseExitToPause` as `true` here + * because these commands do not end up in the regular git rebase's todo file. + */ export const stackedRebaseCommands = { "branch-end": createCommand("branch-end", { + makesGitRebaseExitToPause: false, + maxUseCount: Infinity, isRestValid: branchValidator, parseTargets: ({ rest }) => [rest], }), "branch-end-new": createCommand("branch-end-new", { + makesGitRebaseExitToPause: false, + maxUseCount: Infinity, isRestValid: branchValidator, parseTargets: ({ rest }) => [rest], }), "branch-end-initial": createCommand("branch-end-initial", { + makesGitRebaseExitToPause: false, + maxUseCount: 1, isRestValid: branchValidator, parseTargets: ({ rest }) => [rest], }), "branch-end-last": createCommand("branch-end-last", { + makesGitRebaseExitToPause: false, + maxUseCount: 1, isRestValid: branchValidator, parseTargets: ({ rest }) => [rest], @@ -212,6 +252,14 @@ const allEitherRebaseCommandAliases = { // [key in AllowedRebaseCommandAlias]: CommandAlias; // }; +export const rebaseCommandsThatMakeRebaseExitToPause: Command[] = Object.values(allEitherRebaseCommands).filter( + (cmd) => cmd.makesGitRebaseExitToPause +); + +export const namesOfRebaseCommandsThatMakeRebaseExitToPause: EitherRebaseCommand[] = rebaseCommandsThatMakeRebaseExitToPause.map( + (cmd) => cmd.nameButNeverAlias +); + type BadCommand = { command: string; lineNumber: number;