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

"ZheNing Hu via GitGitGadget" [off-list ref] writes:

From: ZheNing Hu <redacted>

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 [off-list ref]

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

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

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?

Junio C Hamano [off-list ref] 于2021年3月25日周四 上午4:18写道：

"ZheNing Hu via GitGitGadget" [off-list ref] writes:

quoted

From: ZheNing Hu <redacted>

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

Somewhat embarrassing. I have tried to fix it...

quoted

 1:  abc5b04d152f ! 1:  185356d6fc90 [GSOC]trailer: change $ARG to environment variable
     @@ Metadata
      Author: ZheNing Hu [off-list ref]

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

I thought the designs of the two were very similar.

quoted

 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"?

parse.

quoted

+     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?

For the time being, if I make "cmd" and "command" equivalent, it will
only trigger a warning.

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

OK.

quoted

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?

I thought if the "command" will need to be replaced in later releases,
 "is_new_cmd" will be removed at the same time, now I think
 "is_new_cmd" may cause misunderstandings.

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?

It sounds feasible, I will try.

Thanks.