From: ZheNing Hu <adlternative@gmail.com> To: Christian Couder <christian.couder@gmail.com> Cc: Junio C Hamano <gitster@pobox.com>, ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com>, git <git@vger.kernel.org> Subject: Re: [PATCH v4] [GSOC]trailer: pass arg as positional parameter Date: Tue, 30 Mar 2021 19:22:13 +0800 [thread overview] Message-ID: <CAOLTT8SfOKS41uJDHAMAmhWZXc3qZsngfFtsbzXxdNP1cEObzg@mail.gmail.com> (raw) In-Reply-To: <CAP8UFD1XSTAq28LrBe-q+M_Vs78gZhr58mHM6EgYt9g3pPuPDg@mail.gmail.com> Christian Couder <christian.couder@gmail.com> 于2021年3月30日周二 下午4:45写道: > > On Mon, Mar 29, 2021 at 3:44 PM ZheNing Hu <adlternative@gmail.com> wrote: > > > > Christian Couder <christian.couder@gmail.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? > > I am not sure what you mean by "handle all trailers". What I mean is > that it would just work like ".command" does right now before the > "--trailers ..." options are processed. > > Let's suppose the "trailer.foo.command" config option is set to "bar". > Then the "bar" command will be run just before the "--trailers ..." > options are processed and the output of that, let's say "baz" will be > used to add a new "foo: baz" trailer to the ouput of `git > interpret-trailers`. > > For example: > > ------- > $ git -c trailer.foo.command='echo baz' interpret-trailers<<EOF > EOF > > foo: baz > ------- > > In other words an empty value trailer is just a special case when the > command that is run does not output anything. But such commands are > expected to output something not trivial at least in some cases. > > See also the example in the doc that uses: > > $ git config trailer.sign.command 'echo "$(git config user.name) > <$(git config user.email)>"' > I see what you mean, which is to provide a default value for any trailers that haven't been run command yet. > > > - "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. > > Yeah it is the same as before when the "--trailers ..." options are > processed, but not before that. > > To get exactly the same as before one would need to configure both > "beforeCLI" _and_ "forEachCLIToken", for example like this (note that > we use "--add" when adding "forEachCLIToken"): > > $ git config trailer.foo.runMode beforeCLI > $ git config --add trailer.foo.runMode forEachCLIToken > $ git config -l | grep foo > trailer.foo.runmode=beforeCLI > trailer.foo.runmode=forEachCLIToken > > > > - "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? > > The input would be the different values that are used for the token in > the "--trailer ..." CLI arguments. > > For (an hypothetical) example: > > ------ > $ git config trailer.foo.runMode afterCLI > $ git config trailer.foo.cmd 'echo $@' > $ git interpret-trailers --trailer foo=a --trailer foo=b --trailer foo=c<<EOF > EOF > > foo: a b c > $ git interpret-trailers<<EOF > EOF > > foo: > ------ > > I am not sure "afterCLI" would be useful, but we might not want to > implement it right now. It's just an example to show that we could add > other modes to run the configured ".cmd" (and maybe ".command" too). > Yes, not so useful. > > Is users more interested in dealing with trailers value or a line of the > > trailer? > > I am not sure what you mean here. If "a line of the trailer" means a > trailer that is already in the input file that is passed to `git > interpret-trailers`, and if "trailers value" means a "--trailer ..." > argument, then I would say that users could be interested in dealing > with both. > Sorry, I mean after we running those command, a line trailer is "foo: bar" and trailers value will be "bar". > It's true that right now the command configured by a ".command" is not > run when `git interpret-trailers` processes in input file that > contains a trailer with the corresponding token. So new values for > ".runMode" could be implemented to make that happen. > Sure. > > > > 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? > > I am not sure what you mean with "auto-add". Do you mean that fact > that the ".command" runs once before the CLI "--trailer ..." options > are processed? > I'm talking about the empty values $ARG passing to the user's command, those command at least run once, You say "how and when should it run by default", I was wondering if I could not run .cmd without passing trailer. > > This might be a good idea if I can add the three modes you mentioned above > > in the later patch series. > > I like that your are interested in improving trailer handling in Git, > but I must say that if you intend to apply for the GSoC, you might > want to work on your application document first, as it will need to be > discussed on the mailing list too and it will take some time. You are > also free to work on this too, but that shouldn't be your priority. > In fact, I had written the proposal carefully. I have been studying what went wrong with OIga's improvement of cat-file recently. I may have thought of some ideas, and has been written in Proposal, I will submit it in about two days :) > By the way if this (or another Git related) subject is more > interesting to you than the project ideas we propose on > https://git.github.io/SoC-2021-Ideas/, then you are welcome to write a > proposal about working on this (improving trailer handling) rather > than on a project idea from that page. You might want to make sure > that some people would be willing to (co-)mentor you working on it > though. > Aha, for the time being, you are the most suitable mentor, But I might just take improvement of `interpret-tarilers` as my interest to do something. I will choice the project of "git cat-file" . > [...] > > > > 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. > > Ok for me, but see above about GSoC application. > > > > > > > > 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. > > You might want to check if it needs another reroll or if there are > other reasons (like no reviews) why it's not listed in the last > "What's cooking ..." email from Junio. If you think it is ready and > has been forgotten, you can ping reviewers (including me), to ask them > to review it one more time, or Junio if the last version you sent has > already been reviewed. It should still be in "seen" inheritance, Junio is advancing it. Maybe you think it has something to improve, please feel free to tell me. In addition, I now found a small bug in ".cmd", git config -l |grep bug trailer.bug.key=bug-descibe: trailer.bug.ifexists=replace trailer.bug.cmd=echo 123 see what will happen: git interpret-trailers --trailer="bug:text" <<-EOF `heredocd> EOF bug-descibe:123 text "text" seem print to stdout. I'm looking at what's going on here. -- ZheNing Hu
next prev parent reply other threads:[~2021-03-30 11:23 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 2021-03-30 8:45 ` Christian Couder 2021-03-30 11:22 ` ZheNing Hu [this message] 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=CAOLTT8SfOKS41uJDHAMAmhWZXc3qZsngfFtsbzXxdNP1cEObzg@mail.gmail.com \ --to=adlternative@gmail.com \ --cc=christian.couder@gmail.com \ --cc=git@vger.kernel.org \ --cc=gitgitgadget@gmail.com \ --cc=gitster@pobox.com \ --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).