| Message ID | pull.913.v7.git.1617541912381.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v7,GSOC] trailer: add new trailer.<token>.cmd config option | expand |
On Sun, Apr 4, 2021 at 3:11 PM ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: ZheNing Hu <adlternative@gmail.com> > > 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"! > 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@pobox.com> > Helped-by: Christian Couder <christian.couder@gmail.com> > Signed-off-by: ZheNing Hu <adlternative@gmail.com> > --- > [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? > 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. > 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' > +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! > +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@example.com> > + 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. The other parts of the patch look good to me.
Christian Couder <christian.couder@gmail.com> 于2021年4月7日周三 上午12:23写道： > > On Sun, Apr 4, 2021 at 3:11 PM ZheNing Hu via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > > > From: ZheNing Hu <adlternative@gmail.com> > > > > 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@pobox.com> > > Helped-by: Christian Couder <christian.couder@gmail.com> > > Signed-off-by: ZheNing Hu <adlternative@gmail.com> > > --- > > [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@example.com> > > + 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
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. + -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 +only be replaced once and the original way of replacing $ARG was not safe. +Now the preferred option is using "trailer.<token>.cmd", which use position +argument to pass the value. ++ +When both .cmd and .command are given for the same <token>, +.cmd is used and .command is ignored. + +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>' +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. + 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. 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) +------------ + * Configure a 'see' trailer with a command to show the subject of a commit that is related, and show how it works: + diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh index 6602790b5f4c..6d26a2606604 100755 --- a/t/t7513-interpret-trailers.sh +++ b/t/t7513-interpret-trailers.sh @@ -51,6 +51,69 @@ test_expect_success 'setup' ' EOF ' +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 +' + +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 +' + +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 +' + +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 +' + 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@example.com> + EOF + git interpret-trailers --trailer "review:" --trailer "fix=HEAD" \ + <complex_message >actual2 && + test_cmp expected2 actual2 +' + 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" && FIXED=$(git log -1 --oneline --format="%h (%s)" --abbrev-commit --abbrev=14 HEAD) && cat complex_message_body >expected && sed -e "s/ Z\$/ /" >>expected <<-EOF && diff --git a/trailer.c b/trailer.c index be4e9726421c..bd384befe15b 100644 --- a/trailer.c +++ b/trailer.c @@ -14,6 +14,7 @@ struct conf_info { char *name; char *key; char *command; + char *cmd; enum trailer_where where; enum trailer_if_exists if_exists; enum trailer_if_missing if_missing; @@ -127,6 +128,7 @@ static void free_arg_item(struct arg_item *item) free(item->conf.name); free(item->conf.key); free(item->conf.command); + free(item->conf.cmd); free(item->token); free(item->value); free(item); @@ -216,18 +218,24 @@ static int check_if_different(struct trailer_item *in_tok, return 1; } -static char *apply_command(const char *command, const char *arg) +static char *apply_command(struct conf_info *conf, 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 (conf->cmd) { + strbuf_addstr(&cmd, conf->cmd); + strvec_push(&cp.args, cmd.buf); + if (arg) + strvec_push(&cp.args, arg); + } else if (conf->command) { + strbuf_addstr(&cmd, conf->command); + if (arg) + strbuf_replace(&cmd, TRAILER_ARG_STRING, arg); + strvec_push(&cp.args, cmd.buf); + } cp.env = local_repo_env; cp.no_stdin = 1; cp.use_shell = 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; @@ -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, arg); free((char *)arg); } } @@ -430,6 +438,7 @@ static void duplicate_conf(struct conf_info *dst, const struct conf_info *src) dst->name = xstrdup_or_null(src->name); dst->key = xstrdup_or_null(src->key); dst->command = xstrdup_or_null(src->command); + dst->cmd = xstrdup_or_null(src->cmd); } static struct arg_item *get_conf_item(const char *name) @@ -454,8 +463,8 @@ static struct arg_item *get_conf_item(const char *name) return item; } -enum trailer_info_type { TRAILER_KEY, TRAILER_COMMAND, TRAILER_WHERE, - TRAILER_IF_EXISTS, TRAILER_IF_MISSING }; +enum trailer_info_type { TRAILER_KEY, TRAILER_COMMAND, TRAILER_CMD, + TRAILER_WHERE, TRAILER_IF_EXISTS, TRAILER_IF_MISSING }; static struct { const char *name; @@ -463,6 +472,7 @@ static struct { } trailer_config_items[] = { { "key", TRAILER_KEY }, { "command", TRAILER_COMMAND }, + { "cmd", TRAILER_CMD }, { "where", TRAILER_WHERE }, { "ifexists", TRAILER_IF_EXISTS }, { "ifmissing", TRAILER_IF_MISSING } @@ -542,6 +552,11 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb) warning(_("more than one %s"), conf_key); conf->command = xstrdup(value); break; + case TRAILER_CMD: + if (conf->cmd) + warning(_("more than one %s"), conf_key); + conf->cmd = xstrdup(value); + break; case TRAILER_WHERE: if (trailer_set_where(&conf->where, value)) warning(_("unknown value '%s' for key '%s'"), value, conf_key); @@ -708,7 +723,7 @@ static void process_command_line_args(struct list_head *arg_head, /* Add an arg item for each configured trailer with a command */ list_for_each(pos, &conf_head) { item = list_entry(pos, struct arg_item, list); - if (item->conf.command) + if (item->conf.cmd || item->conf.command) add_arg_item(arg_head, xstrdup(token_from_item(item, NULL)), xstrdup(""),