Possible bug in git-rebase man page

Possible bug in git-rebase man page

[Stefan Haller]

The git-rebase documentation has an example for a git-rebase-todo file
when --rebase-merges is used; one of the lines in that file is

  reset refactor-button # Use the Button class for all buttons

From reading the code that parses the file, this doesn't seem to be a
valid line; as far as I can see, comments are not supported for reset or
label. The label is the entire rest of the line after the command.

Just wanted to confirm that I'm not missing anything.

-Stefan

Re: Possible bug in git-rebase man page

[Phillip Wood]

Hi Stefan

On 05/04/2023 18:36, Stefan Haller wrote:
> The git-rebase documentation has an example for a git-rebase-todo file
> when --rebase-merges is used; one of the lines in that file is
> 
>    reset refactor-button # Use the Button class for all buttons
> 
>  From reading the code that parses the file, this doesn't seem to be a
> valid line; as far as I can see, comments are not supported for reset or
> label. The label is the entire rest of the line after the command.
> 
> Just wanted to confirm that I'm not missing anything.

You're right that comments are not supported for labels, but for the 
reset command do_reset() ignores everything after the label so it does 
effectively support comments. I've got some patches to support comments 
on other commands which I'll hopefully submit before too long.

Best Wishes

Phillip

> -Stefan
> 

Re: Possible bug in git-rebase man page

[Stefan Haller]

On 06.04.23 11:49, Phillip Wood wrote:
> Hi Stefan
> 
> On 05/04/2023 18:36, Stefan Haller wrote:
>> The git-rebase documentation has an example for a git-rebase-todo file
>> when --rebase-merges is used; one of the lines in that file is
>>
>>    reset refactor-button # Use the Button class for all buttons
>>
>>  From reading the code that parses the file, this doesn't seem to be a
>> valid line; as far as I can see, comments are not supported for reset or
>> label. The label is the entire rest of the line after the command.
> 
> You're right that comments are not supported for labels, but for the
> reset command do_reset() ignores everything after the label so it does
> effectively support comments.

I don't follow; do_reset() simply uses whatever is stored in
item->arg_len, and this is set to go until the end of the line for
"label" and "reset" in parse_insn_line().

> I've got some patches to support comments on other commands which
> I'll hopefully submit before too long.

Hm, for label and reset this will make it harder for third-parties
parsing todo files. What's a real-world benefit of supporting comments
for these?

-Stefan

Re: Possible bug in git-rebase man page

[Phillip Wood]

Hi Stefan

On 06/04/2023 11:48, Stefan Haller wrote:
> On 06.04.23 11:49, Phillip Wood wrote:
>> Hi Stefan
>>
>> On 05/04/2023 18:36, Stefan Haller wrote:
>>> The git-rebase documentation has an example for a git-rebase-todo file
>>> when --rebase-merges is used; one of the lines in that file is
>>>
>>>     reset refactor-button # Use the Button class for all buttons
>>>
>>>   From reading the code that parses the file, this doesn't seem to be a
>>> valid line; as far as I can see, comments are not supported for reset or
>>> label. The label is the entire rest of the line after the command.
>>
>> You're right that comments are not supported for labels, but for the
>> reset command do_reset() ignores everything after the label so it does
>> effectively support comments.
> 
> I don't follow; do_reset() simply uses whatever is stored in
> item->arg_len, and this is set to go until the end of the line for
> "label" and "reset" in parse_insn_line().

But it splits in line in do_reset()

		/* Determine the length of the label */
		for (i = 0; i < len; i++)
			if (isspace(name[i]))
				break;
		len = i;

		commit = lookup_label(r, name, len, &ref_name);

>> I've got some patches to support comments on other commands which
>> I'll hopefully submit before too long.
> 
> Hm, for label and reset this will make it harder for third-parties
> parsing todo files. What's a real-world benefit of supporting comments
> for these?

It started as a request for comments on the "break" command but the 
discussion convinced me that we'll keep getting asked why doesn't "xxx" 
command support comments if we just add them to some commands. See 
https://lore.kernel.org/git/pull.1460.git.1673519809510.gitgitgadget@gmail.com/

Best Wishes

Phillip

> -Stefan

Re: Possible bug in git-rebase man page

[Stefan Haller]

On 06.04.23 14:07, Phillip Wood wrote:
> Hi Stefan
> 
> On 06/04/2023 11:48, Stefan Haller wrote:
>> On 06.04.23 11:49, Phillip Wood wrote:
>>> Hi Stefan
>>>
>>> On 05/04/2023 18:36, Stefan Haller wrote:
>>>> The git-rebase documentation has an example for a git-rebase-todo file
>>>> when --rebase-merges is used; one of the lines in that file is
>>>>
>>>>     reset refactor-button # Use the Button class for all buttons
>>>>
>>>>   From reading the code that parses the file, this doesn't seem to be a
>>>> valid line; as far as I can see, comments are not supported for
>>>> reset or
>>>> label. The label is the entire rest of the line after the command.
>>>
>>> You're right that comments are not supported for labels, but for the
>>> reset command do_reset() ignores everything after the label so it does
>>> effectively support comments.
>>
>> I don't follow; do_reset() simply uses whatever is stored in
>> item->arg_len, and this is set to go until the end of the line for
>> "label" and "reset" in parse_insn_line().
> 
> But it splits in line in do_reset()
> 
>         /* Determine the length of the label */
>         for (i = 0; i < len; i++)
>             if (isspace(name[i]))
>                 break;
>         len = i;
> 
>         commit = lookup_label(r, name, len, &ref_name);

Ah, I missed that, thanks. But it does this only after special-casing
the "[new root]" label, which does contain a space and needs to go to
the end of the line. This is weird.

This whole topic started because the todo parser that we are using in
lazygit has a bug where it doesn't recognize the "[new root]" label
properly, and this is because it does split the line at the first
whitespace, but without special-casing "[new root]" like git does.

How would you recommend we fix this? We could replicate git's current
behavior exactly, but then this would break when you introduce comments
later. Or we could anticipate that comments are added at some point, and
already split the line at the first # (trimming whitespace before that).
This would not exactly match git's current behavior, but it would be
good enough because git never actually uses whitespace in label or reset
lines except for the "[new root]" case.

I'm leaning towards the second option myself. Any thoughts?

-Stefan

Re: Possible bug in git-rebase man page

[Phillip Wood]

On 07/04/2023 07:12, Stefan Haller wrote:
> On 06.04.23 14:07, Phillip Wood wrote:

> Ah, I missed that, thanks. But it does this only after special-casing
> the "[new root]" label, which does contain a space and needs to go to
> the end of the line. This is weird.

I think it is by design. Unlike normal labels "[new root]" is not a ref 
and by choosing a name that is not a valid refname it will never 
conflict with a normal label.


> This whole topic started because the todo parser that we are using in
> lazygit has a bug where it doesn't recognize the "[new root]" label
> properly, and this is because it does split the line at the first
> whitespace, but without special-casing "[new root]" like git does.
> 
> How would you recommend we fix this? We could replicate git's current
> behavior exactly, but then this would break when you introduce comments
> later. Or we could anticipate that comments are added at some point, and
> already split the line at the first # (trimming whitespace before that).
> This would not exactly match git's current behavior, but it would be
> good enough because git never actually uses whitespace in label or reset
> lines except for the "[new root]" case.
> 
> I'm leaning towards the second option myself. Any thoughts?

I'd got for a slight variation of the second one where you just ignore 
everything after the required arguments for each command just like the 
reset command does - it does not require the comment to start with a 
'#', it will accept "reset mylabel this is a comment".

Best Wishes

Phillip

> 
> -Stefan