{"thread":{"id":"59552","subject":"Possible bug in git-rebase man page","startedAt":"2023-04-05T17:36:29Z","lastAt":"2023-04-11T09:51:17Z","messageCount":6,"participants":["Stefan Haller","Phillip Wood"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"474868","messageId":"db535468-c991-df18-61bd-ec312fdb5ca0@haller-berlin.de","threadId":"59552","inReplyTo":null,"subject":"Possible bug in git-rebase man page","fromName":"Stefan Haller","fromEmail":"lists@haller-berlin.de","sentAt":"2023-04-05T17:36:23Z","receivedAt":"2023-04-05T17:36:29Z","isPatch":false,"sender":{"key":"lists@haller-berlin.de","avatar":null},"body":"The git-rebase documentation has an example for a git-rebase-todo file\nwhen --rebase-merges is used; one of the lines in that file is\n\n  reset refactor-button # Use the Button class for all buttons\n\nFrom reading the code that parses the file, this doesn't seem to be a\nvalid line; as far as I can see, comments are not supported for reset or\nlabel. The label is the entire rest of the line after the command.\n\nJust wanted to confirm that I'm not missing anything.\n\n-Stefan\n\n"},{"id":"474926","messageId":"51aca4bd-0f2b-0689-4b51-260349cc4151@gmail.com","threadId":"59552","inReplyTo":"db535468-c991-df18-61bd-ec312fdb5ca0@haller-berlin.de","subject":"Re: Possible bug in git-rebase man page","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-06T09:49:31Z","receivedAt":"2023-04-06T09:51:54Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Stefan\n\nOn 05/04/2023 18:36, Stefan Haller wrote:\n> The git-rebase documentation has an example for a git-rebase-todo file\n> when --rebase-merges is used; one of the lines in that file is\n> \n>    reset refactor-button # Use the Button class for all buttons\n> \n>  From reading the code that parses the file, this doesn't seem to be a\n> valid line; as far as I can see, comments are not supported for reset or\n> label. The label is the entire rest of the line after the command.\n> \n> Just wanted to confirm that I'm not missing anything.\n\nYou're right that comments are not supported for labels, but for the \nreset command do_reset() ignores everything after the label so it does \neffectively support comments. I've got some patches to support comments \non other commands which I'll hopefully submit before too long.\n\nBest Wishes\n\nPhillip\n\n> -Stefan\n> \n\n"},{"id":"474928","messageId":"f3e522f6-294d-4c08-b261-34d8e5b03b46@haller-berlin.de","threadId":"59552","inReplyTo":"51aca4bd-0f2b-0689-4b51-260349cc4151@gmail.com","subject":"Re: Possible bug in git-rebase man page","fromName":"Stefan Haller","fromEmail":"lists@haller-berlin.de","sentAt":"2023-04-06T10:48:03Z","receivedAt":"2023-04-06T10:48:11Z","isPatch":false,"sender":{"key":"lists@haller-berlin.de","avatar":null},"body":"On 06.04.23 11:49, Phillip Wood wrote:\n> Hi Stefan\n> \n> On 05/04/2023 18:36, Stefan Haller wrote:\n>> The git-rebase documentation has an example for a git-rebase-todo file\n>> when --rebase-merges is used; one of the lines in that file is\n>>\n>>    reset refactor-button # Use the Button class for all buttons\n>>\n>>  From reading the code that parses the file, this doesn't seem to be a\n>> valid line; as far as I can see, comments are not supported for reset or\n>> label. The label is the entire rest of the line after the command.\n> \n> You're right that comments are not supported for labels, but for the\n> reset command do_reset() ignores everything after the label so it does\n> effectively support comments.\n\nI don't follow; do_reset() simply uses whatever is stored in\nitem->arg_len, and this is set to go until the end of the line for\n\"label\" and \"reset\" in parse_insn_line().\n\n> I've got some patches to support comments on other commands which\n> I'll hopefully submit before too long.\n\nHm, for label and reset this will make it harder for third-parties\nparsing todo files. What's a real-world benefit of supporting comments\nfor these?\n\n-Stefan\n"},{"id":"474930","messageId":"7e99ff9b-99b7-1ea9-4131-43f507780284@gmail.com","threadId":"59552","inReplyTo":"f3e522f6-294d-4c08-b261-34d8e5b03b46@haller-berlin.de","subject":"Re: Possible bug in git-rebase man page","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-06T12:07:15Z","receivedAt":"2023-04-06T12:07:21Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Stefan\n\nOn 06/04/2023 11:48, Stefan Haller wrote:\n> On 06.04.23 11:49, Phillip Wood wrote:\n>> Hi Stefan\n>>\n>> On 05/04/2023 18:36, Stefan Haller wrote:\n>>> The git-rebase documentation has an example for a git-rebase-todo file\n>>> when --rebase-merges is used; one of the lines in that file is\n>>>\n>>>     reset refactor-button # Use the Button class for all buttons\n>>>\n>>>   From reading the code that parses the file, this doesn't seem to be a\n>>> valid line; as far as I can see, comments are not supported for reset or\n>>> label. The label is the entire rest of the line after the command.\n>>\n>> You're right that comments are not supported for labels, but for the\n>> reset command do_reset() ignores everything after the label so it does\n>> effectively support comments.\n> \n> I don't follow; do_reset() simply uses whatever is stored in\n> item->arg_len, and this is set to go until the end of the line for\n> \"label\" and \"reset\" in parse_insn_line().\n\nBut it splits in line in do_reset()\n\n\t\t/* Determine the length of the label */\n\t\tfor (i = 0; i < len; i++)\n\t\t\tif (isspace(name[i]))\n\t\t\t\tbreak;\n\t\tlen = i;\n\n\t\tcommit = lookup_label(r, name, len, &ref_name);\n\n>> I've got some patches to support comments on other commands which\n>> I'll hopefully submit before too long.\n> \n> Hm, for label and reset this will make it harder for third-parties\n> parsing todo files. What's a real-world benefit of supporting comments\n> for these?\n\nIt started as a request for comments on the \"break\" command but the \ndiscussion convinced me that we'll keep getting asked why doesn't \"xxx\" \ncommand support comments if we just add them to some commands. See \nhttps://lore.kernel.org/git/pull.1460.git.1673519809510.gitgitgadget@gmail.com/\n\nBest Wishes\n\nPhillip\n\n> -Stefan\n\n"},{"id":"474973","messageId":"dda64e68-1390-0b4d-bb93-65f745ab1300@haller-berlin.de","threadId":"59552","inReplyTo":"7e99ff9b-99b7-1ea9-4131-43f507780284@gmail.com","subject":"Re: Possible bug in git-rebase man page","fromName":"Stefan Haller","fromEmail":"lists@haller-berlin.de","sentAt":"2023-04-07T06:12:26Z","receivedAt":"2023-04-07T06:12:47Z","isPatch":false,"sender":{"key":"lists@haller-berlin.de","avatar":null},"body":"On 06.04.23 14:07, Phillip Wood wrote:\n> Hi Stefan\n> \n> On 06/04/2023 11:48, Stefan Haller wrote:\n>> On 06.04.23 11:49, Phillip Wood wrote:\n>>> Hi Stefan\n>>>\n>>> On 05/04/2023 18:36, Stefan Haller wrote:\n>>>> The git-rebase documentation has an example for a git-rebase-todo file\n>>>> when --rebase-merges is used; one of the lines in that file is\n>>>>\n>>>>     reset refactor-button # Use the Button class for all buttons\n>>>>\n>>>>   From reading the code that parses the file, this doesn't seem to be a\n>>>> valid line; as far as I can see, comments are not supported for\n>>>> reset or\n>>>> label. The label is the entire rest of the line after the command.\n>>>\n>>> You're right that comments are not supported for labels, but for the\n>>> reset command do_reset() ignores everything after the label so it does\n>>> effectively support comments.\n>>\n>> I don't follow; do_reset() simply uses whatever is stored in\n>> item->arg_len, and this is set to go until the end of the line for\n>> \"label\" and \"reset\" in parse_insn_line().\n> \n> But it splits in line in do_reset()\n> \n>         /* Determine the length of the label */\n>         for (i = 0; i < len; i++)\n>             if (isspace(name[i]))\n>                 break;\n>         len = i;\n> \n>         commit = lookup_label(r, name, len, &ref_name);\n\nAh, I missed that, thanks. But it does this only after special-casing\nthe \"[new root]\" label, which does contain a space and needs to go to\nthe end of the line. This is weird.\n\nThis whole topic started because the todo parser that we are using in\nlazygit has a bug where it doesn't recognize the \"[new root]\" label\nproperly, and this is because it does split the line at the first\nwhitespace, but without special-casing \"[new root]\" like git does.\n\nHow would you recommend we fix this? We could replicate git's current\nbehavior exactly, but then this would break when you introduce comments\nlater. Or we could anticipate that comments are added at some point, and\nalready split the line at the first # (trimming whitespace before that).\nThis would not exactly match git's current behavior, but it would be\ngood enough because git never actually uses whitespace in label or reset\nlines except for the \"[new root]\" case.\n\nI'm leaning towards the second option myself. Any thoughts?\n\n-Stefan\n"},{"id":"475137","messageId":"53849a41-405c-e488-8c9f-49b211d9a7d4@gmail.com","threadId":"59552","inReplyTo":"dda64e68-1390-0b4d-bb93-65f745ab1300@haller-berlin.de","subject":"Re: Possible bug in git-rebase man page","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-04-11T09:50:36Z","receivedAt":"2023-04-11T09:51:17Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 07/04/2023 07:12, Stefan Haller wrote:\n> On 06.04.23 14:07, Phillip Wood wrote:\n\n> Ah, I missed that, thanks. But it does this only after special-casing\n> the \"[new root]\" label, which does contain a space and needs to go to\n> the end of the line. This is weird.\n\nI think it is by design. Unlike normal labels \"[new root]\" is not a ref \nand by choosing a name that is not a valid refname it will never \nconflict with a normal label.\n\n\n> This whole topic started because the todo parser that we are using in\n> lazygit has a bug where it doesn't recognize the \"[new root]\" label\n> properly, and this is because it does split the line at the first\n> whitespace, but without special-casing \"[new root]\" like git does.\n> \n> How would you recommend we fix this? We could replicate git's current\n> behavior exactly, but then this would break when you introduce comments\n> later. Or we could anticipate that comments are added at some point, and\n> already split the line at the first # (trimming whitespace before that).\n> This would not exactly match git's current behavior, but it would be\n> good enough because git never actually uses whitespace in label or reset\n> lines except for the \"[new root]\" case.\n> \n> I'm leaning towards the second option myself. Any thoughts?\n\nI'd got for a slight variation of the second one where you just ignore \neverything after the required arguments for each command just like the \nreset command does - it does not require the comment to start with a \n'#', it will accept \"reset mylabel this is a comment\".\n\nBest Wishes\n\nPhillip\n\n> \n> -Stefan\n\n"}]}