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

Christian Couder [off-list ref] 于2021年3月30日周二 下午4:45写道：

On Mon, Mar 29, 2021 at 3:44 PM ZheNing Hu [off-list ref] wrote:

quoted

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

quoted

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

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:

quoted

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

quoted

quoted

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

quoted

quoted

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

quoted

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.

quoted

quoted

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?

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.

quoted

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

[...]

quoted

quoted

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.

quoted

quoted

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.

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