[GSOC] trailer: add new .cmd config option #913

adlternative · 2021-03-23T13:48:27Z

In https://lore.kernel.org/git/xmqqv99i4ck2.fsf@gitster.g/
Junio and Christian talked about the problem of using
strbuf_replace() to replace $ARG:

if the user's script has more than one $ARG, only the first
one will be replaced, which is incorrected.
$ARG is textually replaced without shell syntax, which may
result a broken command when $ARG include some unmatching
single quote, very unsafe.

Now pass trailer value as $1 to the trailer command
with another trailer.<token>.cmd config, to solve these above
problems.

We are now writing documents that are more readable and correct
than before.

Change from last version: Change docs example "Count-count" to
"Helped-by".

cc: Christian Couder christian.couder@gmail.com
cc: Junio C Hamano gitster@pobox.com

adlternative · 2021-03-23T14:51:44Z

/submit

gitgitgadget · 2021-03-23T14:53:48Z

Submitted as pull.913.git.1616511182942.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-913/adlternative/trailer-pass-ARG-env-v1

To fetch this version to local tag pr-913/adlternative/trailer-pass-ARG-env-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-913/adlternative/trailer-pass-ARG-env-v1

adlternative · 2021-03-24T15:41:18Z

/submit

gitgitgadget · 2021-03-24T15:42:53Z

Submitted as pull.913.v2.git.1616600555906.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-913/adlternative/trailer-pass-ARG-env-v2

To fetch this version to local tag pr-913/adlternative/trailer-pass-ARG-env-v2:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-913/adlternative/trailer-pass-ARG-env-v2

gitgitgadget · 2021-03-24T20:21:37Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

gitgitgadget · 2021-03-25T01:46:43Z

On the Git mailing list, ZheNing Hu wrote (reply to this):

Junio C Hamano <gitster@pobox.com> 于2021年3月25日周四 上午4:18写道：
>
> "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-<.
>

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

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

I thought the designs of the two were very similar.

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

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

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

adlternative · 2021-03-25T11:52:04Z

/submit

gitgitgadget · 2021-03-25T11:53:47Z

Submitted as pull.913.v3.git.1616673200809.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-913/adlternative/trailer-pass-ARG-env-v3

To fetch this version to local tag pr-913/adlternative/trailer-pass-ARG-env-v3:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-913/adlternative/trailer-pass-ARG-env-v3

gitgitgadget · 2021-03-25T22:32:51Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

> From: ZheNing Hu <adlternative@gmail.com>
>
> Original implementation of `trailer.<token>.command` use

uses

> `strbuf_replace` to replace $ARG in command with the <value>
> of the trailer, but it have a problem: `strbuf_replace`

has

> replace the $ARG only once, If the user's trailer command

replaces the $ARG only once.

> have used more than one $ARG, the remaining replacement will
> fail.

"will fail" is quite vague.  It is just left unreplaced, and if the
user expects all of them to be replaced, then the expectation and
reality would not match, but all of that you have already said by
"replaces the $ARG only once.", so I think this sentence should be
removed.

> If directly modify the implementation of the original
> `trailer.<token>.command`, The user’s previous `'$ARG'` in
> trailer command will not be replaced.

That statement does not make much sense.  Depending on the way how
the implementation is "directly" modified, you can fix the "replaces
only once" problem without introducing such a problem.  Just look
for '$ARG' in the string and replace all of them, not just the first
one.  It's not too difficult.

This confusion primarily comes from the fact that you forgot to
explain the other problem you are fixing, I think.  Even though the
trailer.<token>.command documentation implies that the user is
expected to give a shell script or some sort as the command and the
use of $ARG makes it look like a shell variable, the original
implementation does not treat it as a shell variable at all.  And
the textual replacement is done without making sure the value being
replaced has characters with special meaning in the shell language.

So existing .command may incorrectly use $ARG inside a single-quote
pair and expect it to be replaced to a string inside a single-quote
pair.  A malformed, or worse, malicious, value would escape out of
the single-quote pair (remember, the '; rm -fr .' example?) and
execute arbitrary code in an unexpected way.  The (ungrammatical)
"if directly modify the implementation" refers to a potential way to
fix these two problems at the same time by doing the $ARG thing
without using textual replacement, namely, exporting the value as an
environment variable ARG.  If that approach was taken, then, $ARG
enclosed in a single-quote pair will no longer be replaced, which
makes it a backward incompatible change.

But without describing what solution you are talking about, your
three-line description does not make much sense.

> So now add new config
> "trailer.<token>.cmd", pass trailer's value as positional
> parameter 1 to the user's command, the user can use $1 as
> trailer's value, to implement original variable replacement.
>
> If the user has these two configuration: "trailer.<token>.cmd"
> and "trailer.<token>.command", "cmd" will execute and "command"
> will not executed.
>
> Original `trailer.<token>.command` can still be used until git
> completely abandoned it.
>
> Signed-off-by: ZheNing Hu <adlternative@gmail.com>

