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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Jan 12 2023, Phillip Wood wrote:

> On 12/01/2023 12:25, Ævar Arnfjörð Bjarmason wrote:
>> On Thu, Jan 12 2023, Phillip Wood via GitGitGadget wrote:
>> 
>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>> 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).
>
> Thanks, Andrei has already mentioned that, I'll update the todo help
> when I re-roll
>> 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.
>> 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.
>
> It's not an accident, labels and commit names can begin with '#' so we
> need to support
>
> 	merge #parent1 #parent2

Ah, I would have told you that '#' was forbidden in refnames, but as
some trivial experimentation shows I was wrong about that. So yes, we
need the "# ".

> For "break" we could just not require '#' at all as we do for "reset
> <label>" where anything following the label is ignored. That would
> mean we couldn't add an argument to break in the future though (I'm
> not sure that is really a problem in practice). If we're going to
> require '#' then we may as well following the existing rules.

I'd think we'd want to parse past the "break" to find a "#", and error
out unknown stuff still, exactly to support future extensions.

I.e. we'd like to close the door on "break# foo", "break # foo" etc, but
not "break foo", unless I'm misunderstanding you here...

>> But in this commit you extend it to "break", but we're going out of or
>> way to e.g. extend this to "noop".
>
> I'm struggling to see why "noop" would need a comment - it only exists
> to avoid an empty todo list and is not meant for general use (it's not
> in the help added by append_todo_help() for this reason)

I'm struggling to see why "break" needs a comment, why not just add it
to the preceding line or something? But it seems some users like it :)

So at that point, it seems easier to both explain & implement something
that just consistently supports comment syntax, rather than overly
special-casing it.

> For "pick", "edit", "reword", "fixup" & "squash" we don't need a
> comment mechanism as we ignore everything after the commit name. For
> "reset" we ignore everything after the label. For "label" we could add
> support for comments but I'm not sure it is that useful and we'd need
> to be careful not to interpret a bad label name as a label + comment.

I think there's been a couple of request to have changing the "argument"
actually reword the $subject (I'm pretty such for "reword" that got as
far as a patch, but I may be misrecalling).

Of course that's an argument against what I was suggesting of making the
comment support a bit more general (although we could probably still
support it for "noop" or whatever).

>> 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.
>
> That would break labels and commit names that contain a '#'
>
> If we think we're never going to want "break" to take an argument then
> maybe we should just make it ignore the rest of the line like "reset
> <label>".

It's unfortunate that we do that, I think it's almost always better to
just error out rather that silently ignore data, except for some
explicit exceptions (such as comment syntax).