Re: [PATCH v13] [GSOC] commit: add --trailer option

[Junio C Hamano <gitster@pobox.com>]

Christian Couder <christian.couder@gmail.com> writes:

> On Tue, Mar 23, 2021 at 7:19 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Christian Couder <christian.couder@gmail.com> writes:
>>
>> > If you want nothing to happen when $ARG isn't set, you can change the
>> > config option to something like:
>> >
>> > $ git config trailer.sign.command "NAME='\$ARG'; test -n \"\$NAME\" &&
>> > git log --author=\"\$NAME\" -1 --format='format:%aN <%aE>' || true"
>> >
>> > (This is because it looks like $ARG is replaced only once with the
>> > actual value, which is perhaps a bug. Otherwise something like the
>> > following might work:
>>
>> I do not know the origin of that code in trailers.c but it feels
>> quite confused and error prone to use textual replacement with
>> strbuf_replace().  Why doesn't the code, which knows it will use
>> shell to execute the command line given by the end user in the
>> configuration, to just export ARG as an environment variable and
>> be done with it?  It would also avoid quoting problem etc.
>
> Yeah, I agree that would be better.

It probably would have been better to do so before the feature got
unleased to the public, but doing such a change retroactively would
introduce regression for those who were using ARG that happens to be
safe from shell quoting rules.

For example, if the trailer.*.command were

	echo '$ARG'

and the argument 'h e l l o' were to be given to it, then the
current code would have textually expanded $ARG with the argument
and caused

	echo 'h e l l o'

to run, which would have been "fine" [*1*].

But exporting the environment ARG would "break" such a setting that
has been "working perfectly well" for the user.  Because of the
single-quotes around $ARG, the command now will give literal four
letter string $ARG and not 'h e l l o'.

We should think such potential ramifications of changing it (and
also not changing it) through before deciding what to do about it.

Although I have a feeling that not many people would miss '$ARG'
inside a pair of single-quotes to be replaced textually and it would
be OK to make a backward incompatible bugfix, the safer and better
way is not all that difficult, so I am inclined to suggest going the
usual "deprecate and replace and then later remove" dance.

The normal sequence of replacing a "sort of works but not
recommended" feature with a "better and safer, but can break a
setting that has been 'working'" feature is:

 - Announce deprecation of trailer.x.command and add and advertise a
   similar traier.x.cmd that (1) exports environment variable ARG,
   or (2) passes the argument as a positional parameter [*], as a
   replacement.  Explain the reason for deprecation (i.e. unsafe
   substitution that works only once).  When .cmd exists, .command
   is ignored for the corresponding trailer.x

 - Wait for a few releases and then remove trailer.x.command.

and that is the safest way to fix this "bug".


[Footnotes]

*1* If the argument were 

	';rm -rf .;'

    then it wouldn't have been fine, though, and that is how the
    current code solicited "Huh?"  reaction out of me.


*2* If we passed the argument as a positional parameter, the example
    you gave in the quoted part of the message would become
    something like this:

      [trailer "sign"]
        cmd = test -n "$1" && git log --author="$1" -1 --format='%aN <%aE>'