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

[ZheNing Hu <adlternative@gmail.com>]

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