On Fri, Apr 9, 2021 at 3:37 PM ZheNing Hu via GitGitGadget <gitgitgadget@xxxxxxxxx> wrote: > > From: ZheNing Hu <adlternative@xxxxxxxxx> > > The `trailer.<token>.command` configuration variable > specifies a command (run via the shell, so it does not have > to be a single name of or path to the command, but can be a s/of or/or/ > shell script), and the first occurrence of substring $ARG is > replaced with the value given to the `interpret-trailer` > command for the token. This has two downsides: Maybe: s/for the token/for the token in a '--trailer <token>=<value>' argument/ > * The use of $ARG in the mechanism misleads the users that > the value is passed in the shell variable, and tempt them > to use $ARG more than once, but that would not work, as > the second and subsequent $ARG are not replaced. > > * Because $ARG is textually replaced without regard to the > shell language syntax, even '$ARG' (inside a single-quote > pair), which a user would expect to stay intact, would be > replaced, and worse, if the value had an unmatched single > quote (imagine a name like "O'Connor", substituted into > NAME='$ARG' to make it NAME='O'Connor'), it would result in > a broken command that is not syntactically correct (or > worse). > > Introduce a new `trailer.<token>.cmd` configuration that > takes higher precedence to deprecate and eventually remove > `trailer.<token>.command`, which passes the value as an > argument to the command. Instead of "$ARG", users can > refer to the value as positional argument, $1, in their > scripts. Good! > Helped-by: Junio C Hamano <gitster@xxxxxxxxx> > Helped-by: Christian Couder <christian.couder@xxxxxxxxx> > Signed-off-by: ZheNing Hu <adlternative@xxxxxxxxx> > --- > Documentation/git-interpret-trailers.txt | 90 ++++++++++++++++++++---- > t/t7513-interpret-trailers.sh | 84 ++++++++++++++++++++++ > trailer.c | 37 +++++++--- > 3 files changed, 187 insertions(+), 24 deletions(-) > > diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt > index 3e5aa3a65ae9..1a874a93f49b 100644 > --- a/Documentation/git-interpret-trailers.txt > +++ b/Documentation/git-interpret-trailers.txt > @@ -236,21 +236,38 @@ trailer.<token>.command:: > be called to automatically add or modify a trailer with the > specified <token>. > + > -When this option is specified, the behavior is as if a special > -'--trailer <token>=<value>' argument was added at the beginning of > -the "git interpret-trailers" command, where <value> is taken to be the > -standard output of the specified command with any leading and trailing > -whitespace trimmed off. > +When this option is specified, the first occurrence of substring $ARG is > +replaced with the value given to the `interpret-trailer` command for the > +same token. It passes the value through `$ARG`, otherwise this option behaves > +in the same way as 'trailer.<token>.cmd'. Actually I think that we should say first that this behaves in the same way as the 'trailer.<token>.cmd'. And this should also replace the first paragraph in the description of 'trailer.<token>.command', not just the second one. Maybe: "This option behaves in the same way as 'trailer.<token>.cmd', except that it doesn't pass anything as argument to the specified command. Instead the first occurrence of substring $ARG is replaced by the value that would be passed as argument." > +The 'trailer.<token>.command' option has been deprecated due to the fact s/deprecated/deprecated in favor of 'trailer.<token>.cmd'/ > +that $ARG in the user's command can only be replaced once and that the s/can only be/is only/ > +original way of replacing $ARG was not safe. s/was/is/ > Now the preferred option is 'trailer.<token>.cmd', which uses a positional argument to pass the value. I think we can remove this sentence especially if we say "deprecated in favor of 'trailer.<token>.cmd'" above. > -The first occurrence of substring `$ARG` will be replaced with the > -<value> part of an existing trailer with the same <token>, if any, > -before the command is launched. > +When both 'trailer.<token>.cmd' and 'trailer.<token>.command' are given > +for the same <token>, 'trailer.<token>.cmd' is used and > +'trailer.<token>.command' is ignored. Ok. > +trailer.<token>.cmd:: I think we should base the description of this option on what I suggest in patch 1/2. So let's agree on patch 1/2 before discussing this.
