git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v7] [GSOC] trailer: add new trailer.<token>.cmd config option

From
Christian Couder <christian.couder@gmail.com>
Date
Apr 6, 2021, 16:23 UTC
Message-ID
<CAP8UFD3Ga=HR4YsS8UvgEQF9982be_wa8dUzniGsri+6qoEMHg@mail.gmail.com>
In-Reply-To
<pull.913.v7.git.1617541912381.gitgitgadget@gmail.com>

On Sun, Apr 4, 2021 at 3:11 PM ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 19 quoted lines
>
> 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/

Show 8 quoted lines
> 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?

Show 5 quoted lines
>     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"
Show 5 quoted lines
>     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.

Show 22 quoted lines
>  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'

Show 8 quoted lines
> +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/
Show 12 quoted lines
> +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.

Show 51 quoted lines
>  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!
Show 15 quoted lines
> +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.

Show 13 quoted lines
> +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".
Show 13 quoted lines
> +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".
Show 18 quoted lines
> +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.
Show 27 quoted lines
>  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.
Show 5 quoted lines
>  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.
Previous: ZheNing Hu via GitGitGadgetNext: ZheNing Hu
Message 51 of 101 in “[GSOC]trailer: change $ARG to environment variable”
  1. [GSOC]trailer: change $ARG to environment variableZheNing Hu via GitGitGadget, Mar 23, 2021
  2. [GSOC]trailer: pass arg as positional parameterZheNing Hu via GitGitGadget, Mar 24, 2021
  3. Junio C HamanoMar 24, 2021
  4. ZheNing HuMar 25, 2021
  5. [GSOC]trailer: pass arg as positional parameterZheNing Hu via GitGitGadget, Mar 25, 2021
  6. Junio C HamanoMar 25, 2021
  7. ZheNing HuMar 26, 2021
  8. [GSOC]trailer: pass arg as positional parameterZheNing Hu via GitGitGadget, Mar 26, 2021
  9. Junio C HamanoMar 27, 2021
  10. Christian CouderMar 27, 2021
  11. ZheNing HuMar 28, 2021
  12. Christian CouderMar 29, 2021
  13. ZheNing HuMar 29, 2021
  14. Christian CouderMar 30, 2021
  15. ZheNing HuMar 30, 2021
  16. ZheNing HuMar 30, 2021
  17. Junio C HamanoMar 30, 2021
  18. ZheNing HuMar 31, 2021
  19. Junio C HamanoMar 31, 2021
  20. Junio C HamanoMar 31, 2021
  21. ZheNing HuApr 1, 2021
  22. Junio C HamanoApr 1, 2021
  23. ZheNing HuApr 2, 2021
  24. ZheNing HuApr 1, 2021
  25. 0/2 [GSOC]trailer: pass arg as positional parameterZheNing Hu via GitGitGadget, Mar 31, 2021
  26. 1/2 [GSOC] run-command: add shell_no_implicit_args optionZheNing Hu via GitGitGadget, Mar 31, 2021
  27. Christian CouderApr 1, 2021
  28. ZheNing HuApr 1, 2021
  29. 2/2 [GSOC]trailer: pass arg as positional parameterZheNing Hu via GitGitGadget, Mar 31, 2021
  30. Christian CouderApr 1, 2021
  31. ZheNing HuApr 1, 2021
  32. [GSOC] trailer: add new trailer.<token>.cmd config optionZheNing Hu via GitGitGadget, Apr 2, 2021
  33. Junio C HamanoApr 2, 2021
  34. ZheNing HuApr 3, 2021
  35. Junio C HamanoApr 4, 2021
  36. Christian CouderApr 3, 2021
  37. Junio C HamanoApr 4, 2021
  38. Christian CouderApr 6, 2021
  39. Christian CouderApr 6, 2021
  40. ZheNing HuApr 6, 2021
  41. Junio C HamanoApr 6, 2021
  42. Junio C HamanoApr 6, 2021
  43. ZheNing HuApr 4, 2021
  44. Christian CouderApr 4, 2021
  45. ZheNing HuApr 4, 2021
  46. Junio C HamanoApr 2, 2021
  47. ZheNing HuApr 3, 2021
  48. Junio C HamanoApr 3, 2021
  49. ZheNing HuApr 3, 2021
  50. [GSOC] trailer: add new trailer.<token>.cmd config optionZheNing Hu via GitGitGadget, Apr 4, 2021
  51. Christian CouderApr 6, 2021
  52. ZheNing HuApr 7, 2021
  53. 0/2 [GSOC] trailer: add new .cmd config optionZheNing Hu via GitGitGadget, Apr 9, 2021
  54. 1/2 [GSOC] docs: correct descript of trailer.<token>.commandZheNing Hu via GitGitGadget, Apr 9, 2021
  55. Christian CouderApr 9, 2021
  56. ZheNing HuApr 10, 2021
  57. 2/2 [GSOC] trailer: add new .cmd config optionZheNing Hu via GitGitGadget, Apr 9, 2021
  58. Christian CouderApr 9, 2021
  59. ZheNing HuApr 10, 2021
  60. Christian CouderApr 9, 2021
  61. 0/2 [GSOC] trailer: add new .cmd config optionZheNing Hu via GitGitGadget, Apr 12, 2021
  62. 1/2 [GSOC] docs: correct descript of trailer.<token>.commandZheNing Hu via GitGitGadget, Apr 12, 2021
  63. Junio C HamanoApr 12, 2021
  64. Christian CouderApr 16, 2021
  65. Junio C HamanoApr 17, 2021
  66. 2/2 [GSOC] trailer: add new .cmd config optionZheNing Hu via GitGitGadget, Apr 12, 2021
  67. Junio C HamanoApr 12, 2021
  68. Christian CouderApr 13, 2021
  69. ZheNing HuApr 13, 2021
  70. Junio C HamanoApr 13, 2021
  71. ZheNing HuApr 14, 2021
  72. Junio C HamanoApr 14, 2021
  73. ZheNing HuApr 15, 2021
  74. Junio C HamanoApr 15, 2021
  75. Christian CouderApr 16, 2021
  76. Junio C HamanoApr 13, 2021
  77. 0/2 [GSOC] trailer: add new .cmd config optionZheNing Hu via GitGitGadget, Apr 16, 2021
  78. 2/2 [GSOC] trailer: add new .cmd config optionZheNing Hu via GitGitGadget, Apr 16, 2021
  79. Junio C HamanoApr 16, 2021
  80. Junio C HamanoApr 16, 2021
  81. Junio C HamanoApr 16, 2021
  82. Junio C HamanoApr 17, 2021
  83. Junio C HamanoApr 17, 2021
  84. ZheNing HuApr 17, 2021
  85. Junio C HamanoApr 17, 2021
  86. 1/2 [GSOC] docs: correct descript of trailer.<token>.commandZheNing Hu via GitGitGadget, Apr 16, 2021
  87. Junio C HamanoApr 16, 2021
  88. 0/2 [GSOC] trailer: add new .cmd config optionZheNing Hu via GitGitGadget, Apr 17, 2021
  89. 1/2 [GSOC] docs: correct description of .commandZheNing Hu via GitGitGadget, Apr 17, 2021
  90. 2/2 [GSOC] trailer: add new .cmd config optionZheNing Hu via GitGitGadget, Apr 17, 2021
  91. Junio C HamanoApr 17, 2021
  92. ZheNing HuApr 18, 2021
  93. Junio C HamanoApr 21, 2021
  94. ZheNing HuApr 21, 2021
  95. Junio C HamanoApr 21, 2021
  96. ZheNing HuApr 22, 2021
  97. Junio C HamanoApr 27, 2021
  98. ZheNing HuApr 27, 2021
  99. 0/2 [GSOC] trailer: add new .cmd config optionZheNing Hu via GitGitGadget, May 3, 2021
  100. 2/2 [GSOC] trailer: add new .cmd config optionZheNing Hu via GitGitGadget, May 3, 2021
  101. 1/2 [GSOC] docs: correct descript of trailer.<token>.commandZheNing Hu via GitGitGadget, May 3, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.