From 45912a7ed9bae7da6c45a2e4221867a4b2f28156 Mon Sep 17 00:00:00 2001 From: Kipras Melnikovas Date: Sat, 22 Jan 2022 21:18:51 +0200 Subject: [PATCH 1/3] refactor: get rid of `Exitable`, use `throw` instead Signed-off-by: Kipras Melnikovas --- branchSequencer.ts | 15 +- git-stacked-rebase.ts | 42 +++-- .../parseNewGoodCommands.ts | 8 +- .../parseTodoOfStackedRebase.ts | 3 +- parse-todo-of-stacked-rebase/validator.ts | 8 +- util/Exitable.ts | 171 ------------------ util/error.ts | 1 + 7 files changed, 44 insertions(+), 204 deletions(-) delete mode 100644 util/Exitable.ts create mode 100644 util/error.ts diff --git a/branchSequencer.ts b/branchSequencer.ts index 44db91b2..e70a3df8 100644 --- a/branchSequencer.ts +++ b/branchSequencer.ts @@ -6,7 +6,7 @@ import Git from "nodegit"; import { filenames } from "./filenames"; import { createExecSyncInRepo } from "./util/execSyncInRepo"; -import { EitherExitFinal, fail } from "./util/Exitable"; +import { Termination } from "./util/error"; import { parseNewGoodCommands } from "./parse-todo-of-stacked-rebase/parseNewGoodCommands"; import { GoodCommand } from "./parse-todo-of-stacked-rebase/validator"; @@ -57,8 +57,8 @@ export type BranchSequencerArgs = BranchSequencerArgsBase & { rewrittenListFile?: typeof filenames.rewrittenList | typeof filenames.rewrittenListApplied; }; -export type BranchSequencerBase = (args: BranchSequencerArgsBase) => Promise; -export type BranchSequencer = (args: BranchSequencerArgs) => Promise; +export type BranchSequencerBase = (args: BranchSequencerArgsBase) => Promise; +export type BranchSequencer = (args: BranchSequencerArgs) => Promise; export const branchSequencer: BranchSequencer = async ({ pathToStackedRebaseDirInsideDotGit, // @@ -73,11 +73,14 @@ export const branchSequencer: BranchSequencer = async ({ rewrittenListFile = filenames.rewrittenList, }) => { if (!fs.existsSync(pathToStackedRebaseDirInsideDotGit)) { - return fail(`\n\nno stacked-rebase in progress? (nothing to ${rootLevelCommandName})\n\n`); + throw new Termination(`\n\nno stacked-rebase in progress? (nothing to ${rootLevelCommandName})\n\n`); } - const [exit, stackedRebaseCommandsNew] = parseNewGoodCommands(repo, pathToStackedRebaseTodoFile, rewrittenListFile); - if (!stackedRebaseCommandsNew) return fail(exit); + const stackedRebaseCommandsNew: GoodCommand[] = parseNewGoodCommands( + repo, + pathToStackedRebaseTodoFile, + rewrittenListFile + ); // const remotes: Git.Remote[] = await repo.getRemotes(); // const remote: Git.Remote | undefined = remotes.find((r) => diff --git a/git-stacked-rebase.ts b/git-stacked-rebase.ts index a5858535..f63e530a 100755 --- a/git-stacked-rebase.ts +++ b/git-stacked-rebase.ts @@ -19,8 +19,8 @@ import { createExecSyncInRepo } from "./util/execSyncInRepo"; import { noop } from "./util/noop"; import { uniq } from "./util/uniq"; 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"; +import { Termination } from "./util/error"; +import { GoodCommand, namesOfRebaseCommandsThatMakeRebaseExitToPause } from "./parse-todo-of-stacked-rebase/validator"; // console.log = () => {}; @@ -84,7 +84,7 @@ function areOptionsIncompetible( export const gitStackedRebase = async ( nameOfInitialBranch: string, specifiedOptions: SomeOptionsForGitStackedRebase = {} -): Promise => { +): Promise => { try { const options: OptionsForGitStackedRebase = { ...getDefaultOptions(), // @@ -95,7 +95,7 @@ export const gitStackedRebase = async ( const reasonsWhatWhyIncompatible: string[] = []; if (areOptionsIncompetible(options, reasonsWhatWhyIncompatible)) { - return fail( + throw new Termination( "\n" + bullets( "error - incompatible options:", // @@ -195,7 +195,7 @@ export const gitStackedRebase = async ( if (options.push) { if (!options.forcePush) { - return fail("\npush without --force will fail (since git rebase overrides history).\n\n"); + throw new Termination("\npush without --force will fail (since git rebase overrides history).\n\n"); } return await forcePush({ @@ -225,7 +225,7 @@ export const gitStackedRebase = async ( * to branchSequencer later. */ - return fail("\n--branch-sequencer (without --exec) - nothing to do?\n\n"); + throw new Termination("\n--branch-sequencer (without --exec) - nothing to do?\n\n"); } } @@ -273,8 +273,7 @@ export const gitStackedRebase = async ( const regularRebaseTodoLines: string[] = []; - const [exit, goodCommands] = parseTodoOfStackedRebase(pathToStackedRebaseTodoFile); - if (!goodCommands) return fail(exit); + const goodCommands: GoodCommand[] = parseTodoOfStackedRebase(pathToStackedRebaseTodoFile); const proms: Promise[] = goodCommands.map(async (cmd) => { if (cmd.rebaseKind === "regular") { @@ -646,8 +645,7 @@ cat "$REWRITTEN_LIST_FILE_PATH" > "$REWRITTEN_LIST_BACKUP_FILE_PATH" return; } catch (e) { - console.error(e); - return fail(e); + throw e; // TODO FIXME - no try/catch at all? } }; @@ -949,7 +947,7 @@ async function getCommitOfBranch(repo: Git.Repository, branchReference: Git.Refe /** * the CLI */ -export async function git_stacked_rebase(): Promise { +export async function git_stacked_rebase(): Promise { const pkgFromSrc = path.join(__dirname, "package.json"); const pkgFromDist = path.join(__dirname, "../", "package.json"); let pkg; @@ -1058,7 +1056,9 @@ git-stacked-rebase ${gitStackedRebaseVersionStr} console.log({ "process.argv after non-positional": process.argv }); const nameOfInitialBranch: string | undefined = eatNextArg(); - if (!nameOfInitialBranch) return fail(helpMsg); + if (!nameOfInitialBranch) { + throw new Termination(helpMsg); + } /** * TODO: improve arg parsing, lmao @@ -1112,17 +1112,19 @@ git-stacked-rebase ${gitStackedRebaseVersionStr} const fourth = eatNextArg(); branchSequencerExec = fourth ? fourth : false; } else { - return fail(`\n--branch-sequencer can only (for now) be followed by ${execNames.join("|")}\n\n`); + throw new Termination( + `\n--branch-sequencer can only (for now) be followed by ${execNames.join("|")}\n\n` + ); } } if (!isForcePush && !branchSequencerExec) { - return fail(`\nunrecognized 3th option (got "${third}")\n\n`); + throw new Termination(`\nunrecognized 3th option (got "${third}")\n\n`); } } if (process.argv.length) { - return fail( + throw new Termination( "" + // "\n" + bullets("\nerror - leftover arguments: ", process.argv, " ") + @@ -1146,5 +1148,13 @@ git-stacked-rebase ${gitStackedRebaseVersionStr} if (!module.parent) { git_stacked_rebase() // - .then(processWriteAndOrExit); + .then(() => process.exit(0)) + .catch((e) => { + if (e instanceof Termination) { + process.stderr.write(e.message); + process.exit(1); + } else { + throw e; + } + }); } diff --git a/parse-todo-of-stacked-rebase/parseNewGoodCommands.ts b/parse-todo-of-stacked-rebase/parseNewGoodCommands.ts index a2cbd272..c15087c4 100644 --- a/parse-todo-of-stacked-rebase/parseNewGoodCommands.ts +++ b/parse-todo-of-stacked-rebase/parseNewGoodCommands.ts @@ -8,7 +8,6 @@ import Git from "nodegit"; import { array } from "nice-comment"; import { filenames } from "../filenames"; -import { EitherExit, fail, succ } from "../util/Exitable"; import { parseTodoOfStackedRebase } from "./parseTodoOfStackedRebase"; import { GoodCommand, stackedRebaseCommands } from "./validator"; @@ -17,9 +16,8 @@ export function parseNewGoodCommands( repo: Git.Repository, pathToStackedRebaseTodoFile: string, // rewrittenListFile: typeof filenames.rewrittenList | typeof filenames.rewrittenListApplied -): EitherExit { - const [exit, goodCommands] = parseTodoOfStackedRebase(pathToStackedRebaseTodoFile); - if (!goodCommands) return fail(exit); +): GoodCommand[] { + const goodCommands: GoodCommand[] = parseTodoOfStackedRebase(pathToStackedRebaseTodoFile); logGoodCmds(goodCommands); @@ -164,7 +162,7 @@ export function parseNewGoodCommands( assert(stackedRebaseCommandsOld.length === stackedRebaseCommandsNew.length); - return succ(stackedRebaseCommandsNew); + return stackedRebaseCommandsNew; } const logGoodCmds = (goodCommands: GoodCommand[]): void => { diff --git a/parse-todo-of-stacked-rebase/parseTodoOfStackedRebase.ts b/parse-todo-of-stacked-rebase/parseTodoOfStackedRebase.ts index afe78bed..7389d9bb 100644 --- a/parse-todo-of-stacked-rebase/parseTodoOfStackedRebase.ts +++ b/parse-todo-of-stacked-rebase/parseTodoOfStackedRebase.ts @@ -2,7 +2,6 @@ import fs from "fs"; -import { EitherExit } from "../util/Exitable"; // import path from "path"; import { GoodCommand, validate } from "./validator"; @@ -10,7 +9,7 @@ import { GoodCommand, validate } from "./validator"; export function parseTodoOfStackedRebase( pathToStackedRebaseTodoFile: string // // goodCommands: GoodCommand[] -): EitherExit { +): GoodCommand[] { const editedRebaseTodo: string = fs.readFileSync(pathToStackedRebaseTodoFile, { encoding: "utf-8" }); const linesOfEditedRebaseTodo: string[] = editedRebaseTodo.split("\n").filter((line) => !!line); diff --git a/parse-todo-of-stacked-rebase/validator.ts b/parse-todo-of-stacked-rebase/validator.ts index 929d587d..f3d9a376 100644 --- a/parse-todo-of-stacked-rebase/validator.ts +++ b/parse-todo-of-stacked-rebase/validator.ts @@ -3,7 +3,7 @@ import { assert } from "console"; import { bullets, joinWith, tick } from "nice-comment"; -import { EitherExit, fail, succ } from "../util/Exitable"; +import { Termination } from "../util/error"; /** * if invalid, should fill the array with reasons why not valid. @@ -292,7 +292,7 @@ export type GoodCommand = { } ); -export function validate(linesOfEditedRebaseTodo: string[]): EitherExit { +export function validate(linesOfEditedRebaseTodo: string[]): GoodCommand[] { const badCommands: BadCommand[] = []; const goodCommands: GoodCommand[] = []; @@ -418,7 +418,7 @@ export function validate(linesOfEditedRebaseTodo: string[]): EitherExit = { -// code: ExitCode.SUCC; -// stdout: string; -// stderr?: never; -// } & ({} | never extends T -// ? { -// return?: never; -// } -// : { -// return: T; -// }); - -// export type ExitFail<_T> = { -// code: ExitCode.FAIL; -// stdout?: never; -// stderr: string; -// }; - -// export type Exitable = ExitSucc | ExitFail; - -// export const succ = (ret?: ExitSucc["return"], stdout: string = ""): ExitSucc => ({ -// return: ret as any, // TS should infer -// stdout, -// code: ExitCode.SUCC, -// }); - -// // export const fail = (args: Exclude, "code"> = { code: "fail" }): ExitFail => ({ -// export const fail = (stderr: ExitFail["stderr"] = ""): ExitFail => ({ -// stderr, -// code: ExitCode.FAIL, -// }); - -// export const processWrite = (exit: Exitable): typeof exit => ( -// exit.code === ExitCode.SUCC && exit.stdout -// ? process.stdout.write(exit.stdout) -// : exit.code === ExitCode.FAIL && exit.stderr -// ? process.stderr.write(exit.stderr) -// : void 0, -// exit -// ); - -// export const processWriteAndExit = (exit: Exit): void => ( -// processWrite(exit), // -// process.exit(exit.code) -// ); - -/** - * --- - * - * change of pace. - * we're no longer FP programmers — we're now C programmers. - * - * ((Gr393iDnN)) - * - * --- - */ - -// // export type Exit = { code: ExitCode; stderr?: string } | void; -// export type MaybeFail = void | { code: 1; stderr?: string }; - -// export type ExitFail = readonly [MaybeFail, null] // TODO ESLINT -// export type ExitSucc = readonly [null, Succ] // TODO ESLINT - -// // export type MaybeExit = Exit | T; -// export type EitherExit = ExitFail | ExitSucc; -// export type Exit = { code: ExitCode; stderr?: string } | void; - -// export type MaybeFail = { code: 1; stderr?: string }; - -// export const neutral = Symbol("neutral") -export const neutral = null; -export type Neutral = typeof neutral; - -// export type MaybeFail = string; -// export type MaybeFail = string | Neutral; -// export type MaybeFail = string; -export type MaybeFail = string | Neutral; - -// const emptySucc = Symbol("success") - -export type ExitFail = readonly [MaybeFail, Neutral] // TODO ESLINT -// export type ExitSucc = readonly [Neutral, Succ] // TODO ESLINT -export type ExitSucc = readonly [Neutral, Succ] // TODO ESLINT - -// export type MaybeExit = Exit | T; -export type EitherExit = ExitFail | ExitSucc; - -// export type EitherExitFinal = void | EitherExit; -export type EitherExitFinal = void | ExitFail; - -// export const fail = (args: Exclude, "code"> = { code: "fail" }): ExitFail => ({ -// export const fail = (stderr: string = ""): ExitFail => [ -// { -// stderr, -// code: ExitCode.FAIL, -// }, -// null -// ]; - -/** - * left - */ -// export const failRet = (stderr: string = ""): ExitFail => [ -// { -// stderr, -// // code: ExitCode.FAIL, -// code: 1, -// }, -// null, -// ]; - -export const fail = (stderr: MaybeFail | Neutral = ""): ExitFail => [stderr, neutral]; - -/** - * unwrap left - */ -// export const fail = (stderr: string = "") => failRet(stderr)[0]; - -// export type succ = { -// (): ExitSucc -// (ret: T): ExitSucc -// } - -/** - * right - */ -export const succ = (ret: T): ExitSucc => [neutral, ret]; - -/** - * TODO - */ -// export const succFinal = (): ExitSucc => [neutral, neutral] - -/** - * - */ -// export const processWriteAndOrExit = (exit: MaybeFail): void => ( -// !exit // -// ? process.exit(0) -// : (exit.stderr && process.stderr.write(exit.stderr), // -// process.exit(exit.code)), -// void 0 -// ); - -/** - * - */ -// export const processWriteAndOrExit = (exit: EitherExit | EitherExitFinal): void => ( -// // !exit || exit[0] === neutral // -// !exit || (exit[1] || exit[1] === neutral) // -// ? process.exit(0) -// : (exit[0] && process.stderr.write(exit[0]), // -// process.exit(1)), -// void 0 -// ); - -export const processWriteAndOrExit = (exit: EitherExit | EitherExitFinal): void => ( - // console.log({exit}), - exit && (!exit[1] || exit[1] === neutral) // - ? (exit[0] && process.stderr.write(exit[0]), // - process.exit(1)) - : process.exit(0), - void 0 -); diff --git a/util/error.ts b/util/error.ts new file mode 100644 index 00000000..aca1fe11 --- /dev/null +++ b/util/error.ts @@ -0,0 +1 @@ +export class Termination extends Error {} From 87e3aab985743d01bbf267d8ce63e72c4904e981 Mon Sep 17 00:00:00 2001 From: Kipras Melnikovas Date: Sat, 22 Jan 2022 21:24:57 +0200 Subject: [PATCH 2/3] fix: import assert from assert, not console Signed-off-by: Kipras Melnikovas --- parse-todo-of-stacked-rebase/validator.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/parse-todo-of-stacked-rebase/validator.ts b/parse-todo-of-stacked-rebase/validator.ts index f3d9a376..70a12633 100644 --- a/parse-todo-of-stacked-rebase/validator.ts +++ b/parse-todo-of-stacked-rebase/validator.ts @@ -1,7 +1,7 @@ /* eslint-disable @typescript-eslint/consistent-type-assertions */ /* eslint-disable indent */ -import { assert } from "console"; +import assert from "assert"; import { bullets, joinWith, tick } from "nice-comment"; import { Termination } from "../util/error"; From 8aae0024f17a0f43e193050753d2e7bd5341e65b Mon Sep 17 00:00:00 2001 From: Kipras Melnikovas Date: Tue, 25 Jan 2022 03:17:37 +0200 Subject: [PATCH 3/3] docs: add info on how it works + (perhaps too thorough?) info on the potential issues Signed-off-by: Kipras Melnikovas --- README.md | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+) diff --git a/README.md b/README.md index 123dda7f..3c4fa516 100644 --- a/README.md +++ b/README.md @@ -46,3 +46,102 @@ cd repo/ # then: git-stacked-rebase --help +``` + + +## how it works, the tricky parts, & things to be aware of + +instead of rebasing one partial branch on top of another, +we always use the latest branch and rebase it onto the initial branch, +and then reset the partial branches to point to new commits. + +the last part is the hardest, and likely still has many bugs yet unsolved. + +the tricky part comes from the power of git-rebase. +as long as a rebase finishes without exiting, we are golden. +but, as soon as a user indicates that they want to do some further +operations, by e.g. changing a command from `pick` to `edit` or `break`, +then git-rebase, when it reaches that point, has to exit (!) to let the user +do whatever actions they want to do, and of course allows the user to continue +the rebase via `git rebase --continue`. + + + +why this matters is because we need to recover the branches +to point to the new commits. + +git, upon finishing (?) the rebase, writes out a file +inside `.git/rebase-{merge|apply}/` -- `rewritten-list`. +this file contains the rewritten list of commits - +either `1 -> 1` mapping, or, if multiple commits got merged into 1 +(e.g. `squash` / `fixup` / manually with `edit`, or similar), +`N -> 1` mapping (?). +it's very simple - old commit SHA, space, new commit SHA, newline. + +again, this is how we understand the new positions that the branches +need to be poiting to. + +and no, we cannot simply use the `git-rebase-todo` file (the one you edit +when launching `git rebase -i` / `git-stacked-rebase`), +because of commands like `break`, `edit`, etc., whom, again, allow you +to modify the history, and we cannot infer of what will happen +by simply using the `git-rebase-todo` file. + + +let's first do a scenario without the issue(s) that we'll describe soon - +you ran `git-stacked-rebase`, you did some `fixup`s, `squash`es, `reword`s, +etc. - stuff that _does not_ make git-rebase exit. + +all went well, git-rebase wrote out the `rewritten-list` file, +we snagged it up with a `post-rewrite` script that we placed +inside `.git/hooks/`, we combined it with the now outdated +`git-rebase-todo` file (old SHAs etc.) to figure out where the +branch boundaries (partial branches) should end up at in the +new history, and we reset them successfully. all good and well. + +now, the scenario w/ a potential issue (?) -- you did +all the same as above, but you also had an `edit` command, +and when git-rebase progressed to it, you `git reset HEAD~` +your commit and instead create 3 new commits, and then continued +& the rebase via `git rebase --continue`. + +since `git-rebase` exited before it was done, +we, `git-stacked-rebase`, were __not__ able to reset the branches +to the new history, because of course, if git-rebase exits, +but the rebase is still in progress, we know you are _not_ done, +thus we exit as well. + +so when you finish, your changes are applied to your latest branch, +__but__ the partial branches are now out of date. + +what you need to do in this case is run `git-stacked-rebase --apply`, +which, in the 1st scenario, happened automatically. + +__the real issue arises when you forget to do the `--apply` part in time__, +i.e. if you do any other history rewritting, including `git commit --amend`, +before you `--apply`ied, `git-stacked-rebase` can no longer (?) properly figure out +where the partial branches should point to, and you're stuck in having to do it +manually. + +in the future, we might consider storing the list of partial branches +and if they end up gone, we could e.g. place them at the end of the +`git-rebase-todo` file the next time you invoke `git-stacked-rebase`, +thus you'd be able to drag them into their proper places yourself. +but until then, beware this issue exists (?).