trailer: change $ARG to environment variable

From: ZheNing Hu <[email protected]>

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.

1: abc5b04d152f ! 1: 185356d6fc90 [GSOC]trailer: change $ARG to environment variable
@@ Metadata
Author: ZheNing Hu <[email protected]>

 Documentation/git-interpret-trailers.txt |  7 +++++++
 t/t7513-interpret-trailers.sh            | 22 +++++++++++++++++++-
 trailer.c                                | 26 +++++++++++++++++-------
 3 files changed, 47 insertions(+), 8 deletions(-)

Documentation/git-interpret-trailers.txt +0 -0

index 96ec6499f001..38656b1b3841 100644

Quoted 16 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

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.

Quoted 2 lines

Original trailer.<token>.command can still be used until git completely abandoned it.

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

Quoted 3 lines

1: abc5b04d152f ! 1: 185356d6fc90 [GSOC]trailer: change $ARG to environment variable
@@ Metadata
Author: ZheNing Hu <[email protected]>

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

Quoted 13 lines

 Documentation/git-interpret-trailers.txt |  7 +++++++
 t/t7513-interpret-trailers.sh            | 22 +++++++++++++++++++-
 trailer.c                                | 26 +++++++++++++++++-------
 3 files changed, 47 insertions(+), 8 deletions(-)

Documentation/git-interpret-trailers.txt +0 -0

index 96ec6499f001..38656b1b3841 100644
@@ -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.
 

Documentation/git-interpret-trailers.txt also be executed for each of these arguments. And the <value> part of · line 258

Quoted 4 lines

Documentation/git-interpret-trailers.txt +4 -0

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

Documentation/git-interpret-trailers.txt also be executed for each of these arguments. And the <value> part of · line 260

Quoted 2 lines

Documentation/git-interpret-trailers.txt +2 -0

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

