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

From: Elijah Newren <newren@gmail.com>
Date: Jan 12, 2023, 18:04 UTC
Message-ID: <CABPp-BHz6JXAyRXoNkDjsme4U6g=9f5i_LyMOJ1A4qysirf7Jw@mail.gmail.com>
In-Reply-To: <230112.86r0vzzp74.gmgdl@evledraar.gmail.com>

On Thu, Jan 12, 2023 at 9:20 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 6 quoted lines

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

[...]

Show 13 quoted lines

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

Personally, I think we should allow all lines in the todo script to have trailing comments.

Show 9 quoted lines

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

Yes, there have been, but it's a bad idea (and it's not just me saying that; Junio has also declared it as such). I think it's mostly based on confusion from us having ignored non-commented stuff on a line, and we could avoid that confusion by just commenting out the commit subject to make it clear we will ignore it and any changes to it.

Show 13 quoted lines

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

I agree.