Re: [PATCH v13] [GSOC] commit: add --trailer option

[ZheNing Hu <adlternative@gmail.com>]

Christian Couder <christian.couder@gmail.com> 于2021年3月22日周一 下午3:43写道：
>
> On Mon, Mar 22, 2021 at 5:24 AM ZheNing Hu via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: ZheNing Hu <adlternative@gmail.com>
> >
> > Historically, Git has supported the 'Signed-off-by' commit trailer
> > using the '--signoff' and the '-s' option from the command line.
> > But users may need to provide other trailer information from the
> > command line such as "Helped-by", "Reported-by", "Mentored-by",
> >
> > Now implement a new `--trailer <token>[(=|:)<value>]` option to pass
> > other trailers to `interpret-trailers` and insert them into commit
> > messages.
> >
> > Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> > Reported-by:
>
> Why is there this "Reported-by:" trailer with an empty value? If you
> are looking to add trailers to this commit message, you might want to
> add them before your "Signed-off-by".
>

Sorry, It was purely a small accident during testing and I didn't notice it.

> > ---
> >     [GSOC] commit: add --trailer option
> >
> >     Now maintainers or developers can also use commit
> >     --trailer="Signed-off-by:commiter<email>" from the command line to
> >     provide trailers to commit messages. This solution may be more
> >     generalized than v1.
>
> It's not a big deal as this is not going into the commit message, but
> at this point (v13) you might want to tell explicitly what solution v1
> implemented, instead of referring to it.

Ok.

>
> >      @@ t/t7502-commit-porcelain.sh: test_expect_success 'sign off' '>       + sed -e "1,/^\$/d" commit.msg >actual &&
> >       + test_cmp expected actual
> >       +'
> >      ++
> >      ++test_expect_success 'commit --trailer with -c and command' '
> >      ++ trailer_commit_base &&
> >      ++ cat >expected <<-\EOF &&
> >      ++ hello
> >      ++
> >      ++ Signed-off-by: C O Mitter <committer@example.com>
> >      ++ Signed-off-by: C1 E1
> >      ++ Helped-by: C2 E2
> >      ++ Mentored-by: C4 E4
> >      ++ Reported-by: A U Thor <author@example.com>
> >      ++ EOF
> >      ++ git -c trailer.report.key="Reported-by: " \
> >      ++         -c trailer.report.ifexists="replace" \
> >      ++         -c trailer.report.command="git log --author=\"\$ARG\" -1 \
> >      ++         --format=\"format:%aN <%aE>\"" \
> >      ++         commit --trailer "report = author" --amend &&
> >      ++ git cat-file commit HEAD >commit.msg &&
> >      ++ sed -e "1,/^\$/d" commit.msg >actual &&
> >      ++ test_cmp expected actual
> >      ++'
>
> Nice that you have added such a test!

Thanks.

But at the same time I have two little doubt.

1.
If we have your config:

$ git config trailer.sign.key "Signed-off-by: "
$ git config trailer.sign.ifexists replace
$ git config trailer.sign.command "git log --author='\$ARG' -1
--format='format:%aN <%aE>'"

Then I touch a test.c and use:

$ git interpret-trailers --in-place test.c

without `--trailer`, See what is happen:

It seem like your local repo last commit "name <email>" pair
have been record in `test.c`.

Could this be considered a bug?

2.
`git interpret-trailers --in-place`  seem like work on git top-dir,
If I am in a sub-dir `b` and I want to change a file such as `d.c`,
then I must use `git interpret-trailers --in-place b/d.c` to add some
trailers.

I think the original intention of `--in-place` is to modify a file similar to
"$COMMIT_MSG_FILE", so make it run at top-dir, but this is not reflected
in the git documentation. This at least confuses people who use this
option for the first time. Is it worth modifying? Or is there something
wrong with the design of `--in-place`?

--
ZheNing Hu