Re: [PATCH v2 04/11] t/lib-rebase: change the implementation of commands with options

[Christian Couder <christian.couder@gmail.com>]

On Mon, Feb 8, 2021 at 10:36 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Charvi Mendiratta <charvi077@gmail.com> writes:
>
> > However, "fixup" is very different from "exec". Its arguments are not
> > arbitrary at all, so there isn't a good reason to mirror the choice of
> > "_" to represent a space, which leads to rather unsightly tokens such
> > as "fixup_-C". Let's replace it with simpler tokens such as "fixup-C"
> > and "fixup-c".
>
> Sadly, I have to say that this change may be making the developer
> experience worse.
>
> To use the original, test writers only need to remember a single
> rule: "when a single command needs to embed a SP, replace it with
> underscore" regardless of which insn they are listing in FAKE_LINES.
>
> Now they need to remember that rule only applies to exec, and merge
> and fixup uses a different rule, namely, a SP immediately before a
> dash must be removed.

I agree with that, and discussed it with Eric. See:

https://lore.kernel.org/git/CAPig+cSBVG0AdyqXH2mZp6Ohrcb8_ec1Mm_vGbQM4zWT_7yYxQ@mail.gmail.com/

The discussion was:

-----------------------

> > > However, "fixup" is a very different beast. Its arguments are not
> > > arbitrary at all, so there isn't a good reason to mirror the choice of
> > > "_" to represent a space, which leads to rather unsightly tokens such
> > > as "fixup_-C". It would work just as well to use simpler tokens such
> > > as "fixup-C" and "fixup-c", in which case t/lib-rebase.sh might parse
> > > them like this (note that I also dropped `g` from the `sed` action):
> > >
> > >     fixup-*)
> > >         action=$(echo "$line" | sed 's/-/ -/');;
> >
> > I agree that "fixup" arguments are not arbitrary at all, but I think
> > it makes things simpler to just use one way to encode spaces instead
> > of many different ways.
>
> Is that the intention here, though? Is the idea that some day `fixup`
> will accept arbitrary arguments thus needs to encode spaces? If not,
> then mirroring the treatment given to `exec` confuses readers into
> thinking that it will/should accept arbitrary arguments. I brought
> this up in my review specifically because it was confusing to a person
> (me) new to this topic and reading the patches for the first time. The
> more specific and exact the code can be, the less likely it will
> confuse readers in the future.

-----------------------

> So, if I didn't know you folks have invested enough hours in this
> patch, I would have said not to do this, but it is such a small
> change, its effect isolated to only those who would be writing tests
> for "rebase -i", it may be OK to let them endure a bit additional
> burden to remember an extra rule with this patch.  I dunno.

I would be ok with dropping this patch. It might be a good idea to
improve the documentation before the function though.