From: Christian Couder <email@example.com> To: ZheNing Hu <firstname.lastname@example.org> Cc: Junio C Hamano <email@example.com>, ZheNing Hu via GitGitGadget <firstname.lastname@example.org>, git <email@example.com> Subject: Re: [PATCH v4] [GSOC]trailer: pass arg as positional parameter Date: Mon, 29 Mar 2021 11:04:50 +0200 [thread overview] Message-ID: <CAP8UFD0OMJfkuX_JoDros7h0B20D8sm0ZbtkVpL3dCYRV_M=OA@mail.gmail.com> (raw) In-Reply-To: <CAOLTT8Ryrp90xJ0=Y2avndYpf_2JvabK=XAucfirstname.lastname@example.org> On Sun, Mar 28, 2021 at 12:46 PM ZheNing Hu <email@example.com> wrote: > > Christian Couder <firstname.lastname@example.org> 于2021年3月28日周日 上午3:53写道： > > > > On Sat, Mar 27, 2021 at 7:04 PM Junio C Hamano <email@example.com> wrote: > > > As I cannot grok what the description for ".command" is trying to > > > say, especially around this part: > > > > > > When this option is specified, the behavior is as if a special > > > '<token>=<value>' argument were added at the beginning of the command > > > line, where <value> is ... > > > > This is because when a number of trailers are passed on the command > > line, and some other trailers are in the input file, the order in > > which the different trailers are processed and their priorities can be > > important. So by saying the above, people can get an idea about at > > which point and with which priority a trailer coming from such a > > config option will be processed. > > This shows that .command itself has the characteristic of alwaysRun: > even if <token> <value> is not specified, the shell in .command will be > executed at least once, $ARG is empty by default. This is why I asked > `log --author=$ARG -1` will show the last commit identity when `--trailer` > is not used. Yeah, that's the reason. > > > and > > > > > > If some '<token>=<value>' arguments are also passed on the command > > > line, when a 'trailer.<token>.command' is configured, the command will > > > also be executed for each of these arguments. > > > > Yeah, this means that when a 'trailer.foo.command' is configured, it > > is always executed at least once. The first time it is executed, it is > > passed nothing ($ARG is replaced with the empty string). Then for each > > 'foo=<value>' argument passed on the command line, it is executed once > > more with $ARG replaced by <value>. > > > > The reason it is always executed first with $ARG replaced with the > > empty string is that this way it makes it possible to set up commands > > that will always be executed when `git interpret-trailers` is run. > > This makes it possible to automatically add some trailers if they are > > missing for example. > > Yes, $ARG or $1 are always exist because of: > > arg = xstrdup(""); > > so I think maybe we don't even need this judge in `apply_command`? > + if (arg) > + strvec_push(&cp.args, arg); Yeah, I haven't looked at the code, but that might be a good simplification. If you work on this, please submit it in a separate commit. > > Another way to do it would be to have another config option called > > `trailer.<token>.alwaysRunCmd` to tell if the cmd specified by > > `trailer.<token>.cmd` should be run even if no '<token>=<value>' > > argument is passed on the command line. As we are introducing > > `trailer.<token>.cmd`, it's a good time to wonder if this would be a > > better design. But this issue is quite complex, because of the fact > > that 'trailer.<token>.ifMissing' and 'trailer.<token>.ifExists' also > > take a part in deciding if the command will be run. Actually after thinking about it, I think it might be better, instead of `trailer.<token>.alwaysRunCmd`, to add something like `trailer.<token>.runMode` that could take multiple values like: - "beforeCLI": would make it run once, like ".command" does now before any CLI trailer are processed - "forEachCLIToken": would make it run once for each trailer that has the token, like ".command" also does now, the difference would be that the value for the token would be passed in the $1 argument - "afterCLI": would make it run once after all the CLI trailers have been processed and it could pass the different values for the token if any in different arguments: $1, $2, $3, ... This would make it possible to extend later if the need arises for more different times or ways to run configured commands. > In fact, I would prefer this design, because if I don’t add any trailers, > the trailer.<token>.command I set will be executed, which may be very > distressing sometimes, and `alwayRunCmd` is the user I hope that "trailers" > can be added automatically, and other trailers.<token>.command will not be > executed automatically. This allows the user to reasonably configure the > commands that need to be executed. This must be a very comfortable thing. I agree that it should be easier and more straightforward, than it is now, to configure this. > But as you said, to disable the automatic addition in the original .command > and use the new .alwaysRunCmd, I’m afraid there are a lot of things to consider. > Perhaps future series of patches can be considered to do it. Yeah, support for `trailer.<token>.runMode` might be added in different commits at least and possibly later in a different patch series. There are the following issues to resolve, though, if we want to focus only on a new ".cmd" config option: - how and when should it run by default, - how to explain that in the doc, and maybe - how to improve the current description of what happens for ".command" > > This mechanism is the reason why a trick, when setting up a > > 'trailer.foo.command' trailer, is to also set 'trailer.foo.ifexists' > > to "replace", so that the first time the command is run (with $ARG > > replaced with the empty string) it will add a foo trailer with a > > default value, and if it is run another time, because a 'foo=bar' > > argument is passed on the command line, then the trailer with the > > default value will be replaced by the value computed from running the > > command again with $ARG replaced with "bar". > > > > Another trick is to have the command output nothing when $ARG is the > > empty string along with using --trim-empty. This way the command will > > create an empty trailer, when it is run the first time, and if it's > > not another time, then this empty trailer will be removed because of > > --trim-empty. > > > > It looks very practical indeed. > > > > I cannot quite judge if what we came up with in the above > > > description is sufficient. > > > > I don't think it's sufficient. I think that, while we are at it, a bit > > more thinking/discussion is required to make sure we want to keep the > > same design as 'trailer.<token>.command'. > > Sure. I agree that more discussion is needed. > I think if the documents that once belonged to .command are copied to .cmd, > will the readers be too burdensome to read them? Will it be better to migrate > its documentation until we completely delete .command? My opinion (if we focus only on adding ".cmd") is that: - for simplicity for now it should run at the same time as ".command", the only difference being how the argument is passed (using $1 instead of textually replacing $ARG) - the doc for ".command" should be first improved if possible, and then moved over to ".cmd" saying for ".command" that ".command" is deprecated in favor of ".cmd" but otherwise works as ".cmd" except that instead using $1 the value is passed by textually replacing $ARG which could be a safety and correctness issue. Another way to work on all this, would be to first work on adding support for `trailer.<token>.runMode` and on improving existing documentation, and then to add ".cmd", which could then by default use a different ".runMode" than ".command". > > > This, too, but until ".command" is removed, wouldn't it be better > > > for readers to keep both variants, as the distinction between $ARG > > > and $1 needs to be illustrated? > > So the correct solution should be to keep the original .command Examples, > and then give the .cmd examples again. Maybe we could take advantage of ".cmd" to show other nice possibilities to use all of this. Especially if support for `git commit --trailer ...` is already merged, we might be able to use it in those examples, or perhaps add some examples to the git commit doc. Best, Christian.
next prev parent reply other threads:[~2021-03-29 9:09 UTC|newest] Thread overview: 101+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-23 14:53 [PATCH] [GSOC]trailer: change $ARG to environment variable ZheNing Hu via GitGitGadget 2021-03-24 15:42 ` [PATCH v2] [GSOC]trailer: pass arg as positional parameter ZheNing Hu via GitGitGadget 2021-03-24 20:18 ` Junio C Hamano 2021-03-25 1:43 ` ZheNing Hu 2021-03-25 11:53 ` [PATCH v3] " ZheNing Hu via GitGitGadget 2021-03-25 22:28 ` Junio C Hamano 2021-03-26 13:29 ` ZheNing Hu 2021-03-26 16:13 ` [PATCH v4] " ZheNing Hu via GitGitGadget 2021-03-27 18:04 ` Junio C Hamano 2021-03-27 19:53 ` Christian Couder 2021-03-28 10:46 ` ZheNing Hu 2021-03-29 9:04 ` Christian Couder [this message] 2021-03-29 13:43 ` ZheNing Hu 2021-03-30 8:45 ` Christian Couder 2021-03-30 11:22 ` ZheNing Hu 2021-03-30 15:07 ` ZheNing Hu 2021-03-30 17:14 ` Junio C Hamano 2021-03-31 5:14 ` ZheNing Hu 2021-03-31 18:19 ` Junio C Hamano 2021-03-31 18:29 ` Junio C Hamano 2021-04-01 3:56 ` ZheNing Hu 2021-04-01 19:49 ` Junio C Hamano 2021-04-02 2:08 ` ZheNing Hu 2021-04-01 3:39 ` ZheNing Hu 2021-03-31 10:05 ` [PATCH v5 0/2] " ZheNing Hu via GitGitGadget 2021-03-31 10:05 ` [PATCH v5 1/2] [GSOC] run-command: add shell_no_implicit_args option ZheNing Hu via GitGitGadget 2021-04-01 7:22 ` Christian Couder 2021-04-01 9:58 ` ZheNing Hu 2021-03-31 10:05 ` [PATCH v5 2/2] [GSOC]trailer: pass arg as positional parameter ZheNing Hu via GitGitGadget 2021-04-01 7:28 ` [PATCH v5 0/2] " Christian Couder 2021-04-01 10:02 ` ZheNing Hu 2021-04-02 13:26 ` [PATCH v6] [GSOC] trailer: add new trailer.<token>.cmd config option ZheNing Hu via GitGitGadget 2021-04-02 20:48 ` Junio C Hamano 2021-04-03 5:08 ` ZheNing Hu 2021-04-04 5:34 ` Junio C Hamano 2021-04-03 5:51 ` Christian Couder 2021-04-04 23:26 ` Junio C Hamano 2021-04-06 3:47 ` Christian Couder 2021-04-06 3:52 ` Christian Couder 2021-04-06 5:16 ` ZheNing Hu 2021-04-06 5:34 ` Junio C Hamano 2021-04-06 5:37 ` Junio C Hamano 2021-04-04 5:43 ` ZheNing Hu 2021-04-04 8:52 ` Christian Couder 2021-04-04 9:53 ` ZheNing Hu 2021-04-02 23:44 ` Junio C Hamano 2021-04-03 3:22 ` ZheNing Hu 2021-04-03 4:31 ` Junio C Hamano 2021-04-03 5:15 ` ZheNing Hu 2021-04-04 13:11 ` [PATCH v7] " ZheNing Hu via GitGitGadget 2021-04-06 16:23 ` Christian Couder 2021-04-07 4:51 ` ZheNing Hu 2021-04-09 13:37 ` [PATCH v8 0/2] [GSOC] trailer: add new .cmd " ZheNing Hu via GitGitGadget 2021-04-09 13:37 ` [PATCH v8 1/2] [GSOC] docs: correct descript of trailer.<token>.command ZheNing Hu via GitGitGadget 2021-04-09 19:02 ` Christian Couder 2021-04-10 13:40 ` ZheNing Hu 2021-04-09 13:37 ` [PATCH v8 2/2] [GSOC] trailer: add new .cmd config option ZheNing Hu via GitGitGadget 2021-04-09 20:18 ` Christian Couder 2021-04-10 14:09 ` ZheNing Hu 2021-04-09 19:59 ` [PATCH v8 0/2] " Christian Couder 2021-04-12 16:39 ` [PATCH v9 " ZheNing Hu via GitGitGadget 2021-04-12 16:39 ` [PATCH v9 1/2] [GSOC] docs: correct descript of trailer.<token>.command ZheNing Hu via GitGitGadget 2021-04-12 20:42 ` Junio C Hamano 2021-04-16 12:03 ` Christian Couder 2021-04-17 1:54 ` Junio C Hamano 2021-04-12 16:39 ` [PATCH v9 2/2] [GSOC] trailer: add new .cmd config option ZheNing Hu via GitGitGadget 2021-04-12 20:51 ` Junio C Hamano 2021-04-13 7:33 ` Christian Couder 2021-04-13 12:02 ` ZheNing Hu 2021-04-13 19:18 ` Junio C Hamano 2021-04-14 13:27 ` ZheNing Hu 2021-04-14 20:33 ` Junio C Hamano 2021-04-15 15:32 ` ZheNing Hu 2021-04-15 17:41 ` Junio C Hamano 2021-04-16 12:54 ` Christian Couder 2021-04-13 18:14 ` Junio C Hamano 2021-04-16 8:47 ` [PATCH v10 0/2] " ZheNing Hu via GitGitGadget 2021-04-16 8:47 ` [PATCH v10 1/2] [GSOC] docs: correct descript of trailer.<token>.command ZheNing Hu via GitGitGadget 2021-04-16 19:11 ` Junio C Hamano 2021-04-16 8:47 ` [PATCH v10 2/2] [GSOC] trailer: add new .cmd config option ZheNing Hu via GitGitGadget 2021-04-16 19:13 ` Junio C Hamano 2021-04-16 19:21 ` Junio C Hamano 2021-04-16 19:25 ` Junio C Hamano 2021-04-17 2:58 ` Junio C Hamano 2021-04-17 3:36 ` Junio C Hamano 2021-04-17 7:41 ` ZheNing Hu 2021-04-17 8:11 ` Junio C Hamano 2021-04-17 15:13 ` [PATCH v11 0/2] " ZheNing Hu via GitGitGadget 2021-04-17 15:13 ` [PATCH v11 1/2] [GSOC] docs: correct description of .command ZheNing Hu via GitGitGadget 2021-04-17 15:13 ` [PATCH v11 2/2] [GSOC] trailer: add new .cmd config option ZheNing Hu via GitGitGadget 2021-04-17 22:26 ` [PATCH v11 0/2] " Junio C Hamano 2021-04-18 7:47 ` ZheNing Hu 2021-04-21 0:09 ` Junio C Hamano 2021-04-21 5:47 ` ZheNing Hu 2021-04-21 23:40 ` Junio C Hamano 2021-04-22 9:20 ` ZheNing Hu 2021-04-27 6:49 ` Junio C Hamano 2021-04-27 12:24 ` ZheNing Hu 2021-05-03 15:41 ` [PATCH v12 " ZheNing Hu via GitGitGadget 2021-05-03 15:41 ` [PATCH v12 1/2] [GSOC] docs: correct descript of trailer.<token>.command ZheNing Hu via GitGitGadget 2021-05-03 15:41 ` [PATCH v12 2/2] [GSOC] trailer: add new .cmd config option ZheNing Hu via GitGitGadget
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style List information: http://vger.kernel.org/majordomo-info.html * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAP8UFD0OMJfkuX_JoDros7h0B20D8sm0ZbtkVpL3dCYRV_M=OA@mail.gmail.com' \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [PATCH v4] [GSOC]trailer: pass arg as positional parameter' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Code repositories for project(s) associated with this inbox: https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).