Re: [PATCH v4] [GSOC]trailer: pass arg as positional parameter

[ZheNing Hu <adlternative@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