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

[Charvi Mendiratta <charvi077@gmail.com>]

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

Earlier from the discussions I thought it would be ok to make separate rules for
command taking arbitrary arguments(exec) and the command taking single
option(fixup).

But I also agree we can make the same rules and will remove it.

> It might be a good idea to
> improve the documentation before the function though.

Okay, Maybe we can improve like below:

update the current comment:
# "exec_cmd_with_args" -- add an "exec cmd with args" line.

with:
# "_" -- add a space, like "fixup_-C" implies "fixup -C" and
#        "exec_cmd_with_args" add an "exec cmd with args" line.

Thanks and Regards,
Charvi.