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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 23, 2021, 17:11 UTC
Message-ID: <xmqqeeg54wye.fsf@gitster.g>
In-Reply-To: <CAP8UFD0s4Gm3PgDPpsXC-8Gyrnn1JTMUBu60XGV7i8nRDCCJ2Q@mail.gmail.com>

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

Show 22 quoted lines

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