From: Phillip Wood Date: Thu, 12 Jan 2023 16:26:09 GMT Subject: Re: [PATCH] rebase -i: allow a comment after a "break" command Message-ID: <569052f5-1573-f500-25d8-ac893b27dbd0@dunelm.org.uk> In-Reply-To: Hi Andrei Thanks for taking a look at this On 12/01/2023 11:14, Andrei Rybak wrote: > On 12/01/2023 11:36, 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? > > Technically, the user can look up the command via `git status`, but it > would make sense to just give the user this information directly, > similar to how exec command prints "Executing: ..." in addition to the > existing break command's message "Stopped at ...". Yes I think that is probable a good idea. >> 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. > > A corresponding update to append_todo_help in rebase-interactive.c > would be helpful. Thanks that's a good suggestion. >> 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" \ > > It seems that helper set_cat_todo_editor could be used here, except that > tests in t3418-rebase-continue.sh use a global set_fake_editor at the > very top of the file, unlike tests in t3404-rebase-interactive.sh which > call set_fake_editor individually.  See also related commits 6a619ca03c > (t3404: remove uneeded calls to set_fake_editor, 2019-10-15) and > b2dbacbddf (t3404: set $EDITOR in subshell, 2019-10-15). I did look at using set_cat_todo_editor but it isn't that simple. It cannot be used with set_fake_editor so I'd need to find another way to supply the todo list and it always exits with a failure. As well as checking that "break" accepts a comment this test also checks that the comment is still present when the user re-edits the todo list which is tricky to do with set_cat_todo_editor. Best Wishes Phillip