Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
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: Mon, 29 Mar 2021 21:43:54 +0800
Message-ID: <CAOLTT8RAe0HhTL6p6MXeqbSazaJF0=PtnDKvh06-FXXBB+w94A@mail.gmail.com> (raw)
In-Reply-To: <CAP8UFD0OMJfkuX_JoDros7h0B20D8sm0ZbtkVpL3dCYRV_M=OA@mail.gmail.com>

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?

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

  reply index

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

* 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' \
    --to=adlternative@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    /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 Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

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

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


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