Let's rewrite it completely.

	The `trailer.<token>.command` configuration variable
	specifies a command (run via the shall, so it does not have
	to be a single name of or path to the command, but can be a
	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:

	* 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 unmatching 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 a
	parameter to the command.  Instead of "$ARG", the users will
	refer to the value as positional argument, $1, in their
	scripts.

I tried to cover everything we need to tell the reviewers about this
change with the above.  Did I miss anything?

> +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 pass the
> +	trailer value multiple times in the command.

We eventually want to deprecate the .command, so we'd prefer not to
rely on its description too much (e.g. try to find a way to say what
you want to say without "instead of the original `$ARG`").

	The command specified by this configuration variable is run
	with a single parameter, which is the <value> part of an
	existing trailer with the same <token>.  The output from the
	command is then used as the value for the <token> in the
	resulting trailer.

would be the replacement for the part that talks about $ARG in the
description of trailer.<token>.command.

The original description for `trailer.<token>.command` is so jumbled
(not your failure at all) that I had a hard time to understand what
it is trying to say (e.g. what does "as if a special <token>=<value>
argument were added at the beginning of the command line" mean?  Is
it making a one-shot export of environment variable to run the
command???), so the above may need further adjustment.  Christian?
Care to help out?

> +	E.g. `git config trailer.sign.cmd "test -n \"$1\" && echo \"$1\" || true "`.

An example is good.  There is a whole EXAMPLES section in this
manual page, and the one that uses $ARG may be a good candidate to
look at and change to use .cmd (instead of .command).

> +	If the user has these two configuration: "trailer.<token>.cmd"
> +	and "trailer.<token>.command", "cmd" will be executed and "command"
> +	will not be executed.

	When both .cmd and .command are given for the same <token>,
	.cmd is used and .command is ignored.

> diff --git a/trailer.c b/trailer.c
> index be4e9726421c..634d3f1ff04a 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -14,6 +14,7 @@ struct conf_info {
>  	char *name;
>  	char *key;
>  	char *command;
> +	char *cmd;
>  	enum trailer_where where;
>  	enum trailer_if_exists if_exists;
>  	enum trailer_if_missing if_missing;
> @@ -127,6 +128,7 @@ static void free_arg_item(struct arg_item *item)
>  	free(item->conf.name);
>  	free(item->conf.key);
>  	free(item->conf.command);
> +	free(item->conf.cmd);
>  	free(item->token);
>  	free(item->value);
>  	free(item);
> @@ -216,18 +218,24 @@ static int check_if_different(struct trailer_item *in_tok,
>  	return 1;
>  }
>  
> -static char *apply_command(const char *command, const char *arg)
> +static char *apply_command(const char *command, const char *cmd_, const char *arg)
>  {
>  	struct strbuf cmd = STRBUF_INIT;
>  	struct strbuf buf = STRBUF_INIT;
>  	struct child_process cp = CHILD_PROCESS_INIT;
>  	char *result;
>  
> -	strbuf_addstr(&cmd, command);
> -	if (arg)
> -		strbuf_replace(&cmd, TRAILER_ARG_STRING, arg);
> -
> -	strvec_push(&cp.args, cmd.buf);
> +	if (cmd_) {
> +		strbuf_addstr(&cmd, cmd_);
> +		strvec_push(&cp.args, cmd.buf);
> +		if (arg)
> +			strvec_push(&cp.args, arg);
> +	} else if (command) {
> +		strbuf_addstr(&cmd, command);
> +		strvec_push(&cp.args, cmd.buf);
> +		if (arg)
> +			strbuf_replace(&cmd, TRAILER_ARG_STRING, arg);
> +	}

OK.  it is clear cmd_ takes precedence this way.

Later (not as part of this patch, but a few releases down the road),
we may want to add a warning() about using a deprecated feature when
"else if (command)" block is taken.

> @@ -247,7 +255,7 @@ static char *apply_command(const char *command, const char *arg)
>  
>  static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg_tok)
>  {
> -	if (arg_tok->conf.command) {
> +	if (arg_tok->conf.command || arg_tok->conf.cmd) {
>  		const char *arg;
>  		if (arg_tok->value && arg_tok->value[0]) {
>  			arg = arg_tok->value;
> @@ -257,7 +265,7 @@ static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg
>  			else
>  				arg = xstrdup("");
>  		}
> -		arg_tok->value = apply_command(arg_tok->conf.command, arg);
> +		arg_tok->value = apply_command(arg_tok->conf.command, arg_tok->conf.cmd, arg);

It might be cleaner to just pass arg_tok->conf to apply_command()
and hide "cmd takes precedence over command" as an implementation
detail of that helper function.

The implementation looks as good as the original "command" with that
change at this point.  Documentation may need a bit more polishing.

Thanks.

gitgitgadget · 2021-03-26T13:34:07Z

On the Git mailing list, ZheNing Hu wrote (reply to this):

Junio C Hamano <gitster@pobox.com> 于2021年3月26日周五 上午6:28写道：
>
> "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: ZheNing Hu <adlternative@gmail.com>
> >
> > Original implementation of `trailer.<token>.command` use
>
> uses
>
> > `strbuf_replace` to replace $ARG in command with the <value>
> > of the trailer, but it have a problem: `strbuf_replace`
>
> has
>
> > replace the $ARG only once, If the user's trailer command
>
> replaces the $ARG only once.
>

Okay... singular and plural problems.

> > have used more than one $ARG, the remaining replacement will
> > fail.
>
> "will fail" is quite vague.  It is just left unreplaced, and if the
> user expects all of them to be replaced, then the expectation and
> reality would not match, but all of that you have already said by
> "replaces the $ARG only once.", so I think this sentence should be
> removed.
>

Indeed so.

> > If directly modify the implementation of the original
> > `trailer.<token>.command`, The user’s previous `'$ARG'` in
> > trailer command will not be replaced.
>
> That statement does not make much sense.  Depending on the way how
> the implementation is "directly" modified, you can fix the "replaces
> only once" problem without introducing such a problem.  Just look
> for '$ARG' in the string and replace all of them, not just the first
> one.  It's not too difficult.
>
> This confusion primarily comes from the fact that you forgot to
> explain the other problem you are fixing, I think.  Even though the
> trailer.<token>.command documentation implies that the user is
> expected to give a shell script or some sort as the command and the
> use of $ARG makes it look like a shell variable, the original
> implementation does not treat it as a shell variable at all.  And
> the textual replacement is done without making sure the value being
> replaced has characters with special meaning in the shell language.
>

Yes! The accidental injection problem caused should have been the focus
of my explanation.

> So existing .command may incorrectly use $ARG inside a single-quote
> pair and expect it to be replaced to a string inside a single-quote
> pair.  A malformed, or worse, malicious, value would escape out of
> the single-quote pair (remember, the '; rm -fr .' example?) and
> execute arbitrary code in an unexpected way.  The (ungrammatical)
> "if directly modify the implementation" refers to a potential way to
> fix these two problems at the same time by doing the $ARG thing
> without using textual replacement, namely, exporting the value as an
> environment variable ARG.  If that approach was taken, then, $ARG
> enclosed in a single-quote pair will no longer be replaced, which
> makes it a backward incompatible change.
>

Oh, I remeber it: terrible shell injection. Specifically, it looks like this:

$ git config trailer.sign.command "git log --author='\$ARG'"
$ git interpret-trailers --trailer "sign = adl' && rm -rf ./repo/'"

now I know that should be "backward incompatible change" as you said.

> But without describing what solution you are talking about, your
> three-line description does not make much sense.
>
> > So now add new config
> > "trailer.<token>.cmd", pass trailer's value as positional
> > parameter 1 to the user's command, the user can use $1 as
> > trailer's value, to implement original variable replacement.
> >
> > If the user has these two configuration: "trailer.<token>.cmd"
> > and "trailer.<token>.command", "cmd" will execute and "command"
> > will not executed.
> >
> > Original `trailer.<token>.command` can still be used until git
> > completely abandoned it.
> >
> > Signed-off-by: ZheNing Hu <adlternative@gmail.com>
>
> Let's rewrite it completely.
>
>         The `trailer.<token>.command` configuration variable
>         specifies a command (run via the shall, so it does not have
>         to be a single name of or path to the command, but can be a
>         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:
>
>         * 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 unmatching 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 a
>         parameter to the command.  Instead of "$ARG", the users will
>         refer to the value as positional argument, $1, in their
>         scripts.
>
> I tried to cover everything we need to tell the reviewers about this
> change with the above.  Did I miss anything?

Nothing to blame, feature of the old command, 2 problems, 1 solution,
this is what the log should look like.

>
> > +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 pass the
> > +     trailer value multiple times in the command.
>
> We eventually want to deprecate the .command, so we'd prefer not to
> rely on its description too much (e.g. try to find a way to say what
> you want to say without "instead of the original `$ARG`").
>

Oh! here I can’t rely on the documentation of the old `.command`, otherwise it’s
not easy to delete the old documentation.

>         The command specified by this configuration variable is run
>         with a single parameter, which is the <value> part of an
>         existing trailer with the same <token>.  The output from the
>         command is then used as the value for the <token> in the
>         resulting trailer.
>
> would be the replacement for the part that talks about $ARG in the
> description of trailer.<token>.command.
>
> The original description for `trailer.<token>.command` is so jumbled
> (not your failure at all) that I had a hard time to understand what
> it is trying to say (e.g. what does "as if a special <token>=<value>
> argument were added at the beginning of the command line" mean?  Is
> it making a one-shot export of environment variable to run the
> command???), so the above may need further adjustment.  Christian?
> Care to help out?
>
> > +     E.g. `git config trailer.sign.cmd "test -n \"$1\" && echo \"$1\" || true "`.
>
> An example is good.  There is a whole EXAMPLES section in this
> manual page, and the one that uses $ARG may be a good candidate to
> look at and change to use .cmd (instead of .command).
>

Okay, I will modify the paragraphs containing `.command` in these examples.

> > +     If the user has these two configuration: "trailer.<token>.cmd"
> > +     and "trailer.<token>.command", "cmd" will be executed and "command"
> > +     will not be executed.
>
>         When both .cmd and .command are given for the same <token>,
>         .cmd is used and .command is ignored.
>
> > diff --git a/trailer.c b/trailer.c
> > index be4e9726421c..634d3f1ff04a 100644
> > --- a/trailer.c
> > +++ b/trailer.c
> > @@ -14,6 +14,7 @@ struct conf_info {
> >       char *name;
> >       char *key;
> >       char *command;
> > +     char *cmd;
> >       enum trailer_where where;
> >       enum trailer_if_exists if_exists;
> >       enum trailer_if_missing if_missing;
> > @@ -127,6 +128,7 @@ static void free_arg_item(struct arg_item *item)
> >       free(item->conf.name);
> >       free(item->conf.key);
> >       free(item->conf.command);
> > +     free(item->conf.cmd);
> >       free(item->token);
> >       free(item->value);
> >       free(item);
> > @@ -216,18 +218,24 @@ static int check_if_different(struct trailer_item *in_tok,
> >       return 1;
> >  }
> >
> > -static char *apply_command(const char *command, const char *arg)
> > +static char *apply_command(const char *command, const char *cmd_, const char *arg)
> >  {
> >       struct strbuf cmd = STRBUF_INIT;
> >       struct strbuf buf = STRBUF_INIT;
> >       struct child_process cp = CHILD_PROCESS_INIT;
> >       char *result;
> >
> > -     strbuf_addstr(&cmd, command);
> > -     if (arg)
> > -             strbuf_replace(&cmd, TRAILER_ARG_STRING, arg);
> > -
> > -     strvec_push(&cp.args, cmd.buf);
> > +     if (cmd_) {
> > +             strbuf_addstr(&cmd, cmd_);
> > +             strvec_push(&cp.args, cmd.buf);
> > +             if (arg)
> > +                     strvec_push(&cp.args, arg);
> > +     } else if (command) {
> > +             strbuf_addstr(&cmd, command);
> > +             strvec_push(&cp.args, cmd.buf);
> > +             if (arg)
> > +                     strbuf_replace(&cmd, TRAILER_ARG_STRING, arg);
> > +     }
>
> OK.  it is clear cmd_ takes precedence this way.
>
> Later (not as part of this patch, but a few releases down the road),
> we may want to add a warning() about using a deprecated feature when
> "else if (command)" block is taken.
>

Fine, I will keep this version of "cmd priority execution" for now.

> > @@ -247,7 +255,7 @@ static char *apply_command(const char *command, const char *arg)
> >
> >  static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg_tok)
> >  {
> > -     if (arg_tok->conf.command) {
> > +     if (arg_tok->conf.command || arg_tok->conf.cmd) {
> >               const char *arg;
> >               if (arg_tok->value && arg_tok->value[0]) {
> >                       arg = arg_tok->value;
> > @@ -257,7 +265,7 @@ static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg
> >                       else
> >                               arg = xstrdup("");
> >               }
> > -             arg_tok->value = apply_command(arg_tok->conf.command, arg);
> > +             arg_tok->value = apply_command(arg_tok->conf.command, arg_tok->conf.cmd, arg);
>
> It might be cleaner to just pass arg_tok->conf to apply_command()
> and hide "cmd takes precedence over command" as an implementation
> detail of that helper function.
>
> The implementation looks as good as the original "command" with that
> change at this point.  Documentation may need a bit more polishing.
>

you're right.

> Thanks.

Thanks, Junio.
You and the people in the git community are very enthusiastic,
You have patiently explained these small mistakes that I made,
and taught me a lot of problems that I didn't notice.

Grateful.

--
ZheNing Hu

adlternative · 2021-03-26T16:11:33Z

/submit

gitgitgadget · 2021-03-26T16:14:07Z

Submitted as pull.913.v4.git.1616775185562.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-913/adlternative/trailer-pass-ARG-env-v4

To fetch this version to local tag pr-913/adlternative/trailer-pass-ARG-env-v4:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-913/adlternative/trailer-pass-ARG-env-v4

gitgitgadget · 2021-03-27T18:09:11Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

> @@ -252,6 +252,16 @@ 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::
> +	The command specified by this configuration variable is run
> +	with a single parameter, which is the <value> part of an
> +	existing trailer with the same <token>.  The output from the
> +	command is then used as the value for the <token> in the
> +	resulting trailer.
> +	The command is expected to replace `trailer.<token>.cmd`.
> +	When both .cmd and .command are given for the same <token>,
> +        .cmd is used and .command is ignored.

Christian, because ".cmd" is trying to eventually replace it, I find
it a bit disturbing that the description we give here looks a lot
smaller compared to the one for ".command".  I am afraid that we may
have failed to reproduce something important from the description of
the ".command" for the above; care to rend a hand or two here to
complete the description?

As I cannot grok what the description for ".command" is trying to
say, especially around this part:

    When this option is specified, the behavior is as if a special
    '<token>=<value>' argument were added at the beginning of the command
    line, where <value> is ...

and

    If some '<token>=<value>' arguments are also passed on the command
    line, when a 'trailer.<token>.command' is configured, the command will
    also be executed for each of these arguments.

I cannot quite judge if what we came up with in the above
description is sufficient.

> -* Configure a 'sign' trailer with a command to automatically add a
> +* Configure a 'sign' trailer with a cmd to automatically add a
>    'Signed-off-by: ' with the author information only if there is no
>    'Signed-off-by: ' already, and show how it works:
>  +
> @@ -309,7 +319,7 @@ $ git interpret-trailers --trailer 'Cc: Alice <alice@example.com>' --trailer 'Re
>  $ git config trailer.sign.key "Signed-off-by: "
>  $ git config trailer.sign.ifmissing add
>  $ git config trailer.sign.ifexists doNothing
> -$ git config trailer.sign.command 'echo "$(git config user.name) <$(git config user.email)>"'
> +$ git config trailer.sign.cmd 'echo "$(git config user.name) <$(git config user.email)>"'
>  $ git interpret-trailers <<EOF
>  > EOF

This change would definitely be needed when the support for
".command" is removed after deprecation period.  As it does not take
any argument, .cmd and .command should behave identically, so making
this change now, without waiting, may make sense.

> @@ -333,14 +343,14 @@ subject
>  Fix #42
>  ------------
>  
> -* Configure a 'see' trailer with a command to show the subject of a
> +* Configure a 'see' trailer with a cmd to show the subject of a
>    commit that is related, and show how it works:
>  +
>  ------------
>  $ git config trailer.see.key "See-also: "
>  $ git config trailer.see.ifExists "replace"
>  $ git config trailer.see.ifMissing "doNothing"
> -$ git config trailer.see.command "git log -1 --oneline --format=\"%h (%s)\" --abbrev-commit --abbrev=14 \$ARG"
> +$ git config trailer.see.cmd "test -n \"\$1\" && git log -1 --oneline --format=\"%h (%s)\" --abbrev-commit --abbrev=14 \"\$1\"|| true "
>  $ git interpret-trailers <<EOF
>  > subject

This, too, but until ".command" is removed, wouldn't it be better
for readers to keep both variants, as the distinction between $ARG
and $1 needs to be illustrated?

Besides, the examples given here are not equivalent.  The original
assumes that ARG is there, or it is OK to default to HEAD; the new
one gives no output when $ARG/$1 is not supplied.  It would confuse
readers to give two too-similar-but-subtly-different examles, as
they will be forced to wonder if the difference is something needed
to transition from .command to .cmd (and I am guessing that it is
not).

Rewriting both to use "--pretty=reference" may be worth doing.  As
can be seen in these examples:

git show -s --pretty=reference \$1
git log -1 --oneline --format=\"%h (%s)\" --abbrev-commit --abbrev=14 \$1

that it makes the result much easier to read.

Thanks.  Do not send a reroll prematurely; I want to see area
expert's input at this point.

gitgitgadget · 2021-03-27T19:57:29Z

On the Git mailing list, Christian Couder wrote (reply to this):

On Sat, Mar 27, 2021 at 7:04 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > @@ -252,6 +252,16 @@ 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::
> > +     The command specified by this configuration variable is run
> > +     with a single parameter, which is the <value> part of an
> > +     existing trailer with the same <token>.  The output from the
> > +     command is then used as the value for the <token> in the
> > +     resulting trailer.
> > +     The command is expected to replace `trailer.<token>.cmd`.

s/trailer.<token>.cmd/trailer.<token>.command/

> > +     When both .cmd and .command are given for the same <token>,
> > +        .cmd is used and .command is ignored.
>
> Christian, because ".cmd" is trying to eventually replace it, I find
> it a bit disturbing that the description we give here looks a lot
> smaller compared to the one for ".command".  I am afraid that we may
> have failed to reproduce something important from the description of
> the ".command" for the above; care to rend a hand or two here to
> complete the description?

Yeah, sure. I just saw that you already asked about this in this
thread. Sorry for not answering earlier.

> As I cannot grok what the description for ".command" is trying to
> say, especially around this part:
>
>     When this option is specified, the behavior is as if a special
>     '<token>=<value>' argument were added at the beginning of the command
>     line, where <value> is ...

This is because when a number of trailers are passed on the command
line, and some other trailers are in the input file, the order in
which the different trailers are processed and their priorities can be
important. So by saying the above, people can get an idea about at
which point and with which priority a trailer coming from such a
config option will be processed.

> and
>
>     If some '<token>=<value>' arguments are also passed on the command
>     line, when a 'trailer.<token>.command' is configured, the command will
>     also be executed for each of these arguments.

Yeah, this means that when a 'trailer.foo.command' is configured, it
is always executed at least once. The first time it is executed, it is
passed nothing ($ARG is replaced with the empty string). Then for each
'foo=<value>' argument passed on the command line, it is executed once
more with $ARG replaced by <value>.

The reason it is always executed first with $ARG replaced with the
empty string is that this way it makes it possible to set up commands
that will always be executed when `git interpret-trailers` is run.
This makes it possible to automatically add some trailers if they are
missing for example.

Another way to do it would be to have another config option called
`trailer.<token>.alwaysRunCmd` to tell if the cmd specified by
`trailer.<token>.cmd` should be run even if no '<token>=<value>'
argument is passed on the command line. As we are introducing
`trailer.<token>.cmd`, it's a good time to wonder if this would be a
better design. But this issue is quite complex, because of the fact
that 'trailer.<token>.ifMissing' and 'trailer.<token>.ifExists' also
take a part in deciding if the command will be run.

This mechanism is the reason why a trick, when setting up a
'trailer.foo.command' trailer, is to also set 'trailer.foo.ifexists'
to "replace", so that the first time the command is run (with $ARG
replaced with the empty string) it will add a foo trailer with a
default value, and if it is run another time, because a 'foo=bar'
argument is passed on the command line, then the trailer with the
default value will be replaced by the value computed from running the
command again with $ARG replaced with "bar".

Another trick is to have the command output nothing when $ARG is the
empty string along with using --trim-empty. This way the command will
create an empty trailer, when it is run the first time, and if it's
not another time, then this empty trailer will be removed because of
--trim-empty.

> I cannot quite judge if what we came up with in the above
> description is sufficient.

I don't think it's sufficient. I think that, while we are at it, a bit
more thinking/discussion is required to make sure we want to keep the
same design as 'trailer.<token>.command'.

> > -* Configure a 'sign' trailer with a command to automatically add a
> > +* Configure a 'sign' trailer with a cmd to automatically add a
> >    'Signed-off-by: ' with the author information only if there is no
> >    'Signed-off-by: ' already, and show how it works:
> >  +
> > @@ -309,7 +319,7 @@ $ git interpret-trailers --trailer 'Cc: Alice <alice@example.com>' --trailer 'Re
> >  $ git config trailer.sign.key "Signed-off-by: "
> >  $ git config trailer.sign.ifmissing add
> >  $ git config trailer.sign.ifexists doNothing
> > -$ git config trailer.sign.command 'echo "$(git config user.name) <$(git config user.email)>"'
> > +$ git config trailer.sign.cmd 'echo "$(git config user.name) <$(git config user.email)>"'
> >  $ git interpret-trailers <<EOF
> >  > EOF
>
> This change would definitely be needed when the support for
> ".command" is removed after deprecation period.  As it does not take
> any argument, .cmd and .command should behave identically, so making
> this change now, without waiting, may make sense.

By the way the above example is an example of why we might want any
configured command to be executed at least once, even when no
corresponding '<token>=<value>' argument is passed on the command
line.

> > @@ -333,14 +343,14 @@ subject
> >  Fix #42
> >  ------------
> >
> > -* Configure a 'see' trailer with a command to show the subject of a
> > +* Configure a 'see' trailer with a cmd to show the subject of a
> >    commit that is related, and show how it works:
> >  +
> >  ------------
> >  $ git config trailer.see.key "See-also: "
> >  $ git config trailer.see.ifExists "replace"
> >  $ git config trailer.see.ifMissing "doNothing"
> > -$ git config trailer.see.command "git log -1 --oneline --format=\"%h (%s)\" --abbrev-commit --abbrev=14 \$ARG"
> > +$ git config trailer.see.cmd "test -n \"\$1\" && git log -1 --oneline --format=\"%h (%s)\" --abbrev-commit --abbrev=14 \"\$1\"|| true "
> >  $ git interpret-trailers <<EOF
> >  > subject
>
> This, too, but until ".command" is removed, wouldn't it be better
> for readers to keep both variants, as the distinction between $ARG
> and $1 needs to be illustrated?
>
> Besides, the examples given here are not equivalent.  The original
> assumes that ARG is there, or it is OK to default to HEAD; the new
> one gives no output when $ARG/$1 is not supplied.

Yeah, I agree they are not equivalent.

> It would confuse
> readers to give two too-similar-but-subtly-different examles, as
> they will be forced to wonder if the difference is something needed
> to transition from .command to .cmd (and I am guessing that it is
> not).

I agree.

> Rewriting both to use "--pretty=reference" may be worth doing.  As
> can be seen in these examples:
>
> git show -s --pretty=reference \$1
> git log -1 --oneline --format=\"%h (%s)\" --abbrev-commit --abbrev=14 \$1
>
> that it makes the result much easier to read.

Yeah, thanks for the good suggestion.

gitgitgadget · 2021-03-28T10:49:49Z

On the Git mailing list, ZheNing Hu wrote (reply to this):

Christian Couder <christian.couder@gmail.com> 于2021年3月28日周日 上午3:53写道：
>
> On Sat, Mar 27, 2021 at 7:04 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> > > @@ -252,6 +252,16 @@ 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::
> > > +     The command specified by this configuration variable is run
> > > +     with a single parameter, which is the <value> part of an
> > > +     existing trailer with the same <token>.  The output from the
> > > +     command is then used as the value for the <token> in the
> > > +     resulting trailer.
> > > +     The command is expected to replace `trailer.<token>.cmd`.
>
> s/trailer.<token>.cmd/trailer.<token>.command/
>
> > > +     When both .cmd and .command are given for the same <token>,
> > > +        .cmd is used and .command is ignored.
> >
> > Christian, because ".cmd" is trying to eventually replace it, I find
> > it a bit disturbing that the description we give here looks a lot
> > smaller compared to the one for ".command".  I am afraid that we may
> > have failed to reproduce something important from the description of
> > the ".command" for the above; care to rend a hand or two here to
> > complete the description?
>
> Yeah, sure. I just saw that you already asked about this in this
> thread. Sorry for not answering earlier.
>
> > As I cannot grok what the description for ".command" is trying to
> > say, especially around this part:
> >
> >     When this option is specified, the behavior is as if a special
> >     '<token>=<value>' argument were added at the beginning of the command
> >     line, where <value> is ...
>
> This is because when a number of trailers are passed on the command
> line, and some other trailers are in the input file, the order in
> which the different trailers are processed and their priorities can be
> important. So by saying the above, people can get an idea about at
> which point and with which priority a trailer coming from such a
> config option will be processed.
>

This shows that .command itself has the characteristic of alwaysRun:
even if <token> <value> is not specified, the shell in .command will be
executed at least once, $ARG is empty by default. This is why I asked
`log --author=$ARG -1` will show the last commit identity when `--trailer`
 is not used.

> > and
> >
> >     If some '<token>=<value>' arguments are also passed on the command
> >     line, when a 'trailer.<token>.command' is configured, the command will
> >     also be executed for each of these arguments.
>
> Yeah, this means that when a 'trailer.foo.command' is configured, it
> is always executed at least once. The first time it is executed, it is
> passed nothing ($ARG is replaced with the empty string). Then for each
> 'foo=<value>' argument passed on the command line, it is executed once
> more with $ARG replaced by <value>.
>
> The reason it is always executed first with $ARG replaced with the
> empty string is that this way it makes it possible to set up commands
> that will always be executed when `git interpret-trailers` is run.
> This makes it possible to automatically add some trailers if they are
> missing for example.
>

Yes, $ARG or $1 are always exist because of:

               arg = xstrdup("");

so I think maybe we don't even need this judge in `apply_command`?
+               if (arg)
+                       strvec_push(&cp.args, arg);

> Another way to do it would be to have another config option called
> `trailer.<token>.alwaysRunCmd` to tell if the cmd specified by
> `trailer.<token>.cmd` should be run even if no '<token>=<value>'
> argument is passed on the command line. As we are introducing
> `trailer.<token>.cmd`, it's a good time to wonder if this would be a
> better design. But this issue is quite complex, because of the fact
> that 'trailer.<token>.ifMissing' and 'trailer.<token>.ifExists' also
> take a part in deciding if the command will be run.
>

In fact, I would prefer this design, because if I don’t add any trailers,
the trailer.<token>.command I set will be executed, which may be very
distressing sometimes, and `alwayRunCmd` is the user I hope that "trailers"
can be added automatically, and other trailers.<token>.command will not be
executed automatically. This allows the user to reasonably configure the
commands that need to be executed. This must be a very comfortable thing.

But as you said, to disable the automatic addition in the original .command
and use the new .alwaysRunCmd, I’m afraid there are a lot of things to consider.
Perhaps future series of patches can be considered to do it.

> This mechanism is the reason why a trick, when setting up a
> 'trailer.foo.command' trailer, is to also set 'trailer.foo.ifexists'
> to "replace", so that the first time the command is run (with $ARG
> replaced with the empty string) it will add a foo trailer with a
> default value, and if it is run another time, because a 'foo=bar'
> argument is passed on the command line, then the trailer with the
> default value will be replaced by the value computed from running the
> command again with $ARG replaced with "bar".
>
> Another trick is to have the command output nothing when $ARG is the
> empty string along with using --trim-empty. This way the command will
> create an empty trailer, when it is run the first time, and if it's
> not another time, then this empty trailer will be removed because of
> --trim-empty.
>

It looks very practical indeed.

> > I cannot quite judge if what we came up with in the above
> > description is sufficient.
>
> I don't think it's sufficient. I think that, while we are at it, a bit
> more thinking/discussion is required to make sure we want to keep the
> same design as 'trailer.<token>.command'.

Sure. I agree that more discussion is needed.
I think if the documents that once belonged to .command are copied to .cmd,
will the readers be too burdensome to read them? Will it be better to migrate
its documentation until we completely delete .command?

>
> > > -* Configure a 'sign' trailer with a command to automatically add a
> > > +* Configure a 'sign' trailer with a cmd to automatically add a
> > >    'Signed-off-by: ' with the author information only if there is no
> > >    'Signed-off-by: ' already, and show how it works:
> > >  +
> > > @@ -309,7 +319,7 @@ $ git interpret-trailers --trailer 'Cc: Alice <alice@example.com>' --trailer 'Re
> > >  $ git config trailer.sign.key "Signed-off-by: "
> > >  $ git config trailer.sign.ifmissing add
> > >  $ git config trailer.sign.ifexists doNothing
> > > -$ git config trailer.sign.command 'echo "$(git config user.name) <$(git config user.email)>"'
> > > +$ git config trailer.sign.cmd 'echo "$(git config user.name) <$(git config user.email)>"'
> > >  $ git interpret-trailers <<EOF
> > >  > EOF
> >
> > This change would definitely be needed when the support for
> > ".command" is removed after deprecation period.  As it does not take
> > any argument, .cmd and .command should behave identically, so making
> > this change now, without waiting, may make sense.
>
> By the way the above example is an example of why we might want any
> configured command to be executed at least once, even when no
> corresponding '<token>=<value>' argument is passed on the command
> line.

Already noticed that.

>
> > > @@ -333,14 +343,14 @@ subject
> > >  Fix #42
> > >  ------------
> > >
> > > -* Configure a 'see' trailer with a command to show the subject of a
> > > +* Configure a 'see' trailer with a cmd to show the subject of a
> > >    commit that is related, and show how it works:
> > >  +
> > >  ------------
> > >  $ git config trailer.see.key "See-also: "
> > >  $ git config trailer.see.ifExists "replace"
> > >  $ git config trailer.see.ifMissing "doNothing"
> > > -$ git config trailer.see.command "git log -1 --oneline --format=\"%h (%s)\" --abbrev-commit --abbrev=14 \$ARG"
> > > +$ git config trailer.see.cmd "test -n \"\$1\" && git log -1 --oneline --format=\"%h (%s)\" --abbrev-commit --abbrev=14 \"\$1\"|| true "
> > >  $ git interpret-trailers <<EOF
> > >  > subject
> >
> > This, too, but until ".command" is removed, wouldn't it be better
> > for readers to keep both variants, as the distinction between $ARG
> > and $1 needs to be illustrated?

So the correct solution should be to keep the original .command Examples,
and then give the .cmd examples again.

> >
> > Besides, the examples given here are not equivalent.  The original
> > assumes that ARG is there, or it is OK to default to HEAD; the new
> > one gives no output when $ARG/$1 is not supplied.
>
> Yeah, I agree they are not equivalent.
>
> > It would confuse
> > readers to give two too-similar-but-subtly-different examles, as
> > they will be forced to wonder if the difference is something needed
> > to transition from .command to .cmd (and I am guessing that it is
> > not).
>
> I agree.

OK...I will modify it.

>
> > Rewriting both to use "--pretty=reference" may be worth doing.  As
> > can be seen in these examples:
> >
> > git show -s --pretty=reference \$1
> > git log -1 --oneline --format=\"%h (%s)\" --abbrev-commit --abbrev=14 \$1
> >
> > that it makes the result much easier to read.
>
> Yeah, thanks for the good suggestion.

Yes, `--pretty=reference` is similar to `--format="%h(%s)"` and provides better
readability.

Thanks,Junio and Christian!

--
ZheNing Hu

gitgitgadget · 2021-03-29T09:08:12Z

On the Git mailing list, Christian Couder wrote (reply to this):

On Sun, Mar 28, 2021 at 12:46 PM ZheNing Hu <adlternative@gmail.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> 于2021年3月28日周日 上午3:53写道：
> >
> > On Sat, Mar 27, 2021 at 7:04 PM Junio C Hamano <gitster@pobox.com> wrote:

> > > As I cannot grok what the description for ".command" is trying to
> > > say, especially around this part:
> > >
> > >     When this option is specified, the behavior is as if a special
> > >     '<token>=<value>' argument were added at the beginning of the command
> > >     line, where <value> is ...
> >
> > This is because when a number of trailers are passed on the command
> > line, and some other trailers are in the input file, the order in
> > which the different trailers are processed and their priorities can be
> > important. So by saying the above, people can get an idea about at
> > which point and with which priority a trailer coming from such a
> > config option will be processed.
>
> This shows that .command itself has the characteristic of alwaysRun:
> even if <token> <value> is not specified, the shell in .command will be
> executed at least once, $ARG is empty by default. This is why I asked
> `log --author=$ARG -1` will show the last commit identity when `--trailer`
>  is not used.

Yeah, that's the reason.

> > > and
> > >
> > >     If some '<token>=<value>' arguments are also passed on the command
> > >     line, when a 'trailer.<token>.command' is configured, the command will
> > >     also be executed for each of these arguments.
> >
> > Yeah, this means that when a 'trailer.foo.command' is configured, it
> > is always executed at least once. The first time it is executed, it is
> > passed nothing ($ARG is replaced with the empty string). Then for each
> > 'foo=<value>' argument passed on the command line, it is executed once
> > more with $ARG replaced by <value>.
> >
> > The reason it is always executed first with $ARG replaced with the
> > empty string is that this way it makes it possible to set up commands
> > that will always be executed when `git interpret-trailers` is run.
> > This makes it possible to automatically add some trailers if they are
> > missing for example.
>
> Yes, $ARG or $1 are always exist because of:
>
>                arg = xstrdup("");
>
> so I think maybe we don't even need this judge in `apply_command`?
> +               if (arg)
> +                       strvec_push(&cp.args, arg);

Yeah, I haven't looked at the code, but that might be a good
simplification. If you work on this, please submit it in a separate
commit.

> > Another way to do it would be to have another config option called
> > `trailer.<token>.alwaysRunCmd` to tell if the cmd specified by
> > `trailer.<token>.cmd` should be run even if no '<token>=<value>'
> > argument is passed on the command line. As we are introducing
> > `trailer.<token>.cmd`, it's a good time to wonder if this would be a
> > better design. But this issue is quite complex, because of the fact
> > that 'trailer.<token>.ifMissing' and 'trailer.<token>.ifExists' also
> > take a part in deciding if the command will be run.

Actually after thinking about it, I think it might be better, instead
of `trailer.<token>.alwaysRunCmd`, to add something like
`trailer.<token>.runMode` that could take multiple values like:

- "beforeCLI": would make it run once, like ".command" does now before
any CLI trailer are processed

- "forEachCLIToken": would make it run once for each trailer that has
the token, like ".command" also does now, the difference would be that
the value for the token would be passed in the $1 argument

- "afterCLI": would make it run once after all the CLI trailers have
been processed and it could pass the different values for the token if
any in different arguments: $1, $2, $3, ...

This would make it possible to extend later if the need arises for
more different times or ways to run configured commands.

> In fact, I would prefer this design, because if I don’t add any trailers,
> the trailer.<token>.command I set will be executed, which may be very
> distressing sometimes, and `alwayRunCmd` is the user I hope that "trailers"
> can be added automatically, and other trailers.<token>.command will not be
> executed automatically. This allows the user to reasonably configure the
> commands that need to be executed. This must be a very comfortable thing.

I agree that it should be easier and more straightforward, than it is
now, to configure this.

> But as you said, to disable the automatic addition in the original .command
> and use the new .alwaysRunCmd, I’m afraid there are a lot of things to consider.
> Perhaps future series of patches can be considered to do it.

Yeah, support for `trailer.<token>.runMode` might be added in
different commits at least and possibly later in a different patch
series. There are the following issues to resolve, though, if we want
to focus only on a new ".cmd" config option:

- how and when should it run by default,
- how to explain that in the doc, and maybe
- how to improve the current description of what happens for ".command"

> > This mechanism is the reason why a trick, when setting up a
> > 'trailer.foo.command' trailer, is to also set 'trailer.foo.ifexists'
> > to "replace", so that the first time the command is run (with $ARG
> > replaced with the empty string) it will add a foo trailer with a
> > default value, and if it is run another time, because a 'foo=bar'
> > argument is passed on the command line, then the trailer with the
> > default value will be replaced by the value computed from running the
> > command again with $ARG replaced with "bar".
> >
> > Another trick is to have the command output nothing when $ARG is the
> > empty string along with using --trim-empty. This way the command will
> > create an empty trailer, when it is run the first time, and if it's
> > not another time, then this empty trailer will be removed because of
> > --trim-empty.
> >
>
> It looks very practical indeed.
>
> > > I cannot quite judge if what we came up with in the above
> > > description is sufficient.
> >
> > I don't think it's sufficient. I think that, while we are at it, a bit
> > more thinking/discussion is required to make sure we want to keep the
> > same design as 'trailer.<token>.command'.
>
> Sure. I agree that more discussion is needed.
> I think if the documents that once belonged to .command are copied to .cmd,
> will the readers be too burdensome to read them? Will it be better to migrate
> its documentation until we completely delete .command?

My opinion (if we focus only on adding ".cmd") is that:

- for simplicity for now it should run at the same time as ".command",
the only difference being how the argument is passed (using $1 instead
of textually replacing $ARG)
- the doc for ".command" should be first improved if possible, and
then moved over to ".cmd" saying for ".command" that ".command" is
deprecated in favor of ".cmd" but otherwise works as ".cmd" except
that instead using $1 the value is passed by textually replacing $ARG
which could be a safety and correctness issue.

Another way to work on all this, would be to first work on adding
support for `trailer.<token>.runMode` and on improving existing
documentation, and then to add ".cmd", which could then by default use
a different ".runMode" than ".command".

> > > This, too, but until ".command" is removed, wouldn't it be better
> > > for readers to keep both variants, as the distinction between $ARG
> > > and $1 needs to be illustrated?
>
> So the correct solution should be to keep the original .command Examples,
> and then give the .cmd examples again.

Maybe we could take advantage of ".cmd" to show other nice
possibilities to use all of this. Especially if support for `git
commit --trailer ...` is already merged, we might be able to use it in
those examples, or perhaps add some examples to the git commit doc.

Best,
Christian.

gitgitgadget · 2021-03-29T13:46:08Z

On the Git mailing list, ZheNing Hu wrote (reply to this):

Christian Couder <christian.couder@gmail.com> 于2021年3月29日周一 下午5:05写道：
>
> >
> > Yes, $ARG or $1 are always exist because of:
> >
> >                arg = xstrdup("");
> >
> > so I think maybe we don't even need this judge in `apply_command`?
> > +               if (arg)
> > +                       strvec_push(&cp.args, arg);
>
> Yeah, I haven't looked at the code, but that might be a good
> simplification. If you work on this, please submit it in a separate
> commit.
>

Well, if necessary, I'll put it in another commit, maybe I should double check
to see if there's anything special going on.

> > > Another way to do it would be to have another config option called
> > > `trailer.<token>.alwaysRunCmd` to tell if the cmd specified by
> > > `trailer.<token>.cmd` should be run even if no '<token>=<value>'
> > > argument is passed on the command line. As we are introducing
> > > `trailer.<token>.cmd`, it's a good time to wonder if this would be a
> > > better design. But this issue is quite complex, because of the fact
> > > that 'trailer.<token>.ifMissing' and 'trailer.<token>.ifExists' also
> > > take a part in deciding if the command will be run.
>
> Actually after thinking about it, I think it might be better, instead
> of `trailer.<token>.alwaysRunCmd`, to add something like
> `trailer.<token>.runMode` that could take multiple values like:
>

If really can achieve it is certainly better than 'alwaysRunCmd'.
The following three small configuration options look delicious.
But I think it needs to be discussed in more detail:

> - "beforeCLI": would make it run once, like ".command" does now before
> any CLI trailer are processed
>

Does "beforeCLI" handle all trailers? Or is it just doing something to add empty
value trailers?

> - "forEachCLIToken": would make it run once for each trailer that has
> the token, like ".command" also does now, the difference would be that
> the value for the token would be passed in the $1 argument
>

This is exactly same as before.

> - "afterCLI": would make it run once after all the CLI trailers have
> been processed and it could pass the different values for the token if
> any in different arguments: $1, $2, $3, ...
>

I might get a little confused here: What's the input for $1,$2,$3?
Is users more interested in dealing with trailers value or a line of the
trailer?

> This would make it possible to extend later if the need arises for
> more different times or ways to run configured commands.
>
> > In fact, I would prefer this design, because if I don’t add any trailers,
> > the trailer.<token>.command I set will be executed, which may be very
> > distressing sometimes, and `alwayRunCmd` is the user I hope that "trailers"
> > can be added automatically, and other trailers.<token>.command will not be
> > executed automatically. This allows the user to reasonably configure the
> > commands that need to be executed. This must be a very comfortable thing.
>
> I agree that it should be easier and more straightforward, than it is
> now, to configure this.
>
> > But as you said, to disable the automatic addition in the original .command
> > and use the new .alwaysRunCmd, I’m afraid there are a lot of things to consider.
> > Perhaps future series of patches can be considered to do it.
>
> Yeah, support for `trailer.<token>.runMode` might be added in
> different commits at least and possibly later in a different patch
> series. There are the following issues to resolve, though, if we want
> to focus only on a new ".cmd" config option:
>
> - how and when should it run by default,

Do you mean that ".cmd" can get rid of the ".command" auto-add problem
in this patch series?
This might be a good idea if I can add the three modes you mentioned above
in the later patch series.

> - how to explain that in the doc, and maybe
> - how to improve the current description of what happens for ".command"
>
> > > This mechanism is the reason why a trick, when setting up a
> > > 'trailer.foo.command' trailer, is to also set 'trailer.foo.ifexists'
> > > to "replace", so that the first time the command is run (with $ARG
> > > replaced with the empty string) it will add a foo trailer with a
> > > default value, and if it is run another time, because a 'foo=bar'
> > > argument is passed on the command line, then the trailer with the
> > > default value will be replaced by the value computed from running the
> > > command again with $ARG replaced with "bar".
> > >
> > > Another trick is to have the command output nothing when $ARG is the
> > > empty string along with using --trim-empty. This way the command will
> > > create an empty trailer, when it is run the first time, and if it's
> > > not another time, then this empty trailer will be removed because of
> > > --trim-empty.
> > >
> >
> > It looks very practical indeed.
> >
> > > > I cannot quite judge if what we came up with in the above
> > > > description is sufficient.
> > >
> > > I don't think it's sufficient. I think that, while we are at it, a bit
> > > more thinking/discussion is required to make sure we want to keep the
> > > same design as 'trailer.<token>.command'.
> >
> > Sure. I agree that more discussion is needed.
> > I think if the documents that once belonged to .command are copied to .cmd,
> > will the readers be too burdensome to read them? Will it be better to migrate
> > its documentation until we completely delete .command?
>
> My opinion (if we focus only on adding ".cmd") is that:
>
> - for simplicity for now it should run at the same time as ".command",
> the only difference being how the argument is passed (using $1 instead
> of textually replacing $ARG)
> - the doc for ".command" should be first improved if possible, and
> then moved over to ".cmd" saying for ".command" that ".command" is
> deprecated in favor of ".cmd" but otherwise works as ".cmd" except
> that instead using $1 the value is passed by textually replacing $ARG
> which could be a safety and correctness issue.
>

I agree with you. There may be need some discretion.

> Another way to work on all this, would be to first work on adding
> support for `trailer.<token>.runMode` and on improving existing
> documentation, and then to add ".cmd", which could then by default use
> a different ".runMode" than ".command".
>

I think the task can be put off until April.
Deal with the easier ".cmd" first.

> > > > This, too, but until ".command" is removed, wouldn't it be better
> > > > for readers to keep both variants, as the distinction between $ARG
> > > > and $1 needs to be illustrated?
> >
> > So the correct solution should be to keep the original .command Examples,
> > and then give the .cmd examples again.
>
> Maybe we could take advantage of ".cmd" to show other nice
> possibilities to use all of this. Especially if support for `git
> commit --trailer ...` is already merged, we might be able to use it in
> those examples, or perhaps add some examples to the git commit doc.
>

Oh, the 'commit --trailer' may still be queuing, It may take a while.

> Best,
> Christian.

Thanks.

--
ZheNing Hu

gitgitgadget · 2021-03-30T08:51:25Z

On the Git mailing list, Christian Couder wrote (reply to this):

On Mon, Mar 29, 2021 at 3:44 PM ZheNing Hu <adlternative@gmail.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> 于2021年3月29日周一 下午5:05写道：
> >
> > >
> > > Yes, $ARG or $1 are always exist because of:
> > >
> > >                arg = xstrdup("");
> > >
> > > so I think maybe we don't even need this judge in `apply_command`?
> > > +               if (arg)
> > > +                       strvec_push(&cp.args, arg);
> >
> > Yeah, I haven't looked at the code, but that might be a good
> > simplification. If you work on this, please submit it in a separate
> > commit.
>
> Well, if necessary, I'll put it in another commit, maybe I should double check
> to see if there's anything special going on.
>
> > > > Another way to do it would be to have another config option called
> > > > `trailer.<token>.alwaysRunCmd` to tell if the cmd specified by
> > > > `trailer.<token>.cmd` should be run even if no '<token>=<value>'
> > > > argument is passed on the command line. As we are introducing
> > > > `trailer.<token>.cmd`, it's a good time to wonder if this would be a
> > > > better design. But this issue is quite complex, because of the fact
> > > > that 'trailer.<token>.ifMissing' and 'trailer.<token>.ifExists' also
> > > > take a part in deciding if the command will be run.
> >
> > Actually after thinking about it, I think it might be better, instead
> > of `trailer.<token>.alwaysRunCmd`, to add something like
> > `trailer.<token>.runMode` that could take multiple values like:
>
> If really can achieve it is certainly better than 'alwaysRunCmd'.
> The following three small configuration options look delicious.
> But I think it needs to be discussed in more detail:
>
> > - "beforeCLI": would make it run once, like ".command" does now before
> > any CLI trailer are processed
>
> Does "beforeCLI" handle all trailers? Or is it just doing something to add empty
> value trailers?

I am not sure what you mean by "handle all trailers". What I mean is
that it would just work like ".command" does right now before the
"--trailers ..." options are processed.

Let's suppose the "trailer.foo.command" config option is set to "bar".
Then the "bar" command will be run just before the "--trailers ..."
options are processed and the output of that, let's say "baz" will be
used to add a new "foo: baz" trailer to the ouput of `git
interpret-trailers`.

For example:

-------
$ git -c trailer.foo.command='echo baz' interpret-trailers<<EOF
EOF

foo: baz
-------

In other words an empty value trailer is just a special case when the
command that is run does not output anything. But such commands are
expected to output something not trivial at least in some cases.

See also the example in the doc that uses:

$ git config trailer.sign.command 'echo "$(git config user.name)
<$(git config user.email)>"'

> > - "forEachCLIToken": would make it run once for each trailer that has
> > the token, like ".command" also does now, the difference would be that
> > the value for the token would be passed in the $1 argument
>
> This is exactly same as before.

Yeah it is the same as before when the "--trailers ..." options are
processed, but not before that.

To get exactly the same as before one would need to configure both
"beforeCLI" _and_ "forEachCLIToken", for example like this (note that
we use "--add" when adding "forEachCLIToken"):

$ git config trailer.foo.runMode beforeCLI
$ git config --add trailer.foo.runMode forEachCLIToken
$ git config -l | grep foo
trailer.foo.runmode=beforeCLI
trailer.foo.runmode=forEachCLIToken

> > - "afterCLI": would make it run once after all the CLI trailers have
> > been processed and it could pass the different values for the token if
> > any in different arguments: $1, $2, $3, ...
>
> I might get a little confused here: What's the input for $1,$2,$3?

The input would be the different values that are used for the token in
the "--trailer ..." CLI arguments.

For (an hypothetical) example:

------
$ git config trailer.foo.runMode afterCLI
$ git config trailer.foo.cmd 'echo $@'
$ git interpret-trailers --trailer foo=a --trailer foo=b --trailer foo=c<<EOF
EOF

foo: a b c
$ git interpret-trailers<<EOF
EOF

foo:
------

I am not sure "afterCLI" would be useful, but we might not want to
implement it right now. It's just an example to show that we could add
other modes to run the configured ".cmd" (and maybe ".command" too).

> Is users more interested in dealing with trailers value or a line of the
> trailer?

I am not sure what you mean here. If "a line of the trailer" means a
trailer that is already in the input file that is passed to `git
interpret-trailers`, and if "trailers value" means a "--trailer ..."
argument, then I would say that users could be interested in dealing
with both.

It's true that right now the command configured by a ".command" is not
run when `git interpret-trailers` processes in input file that
contains a trailer with the corresponding token. So new values for
".runMode" could be implemented to make that happen.

> > > But as you said, to disable the automatic addition in the original .command
> > > and use the new .alwaysRunCmd, I’m afraid there are a lot of things to consider.
> > > Perhaps future series of patches can be considered to do it.
> >
> > Yeah, support for `trailer.<token>.runMode` might be added in
> > different commits at least and possibly later in a different patch
> > series. There are the following issues to resolve, though, if we want
> > to focus only on a new ".cmd" config option:
> >
> > - how and when should it run by default,
>
> Do you mean that ".cmd" can get rid of the ".command" auto-add problem
> in this patch series?

I am not sure what you mean with "auto-add". Do you mean that fact
that the ".command" runs once before the CLI "--trailer ..." options
are processed?

> This might be a good idea if I can add the three modes you mentioned above
> in the later patch series.

I like that your are interested in improving trailer handling in Git,
but I must say that if you intend to apply for the GSoC, you might
want to work on your application document first, as it will need to be
discussed on the mailing list too and it will take some time. You are
also free to work on this too, but that shouldn't be your priority.

By the way if this (or another Git related) subject is more
interesting to you than the project ideas we propose on
https://git.github.io/SoC-2021-Ideas/, then you are welcome to write a
proposal about working on this (improving trailer handling) rather
than on a project idea from that page. You might want to make sure
that some people would be willing to (co-)mentor you working on it
though.

[...]

> > Another way to work on all this, would be to first work on adding
> > support for `trailer.<token>.runMode` and on improving existing
> > documentation, and then to add ".cmd", which could then by default use
> > a different ".runMode" than ".command".
>
> I think the task can be put off until April.
> Deal with the easier ".cmd" first.

Ok for me, but see above about GSoC application.

> > > > > This, too, but until ".command" is removed, wouldn't it be better
> > > > > for readers to keep both variants, as the distinction between $ARG
> > > > > and $1 needs to be illustrated?
> > >
> > > So the correct solution should be to keep the original .command Examples,
> > > and then give the .cmd examples again.
> >
> > Maybe we could take advantage of ".cmd" to show other nice
> > possibilities to use all of this. Especially if support for `git
> > commit --trailer ...` is already merged, we might be able to use it in
> > those examples, or perhaps add some examples to the git commit doc.
>
> Oh, the 'commit --trailer' may still be queuing, It may take a while.

You might want to check if it needs another reroll or if there are
other reasons (like no reviews) why it's not listed in the last
"What's cooking ..." email from Junio. If you think it is ready and
has been forgotten, you can ping reviewers (including me), to ask them
to review it one more time, or Junio if the last version you sent has
already been reviewed.

gitgitgadget · 2021-03-30T11:26:31Z

On the Git mailing list, ZheNing Hu wrote (reply to this):

Christian Couder <christian.couder@gmail.com> 于2021年3月30日周二 下午4:45写道：
>
> On Mon, Mar 29, 2021 at 3:44 PM ZheNing Hu <adlternative@gmail.com> wrote:
> >
> > Christian Couder <christian.couder@gmail.com> 于2021年3月29日周一 下午5:05写道：
> > >
> > > >
> > > > Yes, $ARG or $1 are always exist because of:
> > > >
> > > >                arg = xstrdup("");
> > > >
> > > > so I think maybe we don't even need this judge in `apply_command`?
> > > > +               if (arg)
> > > > +                       strvec_push(&cp.args, arg);
> > >
> > > Yeah, I haven't looked at the code, but that might be a good
> > > simplification. If you work on this, please submit it in a separate
> > > commit.
> >
> > Well, if necessary, I'll put it in another commit, maybe I should double check
> > to see if there's anything special going on.
> >
> > > > > Another way to do it would be to have another config option called
> > > > > `trailer.<token>.alwaysRunCmd` to tell if the cmd specified by
> > > > > `trailer.<token>.cmd` should be run even if no '<token>=<value>'
> > > > > argument is passed on the command line. As we are introducing
> > > > > `trailer.<token>.cmd`, it's a good time to wonder if this would be a
> > > > > better design. But this issue is quite complex, because of the fact
> > > > > that 'trailer.<token>.ifMissing' and 'trailer.<token>.ifExists' also
> > > > > take a part in deciding if the command will be run.
> > >
> > > Actually after thinking about it, I think it might be better, instead
> > > of `trailer.<token>.alwaysRunCmd`, to add something like
> > > `trailer.<token>.runMode` that could take multiple values like:
> >
> > If really can achieve it is certainly better than 'alwaysRunCmd'.
> > The following three small configuration options look delicious.
> > But I think it needs to be discussed in more detail:
> >
> > > - "beforeCLI": would make it run once, like ".command" does now before
> > > any CLI trailer are processed
> >
> > Does "beforeCLI" handle all trailers? Or is it just doing something to add empty
> > value trailers?
>
> I am not sure what you mean by "handle all trailers". What I mean is
> that it would just work like ".command" does right now before the
> "--trailers ..." options are processed.
>
> Let's suppose the "trailer.foo.command" config option is set to "bar".
> Then the "bar" command will be run just before the "--trailers ..."
> options are processed and the output of that, let's say "baz" will be
> used to add a new "foo: baz" trailer to the ouput of `git
> interpret-trailers`.
>
> For example:
>
> -------
> $ git -c trailer.foo.command='echo baz' interpret-trailers<<EOF
> EOF
>
> foo: baz
> -------
>
> In other words an empty value trailer is just a special case when the
> command that is run does not output anything. But such commands are
> expected to output something not trivial at least in some cases.
>
> See also the example in the doc that uses:
>
> $ git config trailer.sign.command 'echo "$(git config user.name)
> <$(git config user.email)>"'
>

I see what you mean, which is to provide a default value for any
trailers that haven't been run command yet.

> > > - "forEachCLIToken": would make it run once for each trailer that has
> > > the token, like ".command" also does now, the difference would be that
> > > the value for the token would be passed in the $1 argument
> >
> > This is exactly same as before.
>
> Yeah it is the same as before when the "--trailers ..." options are
> processed, but not before that.
>
> To get exactly the same as before one would need to configure both
> "beforeCLI" _and_ "forEachCLIToken", for example like this (note that
> we use "--add" when adding "forEachCLIToken"):
>
> $ git config trailer.foo.runMode beforeCLI
> $ git config --add trailer.foo.runMode forEachCLIToken
> $ git config -l | grep foo
> trailer.foo.runmode=beforeCLI
> trailer.foo.runmode=forEachCLIToken
>
> > > - "afterCLI": would make it run once after all the CLI trailers have
> > > been processed and it could pass the different values for the token if
> > > any in different arguments: $1, $2, $3, ...
> >
> > I might get a little confused here: What's the input for $1,$2,$3?
>
> The input would be the different values that are used for the token in
> the "--trailer ..." CLI arguments.
>
> For (an hypothetical) example:
>
> ------
> $ git config trailer.foo.runMode afterCLI
> $ git config trailer.foo.cmd 'echo $@'
> $ git interpret-trailers --trailer foo=a --trailer foo=b --trailer foo=c<<EOF
> EOF
>
> foo: a b c
> $ git interpret-trailers<<EOF
> EOF
>
> foo:
> ------
>
> I am not sure "afterCLI" would be useful, but we might not want to
> implement it right now. It's just an example to show that we could add
> other modes to run the configured ".cmd" (and maybe ".command" too).
>

Yes, not so useful.

> > Is users more interested in dealing with trailers value or a line of the
> > trailer?
>
> I am not sure what you mean here. If "a line of the trailer" means a
> trailer that is already in the input file that is passed to `git
> interpret-trailers`, and if "trailers value" means a "--trailer ..."
> argument, then I would say that users could be interested in dealing
> with both.
>

Sorry, I mean after we running those command, a line trailer is
"foo: bar" and trailers value will be "bar".

> It's true that right now the command configured by a ".command" is not
> run when `git interpret-trailers` processes in input file that
> contains a trailer with the corresponding token. So new values for
> ".runMode" could be implemented to make that happen.
>

Sure.

> > > > But as you said, to disable the automatic addition in the original .command
> > > > and use the new .alwaysRunCmd, I’m afraid there are a lot of things to consider.
> > > > Perhaps future series of patches can be considered to do it.
> > >
> > > Yeah, support for `trailer.<token>.runMode` might be added in
> > > different commits at least and possibly later in a different patch
> > > series. There are the following issues to resolve, though, if we want
> > > to focus only on a new ".cmd" config option:
> > >
> > > - how and when should it run by default,
> >
> > Do you mean that ".cmd" can get rid of the ".command" auto-add problem
> > in this patch series?
>
> I am not sure what you mean with "auto-add". Do you mean that fact
> that the ".command" runs once before the CLI "--trailer ..." options
> are processed?
>

I'm talking about the empty values $ARG passing to the user's command,
those command  at least run once, You say "how and when should it run by
default", I was wondering if I could not run .cmd without passing trailer.

> > This might be a good idea if I can add the three modes you mentioned above
> > in the later patch series.
>
> I like that your are interested in improving trailer handling in Git,
> but I must say that if you intend to apply for the GSoC, you might
> want to work on your application document first, as it will need to be
> discussed on the mailing list too and it will take some time. You are
> also free to work on this too, but that shouldn't be your priority.
>

In fact, I had written the proposal carefully.
I have been studying what went wrong with OIga's improvement of cat-file
recently.

I may have thought of some ideas, and has been written in Proposal,
I will submit it in about two days :)

> By the way if this (or another Git related) subject is more
> interesting to you than the project ideas we propose on
> https://git.github.io/SoC-2021-Ideas/, then you are welcome to write a
> proposal about working on this (improving trailer handling) rather
> than on a project idea from that page. You might want to make sure
> that some people would be willing to (co-)mentor you working on it
> though.
>

Aha, for the time being, you are the most suitable mentor,
But I might just take improvement of `interpret-tarilers` as my interest to
do something. I will choice the project of "git cat-file" .

> [...]
>
> > > Another way to work on all this, would be to first work on adding
> > > support for `trailer.<token>.runMode` and on improving existing
> > > documentation, and then to add ".cmd", which could then by default use
> > > a different ".runMode" than ".command".
> >
> > I think the task can be put off until April.
> > Deal with the easier ".cmd" first.
>
> Ok for me, but see above about GSoC application.
>
>
> > > > > > This, too, but until ".command" is removed, wouldn't it be better
> > > > > > for readers to keep both variants, as the distinction between $ARG
> > > > > > and $1 needs to be illustrated?
> > > >
> > > > So the correct solution should be to keep the original .command Examples,
> > > > and then give the .cmd examples again.
> > >
> > > Maybe we could take advantage of ".cmd" to show other nice
> > > possibilities to use all of this. Especially if support for `git
> > > commit --trailer ...` is already merged, we might be able to use it in
> > > those examples, or perhaps add some examples to the git commit doc.
> >
> > Oh, the 'commit --trailer' may still be queuing, It may take a while.
>
> You might want to check if it needs another reroll or if there are
> other reasons (like no reviews) why it's not listed in the last
> "What's cooking ..." email from Junio. If you think it is ready and
> has been forgotten, you can ping reviewers (including me), to ask them
> to review it one more time, or Junio if the last version you sent has
> already been reviewed.

It should still be in "seen" inheritance, Junio is advancing it.
Maybe you think it has something to improve, please feel free to tell me.

In addition, I now found a small bug in ".cmd",

git config -l |grep bug
trailer.bug.key=bug-descibe:
trailer.bug.ifexists=replace
trailer.bug.cmd=echo 123

see what will happen:

git interpret-trailers --trailer="bug:text" <<-EOF
`heredocd> EOF

bug-descibe:123 text

"text" seem print to stdout.

I'm looking at what's going on here.

--
ZheNing Hu

gitgitgadget · 2021-03-30T15:11:57Z

On the Git mailing list, ZheNing Hu wrote (reply to this):

Hi, Junio,

ZheNing Hu <adlternative@gmail.com> 于2021年3月30日周二 下午7:22写道：
>
> In addition, I now found a small bug in ".cmd",
>
> git config -l |grep bug
> trailer.bug.key=bug-descibe:
> trailer.bug.ifexists=replace
> trailer.bug.cmd=echo 123
>
> see what will happen:
>
> git interpret-trailers --trailer="bug:text" <<-EOF
> `heredocd> EOF
>
> bug-descibe:123 text
>
> "text" seem print to stdout.
>
> I'm looking at what's going on here.
>

Here I may need to think with you whether it is reasonable to pass "$1".

I found that we passed the parameters in the above situation like this:

(gdb) print cp.args.v[0]
$7 = 0x5555558f4e20 "echo \"123\""
(gdb) print cp.args.v[1]
$8 = 0x5555558ee150 "text"

At this time, our idea is base on that v[0] will be the content of the shell,
and v[1] will be the $1 of the shell.

But in fact, git handles shell subprocesses in a special way:

The `prepare_shell_cmd()` in "run-command.c" seem to use "$@" to pass
shell args.

Before exec:

(gdb) print argv.v[1]
$22 = 0x5555558edfd0 "/bin/sh"
(gdb) print argv.v[2]
$23 = 0x5555558f4c80 "-c"
(gdb) print argv.v[3]
$24 = 0x5555558ed4b0 "echo \"123\" \"$@\""
(gdb) print argv.v[4]
$25 = 0x5555558f5980 "echo \"123\""
(gdb) print argv.v[5]
$26 = 0x5555558edab0 "abc"
(gdb) print argv.v[6]
$27 = 0x0

Some unexpected things happened here.
Maybe "abc" was wrongly used as the parameter of "echo"?
Looking forward to your reply.

--
ZheNing Hu

gitgitgadget · 2021-03-30T17:17:42Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

ZheNing Hu <adlternative@gmail.com> writes:

> The `prepare_shell_cmd()` in "run-command.c" seem to use "$@" to pass
> shell args.

Yes. "$@" is a way to write "$1" "$2" "$3"...
Since you are passing only one, 

	echo "$@"

and

	echo "$1"

would be the equivalent.

I am not sure what program you fed to the gdb (and remote debugging
over e-mail is not my forte ;-), but let's see.

> Before exec:
>
> (gdb) print argv.v[1]
> $22 = 0x5555558edfd0 "/bin/sh"
> (gdb) print argv.v[2]
> $23 = 0x5555558f4c80 "-c"
> (gdb) print argv.v[3]
> $24 = 0x5555558ed4b0 "echo \"123\" \"$@\""
> (gdb) print argv.v[4]
> $25 = 0x5555558f5980 "echo \"123\""
> (gdb) print argv.v[5]
> $26 = 0x5555558edab0 "abc"
> (gdb) print argv.v[6]
> $27 = 0x0
>
> Some unexpected things happened here.
> Maybe "abc" was wrongly used as the parameter of "echo"?
> Looking forward to your reply.

Observe

	$ sh -c '
		echo "\$0 == $0"
		count=0
		for arg in "$@"
		do
			count=$(( $count + 1 ))
			echo "\$$count == $arg"
		done
	' 0 1 2
	$0 == 0
	$1 == 1
	$2 == 2

i.e. the first arg after

	argv[1] = "/bin/sh"
        argv[2] = "-c"
	argv[3] = "script"

is used to give the script the name of the program ($0).  Are we
getting hit by this common confusion?

It is customery to write such an invocation with '-' as the "name of
the program" thing, so that ordinary positional parameters are
available starting at $1, not $0, like so:

	sh -c 'script' - arg1 arg2 ...

gitgitgadget · 2021-03-31T05:19:27Z

On the Git mailing list, ZheNing Hu wrote (reply to this):

Junio C Hamano <gitster@pobox.com> 于2021年3月31日周三 上午1:14写道：
>
> ZheNing Hu <adlternative@gmail.com> writes:
>
> > The `prepare_shell_cmd()` in "run-command.c" seem to use "$@" to pass
> > shell args.
>
> Yes. "$@" is a way to write "$1" "$2" "$3"...
> Since you are passing only one,
>
>         echo "$@"
>
> and
>
>         echo "$1"
>
> would be the equivalent.
>
> I am not sure what program you fed to the gdb (and remote debugging
> over e-mail is not my forte ;-), but let's see.
>



> > Before exec:
> >
> > (gdb) print argv.v[1]
> > $22 = 0x5555558edfd0 "/bin/sh"
> > (gdb) print argv.v[2]
> > $23 = 0x5555558f4c80 "-c"
> > (gdb) print argv.v[3]
> > $24 = 0x5555558ed4b0 "echo \"123\" \"$@\""
> > (gdb) print argv.v[4]
> > $25 = 0x5555558f5980 "echo \"123\""
> > (gdb) print argv.v[5]
> > $26 = 0x5555558edab0 "abc"
> > (gdb) print argv.v[6]
> > $27 = 0x0
> >
> > Some unexpected things happened here.
> > Maybe "abc" was wrongly used as the parameter of "echo"?
> > Looking forward to your reply.
>
> Observe
>
>         $ sh -c '
>                 echo "\$0 == $0"
>                 count=0
>                 for arg in "$@"
>                 do
>                         count=$(( $count + 1 ))
>                         echo "\$$count == $arg"
>                 done
>         ' 0 1 2
>         $0 == 0
>         $1 == 1
>         $2 == 2
>
> i.e. the first arg after
>
>         argv[1] = "/bin/sh"
>         argv[2] = "-c"
>         argv[3] = "script"
>
> is used to give the script the name of the program ($0).  Are we
> getting hit by this common confusion?
>
> It is customery to write such an invocation with '-' as the "name of
> the program" thing, so that ordinary positional parameters are
> available starting at $1, not $0, like so:
>
>         sh -c 'script' - arg1 arg2 ...

The configuration is like this:
trailer.bug.key=BUG:
trailer.bug.ifexists=add
trailer.bug.cmd=echo "123"

And use:

$ git interpret-trailers --trailer="bug:456" --trailer="bug:789"<<-EOF
EOF

BUG: 123
BUG: 123 456
BUG: 123 789

I just want three "BUG: 123", but "456" and "789" appeared...

In fact, I think about this problem like this way:
When we execute a child process that runs the shell,
the function`prepare_shell_cmd()` will actively add "$@" to the end of our
shell command when we have more than zero args ,

e.g.

"echo \"123\"" "abc"

will turn to

 "echo \"123\" \"$@\"" "echo \"123\"" "abc"

Normally, $@ should not cause any problems because it passes arguments
to the script what we provide.

But now, what we actually want is take any $1 that appears in the script as an
argument, the automatically added $@ causes $1 to be implicitly included.
And the original $ARG does not have this problem, Or if we pass environment
variables, this kind of problem will not occur.

Or If we want to avoid this problem, should we add one new options in
`struct child_process` , such as: "shell_no_implicit_args" , let git not add
 extra "$@" before we run the shell script?

Thanks.

--
ZheNing Hu

adlternative · 2021-03-31T10:04:29Z

/submit

gitgitgadget · 2021-03-31T10:06:06Z

Submitted as pull.913.v5.git.1617185147.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-913/adlternative/trailer-pass-ARG-env-v5

To fetch this version to local tag pr-913/adlternative/trailer-pass-ARG-env-v5:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-913/adlternative/trailer-pass-ARG-env-v5

gitgitgadget · 2021-04-21T17:56:27Z

This patch series was integrated into seen via git@82873a4.

gitgitgadget · 2021-04-21T23:42:32Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

ZheNing Hu <adlternative@gmail.com> writes:

> I admit that your idea makes sense, but we actually have another requirement:
> Construct a trailer with an empty value.

It can be done with a different script given to .cmd, which would
say "exit 0" when allowing an empty string given as its input to
appear in the output.

I was reacting what the "count" example does, and found that
counting commits by all authors (that is what an empty string would
match when given to --author="") somewhat illogical in the context
of that example.

After all, these examples are to pique readers' interest by
demonstrating what the mechanism can do and how it can be used, and
for this feature, I think showing that

 (1) we can squelch the output from unasked-for execution;

 (2) we can reject --trailer=<key>:<value> when we do not like the
     given <value>;

 (3) we can insert the trailer with the value we compute (and it is
     OK for the computed result happens to be an empty string).

should be plenty sufficient.

gitgitgadget · 2021-04-22T09:24:02Z

On the Git mailing list, ZheNing Hu wrote (reply to this):

Junio C Hamano <gitster@pobox.com> 于2021年4月22日周四 上午7:40写道：
>
> ZheNing Hu <adlternative@gmail.com> writes:
>
> > I admit that your idea makes sense, but we actually have another requirement:
> > Construct a trailer with an empty value.
>
> It can be done with a different script given to .cmd, which would
> say "exit 0" when allowing an empty string given as its input to
> appear in the output.
>

Now I think that we should keep those trailers which ask for a
"name <email>" pair, like "Helped-by", "Signed-off-by", when we
provide a "help:","sign:" in command line, This allows the user to
dynamically fill in the "name <email>" pair of other people in the
commit message later. It is worthwhile for users to exit with exit(0).

But those dispensable things like "Commit-Count", It must depend
on a person's statistics in the git repository. So "cnt:" is meaningless,
users' script can let it exit(1).

> I was reacting what the "count" example does, and found that
> counting commits by all authors (that is what an empty string would
> match when given to --author="") somewhat illogical in the context
> of that example.
>

The "Commit-Count" in the example here can only target a specific person,
which has great limitations.

I have a bold idea:

Our current --trailer can only fill in one data item, and we don't
expect it to fill
in multiple rows. something like "Commit-Count", we hope to count the number of
commits from everyone. But as we can see:

Commit-count: 7 Linus Arver
  1117  Linus Torvalds

So If we have the opportunity to capture every line or every "block" of content,
and feed it to git interpret-trailer, maybe we can get something like:

Commit-count: 7 Linus Arver
Commit-count: 1117  Linus Torvalds

This will definitely make it easy for us to generate a lot of trailer at once.
For example, a newbie like me, after making a patch, want to --cc all authors
of one file, maybe I only need a command to get it.

I don't know if it's a bit whimsical.

> After all, these examples are to pique readers' interest by
> demonstrating what the mechanism can do and how it can be used, and
> for this feature, I think showing that
>
>  (1) we can squelch the output from unasked-for execution;
>
>  (2) we can reject --trailer=<key>:<value> when we do not like the
>      given <value>;
>
>  (3) we can insert the trailer with the value we compute (and it is
>      OK for the computed result happens to be an empty string).
>
> should be plenty sufficient.

OK. I will add these three examples in the new patch (when .cmd merge to
master).

Thanks.
--
ZheNing Hu

gitgitgadget · 2021-04-27T06:53:31Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

ZheNing Hu <adlternative@gmail.com> writes:

> Now I think that we should keep those trailers which ask for a
> "name <email>" pair, like "Helped-by", "Signed-off-by", when we
> provide a "help:","sign:" in command line, This allows the user to
> dynamically fill in the "name <email>" pair of other people in the
> commit message later. It is worthwhile for users to exit with exit(0).
>
> But those dispensable things like "Commit-Count", It must depend
> on a person's statistics in the git repository. So "cnt:" is meaningless,
> users' script can let it exit(1).

Perhaps, but at this point what you think (or what I think) does not
matter.  That was the whole point of letting .cmd script signal Git
if the result from the invocation should be kept or discarded with
its exit status.  What would be sufficient here for us to do is to
agree that it would be good to have a minimal set (perhaps a pair)
of examples to demonstrate that the script can choose to keep or
discard a meaningless trailer entry with its exit status.

gitgitgadget · 2021-04-27T12:27:04Z

On the Git mailing list, ZheNing Hu wrote (reply to this):

Junio C Hamano <gitster@pobox.com> 于2021年4月27日周二 下午2:49写道：
>
> ZheNing Hu <adlternative@gmail.com> writes:
>
> > Now I think that we should keep those trailers which ask for a
> > "name <email>" pair, like "Helped-by", "Signed-off-by", when we
> > provide a "help:","sign:" in command line, This allows the user to
> > dynamically fill in the "name <email>" pair of other people in the
> > commit message later. It is worthwhile for users to exit with exit(0).
> >
> > But those dispensable things like "Commit-Count", It must depend
> > on a person's statistics in the git repository. So "cnt:" is meaningless,
> > users' script can let it exit(1).
>
> Perhaps, but at this point what you think (or what I think) does not
> matter.  That was the whole point of letting .cmd script signal Git
> if the result from the invocation should be kept or discarded with
> its exit status.  What would be sufficient here for us to do is to
> agree that it would be good to have a minimal set (perhaps a pair)
> of examples to demonstrate that the script can choose to keep or
> discard a meaningless trailer entry with its exit status.

Yes, I argee.
Due to previous attempts, it seems that such an example is well given:
"Commit-Count" is the trailer that should be discarded.
"Signed-off-by" is the trailer worth be kept.

Thanks.
--
ZheNing Hu

gitgitgadget · 2021-04-28T06:17:09Z

This patch series was integrated into seen via git@433bac6.

gitgitgadget · 2021-04-29T01:01:28Z

This patch series was integrated into seen via git@78d3714.

gitgitgadget · 2021-04-29T08:37:49Z

This patch series was integrated into seen via git@85b8c80.

gitgitgadget · 2021-04-29T09:19:37Z

There was a status update in the "Cooking" section about the branch zh/trailer-cmd on the Git mailing list:

The way the command line specified by the trailer.<token>.command
configuration variable receives the end-user supplied value was
both error prone and misleading.  An alternative to achieve the
same goal in a safer and more intuitive way has been added, as
the trailer.<token>.cmd configuration variable, to replace it.

Expecting a reroll, after waiting for "commit --trailer" to stabilize.

gitgitgadget · 2021-04-30T04:32:42Z

This patch series was integrated into seen via git@c4405a8.

gitgitgadget · 2021-04-30T06:33:33Z

This patch series was integrated into seen via git@85c91f3.

gitgitgadget · 2021-05-03T06:44:06Z

This patch series was integrated into seen via git@dadb038.

The `trailer.<token>.command` configuration variable specifies a command (run via the shell, so it does not have to be a single name or path to the command, but can be a shell script), and the first occurrence of substring $ARG is replaced with the value given to the `interpret-trailer` command for the token in a '--trailer <token>=<value>' argument. This has three downsides: * 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). * The first occurrence of substring `$ARG` will be replaced with the empty string, in the command when the command is first called to add a trailer with the specified <token>. This is a bad design, the nature of automatic execution causes it to add a trailer that we don't expect. 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. At the same time, in order to allow `git interpret-trailers` to better simulate the behavior of `git command -s`, 'trailer.<token>.cmd' will not automatically execute. Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: Christian Couder <christian.couder@gmail.com> Signed-off-by: ZheNing Hu <adlternative@gmail.com>

adlternative · 2021-05-03T15:38:47Z

/submit

gitgitgadget · 2021-05-03T15:41:23Z

Submitted as pull.913.v12.git.1620056466.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-913/adlternative/trailer-pass-ARG-env-v12

To fetch this version to local tag pr-913/adlternative/trailer-pass-ARG-env-v12:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-913/adlternative/trailer-pass-ARG-env-v12

gitgitgadget · 2021-05-04T05:04:38Z

This patch series was integrated into seen via git@86320fa.

gitgitgadget · 2021-05-04T05:40:01Z

This patch series was integrated into next via git@fb67787.

gitgitgadget · 2021-05-04T06:27:29Z

This patch series was integrated into seen via git@46756f2.

gitgitgadget · 2021-05-06T05:48:14Z

This patch series was integrated into seen via git@e95853c.

gitgitgadget · 2021-05-06T06:02:44Z

There was a status update in the "Cooking" section about the branch zh/trailer-cmd on the Git mailing list:

The way the command line specified by the trailer.<token>.command
configuration variable receives the end-user supplied value was
both error prone and misleading.  An alternative to achieve the
same goal in a safer and more intuitive way has been added, as
the trailer.<token>.cmd configuration variable, to replace it.

Will merge to 'master'.

gitgitgadget · 2021-05-07T05:00:58Z

This patch series was integrated into seen via git@d3aaa64.

gitgitgadget · 2021-05-11T06:11:20Z

This patch series was integrated into seen via git@691d621.

gitgitgadget · 2021-05-12T05:36:24Z

This patch series was integrated into seen via git@2cd6ce2.

gitgitgadget · 2021-05-12T05:36:25Z

This patch series was integrated into next via git@2cd6ce2.

gitgitgadget · 2021-05-12T05:36:26Z

This patch series was integrated into master via git@2cd6ce2.

gitgitgadget · 2021-05-12T05:36:28Z

Closed via 2cd6ce2.

adlternative force-pushed the trailer-pass-ARG-env branch from abc5b04 to 185356d Compare March 24, 2021 14:59

adlternative changed the title ~~[GSOC] trailer: change $ARG to environment variable~~ [GSOC]trailer: pass arg as positional parameter Mar 24, 2021

adlternative force-pushed the trailer-pass-ARG-env branch from 185356d to b268ecd Compare March 25, 2021 10:00

adlternative force-pushed the trailer-pass-ARG-env branch from b268ecd to e2bbdcb Compare March 26, 2021 14:47

adlternative force-pushed the trailer-pass-ARG-env branch 2 times, most recently from 3033552 to 5894d8c Compare March 31, 2021 07:52

adlternative force-pushed the trailer-pass-ARG-env branch from 9c0fc91 to edb7f19 Compare May 3, 2021 14:26

gitgitgadget bot added the next label May 4, 2021

gitgitgadget bot added the master label May 12, 2021

gitgitgadget bot closed this May 12, 2021

[GSOC] trailer: add new .cmd config option #913

[GSOC] trailer: add new .cmd config option #913

Conversation

adlternative commented Mar 23, 2021 • edited

adlternative commented Mar 23, 2021

gitgitgadget bot commented Mar 23, 2021

adlternative commented Mar 24, 2021

gitgitgadget bot commented Mar 24, 2021

gitgitgadget bot commented Mar 24, 2021

gitgitgadget bot commented Mar 25, 2021

adlternative commented Mar 25, 2021

gitgitgadget bot commented Mar 25, 2021

gitgitgadget bot commented Mar 25, 2021

gitgitgadget bot commented Mar 26, 2021

adlternative commented Mar 26, 2021

gitgitgadget bot commented Mar 26, 2021

gitgitgadget bot commented Mar 27, 2021

gitgitgadget bot commented Mar 27, 2021

gitgitgadget bot commented Mar 28, 2021

gitgitgadget bot commented Mar 29, 2021

gitgitgadget bot commented Mar 29, 2021

gitgitgadget bot commented Mar 30, 2021

gitgitgadget bot commented Mar 30, 2021

gitgitgadget bot commented Mar 30, 2021

gitgitgadget bot commented Mar 30, 2021

gitgitgadget bot commented Mar 31, 2021

adlternative commented Mar 31, 2021

gitgitgadget bot commented Mar 31, 2021

gitgitgadget bot commented Apr 21, 2021

gitgitgadget bot commented Apr 21, 2021

gitgitgadget bot commented Apr 22, 2021

gitgitgadget bot commented Apr 27, 2021

gitgitgadget bot commented Apr 27, 2021

gitgitgadget bot commented Apr 28, 2021

gitgitgadget bot commented Apr 29, 2021

gitgitgadget bot commented Apr 29, 2021

gitgitgadget bot commented Apr 29, 2021

gitgitgadget bot commented Apr 30, 2021

gitgitgadget bot commented Apr 30, 2021

gitgitgadget bot commented May 3, 2021

adlternative commented May 3, 2021

gitgitgadget bot commented May 3, 2021

gitgitgadget bot commented May 4, 2021

gitgitgadget bot commented May 4, 2021

gitgitgadget bot commented May 4, 2021

gitgitgadget bot commented May 6, 2021

gitgitgadget bot commented May 6, 2021

gitgitgadget bot commented May 7, 2021

gitgitgadget bot commented May 11, 2021

gitgitgadget bot commented May 12, 2021

gitgitgadget bot commented May 12, 2021

gitgitgadget bot commented May 12, 2021

gitgitgadget bot commented May 12, 2021

adlternative commented Mar 23, 2021 •

edited