Christian Couder <christian.couder@xxxxxxxxx> 于2021年4月7日周三 上午12:23写道： > > On Sun, Apr 4, 2021 at 3:11 PM ZheNing Hu via GitGitGadget > <gitgitgadget@xxxxxxxxx> wrote: > > > > From: ZheNing Hu <adlternative@xxxxxxxxx> > > > > 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: > > > > * 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 > > s/unmatching/unmatched/ > > > 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). > > Good explanation of the issues with ".command"! > Credit for Junio. > > 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 > > s/a parameter/an argument/ > s/the users will/users can/ > > > refer to the value as positional argument, $1, in their > > scripts. > > > > Helped-by: Junio C Hamano <gitster@xxxxxxxxx> > > Helped-by: Christian Couder <christian.couder@xxxxxxxxx> > > Signed-off-by: ZheNing Hu <adlternative@xxxxxxxxx> > > --- > > [GSOC] trailer: add new trailer..cmd config option > > Maybe <token> has been removed from "trailer.<token>.cmd" above > because it has been interpreted as an HTML or XML tag? > Aha, It may well be so. Maybe change to "[GSOC] trailer add .cmd config option"? > > In https://lore.kernel.org/git/xmqqv99i4ck2.fsf@gitster.g/ Junio and > > Christian talked about the problem of using strbuf_replace() to replace > > $ARG: > > > > 1. if user's script have more than one $ARG, only the first one will be > > s/user's script have/the user's script has/ > > > replaced, which is incorrected. > > 2. $ARG is textually replaced without shell syntax, which may result a > > broken command when $ARG include some unmatching single quote, very > > unsafe. > > Yeah, good summary of the issues with ".command" > > > Now pass trailer value as $1 to the trailer command with another > > trailer.<token>.cmd config, to solve these above two problems, > > > > We are now writing documents that are more readable and correct than > > before. > > Yeah, correcting the doc is a good thing to do. By the way, as I said > to Junio, it might be better to make the doc for ".command" more > readable and correct in a first patch separate from the patch > introducing ".cmd". > > If you really want to do both in the same patch you should tell that > in the commit message too, not just here after the "---" line. > I actually tried to write it yesterday, but... diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt index 96ec6499f0..39f742b3dc 100644 --- a/Documentation/git-interpret-trailers.txt +++ b/Documentation/git-interpret-trailers.txt @@ -237,20 +237,20 @@ trailer.<token>.command:: 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. +'<token>=<value>' argument were added at the beginning of the +"git interpret-trailers" command, where <value> is taken to be the +standard output of the specified command with any leading and +trailing whitespace trimmed off. + -If the command contains the `$ARG` string, this string will be -replaced with the <value> part of an existing trailer with the same -<token>, if any, before the command is launched. +The first occurrence of substring $ARG will be replaced with the +<value> part of an existing trailer with the same <token>, if any, +before the command is launched. + 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 -these arguments, if any, will be used to replace the `$ARG` string in -the command. +line, when a 'trailer.<token>.cmd' is configured, the command is run +once for each `--trailer <token>=<value>` on the command line with the +same <token>. And the <value> part of these arguments, if any, will be +used to replace the fist $ARG string in the command. Since I am based on the document in the second patch, you have also uncovered some points worthy of modification below, so temporarily what I wrote is not accurate enough. > > Documentation/git-interpret-trailers.txt | 86 +++++++++++++++++++---- > > t/t7513-interpret-trailers.sh | 87 +++++++++++++++++++++++- > > trailer.c | 37 +++++++--- > > 3 files changed, 186 insertions(+), 24 deletions(-) > > > > diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt > > index 96ec6499f001..83600e93390d 100644 > > --- a/Documentation/git-interpret-trailers.txt > > +++ b/Documentation/git-interpret-trailers.txt > > @@ -236,21 +236,36 @@ 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. This option behaves in a similar way as ".cmd", however, it > > +passes the value through $ARG. > > Maybe this last sentence could be replaced with "Otherwise this option > behaves in the same way as 'trailer.<token>.cmd'." > > > -If the command contains the `$ARG` string, this string will be > > -replaced with the <value> part of an existing trailer with the same > > -<token>, if any, before the command is launched. > > +".command" has been deprecated due to the $ARG in the user's command can > > s/".command"/The 'trailer.<token>.command' option/ > s/to the $ARG/to the fact that `$ARG`/ > > > +only be replaced once and the original way of replacing $ARG was not safe. > > s/only be/can only be/ > s/and the/and that the/ > > > +Now the preferred option is using "trailer.<token>.cmd", which use position > > s/using// > s/use/uses/ > s/position/a positional/ > > Also please make sure that trailer.<token>.cmd and > trailer.<token>.command are always quoted in the same way. I think > single quotes are used in the current doc, so please keep using single > quotes. > > > +argument to pass the value. > > ++ > > +When both .cmd and .command are given for the same <token>, > > +.cmd is used and .command is ignored. > > Please spell and quote ".cmd" and ".command" consistently, so for > example like: 'trailer.<token>.cmd' > OK. > > +trailer.<token>.cmd:: > > + The command specified by this configuration variable is run > > + with a single argument, which is the <value> part of a > > + `--trailer <token>=<value>` on the command line. The output > > + from the command is then used as the value for the <token> > > + in the resulting trailer. > > ++ > > +When this option is specified, the behavior is as if a '<token>=<value>' > > s/'<token>=<value>'/`--trailer <token>=<value>`/ (let's try to be as > explicit as possible) > > > +argument were added at the beginning of the "git interpret-trailers" > > s/were/was/ > > > +command, the command specified by this configuration variable will be > > +called with an empty string as the argument. > > + > > 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 > > -these arguments, if any, will be used to replace the `$ARG` string in > > -the command. > > +line, when a 'trailer.<token>.cmd' is configured, the command is run > > +once for each `--trailer <token>=<value>` on the command line with the > > +same <token>. And the <value> part of these arguments, if any, will be > > +passed to the command as its first argument. > > Yeah, it's much better than it was, but I think we can do better. I > will try to come up with something soon. > > Also as I said above and in reply to Junio, I think it might be better > to split this in 2 patches. > > > EXAMPLES > > -------- > > @@ -333,6 +348,53 @@ subject > > Fix #42 > > ------------ > > > > +* Configure a 'cnt' trailer with a cmd use a global script `gcount` > > +to record commit counts of a specified author and show how it works: > > ++ > > +------------ > > +$ cat ~/bin/gcount > > +#!/bin/sh > > +test -n "$1" && git shortlog -s --author="$1" HEAD || true > > +$ git config trailer.cnt.key "Commit-count: " > > +$ git config trailer.cnt.ifExists "replace" > > +$ git config trailer.cnt.cmd "~/bin/gcount" > > +$ git interpret-trailers --trailer="cnt:Junio" <<EOF > > +> subject > > +> > > +> message > > +> > > +> EOF > > +subject > > + > > +message > > + > > +Commit-count: 22484 Junio C Hamano > > +------------ > > + > > +* Configure a 'ref' trailer with a cmd use a global script `glog-grep` > > + to grep last relevant commit from git log in the git repository > > + and show how it works: > > ++ > > +------------ > > +$ cat ~/bin/glog-grep > > +#!/bin/sh > > +test -n "$1" && git log --grep "$1" --pretty=reference -1 || true > > +$ git config trailer.ref.key "Reference-to: " > > +$ git config trailer.ref.ifExists "replace" > > +$ git config trailer.ref.cmd "~/bin/glog-grep" > > +$ git interpret-trailers --trailer="ref:Add copyright notices." <<EOF > > +> subject > > +> > > +> message > > +> > > +> EOF > > +subject > > + > > +message > > + > > +Reference-to: 8bc9a0c769 (Add copyright notices., 2005-04-07) > > The added examples look good! > Thanks. > > +test_expect_success 'with cmd' ' > > + test_when_finished "git config --remove-section trailer.bug" && > > + git config trailer.bug.key "Bug-maker: " && > > + git config trailer.bug.ifExists "add" && > > + git config trailer.bug.cmd "echo \"maybe is\"" && > > + cat >expected2 <<-EOF && > > + > > + Bug-maker: maybe is > > + Bug-maker: maybe is him > > + Bug-maker: maybe is me > > + EOF > > + git interpret-trailers --trailer "bug: him" --trailer "bug:me" \ > > + >actual2 && > > + test_cmp expected2 actual2 > > +' > > I guess this shows that the command is called multiple times, the > first time with an empty first arg. > > > +test_expect_success 'with cmd and $1' ' > > + test_when_finished "git config --remove-section trailer.bug" && > > + git config trailer.bug.key "Bug-maker: " && > > + git config trailer.bug.ifExists "replace" && > > + git config trailer.bug.cmd "echo \"\$1\" is" && > > + cat >expected2 <<-EOF && > > + > > + Bug-maker: me is me > > + EOF > > + git interpret-trailers --trailer "bug: him" --trailer "bug:me" \ > > + >actual2 && > > + test_cmp expected2 actual2 > > +' > > I guess this shows that the argument is also available as "$1". > > > +test_expect_success 'with cmd and $1 with sh -c' ' > > + test_when_finished "git config --remove-section trailer.bug" && > > + git config trailer.bug.key "Bug-maker: " && > > + git config trailer.bug.ifExists "replace" && > > + git config trailer.bug.cmd "sh -c \"echo who is \"\$1\"\"" && > > + cat >expected2 <<-EOF && > > + > > + Bug-maker: who is me > > + EOF > > + git interpret-trailers --trailer "bug: him" --trailer "bug:me" \ > > + >actual2 && > > + test_cmp expected2 actual2 > > +' > > Ok, this shows how `sh -c ...` can be used in ".cmd". > > > +test_expect_success 'with cmd and $1 with shell script' ' > > + test_when_finished "git config --remove-section trailer.bug" && > > + git config trailer.bug.key "Bug-maker: " && > > + git config trailer.bug.ifExists "replace" && > > + git config trailer.bug.cmd "./echoscript" && > > + cat >expected2 <<-EOF && > > + > > + Bug-maker: who is me > > + EOF > > + cat >echoscript <<-EOF && > > + #!/bin/sh > > + echo who is "\$1" > > + EOF > > + chmod +x echoscript && > > + git interpret-trailers --trailer "bug: him" --trailer "bug:me" \ > > + >actual2 && > > + test_cmp expected2 actual2 > > +' > > Ok. > > > test_expect_success 'without config' ' > > sed -e "s/ Z\$/ /" >expected <<-\EOF && > > > > @@ -1274,9 +1337,31 @@ test_expect_success 'setup a commit' ' > > git commit -m "Add file a.txt" > > ' > > > > +test_expect_success 'cmd takes precedence over command' ' > > + test_when_finished "git config --unset trailer.fix.cmd" && > > + git config trailer.fix.ifExists "replace" && > > + git config trailer.fix.cmd "test -n \"\$1\" && git log -1 --oneline --format=\"%h (%aN)\" \ > > + --abbrev-commit --abbrev=14 \"\$1\" || true" && > > + git config trailer.fix.command "git log -1 --oneline --format=\"%h (%s)\" \ > > + --abbrev-commit --abbrev=14 \$ARG" && > > + FIXED=$(git log -1 --oneline --format="%h (%aN)" --abbrev-commit --abbrev=14 HEAD) && > > + cat complex_message_body >expected2 && > > + sed -e "s/ Z\$/ /" >>expected2 <<-EOF && > > + Fixes: $FIXED > > + Acked-by= Z > > + Reviewed-by: > > + Signed-off-by: Z > > + Signed-off-by: A U Thor <author@xxxxxxxxxxx> > > + EOF > > + git interpret-trailers --trailer "review:" --trailer "fix=HEAD" \ > > + <complex_message >actual2 && > > + test_cmp expected2 actual2 > > +' > > Ok. > > > test_expect_success 'with command using $ARG' ' > > git config trailer.fix.ifExists "replace" && > > - git config trailer.fix.command "git log -1 --oneline --format=\"%h (%s)\" --abbrev-commit --abbrev=14 \$ARG" && > > + git config trailer.fix.command "git log -1 --oneline --format=\"%h (%s)\" \ > > + --abbrev-commit --abbrev=14 \$ARG" && > > This is just an indent change. I am not sure it's worth doing in this > patch. If you think that the file needs better indentation though, > then you might do it in a separate preparatory patch at the beginning > of the current patch series. > > If there is only this place in the file where such an indentation > improvement is needed, this might be ok, but please mention in the > commit message that while at it you are also doing this small change. > Well, I may often think that these small changes are irrelevant. Since you said that, I will cancel the modification of this place. > The other parts of the patch look good to me. Thanks, Christian. -- ZheNing Hu