trailer.c struct conf_info { · line 17

Quoted 10 lines

trailer.c +1 -0

index be4e9726421c..80f47657ff1a 100644
@@ -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?

From: ZheNing Hu <[email protected]>

Original implementation of trailer.<token>.command use

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

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

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

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, 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 <[email protected]>

trailer.c +4 -6

index be4e9726421c..634d3f1ff04a 100644
@@ -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_) {

trailer.c +1 -1

@@ -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;

Quoted 3 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

Original implementation of trailer.<token>.command use

uses

Quoted 2 lines

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

has

Quoted 1 line

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

replaces the $ARG only once.

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 8 lines

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 <[email protected]>

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?

Documentation/git-interpret-trailers.txt also be executed for each of these arguments. And the <value> part of · line 259

Quoted 6 lines

Documentation/git-interpret-trailers.txt +5 -0

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

Quoted 1 line

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

Documentation/git-interpret-trailers.txt also be executed for each of these arguments. And the <value> part of · line 263

Quoted 3 lines

Documentation/git-interpret-trailers.txt +3 -0

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

Quoted 38 lines

trailer.c +4 -6

index be4e9726421c..634d3f1ff04a 100644
@@ -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_) {

trailer.c static int check_if_different(struct trailer_item *in_tok, · line 238

Quoted 10 lines

trailer.c +10 -0

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

Quoted 10 lines

trailer.c +1 -1

@@ -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;

trailer.c static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg · line 268

Quoted 5 lines

trailer.c +1 -1

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

Documentation/git-interpret-trailers.txt +1 -1

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

Quoted 4 lines · ZheNing Hu via GitGitGadget

Documentation/git-interpret-trailers.txt +0 -0

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

Documentation/git-interpret-trailers.txt also be executed for each of these arguments. And the <value> part of · line 261

Quoted 7 lines

Documentation/git-interpret-trailers.txt +7 -0

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

Documentation/git-interpret-trailers.txt also be executed for each of these arguments. And the <value> part of · line 263

Quoted 2 lines

Documentation/git-interpret-trailers.txt +2 -0

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

Quoted 10 lines

Documentation/git-interpret-trailers.txt +1 -2

-* 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 <[email protected]>' --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)>"'

Documentation/git-interpret-trailers.txt $ git interpret-trailers --trailer 'Cc: Alice <[email protected]>' --trailer 'Re · line 324

Quoted 3 lines

Documentation/git-interpret-trailers.txt +1 -0

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

Quoted 6 lines

Documentation/git-interpret-trailers.txt +1 -1

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

Documentation/git-interpret-trailers.txt subject · line 355

Quoted 10 lines

Documentation/git-interpret-trailers.txt +1 -1

   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.

Quoted 7 lines · Junio C Hamano

Quoted 4 lines · ZheNing Hu via GitGitGadget

Documentation/git-interpret-trailers.txt +0 -0

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

Quoted 7 lines

Documentation/git-interpret-trailers.txt also be executed for each of these arguments. And the <value> part of · line 261

Quoted 7 lines

Documentation/git-interpret-trailers.txt +7 -0

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

Quoted 9 lines

Documentation/git-interpret-trailers.txt also be executed for each of these arguments. And the <value> part of · line 263

Quoted 2 lines

Documentation/git-interpret-trailers.txt +2 -0

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

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 2 lines

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

Quoted 11 lines

Quoted 11 lines

Documentation/git-interpret-trailers.txt +1 -2

-* 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 <[email protected]>' --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)>"'

Quoted 8 lines

Documentation/git-interpret-trailers.txt $ git interpret-trailers --trailer 'Cc: Alice <[email protected]>' --trailer 'Re · line 324

Quoted 3 lines

Documentation/git-interpret-trailers.txt +1 -0

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

Quoted 5 lines

Quoted 5 lines

Documentation/git-interpret-trailers.txt +0 -0

@@ -333,14 +343,14 @@ subject
 Fix #42
 ------------
 

Quoted 8 lines

Quoted 8 lines

Documentation/git-interpret-trailers.txt +1 -1

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

Quoted 8 lines

Documentation/git-interpret-trailers.txt subject · line 355

Quoted 4 lines

Documentation/git-interpret-trailers.txt +1 -1

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

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 8 lines

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.

Quoted 10 lines · Christian Couder <[email protected]> 于2021年3月28日周日 上午3:53写道：

Quoted 6 lines · Junio C Hamano

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

Quoted 6 lines

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.

Quoted 12 lines

Quoted 5 lines

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

Quoted 5 lines

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?

Quoted 8 lines

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.

Quoted 9 lines

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

Quoted 6 lines

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.

Quoted 3 lines

Quoted 2 lines

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

Quoted 3 lines

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?

Quoted 3 lines

Quoted 3 lines

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.

Quoted 6 lines

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

               arg = xstrdup("");

trailer.c static int check_if_different(struct trailer_item *in_tok, · line 232

Quoted 3 lines

so I think maybe we don't even need this judge in apply_command?

trailer.c +2 -0

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

Quoted 8 lines

Quoted 8 lines

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.

Quoted 6 lines

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.

Quoted 3 lines

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"

Quoted 21 lines

Quoted 9 lines

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

Quoted 6 lines

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.

Quoted 3 lines

Quoted 2 lines

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

Quoted 8 lines

Quoted 3 lines

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

Quoted 4 lines

Quoted 3 lines

Quoted 3 lines

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?

Quoted 2 lines

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.

Quoted 6 lines · Christian Couder <[email protected]> 于2021年3月29日周一 下午5:05写道：

Quoted 6 lines

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

               arg = xstrdup("");

Quoted 7 lines

trailer.c static int check_if_different(struct trailer_item *in_tok, · line 232

Quoted 3 lines

so I think maybe we don't even need this judge in apply_command?

trailer.c +2 -0

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

Quoted 9 lines

Quoted 8 lines

Quoted 8 lines

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.

Quoted 3 lines

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:

Quoted 2 lines

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

Quoted 3 lines

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

Quoted 3 lines

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

Quoted 9 lines

Quoted 3 lines

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:

Quoted 1 line

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

Quoted 4 lines

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.

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 36 lines · ZheNing Hu

Quoted 6 lines · Christian Couder <[email protected]> 于2021年3月29日周一 下午5:05写道：

Quoted 6 lines

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

               arg = xstrdup("");

Quoted 7 lines

Review

Quoted 3 lines

so I think maybe we don't even need this judge in apply_command?

(quoted diff) +2 -0

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

Quoted 9 lines

Quoted 8 lines

Quoted 8 lines

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.

Quoted 3 lines

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:

Quoted 5 lines

Quoted 2 lines

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

Quoted 5 lines

Quoted 3 lines

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

Quoted 5 lines

Quoted 3 lines

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

Quoted 2 lines

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.

Quoted 9 lines

Quoted 9 lines

Quoted 3 lines

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:

Quoted 4 lines

Quoted 1 line

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

Quoted 2 lines

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.

[...]

Quoted 7 lines

Quoted 4 lines

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.

Quoted 12 lines

Quoted 11 lines

Quoted 6 lines

Quoted 3 lines

Quoted 3 lines

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.

Quoted 1 line

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.

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.

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.

Quoted 2 lines · ZheNing Hu

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.

Quoted 15 lines

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

Quoted 3 lines

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

Quoted 5 lines · ZheNing Hu

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

Quoted 8 lines

And use:

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

BUG: 123
BUG: 123 456
BUG: 123 789

I think that is quite expected. You said the command to run is 'echo 123', and that is not "pick a directory $D on $PATH where there is an executable '$D/echo 123' exists, and run that". It runs the given command with the shell, and in general that is what we want for end-user supplied commands specified in the configuration file [*1*].
...
*1* Imagine .editor set to 'emacs -nw' or 'vim -f'; we do not want
Git to find a directory on $PATH that has an executable whose name is 'emacs -nw' and run that file (i.e. give 'emacs -nw' as the first argument to execlp()). Instead, you'd want to behave as if the user typed "emacs -nw", followed by any arguments we want to give to it (in .editor's case, the name of the file to be edited) properly quoted for the shell.

And the way we do so is to form a moral equivalent of

execlp("sh", "-c", "emacs -nw \"$@\"", ...);

and put the arguments at the end where I wrote ... (we actually do so with execvp(), but illustrating with execlp() is easier to read and write---hence "a moral equivalent of").

Quoted 31 lines · Junio C Hamano

Quoted 5 lines · ZheNing Hu

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

Quoted 8 lines

And use:

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

BUG: 123
BUG: 123 456
BUG: 123 789

I think that is quite expected. You said the command to run is 'echo 123', and that is not "pick a directory $D on $PATH where there is an executable '$D/echo 123' exists, and run that". It runs the given command with the shell, and in general that is what we want for end-user supplied commands specified in the configuration file [*1*].
...
*1* Imagine .editor set to 'emacs -nw' or 'vim -f'; we do not want
Git to find a directory on $PATH that has an executable whose name is 'emacs -nw' and run that file (i.e. give 'emacs -nw' as the first argument to execlp()). Instead, you'd want to behave as if the user typed "emacs -nw", followed by any arguments we want to give to it (in .editor's case, the name of the file to be edited) properly quoted for the shell.

Quoted 7 lines

And the way we do so is to form a moral equivalent of

execlp("sh", "-c", "emacs -nw \"$@\"", ...);

and put the arguments at the end where I wrote ... (we actually do so with execvp(), but illustrating with execlp() is easier to read and write---hence "a moral equivalent of").

So, learning from that .editor example, what you can do when you do not want to take any parameter is to explicitly ignore them.

Let's take the very basic form first. Imagine you wrote a little script and wanted to see three "123", ignoring end-user input after "--trailer=bug:".

    .cmd = my-script 123

would run 'my-script "$@"'. What should you write in my-script to cause that happen? Here is an example solution:

    #!/bin/sh
    echo 123

Notice that "$1" is completely ignored, even if the machinery that drives .cmd makes three calls?

        sh -c 'my-script 123 "$@"'
        sh -c 'my-script 123 "$@"' 456
        sh -c 'my-script 123 "$@"' 789

The way to do the same without an extra script on disk is for you to use sh-c yourself.

    .cmd = sh -c 'echo 123'

And if you do want to use $1, you can do the same. E.g. if you want to double them in the output, you'd probably do something like this:

    .cmd = sh -c 'echo "<$1 - $1>"'

You'd need to quote the value appropriately for the config file, though.

Quoted 4 lines

The way to do the same without an extra script on disk is for you to use sh-c yourself.

    .cmd = sh -c 'echo 123'

This is indeed a viable solution, But the extra "sh -c" seems to put an unnecessary burden on the user.

Sometimes I wonder, why not recommend using environment variables like $ARG?

Quoted 7 lines · ZheNing Hu

Quoted 4 lines

The way to do the same without an extra script on disk is for you to use sh-c yourself.

    .cmd = sh -c 'echo 123'

This is indeed a viable solution, But the extra "sh -c" seems to put an unnecessary burden on the user.

Nobody forces you to write long script in the configuration file. In fact, the "find author from history" is so useful that I'd think people have an alias or a script in ~/bin/ already for their own interactive use. E.g.

    $ cat ~/bin/git-who
    #!/bin/sh
    git log -1 --format="%an <%ae>" --author="$1"
    $ cat ~/bin/git-one
    #!/bin/sh
    git show -s --pretty=reference "$1"

and with them:

trailer.key.cmd = git who

that is internally wrapped into

        sh -c 'git who "$@"'

and fed "gitster@" as the first parameter when "--trailer=key:gitster@" is given would work just fine.

Quoted 2 lines

Sometimes I wonder, why not recommend using environment variables like $ARG?

I am also fine with that; when we discovered the design flaw of .command, I think I suggested either would make a viable choice. The only downside is that it would squat on a good name $ARG and forbids end-users from using the symbol for other purpose, but as long as the application is limited in scope, that would be fine.

Quoted 5 lines · ZheNing Hu

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

Quoted 8 lines

And use:

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

BUG: 123
BUG: 123 456
BUG: 123 789

I think that is quite expected. You said the command to run is 'echo 123', and that is not "pick a directory $D on $PATH where there is an executable '$D/echo 123' exists, and run that". It runs the given command with the shell, and in general that is what we want for end-user supplied commands specified in the configuration file [*1*].

So we form a shell command whose beginning is 'echo 123' and tuck the argument after that command line, so it is understandable that "echo 123 456" gets executed for "--trailer=bug:456".

I wasn't following the discussion between you and Christian closely but I recall seeing him saying that the command is executed one extra time without any arg before it is run for actual --trailer requests with the value? I am guessing that is where the first output "BUG: 123" (without anything else) is coming from.

*1* Imagine .editor set to 'emacs -nw' or 'vim -f'; we do not want
Git to find a directory on $PATH that has an executable whose name is 'emacs -nw' and run that file (i.e. give 'emacs -nw' as the first argument to execlp()). Instead, you'd want to behave as if the user typed "emacs -nw", followed by any arguments we want to give to it (in .editor's case, the name of the file to be edited) properly quoted for the shell.

And the way we do so is to form a moral equivalent of

execlp("sh", "-c", "emacs -nw $@", ...);

and put the arguments at the end where I wrote ... (we actually do so with execvp(), but illustrating with execlp() is easier to read and write---hence "a moral equivalent of").

From: ZheNing Hu <[email protected]>

When we use subprocess to run a shell-script, if we have any

args, git will add extra $@ to the end of the shell-script, This can pass positional parameters correctly, But if we just want to use some of these passed parameters, git will still add an extra "$@", which contains all positional parameters we passed. This does not meet our expectations.

E.g. our shell-script is:
"echo \"\$1\""
and pass $1 "abc",

git will change our script to:
"echo \"\$1\" \"$@\""

The positional parameters we entered will be printed repeatedly.

So let add a new shell_no_implicit_args

to struct child_process, which can suppress the joining of $@ if shell_no_implicit_args is set to 1, this will allow us to use only few of positional args in multi-parameter shell script, instead of using all of them.

Quoted 4 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

When we use subprocess to run a shell-script, if we have any

Maybe: s/subprocess/a subprocess/

Quoted 5 lines

args, git will add extra $@ to the end of the shell-script, This can pass positional parameters correctly, But if we just want to use some of these passed parameters, git will still add an extra "$@", which contains all positional parameters we passed. This does not meet our expectations.

I am not sure explaining things using $@ is the best way to make this as clear as possible. I don't have a clear alternative right now though.

Quoted 3 lines

E.g. our shell-script is:
"echo \"\$1\""
and pass $1 "abc",

Maybe: s/pass $1 "abc"/we pass "abc" as $1/

Quoted 2 lines

git will change our script to:
"echo \"\$1\" \"$@\""

Where will "abc" appear then?

Quoted 2 lines

The positional parameters we entered will be printed repeatedly.

If you take us passing "abc" in $1 as an example, then I think it's a good idea to show us the result of that.

Quoted 1 line

So let add a new shell_no_implicit_args

Maybe: s/shell_no_implicit_args/shell_no_implicit_args flag/

Quoted 5 lines

to struct child_process, which can suppress the joining of $@ if shell_no_implicit_args is set to 1, this will allow us to use only few of positional args in multi-parameter shell script, instead of using all of them.

I think our goal is more to have each argument we pass be passed just once.

In https://lore.kernel.org/git/[email protected]/ Junio and Christian talked about the problem of using strbuf_replace() to replace $ARG.

Now pass trailer value as $1 to the trailer command with another trailer.<token>.cmd config.

ZheNing Hu (2):
[GSOC] run-command: add shell_no_implicit_args option [GSOC]trailer: pass arg as positional parameter

Quoted 4 lines · ZheNing Hu via GitGitGadget

In https://lore.kernel.org/git/[email protected]/ Junio and Christian talked about the problem of using strbuf_replace() to replace $ARG.

It's better to sum up a bit the discussions. It's nice to provide a link to the discussions though.

Quoted 2 lines

Now pass trailer value as $1 to the trailer command with another trailer.<token>.cmd config.

If this patch series introduces a new trailer.<token>.cmd config option, then I would expect one of the patch in the series to have a subject like "trailer: add new trailer.<token>.cmd config option".

Quoted 3 lines

ZheNing Hu (2):
[GSOC] run-command: add shell_no_implicit_args option [GSOC]trailer: pass arg as positional parameter

I guess the "trailer: pass arg as positional parameter" is the one introducing the new trailer.<token>.cmd config option.

Also it seems strange that there is no space between "[GSOC]" and "trailer".

quote (imagine a name like "O'Connor", substituted into NAME='$ARG' to make it NAME='O'Connor), it would result in

 Documentation/git-interpret-trailers.txt | 82 +++++++++++++++++---
 t/t7513-interpret-trailers.sh            | 95 +++++++++++++++++++++++-
 trailer.c                                | 38 +++++++---
 3 files changed, 193 insertions(+), 22 deletions(-)

Documentation/git-interpret-trailers.txt +3 -5

index 96ec6499f001..67649ec6134c 100644
@@ -236,21 +236,34 @@ trailer.<token>.command::
 	be called to automatically add or modify a trailer with the
 	specified <token>.
 +
-When this option is specified, the behavior is as if a special
-'<token>=<value>' argument were added at the beginning of the command
-line, where <value> is taken to be the standard output of the
-specified command with any leading and trailing whitespace trimmed
-off.
+When this option is specified, the first occurrence of substring $ARG is
+replaced with the value given to the `interpret-trailer` command for the
+same token.
 +

Documentation/git-interpret-trailers.txt +24 -2

-these arguments, if any, will be used to replace the `$ARG` string in
-the command.
+these arguments, if any, will be passed to the command as first parameter.
 
 EXAMPLES
 --------

@@ -333,6 +346,55 @@ subject
 Fix #42
 ------------
 
+* Configure a 'see' trailer with a cmd use a global script `git-one`
+  to show the subject of a commit that is related, and show how it works:
++
+------------
+$ cat ~/bin/git-one
+#!/bin/sh
+git show -s --pretty=reference "$1"
+$ git config trailer.see.key "See-also: "
+$ git config trailer.see.ifExists "replace"
+$ git config trailer.see.ifMissing "doNothing"
+$ git config trailer.see.cmd "~/bin/git-one"
+$ git interpret-trailers <<EOF
+> subject
+> 
+> message
+> 
+> see: HEAD~2
+> EOF
+subject
+
+message
+
+See-also: fe3187e (subject of related commit, 2021-4-2)

Documentation/git-interpret-trailers.txt +16 -0

+$ git config trailer.help.key "Helped-by: "
+$ git config trailer.help.ifExists "replace"
+$ git config trailer.help.cmd "~/bin/git-who"
+$ git interpret-trailers --trailer="help:gitster@" <<EOF
+> subject
+> 
+> message
+> 
+> EOF
+subject
+
+message
+
+Helped-by: Junio C Hamano <[email protected]>
+------------
+
 * Configure a 'see' trailer with a command to show the subject of a
   commit that is related, and show how it works:
 +

t/t7513-interpret-trailers.sh +0 -0

index 6602790b5f4c..923923e57573 100755

trailer.c +1 -0

index be4e9726421c..6aeff6a1bd33 100644
...
+static char *apply_command(struct conf_info *conf, const char *arg)
 {
 	struct strbuf cmd = STRBUF_INIT;
 	struct strbuf buf = STRBUF_INIT;

Documentation/git-interpret-trailers.txt trailer.<token>.command:: · line 261

Quoted 3 lines

Documentation/git-interpret-trailers.txt +3 -0

+the behavior is as if a special '<token>=<value>' argument were added at
+the beginning of the command, <value> will be passed to the user's
+command as an empty value.

Do the two occurrences of the word "command" in the sentence refer to different things? I do not think this is an existing problem inherited from the original, but as we are trying to improve the description, I wonder if we can clarify them a bit.

... as if a '<token>=<value>' argument were added at the beginning of the "git interpret-trailers" command, the command specified by this configuration variable will be called with an empty string as the argument.

is my attempt, but I am not still sure what that "as if" part is trying to say. Does it mean with

[trailer "Foo"] cmd = foo-cmd

and the 'input-file' does not have "Foo: <some existing value>" trailer in it, the command "git interpret-trailers input-file" would behave as if this command was run

        $ Foo= git interpret-trailers input-file

(as there is no <value>, I am not sure what <value> is used when <token>=<value> is prefixed to the command)?

Puzzled and confused utterly am I... Help, Christian?

Quoted 4 lines

+
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. And the <value> part of

This talks about 'trailer.<token>.command'. Should this be changed to '.cmd'?

Or does everything after "When this option is specified, if there is no trailer with ..." apply to both the old .command and new .cmd? If so, that was not clear at all---we'd need to clarify this part.

Quoted 40 lines

Documentation/git-interpret-trailers.txt +31 -2

-these arguments, if any, will be used to replace the `$ARG` string in
-the command.
+these arguments, if any, will be passed to the command as first parameter.
 
 EXAMPLES
 --------

@@ -333,6 +346,55 @@ subject
 Fix #42
 ------------
 
+* Configure a 'see' trailer with a cmd use a global script `git-one`
+  to show the subject of a commit that is related, and show how it works:
++
+------------
+$ cat ~/bin/git-one
+#!/bin/sh
+git show -s --pretty=reference "$1"
+$ git config trailer.see.key "See-also: "
+$ git config trailer.see.ifExists "replace"
+$ git config trailer.see.ifMissing "doNothing"
+$ git config trailer.see.cmd "~/bin/git-one"
+$ git interpret-trailers <<EOF
+> subject
+>
+> message
+>
+> see: HEAD~2
+> EOF
+subject
+
+message
+
+See-also: fe3187e (subject of related commit, 2021-4-2)
+------------
+
+* Configure a 'who' trailer with a cmd use a global script `git-who`
+  to find the recent matching "author <mail>" pair in git log and
+  show how it works:
++
+------------

Documentation/git-interpret-trailers.txt subject · line 381

Quoted 3 lines

Documentation/git-interpret-trailers.txt +3 -0

+$ cat ~/bin/git-who
+ #!/bin/sh
+    git log -1 --format="%an <%ae>" --author="$1"

Unusual indentation here. But more importantly, I am not sure if having both 'see' and 'help' examples is worth it---they are similar enough that the second one does not teach anything new to those who studied the first one already, aren't they?

Quoted 27 lines

Documentation/git-interpret-trailers.txt +16 -0

+$ git config trailer.help.key "Helped-by: "
+$ git config trailer.help.ifExists "replace"
+$ git config trailer.help.cmd "~/bin/git-who"
+$ git interpret-trailers --trailer="help:gitster@" <<EOF
+> subject
+>
+> message
+>
+> EOF
+subject
+
+message
+
+Helped-by: Junio C Hamano <[email protected]>
+------------
+
 * Configure a 'see' trailer with a command to show the subject of a
   commit that is related, and show how it works:
 +

t/t7513-interpret-trailers.sh +0 -0

index 6602790b5f4c..923923e57573 100755

t/t7513-interpret-trailers.sh test_expect_success 'setup' ' · line 57

Quoted 7 lines

t/t7513-interpret-trailers.sh +4 -0

@@ -51,6 +51,77 @@ test_expect_success 'setup' '
      EOF
 '
 
+test_expect_success 'with cmd' '
+     test_when_finished "git config --unset trailer.bug.key && \
+                         git config --unset trailer.bug.ifExists && \
+                         git config --unset trailer.bug.cmd" &&

It is unwise to use && between these three "git config" invocations, I suspect. "git config --unset" exits with non-zero status when you attempt to remove with an non-existent key, but you would remove the .ifExists and .cmd even if .key is not defined. Perhaps

        test_when_finished "git config --unset-all trailer.bug.key
                            git config --unset-all trailer.bug.ifExists
                            git config --unset-all trailer.bug.cmd" &&

would be more sensible. Or if we just want to remove everything under the trailer.bug.* section, this might be even better:

        test_when_finished "git config --remove-section trailer.bug" &&

as we can add new trailer.bug.* to the system and use them in this test, but removing the entire section would still be a good way to clean after ourselves.

Quoted 9 lines

trailer.c +1 -0

index be4e9726421c..6aeff6a1bd33 100644
...
+static char *apply_command(struct conf_info *conf, const char *arg)
 {
      struct strbuf cmd = STRBUF_INIT;
      struct strbuf buf = STRBUF_INIT;

trailer.c static int check_if_different(struct trailer_item *in_tok, · line 229

Quoted 10 lines

trailer.c +2 -5

      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 (conf->cmd) {
+             // cp.shell_no_implicit_args = 1;

Do not add new code that is commented out. Besides we do not use // comment.

trailer.c static int check_if_different(struct trailer_item *in_tok, · line 233

Quoted 4 lines

trailer.c +4 -0

+             strbuf_addstr(&cmd, conf->cmd);
+             strvec_push(&cp.args, cmd.buf);
+             if (arg)
+                     strvec_push(&cp.args, arg);

Thanks.

Quoted 7 lines

Quoted 3 lines

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. And the <value> part of

This talks about 'trailer.<token>.command'. Should this be changed to '.cmd'?

Quoted 4 lines

Or does everything after "When this option is specified, if there is no trailer with ..." apply to both the old .command and new .cmd? If so, that was not clear at all---we'd need to clarify this part.

Because ".command" will be eliminated, may be only leave those description Information to ".cmd" is better.

Quoted 5 lines

Unusual indentation here. But more importantly, I am not sure if having both 'see' and 'help' examples is worth it---they are similar enough that the second one does not teach anything new to those who studied the first one already, aren't they?

Ok, I will think about other examples.

Quoted 11 lines

trailer.c static int check_if_different(struct trailer_item *in_tok, · line 229

Quoted 8 lines

trailer.c +2 -1

index be4e9726421c..6aeff6a1bd33 100644
...
-     strvec_push(&cp.args, cmd.buf);
+     if (conf->cmd) {
+             // cp.shell_no_implicit_args = 1;

Do not add new code that is commented out. Besides we do not use // comment.

Quoted 6 lines

trailer.c static int check_if_different(struct trailer_item *in_tok, · line 233

Quoted 4 lines

trailer.c +4 -0

+             strbuf_addstr(&cmd, conf->cmd);
+             strvec_push(&cp.args, cmd.buf);
+             if (arg)
+                     strvec_push(&cp.args, arg);

Thanks.

Thanks.

Documentation/git-interpret-trailers.txt trailer.<token>.command:: · line 253

Quoted 3 lines · ZheNing Hu via GitGitGadget

Documentation/git-interpret-trailers.txt +3 -0

+trailer.<token>.cmd::
+     The command specified by this configuration variable is run
+     with a single parameter, which is the <value> part of an

Quoted 1 line

+ existing trailer with the same <token>.

Documentation/git-interpret-trailers.txt trailer.<token>.command:: · line 258

Quoted 5 lines

The output from the

Documentation/git-interpret-trailers.txt +4 -0

+     command is then used as the value for the <token> in the
+     resulting trailer.
++
+When this option is specified, If there is no trailer with same <token>,

s/If/if/ (downcase).

Documentation/git-interpret-trailers.txt trailer.<token>.command:: · line 261

Quoted 3 lines

Documentation/git-interpret-trailers.txt +3 -0

+the behavior is as if a special '<token>=<value>' argument were added at
+the beginning of the command, <value> will be passed to the user's
+command as an empty value.

Do the two occurrences of the word "command" in the sentence refer to different things? I do not think this is an existing problem inherited from the original, but as we are trying to improve the description, I wonder if we can clarify them a bit.

... as if a '<token>=<value>' argument were added at the beginning of the "git interpret-trailers" command, the command specified by this configuration variable will be called with an empty string as the argument.

is my attempt,

but I am not still sure what that "as if" part is
trying to say. Does it mean with

[trailer "Foo"] cmd = foo-cmd

and the 'input-file' does not have "Foo: <some existing value>" trailer in it, the command "git interpret-trailers input-file" would behave as if this command was run

        $ Foo= git interpret-trailers input-file

(as there is no <value>, I am not sure what <value> is used when <token>=<value> is prefixed to the command)?

Puzzled and confused utterly am I... Help, Christian?

Quoted 3 lines

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. And the <value> part of

This talks about 'trailer.<token>.command'. Should this be changed to '.cmd'?

Or does everything after "When this option is specified, if there is no trailer with ..." apply to both the old .command and new .cmd? If so, that was not clear at all---we'd need to clarify this part.

Documentation/git-interpret-trailers.txt trailer.<token>.command:: · line 266

Quoted 3 lines

Documentation/git-interpret-trailers.txt +1 -2

-these arguments, if any, will be used to replace the `$ARG` string in
-the command.
+these arguments, if any, will be passed to the command as first parameter.

Quoted 7 lines · Junio C Hamano

...
... as if a '<token>=<value>' argument were added at the beginning of the "git interpret-trailers" command, the command specified by this configuration variable will be called with an empty string as the argument.

is my attempt,

It looks better to me.

Quoted 9 lines

but I am not still sure what that "as if" part is
trying to say. Does it mean with

[trailer "Foo"] cmd = foo-cmd

and the 'input-file' does not have "Foo: <some existing value>" trailer in it, the command "git interpret-trailers input-file" would behave as if this command was run

Quoted 1 line

        $ Foo= git interpret-trailers input-file

I would say it would behave as if:

$ git interpret-trailers --trailer Foo= input-file

Quoted 20 lines · Christian Couder

Quoted 7 lines · Junio C Hamano

...
... as if a '<token>=<value>' argument were added at the beginning of the "git interpret-trailers" command, the command specified by this configuration variable will be called with an empty string as the argument.

is my attempt,

It looks better to me.

Quoted 9 lines

but I am not still sure what that "as if" part is
trying to say. Does it mean with

[trailer "Foo"] cmd = foo-cmd

and the 'input-file' does not have "Foo: <some existing value>" trailer in it, the command "git interpret-trailers input-file" would behave as if this command was run

Quoted 5 lines

Quoted 1 line

        $ Foo= git interpret-trailers input-file

I would say it would behave as if:

$ git interpret-trailers --trailer Foo= input-file

Hmmm. That means that the descrition in the original is quite misleading, no?

If it said

... as if "--trailer" "<token>=<value>" arguments were given to "git interpret-trailers" command near the beginning of its command line

then that may be closer description of the command line you are forming, but as its written (with or without my attempt to clarify above), it was impossible to infer that you are behaving as if another --trailer option (with <token>=<value> as its value) was given.

Quoted 9 lines · Junio C Hamano

Quoted 3 lines · Christian Couder

I would say it would behave as if:

$ git interpret-trailers --trailer Foo= input-file

Hmmm. That means that the descrition in the original is quite misleading, no?

Yeah, I agree it is misleading and difficult to understand.

Quoted 6 lines

If it said

... as if "--trailer" "<token>=<value>" arguments were given to "git interpret-trailers" command near the beginning of its command line

Quoted 5 lines

then that may be closer description of the command line you are forming, but as its written (with or without my attempt to clarify above), it was impossible to infer that you are behaving as if another --trailer option (with <token>=<value> as its value) was given.

I agree.

Quoted 26 lines · Christian Couder

Quoted 9 lines · Junio C Hamano

Quoted 3 lines · Christian Couder

I would say it would behave as if:

$ git interpret-trailers --trailer Foo= input-file

Hmmm. That means that the descrition in the original is quite misleading, no?

Yeah, I agree it is misleading and difficult to understand.

Quoted 11 lines

If it said

... as if "--trailer" "<token>=<value>" arguments were given to "git interpret-trailers" command near the beginning of its command line

then that may be closer description of the command line you are forming, but as its written (with or without my attempt to clarify above), it was impossible to infer that you are behaving as if another --trailer option (with <token>=<value> as its value) was given.

Quoted 1 line

I agree.

By the way it might be better to first have a patch that clarifies the existing documentation of ".command", before the patch that adds ".cmd". This way the first patch that only clarifies the documentation could be backported to maintenance branches of old releases.

I find in the status update description:
"Seems to break new tests for .command variant in zh/commit-trailer" I have tryed to merge my local branch 1e9a657(trailer-cmd) and 2daae3d(upstream/zh/commit-trailer)

Quoted 4 lines

By the way it might be better to first have a patch that clarifies the existing documentation of ".command", before the patch that adds ".cmd". This way the first patch that only clarifies the documentation could be backported to maintenance branches of old releases.

This first patch should explain errors of ".command", and then explain that we will no longer use it, right?

Quoted 27 lines

+------------
+$ cat ~/bin/git-one

Documentation/git-interpret-trailers.txt +25 -0

+#!/bin/sh
+git show -s --pretty=reference "$1"
+$ git config trailer.see.key "See-also: "
+$ git config trailer.see.ifExists "replace"
+$ git config trailer.see.ifMissing "doNothing"
+$ git config trailer.see.cmd "~/bin/git-one"
+$ git interpret-trailers <<EOF
+> subject
+>
+> message
+>
+> see: HEAD~2
+> EOF
+subject
+
+message
+
+See-also: fe3187e (subject of related commit, 2021-4-2)
+------------
+
+* Configure a 'who' trailer with a cmd use a global script `git-who`
+  to find the recent matching "author <mail>" pair in git log and
+  show how it works:
++
+------------

Quoted 3 lines

+$ cat ~/bin/git-who
+ #!/bin/sh
+ git log -1 --format="%an <%ae>" --author="$1"

Unusual indentation here. But more importantly, I am not sure if having both 'see' and 'help' examples is worth it---they are similar enough that the second one does not teach anything new to those who studied the first one already, aren't they?

Quoted 19 lines

+$ git config trailer.help.key "Helped-by: "
+$ git config trailer.help.ifExists "replace"
+$ git config trailer.help.cmd "~/bin/git-who" +$ git interpret-trailers --trailer="help:gitster@" <<EOF +> subject
+>
+> message
+>
+> EOF

Documentation/git-interpret-trailers.txt +7 -0

+subject
+
+message
+
+Helped-by: Junio C Hamano <[email protected]>
+------------
+
 * Configure a 'see' trailer with a command to show the subject of a
   commit that is related, and show how it works:
 +

This may be an off-topic question:
I wanted to use shortlog -s instead of the document example, But I found a very strange place:
Here we have two shell scripts:

$ cat ~/bin/gcount
#!/bin/sh
if test "$1" != ""

then
git log -1 --author="$1"
else
echo "hello there"
fi

cat ~/bin/gcount2
#!/bin/sh
if test "$1" != ""
then
git shortlog -1 --author="$1"
else
echo "hello there"
fi

If I use them in the terminal, there is no problem with them,

$ ~/bin/gcount gitster
commit 142430338477d9d1bb25be66267225fb58498d92
(interpret-trailers-command-arg, abc5b)
Author: Junio C Hamano <[email protected]>
Date:   Mon Mar 22 14:00:00 2021 -0700

The second batch

Signed-off-by Junio C Hamano <[email protected]>

$ ~/bin/gcount2 gitster
Junio C Hamano (1):
      The second batch

if I use .cmd to run these scripts, the situation is totally different:

$ git config -l | grep trailer
trailer.cnt.ifexists=add
trailer.cnt.key=Cnt:
trailer.cnt.cmd=~/bin/gcount

$ git interpret-trailers --trailer="cnt:gitster" <<EOF
EOF

Cnt: hello there
Cnt: commit 142430338477d9d1bb25be66267225fb58498d92
Author: Junio C Hamano <[email protected]>
Date: Mon Mar 22 14:00:00 2021 -0700

The second batch

Signed-off-by Junio C Hamano <[email protected]>

And if I turn to use gcount2:

$ git config trailer.cnt.cmd "~/bin/gcount2"
$ git interpret-trailers --trailer="cnt:gitster" <<EOF
EOF

Cnt: hello there
Cnt:

It looks like shortlog does not write to standard output.

Note that in short_log.c, log will be output to log->file. Does it make the above behavior different?
Is there a good solution?

Quoted 8 lines · ZheNing Hu

This may be an off-topic question:
I wanted to use shortlog -s instead of the document example, But I found a very strange place:
Here we have two shell scripts:

$ cat ~/bin/gcount
#!/bin/sh
if test "$1" != ""

It's better to use test -n "$1" instead of test "$1" != "".

Quoted 31 lines

then
git log -1 --author="$1"
else
echo "hello there"
fi

cat ~/bin/gcount2
#!/bin/sh
if test "$1" != ""
then
git shortlog -1 --author="$1"
else
echo "hello there"
fi

If I use them in the terminal, there is no problem with them,

$ ~/bin/gcount gitster
commit 142430338477d9d1bb25be66267225fb58498d92
(interpret-trailers-command-arg, abc5b)
Author: Junio C Hamano <[email protected]>
Date:   Mon Mar 22 14:00:00 2021 -0700

The second batch

Signed-off-by Junio C Hamano <[email protected]>

$ ~/bin/gcount2 gitster
Junio C Hamano (1):
      The second batch

Quoted 6 lines

if I use .cmd to run these scripts, the situation is totally different:

$ git config -l | grep trailer
trailer.cnt.ifexists=add
trailer.cnt.key=Cnt:
trailer.cnt.cmd=~/bin/gcount

The script/command used in ".cmd" or ".command" should really return a short string with no newline or line feed char in it. Here your script can return multiple lines which could mess things up and be difficult to understand.

Quoted 13 lines

$ git interpret-trailers --trailer="cnt:gitster" <<EOF
EOF

Cnt: hello there
Cnt: commit 142430338477d9d1bb25be66267225fb58498d92
Author: Junio C Hamano <[email protected]>
Date: Mon Mar 22 14:00:00 2021 -0700

The second batch

Signed-off-by Junio C Hamano <[email protected]>

And if I turn to use gcount2:

Quoted 8 lines

$ git config trailer.cnt.cmd "~/bin/gcount2"
$ git interpret-trailers --trailer="cnt:gitster" <<EOF
EOF

Cnt: hello there
Cnt:

It looks like shortlog does not write to standard output.

In shortlog's doc there is:

"If no revisions are passed on the command line and either standard input is not a terminal or there is no current branch, git shortlog will output a summary of the log read from standard input, without reference to the current repository."

I guess that's what's happening here.

Quoted 3 lines

Note that in short_log.c, log will be output to log->file. Does it make the above behavior different?
Is there a good solution?

I would try to pass a revision on the command line.

From: ZheNing Hu <[email protected]>

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

 Documentation/git-interpret-trailers.txt | 82 +++++++++++++++++---
 t/t7513-interpret-trailers.sh            | 95 +++++++++++++++++++++++-
 trailer.c                                | 38 +++++++---
 3 files changed, 193 insertions(+), 22 deletions(-)