Christian Couder <christian.couder@xxxxxxxxx> writes:
> On Tue, Mar 23, 2021 at 7:19 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>>
>> Christian Couder <christian.couder@xxxxxxxxx> 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>'
