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