Re: [PATCH] rebase -i: allow a comment after a "break" command

[Phillip Wood <phillip.wood123@gmail.com>]

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 <phillip.wood@dunelm.org.uk>
>>
>> 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@schinagl.nl>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>      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