Re: [PATCH v2] [GSOC]trailer: pass arg as positional parameter

[Junio C Hamano <gitster@pobox.com>]

"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: ZheNing Hu <adlternative@gmail.com>
>
> In the original implementation of `trailer.<token>.command`,
> use `strbuf_replace` to replace $ARG in the <value> of the
> trailer, but it have a problem: `strbuf_replace` replace the
> $ARG in command only once, If the user's trailer command have
> used more than one $ARG, error will occur.
>
> If directly modify the implementation of the original
> `trailer.<token>.command`, The user’s previous `'$ARG'` in
> trailer command will not be replaced. So now add new
> config "trailer.<token>.cmd", pass trailer's value as
> positional parameter 1 to the user's command, users can
> use $1 as trailer's value, to implement original variable
> replacement.
>
> Original `trailer.<token>.command` can still be used until git
> completely abandoned it.

Sorry, but that's quite an ungrammatical mess X-<.

>  1:  abc5b04d152f ! 1:  185356d6fc90 [GSOC]trailer: change $ARG to environment variable
>      @@ Metadata
>       Author: ZheNing Hu <adlternative@gmail.com>

As this is completely a different design and does not share anything
with the earlier round, the range-diff is merely distracting and
useless.

>  Documentation/git-interpret-trailers.txt |  7 +++++++
>  t/t7513-interpret-trailers.sh            | 22 +++++++++++++++++++-
>  trailer.c                                | 26 +++++++++++++++++-------
>  3 files changed, 47 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
> index 96ec6499f001..38656b1b3841 100644
> --- a/Documentation/git-interpret-trailers.txt
> +++ b/Documentation/git-interpret-trailers.txt
> @@ -252,6 +252,13 @@ also be executed for each of these arguments. And the <value> part of
>  these arguments, if any, will be used to replace the `$ARG` string in
>  the command.
>  
> +trailer.<token>.cmd::
> +	Similar to 'trailer.<token>.command'. But the difference is that
> +	`$1` is used in the command to replace the value of the trailer
> +	instead of the original `$ARG`, which means that we can quote the

"quote"?

> +	trailer value multiple times in the command.
> +	E.g. `trailer.sign.cmd="test -n \"$1\" && echo \"$1\" || true "`

This needs to explain what happens if the user gives both .cmd and
.command to the same token.  Is it an error?  Is the newly invented
.cmd takes precedence?  Something else?

Whatever the answer is, the reasoning behind reaching the design
must be explained and defended in the proposed log message.


> diff --git a/trailer.c b/trailer.c
> index be4e9726421c..80f47657ff1a 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -14,6 +14,7 @@ struct conf_info {
>  	char *name;
>  	char *key;
>  	char *command;
> +	int is_new_cmd;

Poor naming.  The .cmd thing may be "new" right now in your mind,
but how would you transition out of it when design flaws are
discovered in it and replace it with yet another mechanism?

Add a new "char *cmd" field, and at the using site, define the
precedence between the two when both cmd and command members of the
structure are populated, perhaps?