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

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

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.

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.

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.

On Sat, Mar 27, 2021 at 7:04 PM Junio C Hamano [off-list ref] wrote:

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

quoted

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

quoted

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

quoted

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

quoted

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

Christian Couder [off-list ref] 于2021年3月28日周日 上午3:53写道：

On Sat, Mar 27, 2021 at 7:04 PM Junio C Hamano [off-list ref] wrote:

quoted

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

quoted

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

quoted

quoted

+     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

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.

quoted

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.

quoted

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?

quoted

quoted

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

quoted

quoted

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

quoted

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

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.

quoted

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

On Sun, Mar 28, 2021 at 12:46 PM ZheNing Hu [off-list ref] wrote:

Christian Couder [off-list ref] 于2021年3月28日周日 上午3:53写道：

quoted

On Sat, Mar 27, 2021 at 7:04 PM Junio C Hamano [off-list ref] wrote:

quoted

quoted

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.

quoted

quoted

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.

quoted

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"

quoted

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.

quoted

quoted

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

quoted

quoted

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.

Christian Couder [off-list ref] 于2021年3月29日周一 下午5:05写道：

quoted

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.

quoted

quoted

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.

quoted

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

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"

quoted

quoted

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.

quoted

quoted

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.

quoted

quoted

quoted

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

On Mon, Mar 29, 2021 at 3:44 PM ZheNing Hu [off-list ref] wrote:

Christian Couder [off-list ref] 于2021年3月29日周一 下午5:05写道：

quoted

quoted

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.

quoted

quoted

quoted

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:

quoted

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

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

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

quoted

quoted

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.

[...]

quoted

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

quoted

quoted

quoted

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.

Christian Couder [off-list ref] 于2021年3月30日周二 下午4:45写道：

On Mon, Mar 29, 2021 at 3:44 PM ZheNing Hu [off-list ref] wrote:

quoted

Christian Couder [off-list ref] 于2021年3月29日周一 下午5:05写道：

quoted

quoted

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.

quoted

quoted

quoted

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:

quoted

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

quoted

quoted

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

quoted

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

quoted

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.

quoted

quoted

quoted

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.

quoted

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

[...]

quoted

quoted

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

quoted

quoted

quoted

quoted

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

Hi, Junio,

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