From: ZheNing Hu <firstname.lastname@example.org> To: Christian Couder <email@example.com> Cc: Junio C Hamano <firstname.lastname@example.org>, ZheNing Hu via GitGitGadget <email@example.com>, git <firstname.lastname@example.org> Subject: Re: [PATCH v4] [GSOC]trailer: pass arg as positional parameter Date: Mon, 29 Mar 2021 21:43:54 +0800 [thread overview] Message-ID: <CAOLTT8RAe0HhTL6p6MXeqbSazaJF0=PtnDKvh06-FXXBB+w94A@mail.gmail.com> (raw) In-Reply-To: <CAP8UFD0OMJfkuX_JoDros7h0B20D8sm0ZbtkVpL3dCYRV_M=OA@mail.gmail.com> Christian Couder <email@example.com> 于2021年3月29日周一 下午5:05写道： > > > > > 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. > Well, if necessary, I'll put it in another commit, maybe I should double check to see if there's anything special going on. > > > 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: > If really can achieve it is certainly better than 'alwaysRunCmd'. The following three small configuration options look delicious. But I think it needs to be discussed in more detail: > - "beforeCLI": would make it run once, like ".command" does now before > any CLI trailer are processed > Does "beforeCLI" handle all trailers? Or is it just doing something to add empty value trailers? > - "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 > This is exactly same as before. > - "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, ... > I might get a little confused here: What's the input for $1,$2,$3? Is users more interested in dealing with trailers value or a line of the trailer? > 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, Do you mean that ".cmd" can get rid of the ".command" auto-add problem in this patch series? This might be a good idea if I can add the three modes you mentioned above in the later patch series. > - 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. > I agree with you. There may be need some discretion. > 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". > I think the task can be put off until April. Deal with the easier ".cmd" first. > > > > 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. > Oh, the 'commit --trailer' may still be queuing, It may take a while. > Best, > Christian. Thanks. -- ZheNing Hu
next prev parent reply other threads:[~2021-03-29 13:44 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 2021-03-29 13:43 ` ZheNing Hu [this message] 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='CAOLTT8RAe0HhTL6p6MXeqbSazaJF0=PtnDKvh06-FXXBB+w94A@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).