From: Olliver Schinagl Date: Thu, 12 Jan 2023 12:47:23 GMT Subject: Re: [PATCH] rebase -i: allow a comment after a "break" command Message-ID: <1d1becbc-5f4b-fb28-961c-112755e0c58e@schinagl.nl> In-Reply-To: <230112.86pmbk0vvj.gmgdl@evledraar.gmail.com> On 12-01-2023 13:25, Ævar Arnfjörð Bjarmason wrote: > On Thu, Jan 12 2023, Phillip Wood via GitGitGadget wrote: > >> From: Phillip Wood >> >> When adding a "break" command to a rebase todo list it can be helpful to >> add a comment as a reminder as to what the user was planning to do when >> the rebase stopped. Anything following the command is interpreted as an >> argument to the command and results in an error. Change this so that a >> "break command may be followed by "# " in the same way as >> a "merge" command. Requiring the comment to begin with "# " allows the >> break command to start taking an argument in the future if that turns >> out to be useful. >> >> Reported-by: Olliver Schinagl >> Signed-off-by: Phillip Wood >> --- >> rebase -i: allow a comment after a "break" command >> >> I'm open to suggestions for other ways to handle comments but copying >> what we do to separate merge parents from the merge commit subject >> seemed simplest. >> >> Should this print the comment when stopping for a break command? >> >> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1460%2Fphillipwood%2Fsequencer-allow-comment-after-break-v1 >> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1460/phillipwood/sequencer-allow-comment-after-break-v1 >> Pull-Request: https://github.com/gitgitgadget/git/pull/1460 >> >> Documentation/git-rebase.txt | 4 +++- >> sequencer.c | 7 +++++-- >> t/lib-rebase.sh | 2 +- >> t/t3418-rebase-continue.sh | 16 ++++++++++++++++ >> 4 files changed, 25 insertions(+), 4 deletions(-) >> >> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt >> index f9675bd24e6..511ace43db0 100644 >> --- a/Documentation/git-rebase.txt >> +++ b/Documentation/git-rebase.txt >> @@ -869,7 +869,9 @@ the files and/or the commit message, amend the commit, and continue >> rebasing. >> >> To interrupt the rebase (just like an "edit" command would do, but without >> -cherry-picking any commit first), use the "break" command. >> +cherry-picking any commit first), use the "break" command. A "break" >> +command may be followed by a comment beginning with `#` followed by a >> +space. > You're missing a corresponding edit here to the help string in > append_todo_help(), as you note you're making "break" support what > "merge" does, and that help string documents that "merge" accepts a > comment, after this we don't do that for "break", but should one way or > the other (see below). > >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -2509,7 +2509,9 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, >> padding = strspn(bol, " \t"); >> bol += padding; >> >> - if (item->command == TODO_NOOP || item->command == TODO_BREAK) { >> + if (item->command == TODO_NOOP || >> + (item->command == TODO_BREAK && >> + (bol[0] != '#' || (bol[1] && !isspace (bol[1]))))) { >> if (bol != eol) >> return error(_("%s does not accept arguments: '%s'"), >> command_to_string(item->command), bol); >> @@ -2524,7 +2526,8 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, >> command_to_string(item->command)); >> >> if (item->command == TODO_EXEC || item->command == TODO_LABEL || >> - item->command == TODO_RESET || item->command == TODO_UPDATE_REF) { >> + item->command == TODO_RESET || item->command == TODO_UPDATE_REF || >> + item->command == TODO_BREAK) { >> item->commit = NULL; >> item->arg_offset = bol - buf; >> item->arg_len = (int)(eol - bol); >> diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh >> index b57541356bd..a648013f299 100644 >> --- a/t/lib-rebase.sh >> +++ b/t/lib-rebase.sh >> @@ -51,7 +51,7 @@ set_fake_editor () { >> case $line in >> pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d|label|l|reset|r|merge|m) >> action="$line";; >> - exec_*|x_*|break|b) >> + exec_*|x_*|break_*|b_*|break|b) >> echo "$line" | sed 's/_/ /g' >> "$1";; >> merge_*|fixup_*) >> action=$(echo "$line" | sed 's/_/ /g');; >> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh >> index 130e2f9b553..18d82869b38 100755 >> --- a/t/t3418-rebase-continue.sh >> +++ b/t/t3418-rebase-continue.sh >> @@ -266,6 +266,22 @@ test_expect_success 'the todo command "break" works' ' >> test_path_is_file execed >> ' >> >> +test_expect_success 'the todo command "break" accepts a comment' ' >> + rm -f execed && >> + test_write_lines "break # comment" "break #" "exec >execed" >expect && >> + write_script cat-todo.sh <<-\EOS && >> + GIT_SEQUENCE_EDITOR="grep ^\[^#\]" git rebase --edit-todo >actual >> + EOS >> + FAKE_LINES="exec_./cat-todo.sh break_#_comment b_# exec_>execed" \ >> + git rebase -i HEAD && >> + test_cmp expect actual && >> + test_path_is_missing execed && >> + git rebase --continue && >> + test_path_is_missing execed && >> + git rebase --continue && >> + test_path_is_file execed >> +' >> + >> test_expect_success '--reschedule-failed-exec' ' >> test_when_finished "git rebase --abort" && >> test_must_fail git rebase -x false --reschedule-failed-exec HEAD^ && >> >> base-commit: 8a4e8f6a67e7fc97048d4666eec38399b88e0e3b > I like this direction, but I don't see why we need to continue this > special-snowflakeness of only allowing specific commands to accept these > #-comments. Oh, yes! That please :) I often want to put comments during a rebase. Also retrieving it becomes an issue at that point though. E.g. print it during status/etc. E.g. if I do 'edit' on a rebase and I add a comment, for example: 'edit/reword [original title]#Fix typo in title/commit message' I obviouslly also need to be reminded. e.g. what about commands that pop up the editor to 'amend' the commit message (as rebase --continue would do for an edit) also add the comment (using git commitChar) in the commit message. Kind of like how squash/fixup do "this is a commit based on two ..." with comments splattered around. (also kind of like git fixup -m does without a rebase?). Feature creep, sorry though :) > > Why not just have them all support it? It started with "merge", which as > 4c68e7ddb59 (sequencer: introduce the `merge` command, 2018-04-25) note > can be used for: > > merge -C baaabaaa abc # Merge the branch 'abc' into master > > As Olliver points out we should probably support "#" without the > following " ", which seems to be an accident of history & > over-strictness. :+1: Olliver > > But in this commit you extend it to "break", but we're going out of or > way to e.g. extend this to "noop". > > So I'd expect that just like the first for-loop in "parse_insn_line()" > we'd check if strchr(bol, '#') returns non-NULL, and if so set eol to > that result. > > The "just like" being that we may want to explicitly forbid this or > handle it specially for some, e.g. I didn't check but do the "label" and > "reset" perhaps support arbitrary non-'\n' (probably by accident, and we > could support commments there too). > > For "pick" we probably need to explicitly exclude it, although I can't > remember if we do anything useful with the part of the line after the > OID (maybe not...).