Re: [PATCH v2] [GSOC]trailer: pass arg as positional parameter
- From
ZheNing Hu <adlternative@gmail.com>
- Date
- Mar 25, 2021, 01:43 UTC
- Message-ID
- <CAOLTT8TeL3MofqoGUR2fXq+b+s8ZuxdjkdVnfiuG+vk2TUcwcg@mail.gmail.com>
- In-Reply-To
- <xmqqft0kxq4m.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> 于2021年3月25日周四 上午4:18写道：
Show 24 quoted lines
> > "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: ZheNing Hu <adlternative@gmail.com> > > > > In the original implementation of `trailer.<token>.command`, > > use `strbuf_replace` to replace $ARG in the <value> of the > > trailer, but it have a problem: `strbuf_replace` replace the > > $ARG in command only once, If the user's trailer command have > > used more than one $ARG, error will occur. > > > > If directly modify the implementation of the original > > `trailer.<token>.command`, The user’s previous `'$ARG'` in > > trailer command will not be replaced. So now add new > > config "trailer.<token>.cmd", pass trailer's value as > > positional parameter 1 to the user's command, users can > > use $1 as trailer's value, to implement original variable > > replacement. > > > > Original `trailer.<token>.command` can still be used until git > > completely abandoned it. > > Sorry, but that's quite an ungrammatical mess X-<. >
Somewhat embarrassing. I have tried to fix it...
Show 8 quoted lines
> > 1: abc5b04d152f ! 1: 185356d6fc90 [GSOC]trailer: change $ARG to environment variable > > @@ Metadata > > Author: ZheNing Hu <adlternative@gmail.com> > > As this is completely a different design and does not share anything > with the earlier round, the range-diff is merely distracting and > useless. >
I thought the designs of the two were very similar.
Show 20 quoted lines
> > Documentation/git-interpret-trailers.txt | 7 +++++++ > > t/t7513-interpret-trailers.sh | 22 +++++++++++++++++++- > > trailer.c | 26 +++++++++++++++++------- > > 3 files changed, 47 insertions(+), 8 deletions(-) > > > > diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt > > index 96ec6499f001..38656b1b3841 100644 > > --- a/Documentation/git-interpret-trailers.txt > > +++ b/Documentation/git-interpret-trailers.txt > > @@ -252,6 +252,13 @@ also be executed for each of these arguments. And the <value> part of > > these arguments, if any, will be used to replace the `$ARG` string in > > the command. > > > > +trailer.<token>.cmd:: > > + Similar to 'trailer.<token>.command'. But the difference is that > > + `$1` is used in the command to replace the value of the trailer > > + instead of the original `$ARG`, which means that we can quote the > > "quote"? >
parse.
Show 7 quoted lines
> > + trailer value multiple times in the command. > > + E.g. `trailer.sign.cmd="test -n \"$1\" && echo \"$1\" || true "` > > This needs to explain what happens if the user gives both .cmd and > .command to the same token. Is it an error? Is the newly invented > .cmd takes precedence? Something else? >
For the time being, if I make "cmd" and "command" equivalent, it will only trigger a warning.
> Whatever the answer is, the reasoning behind reaching the design > must be explained and defended in the proposed log message. >
OK.
Show 15 quoted lines
>
> > diff --git a/trailer.c b/trailer.c
> > index be4e9726421c..80f47657ff1a 100644
> > --- a/trailer.c
> > +++ b/trailer.c
> > @@ -14,6 +14,7 @@ struct conf_info {
> > char *name;
> > char *key;
> > char *command;
> > + int is_new_cmd;
>
> Poor naming. The .cmd thing may be "new" right now in your mind,
> but how would you transition out of it when design flaws are
> discovered in it and replace it with yet another mechanism?
>I thought if the "command" will need to be replaced in later releases, "is_new_cmd" will be removed at the same time, now I think "is_new_cmd" may cause misunderstandings.
> Add a new "char *cmd" field, and at the using site, define the > precedence between the two when both cmd and command members of the > structure are populated, perhaps?
It sounds feasible, I will try.
Thanks.