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

Christian Couder [off-list ref] 于2021年3月29日周一 下午5:05写道：

quoted

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.

quoted

quoted

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.

quoted

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.

quoted

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"

quoted

quoted

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.

quoted

quoted

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.

quoted

quoted

quoted

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