On Thu, Jan 12, 2023 at 5:28 AM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote: > > > On Thu, Jan 12 2023, Phillip Wood via GitGitGadget wrote: > > > From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx> > > > > 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 "# <comment>" 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 <oliver@xxxxxxxxxxx> > > Signed-off-by: Phillip Wood <phillip.wood@xxxxxxxxxxxxx> > > --- > > 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. [...] > 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...). I disagree; pick should automatically always have comments. In particular, everything after the OID should be preceded by a '# '. The fact that we don't do that confuses people into thinking the commit summary is part of the directive or that by editing the commit summary in the todo list, that the commit message of the picked commit will somehow be updated to reflect such edits. I think the fact that even you can't remember whether we do anything useful with the part of the line after the OID, despite being one of the most prolific git contributors of all time, reinforces the point that no one knows what that commit summary means and we shouldn't be surprised that users get confused by it. We should just comment it out to make it clear that we will always ignore those words (and any changes thereto); the commit summary is only there as an aid to the user. I've been meaning to send in a patch to do precisely this (https://github.com/newren/git/commit/f1ae608477e010b96557d6fc87eed9f3f39b905e). Of course, the same applies to edit/squash/fixup/reword, though if I could go back in time...(warning, long tangent coming)...I would make it so those four directives did not accept any commit ID argument. Only "pick" and "reset" would accept a commit ID. Instead, today's "edit X" would be two commands ("pick X" followed by either "break" or "edit"), "fixup X" would be "pick X" + "fixup", and "reword X" would be "pick X" + "reword". That'd help users understand rebase state much better (it's so easy for users to get confused by whether they should be using `git commit --amend` vs. `git rebase --continue` and I think this is partially to blame, though there's other changes we could make to help with that dichotomy as well). The separate directives would also make it much easier to figure out how to both fixup and edit a single commit in the same rebase (pick the commit, then add a fixup directive, then an edit directive). In fact, "squash X" could just be discarded as superfluous, since it's just "pick X" + "fixup" + "reword" (or we could keep squash as an abbreviation for both "fixup" + "reword"). There's other things I'd change as well, which are tangential to this tangent, but I'm clearly veering way off the topic of comments in the rebase todo list, so I'll stop here.
