| Message ID | 7f645ec95f48a206311973ee45578ba14ac58b7f.1618245568.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | trailer: add new .cmd config option | expand |
"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > +For consistency, the $1 is also passed, this time with the empty string, > +in the command when the command is first called to add a trailer with > +the specified <token>. I guess the same question as 1/2 applies to this part. I am not sure what "consistency" the behaviour of calling the configured command with no argument is trying to achieve. To me, .cmd doing this may be for consistency with .command but I am not sure why the consistency is even desiable. > +$ 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 > +------------ This and the other (omitted) example demonstrates how the initial "empty" invocation is useless by using "replace". Which also means that you cannot add more than one trailer of the same <key> with the mechanism (since the older ones are replaced with the latest). The code change and the test change are consistent with the design, though. Thanks.
On Mon, Apr 12, 2021 at 10:51 PM Junio C Hamano <gitster@pobox.com> wrote: > > "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > +For consistency, the $1 is also passed, this time with the empty string, > > +in the command when the command is first called to add a trailer with > > +the specified <token>. > > I guess the same question as 1/2 applies to this part. I am not > sure what "consistency" the behaviour of calling the configured > command with no argument is trying to achieve. To me, .cmd doing > this may be for consistency with .command but I am not sure why > the consistency is even desiable. It might be desirable to make it easier for people to migrate from ".command" to ".cmd". I agree this is debatable, but I don't see a big downside in it. Maybe, if no argument was passed at all the first time the command is called instead of the empty string, the command could then know that it's called for the first time. I am not sure this would be very helpful in practice though. > > +$ 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 > > +------------ > > This and the other (omitted) example demonstrates how the initial > "empty" invocation is useless by using "replace". Which also means > that you cannot add more than one trailer of the same <key> with the > mechanism (since the older ones are replaced with the latest). You can add more than one trailer with the same key if you don't use "replace" but use "--trim-empty" on the command line, so that an empty trailer added by the initial invocation is removed. And we can later add a `trailer.<token>.runMode` to remove the initial invocation. > The code change and the test change are consistent with the design, > though. Yeah, this patch looks good to me now. Thanks!
Hi, Christian and Junio, Christian Couder <christian.couder@gmail.com> 于2021年4月13日周二 下午3:33写道： > > On Mon, Apr 12, 2021 at 10:51 PM Junio C Hamano <gitster@pobox.com> wrote: > > > > "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > > > +For consistency, the $1 is also passed, this time with the empty string, > > > +in the command when the command is first called to add a trailer with > > > +the specified <token>. > > > > I guess the same question as 1/2 applies to this part. I am not > > sure what "consistency" the behaviour of calling the configured > > command with no argument is trying to achieve. To me, .cmd doing > > this may be for consistency with .command but I am not sure why > > the consistency is even desirable. > > It might be desirable to make it easier for people to migrate from > ".command" to ".cmd". I agree this is debatable, but I don't see a big > downside in it. Maybe, if no argument was passed at all the first time > the command is called instead of the empty string, the command could > then know that it's called for the first time. I am not sure this > would be very helpful in practice though. > If i'm not wrong, Christan meant that this command must run so it's "consistency", and Junio thinks this "consistency" is not needed. It is true that there is not much harm in keeping `.cmd` at the behavior of `.command` now. But I remember the `trailer.<token>.runmode` that Christan said before, perhaps adding it in the subsequent iterations can solve Junio's doubts. Sometimes I also confirm that the behavior of `git interpret-trailers` is very strange too. > > > +$ 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 > > > +------------ > > > > This and the other (omitted) example demonstrates how the initial > > "empty" invocation is useless by using "replace". Which also means > > that you cannot add more than one trailer of the same <key> with the > > mechanism (since the older ones are replaced with the latest). > > You can add more than one trailer with the same key if you don't use > "replace" but use "--trim-empty" on the command line, so that an empty > trailer added by the initial invocation is removed. And we can later > add a `trailer.<token>.runMode` to remove the initial invocation. > Yes, something like: trailer.see.command=echo "$ARG" git interpret-trailers --trim-empty --trailer="see = lll" --trailer="see:jj"</dev/null see: lll see: jj > > The code change and the test change are consistent with the design, > > though. > > Yeah, this patch looks good to me now. > > Thanks! So is there anything else should I improve? Thanks. -- ZheNing Hu
Christian Couder <christian.couder@gmail.com> writes: > It might be desirable to make it easier for people to migrate from > ".command" to ".cmd". I agree this is debatable, but I don't see a big > downside in it. Maybe, if no argument was passed at all the first time > the command is called instead of the empty string, the command could > then know that it's called for the first time. I am not sure this > would be very helpful in practice though. An integration of the trailer mechanism into the "git commit" command, which cannot emulate "-s" at the end-user level, is something I would call a big failure, though. >> This and the other (omitted) example demonstrates how the initial >> "empty" invocation is useless by using "replace". Which also means >> that you cannot add more than one trailer of the same <key> with the >> mechanism (since the older ones are replaced with the latest). > > You can add more than one trailer with the same key if you don't use > "replace" but use "--trim-empty" on the command line, so that an empty > trailer added by the initial invocation is removed. And we can later > add a `trailer.<token>.runMode` to remove the initial invocation. As I said in the other message (not the one you are responding to), trim-empty is not a viable workaround. Imagine what happens when you want to always add a trailer (unrelated to signed-off-by), for which it is perfectly valid not to have any value. An attempt to work around the problem "extra call without being asked from the command line" behaviour has on our emulated "signed-off-by" example with --trim-empty would nuke such an unrelated trailer. Besides, if the command produced non-empty value, e.g. [trailer "baz"] command = "echo B $ARG Z" the extra call without being asked from the command line will still give "baz: B Z" that is not empty. So... I am very curious to learn what the intended use case is for this puzzling feature. If it turns out to be that it behaves only because it was coded that way without any real benefit, I would strongly prefer not to carry it over to the new .cmd thing, which is an attempt to deprecate .command to get rid of its broken parts (so far, we identified two---only the first occurrence is replaced, and the replacement can break command line syntax) while salvaging its good parts (being able to programmatically decide the value). And so far what I heard hasn't convinced me that this behaviour falls into the latter category. Thanks.
ZheNing Hu <adlternative@gmail.com> writes: >> It might be desirable to make it easier for people to migrate from >> ".command" to ".cmd". I agree this is debatable, but I don't see a big >> downside in it. Maybe, if no argument was passed at all the first time >> the command is called instead of the empty string, the command could >> then know that it's called for the first time. I am not sure this >> would be very helpful in practice though. >> > > If i'm not wrong, Christan meant that this command must run so it's > "consistency", and Junio thinks this "consistency" is not needed. My stance actually is a bit stronger than that. I suspect that running the command without argument once even when no --trailer on the command line asks for that <key> is a misfeature, if not a bug (only because it is now documented by 1/2 as such---before that, at least I did not read the document that way). And unless it is shown that it is not a misfeature but is a useful behaviour with an example use case that benefits from it, I would prefer not to replicate it in the ".cmd". > It is true that there is not much harm in keeping `.cmd` at the behavior > of `.command` now. It is far from "there is not much harm". It misses the whole point of the exercise. Only replacing the first occurrence and not the second and subsequent occurrence is not what we are keeping in ".cmd". Replacing in an unsafe way with respect to the command line syntax is not what we are keeping in ".cmd". That is because these two are misfeatures if not bugs (only because they are documented). In fact, the only reason why we are introducing .cmd is so that we can deprecate .command and get rid of its misfeatures while keeping only good bits. So I am waiting to hear why it is not a misfeature. If it is not, then surely I am fine to keep it for now and add a workaround later, but until that happens, I do not think "commit --trailer" can be used as a way to allow end-users emulate "-s" for their favorite trailer like helped-by, acked-by, etc.
Junio C Hamano <gitster@pobox.com> 于2021年4月14日周三 上午3:18写道： > > ZheNing Hu <adlternative@gmail.com> writes: > > >> It might be desirable to make it easier for people to migrate from > >> ".command" to ".cmd". I agree this is debatable, but I don't see a big > >> downside in it. Maybe, if no argument was passed at all the first time > >> the command is called instead of the empty string, the command could > >> then know that it's called for the first time. I am not sure this > >> would be very helpful in practice though. > >> > > > > If i'm not wrong, Christan meant that this command must run so it's > > "consistency", and Junio thinks this "consistency" is not needed. > > My stance actually is a bit stronger than that. I suspect that > running the command without argument once even when no --trailer on > the command line asks for that <key> is a misfeature, if not a bug > (only because it is now documented by 1/2 as such---before that, at > least I did not read the document that way). And unless it is shown > that it is not a misfeature but is a useful behaviour with an example > use case that benefits from it, I would prefer not to replicate it > in the ".cmd". > > > It is true that there is not much harm in keeping `.cmd` at the behavior > > of `.command` now. > > It is far from "there is not much harm". It misses the whole point > of the exercise. > > Only replacing the first occurrence and not the second and > subsequent occurrence is not what we are keeping in ".cmd". > > Replacing in an unsafe way with respect to the command line syntax > is not what we are keeping in ".cmd". > > That is because these two are misfeatures if not bugs (only because > they are documented). > > In fact, the only reason why we are introducing .cmd is so that we > can deprecate .command and get rid of its misfeatures while keeping > only good bits. > > So I am waiting to hear why it is not a misfeature. If it is not, > then surely I am fine to keep it for now and add a workaround later, > but until that happens, I do not think "commit --trailer" can be > used as a way to allow end-users emulate "-s" for their favorite > trailer like helped-by, acked-by, etc. > If it is really necessary to solve this "empty execution" in .cmd, Maybe we need to consider two points: * Do we need a new config flag as you said `[implicitExecution = false]` or just drop it? Has anyone been relying on the "empty execution" of .command before? This may be worthy of concern. * Do we need `trailer.<token>.runMode` as Christan said before? I rejected his this suggestion before, and now I regret it a bit. -- ZheNing Hu
ZheNing Hu <adlternative@gmail.com> writes: >> So I am waiting to hear why it is not a misfeature. If it is not, >> then surely I am fine to keep it for now and add a workaround later, >> but until that happens, I do not think "commit --trailer" can be >> used as a way to allow end-users emulate "-s" for their favorite >> trailer like helped-by, acked-by, etc. >> > > If it is really necessary to solve this "empty execution" in .cmd, > Maybe we need to consider two points: > * Do we need a new config flag as you said `[implicitExecution = false]` > or just drop it? Has anyone been relying on the "empty execution" of > .command before? This may be worthy of concern. Yes, if it is useful sometimes to run the .command or .cmd with empty <value> even when nobody asks for it on the command line with a "--trailer=<key>:<value>" option, then I agree that the users should be able to choose between running and not running [*]. > * Do we need `trailer.<token>.runMode` as Christan said before? > I rejected his this suggestion before, and now I regret it a bit. So far, I haven't heard anything that indicates it is a useful behaviour for .command, so my preference is to get rid of the behaviour when we introduce .cmd to deprecate .command; yes, until we get rid of .command, the behaviour between the two would be inconsistent but that is unavoidable when making a bugfix that is backward incompatible. When (and only when) anybody finds a credible use case, we can add a mechanism to optionally turn it on (e.g. .runMode). That is my thinking right at this moment, but that's of course subject to change when a use case that would be helped by having this extra execution. [Footnote] * Right now, all I know is that not being able to turn it off makes it impossible to use "git commit --trailer" as a more general substitute for "git commit --signoff" without breaking other trailers (e.g. --trim-empty may get rid of the result of the extra execution, but would remove other trailers that can be legitimately empty). And making it on by default with configuration would mean that even though we designed .cmd as a better version of the .command feature with its misdesign corrected, we'd inherit one misdesign from it, which defeats one third of the point of introducing the .cmd in the first place ;-)
Junio C Hamano <gitster@pobox.com> 于2021年4月15日周四 上午4:33写道： > > ZheNing Hu <adlternative@gmail.com> writes: > > >> So I am waiting to hear why it is not a misfeature. If it is not, > >> then surely I am fine to keep it for now and add a workaround later, > >> but until that happens, I do not think "commit --trailer" can be > >> used as a way to allow end-users emulate "-s" for their favorite > >> trailer like helped-by, acked-by, etc. > >> > > > > If it is really necessary to solve this "empty execution" in .cmd, > > > Maybe we need to consider two points: > > * Do we need a new config flag as you said `[implicitExecution = false]` > > or just drop it? Has anyone been relying on the "empty execution" of > > .command before? This may be worthy of concern. > > Yes, if it is useful sometimes to run the .command or .cmd with > empty <value> even when nobody asks for it on the command line with > a "--trailer=<key>:<value>" option, then I agree that the users > should be able to choose between running and not running [*]. > > > * Do we need `trailer.<token>.runMode` as Christan said before? > > I rejected his this suggestion before, and now I regret it a bit. > > So far, I haven't heard anything that indicates it is a useful > behaviour for .command, so my preference is to get rid of the > behaviour when we introduce .cmd to deprecate .command; yes, until > we get rid of .command, the behaviour between the two would be > inconsistent but that is unavoidable when making a bugfix that is > backward incompatible. > > When (and only when) anybody finds a credible use case, we can add a > mechanism to optionally turn it on (e.g. .runMode). > > That is my thinking right at this moment, but that's of course > subject to change when a use case that would be helped by having > this extra execution. > > > [Footnote] > > * Right now, all I know is that not being able to turn it off makes > it impossible to use "git commit --trailer" as a more general > substitute for "git commit --signoff" without breaking other > trailers (e.g. --trim-empty may get rid of the result of the extra > execution, but would remove other trailers that can be > legitimately empty). And making it on by default with > configuration would mean that even though we designed .cmd as a > better version of the .command feature with its misdesign > corrected, we'd inherit one misdesign from it, which defeats one > third of the point of introducing the .cmd in the first place ;-) Perhaps such a modification can meet our temporary needs! @@ -721,9 +738,10 @@ static void process_command_line_args(struct list_head *arg_head, char *cl_separators = xstrfmt("=%s", separators); /* 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.cmd || item->conf.command) + if (item->conf.command) I'm so busy today that I probably haven't had time to put this into a patch today, If this solution is reasonable, I will send a new version of the patch tomorrow. As you said, first solve the misfeature, and we can add [trailer.runMode] later. Thanks, Junio! -- ZheNing Hu
ZheNing Hu <adlternative@gmail.com> writes: > Junio C Hamano <gitster@pobox.com> 于2021年4月15日周四 上午4:33写道： >> >> So far, I haven't heard anything that indicates it is a useful >> behaviour for .command, so my preference is to get rid of the >> behaviour when we introduce .cmd to deprecate .command; yes, until >> we get rid of .command, the behaviour between the two would be >> inconsistent but that is unavoidable when making a bugfix that is >> backward incompatible. >> >> When (and only when) anybody finds a credible use case, we can add a >> mechanism to optionally turn it on (e.g. .runMode). > ... > Perhaps such a modification can meet our temporary needs! > > @@ -721,9 +738,10 @@ static void process_command_line_args(struct > list_head *arg_head, > char *cl_separators = xstrfmt("=%s", separators); > > /* 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.cmd || item->conf.command) > + if (item->conf.command) > > I'm so busy today that I probably haven't had time to put this into a > patch today, > If this solution is reasonable, I will send a new version of the patch tomorrow. > As you said, first solve the misfeature, and we can add [trailer.runMode] later. I am perfectly fine (or rather I'd be happy) to wait for a while to see such a patch, as it would be a good idea to give time to those who do have need for this extra/empty execution to chime in. So, there is no need to rush. Thanks.
On Wed, Apr 14, 2021 at 10:33 PM Junio C Hamano <gitster@pobox.com> wrote: > > ZheNing Hu <adlternative@gmail.com> writes: > > >> So I am waiting to hear why it is not a misfeature. If it is not, > >> then surely I am fine to keep it for now and add a workaround later, > >> but until that happens, I do not think "commit --trailer" can be > >> used as a way to allow end-users emulate "-s" for their favorite > >> trailer like helped-by, acked-by, etc. > >> > > > > If it is really necessary to solve this "empty execution" in .cmd, > > > Maybe we need to consider two points: > > * Do we need a new config flag as you said `[implicitExecution = false]` > > or just drop it? Has anyone been relying on the "empty execution" of > > .command before? This may be worthy of concern. > > Yes, if it is useful sometimes to run the .command or .cmd with > empty <value> even when nobody asks for it on the command line with > a "--trailer=<key>:<value>" option, then I agree that the users > should be able to choose between running and not running [*]. > > > * Do we need `trailer.<token>.runMode` as Christan said before? > > I rejected his this suggestion before, and now I regret it a bit. > > So far, I haven't heard anything that indicates it is a useful > behaviour for .command, Well the `git config trailer.sign.command 'echo "$(git config user.name) <$(git config user.email)>"'` has been documented in the EXAMPLES section of the `git interpret-trailers` doc since when ".command" was implemented, and I believe that reviewers thought that it was a good feature to have then. > so my preference is to get rid of the > behaviour when we introduce .cmd to deprecate .command; yes, until > we get rid of .command, the behaviour between the two would be > inconsistent but that is unavoidable when making a bugfix that is > backward incompatible. > > When (and only when) anybody finds a credible use case, we can add a > mechanism to optionally turn it on (e.g. .runMode). > > That is my thinking right at this moment, but that's of course > subject to change when a use case that would be helped by having > this extra execution. Such use cases and some of this were discussed when `git interpret-trailers` was initially implemented. For example in https://lore.kernel.org/git/CAP8UFD2oXpW9QEkSh+vpNGRAxRFp0zJF39ZZ8sUZLTcKB9mHWQ@mail.gmail.com/ I suggested the following: [trailer "m-o-b"] key = "Made-on-branch: " command = "git name-rev --name-only HEAD" when someone wanted a way to always add "a trailer for the branch that the commit was checked in to at the time" In https://lore.kernel.org/git/534414FB.6040604@alum.mit.edu/ Michael Haggerty suggested: "Maybe there should be a trailer.<token>.trimEmpty config option." I haven't fully checked the discussions, so there might be other examples.
diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt index 6f2a7a130464..dc1f69fa8b67 100644 --- a/Documentation/git-interpret-trailers.txt +++ b/Documentation/git-interpret-trailers.txt @@ -232,6 +232,20 @@ trailer.<token>.ifmissing:: that option for trailers with the specified <token>. trailer.<token>.command:: + This option behaves in the same way as 'trailer.<token>.cmd', except + that it doesn't pass anything as argument to the specified command. + Instead the first occurrence of substring $ARG is replaced by the + value that would be passed as argument. ++ +The 'trailer.<token>.command' option has been deprecated in favor of +'trailer.<token>.cmd' due to the fact that $ARG in the user's command is +only replaced once and that the original way of replacing $ARG is not safe. ++ +When both 'trailer.<token>.cmd' and 'trailer.<token>.command' are given +for the same <token>, 'trailer.<token>.cmd' is used and +'trailer.<token>.command' is ignored. + +trailer.<token>.cmd:: This option can be used to specify a shell command that will be called: once to automatically add a trailer with the specified <token>, and then each time a '--trailer <token>=<value>' argument to modify the <value> of @@ -247,15 +261,13 @@ leading and trailing whitespace trimmed off. If some '--trailer <token>=<value>' arguments are also passed on the command line, the command is called again once for each of these arguments with the same <token>. And the <value> part -of these arguments, if any, will be used to replace the first -occurrence of substring `$ARG` in the command. This way the -command can produce a <value> computed from the <value> passed -in the '--trailer <token>=<value>' argument. +of these arguments, if any, will be passed to the command as its +first argument. This way the command can produce a <value> computed +from the <value> passed in the '--trailer <token>=<value>' argument. + -For consistency, the first occurrence of substring `$ARG` is -also replaced, this time with the empty string, in the command -when the command is first called to add a trailer with the -specified <token>. +For consistency, the $1 is also passed, this time with the empty string, +in the command when the command is first called to add a trailer with +the specified <token>. EXAMPLES -------- @@ -338,6 +350,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..68353af3e62e 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,6 +1337,27 @@ 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" && 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(""),