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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jan 12, 2023, 16:20 UTC
Message-ID: <4e2bb2da-0c42-ae9c-2a05-9b23db55c2ce@dunelm.org.uk>
In-Reply-To: <230112.86pmbk0vvj.gmgdl@evledraar.gmail.com>

On 12/01/2023 12:25, Ævar Arnfjörð Bjarmason wrote:

Show 22 quoted lines

> 
> 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

Show 13 quoted lines

> 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

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.

> 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)

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.

> 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>".

Best Wishes

Phillip

Show 8 quoted lines

> 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...).