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

Christian Couder [off-list ref] writes:

On Tue, Mar 23, 2021 at 7:19 AM Junio C Hamano [off-list ref] wrote:

quoted

Christian Couder [off-list ref] writes:

quoted

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

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.

I think what you mean is that my patch breaks the principle of
"forward compatibility", which may make it impossible for users who
previously worked with `'$ARG'`.

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

As your example below:

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

I would like to use this way.

It may be a bit cumbersome to do "deprecate and replace and
then later remove", Should documents tell users that the old method
"trailer.command" has been replaced by "trialer.cmd"? Or tell users that
"trailer.cmd" is a new feature?

Keep these doubts, I will try go to code it.

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

A little scary. :-)

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