git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Christian Couder <christian.couder@gmail.com>
To: ZheNing Hu <adlternative@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 10:45:06 +0200	[thread overview]
Message-ID: <CAP8UFD1XSTAq28LrBe-q+M_Vs78gZhr58mHM6EgYt9g3pPuPDg@mail.gmail.com> (raw)
In-Reply-To: <CAOLTT8RAe0HhTL6p6MXeqbSazaJF0=PtnDKvh06-FXXBB+w94A@mail.gmail.com>

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)>"'

> > - "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).

> 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.

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.

> > > 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?

> 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.

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.

[...]

> > 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.

  reply	other threads:[~2021-03-30  8:46 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 [this message]
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=CAP8UFD1XSTAq28LrBe-q+M_Vs78gZhr58mHM6EgYt9g3pPuPDg@mail.gmail.com \
    --to=christian.couder@gmail.com \
    --cc=adlternative@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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git