[GSOC] interpret-trailer: easy parse trailer value #911
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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>
|
/submit |
|
Submitted as pull.911.git.1616251299.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| each subsequent line starting with whitespace, like the "folding" in RFC 822. | ||
|
|
||
| Note that 'trailers' do not follow and are not intended to follow many | ||
| rules for RFC 822 headers. For example they do not follow |
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/commit.h b/commit.h
> index 49c0f503964e..970a73ccd5be 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -371,4 +371,6 @@ int parse_buffer_signed_by_header(const char *buffer,
> struct strbuf *signature,
> const struct git_hash_algo *algop);
>
> +const char *find_author_by_nickname(const char *name);
> +
> #endif /* COMMIT_H */
As I already said, we do not want to pretend that this is a
generally reusable helper function. We should at least have a
comment to tell people to never add new callers to this function,
with explanation of the reason.
There was a problem hiding this comment.
On the Git mailing list, ZheNing Hu wrote (reply to this):
Junio C Hamano <gitster@pobox.com> 于2021年3月21日周日 上午5:06写道：
>
> "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > diff --git a/commit.h b/commit.h
> > index 49c0f503964e..970a73ccd5be 100644
> > --- a/commit.h
> > +++ b/commit.h
> > @@ -371,4 +371,6 @@ int parse_buffer_signed_by_header(const char *buffer,
> > struct strbuf *signature,
> > const struct git_hash_algo *algop);
> >
> > +const char *find_author_by_nickname(const char *name);
> > +
> > #endif /* COMMIT_H */
>
> As I already said, we do not want to pretend that this is a
> generally reusable helper function. We should at least have a
> comment to tell people to never add new callers to this function,
> with explanation of the reason.
Do you think this is appropriate?
@@ -370,5 +370,15 @@ int parse_buffer_signed_by_header(const char *buffer,
struct strbuf *payload,
struct strbuf *signature,
const struct git_hash_algo *algop);
+/*
+ * Calling `find_author_by_nickname` to find the "author <email>" pair
+ * in the most recent commit which matches "--author=name".
+ *
+ * Note that `find_author_by_nickname` is not reusable, because it haven't
+ * reset flags for parsed objects. The only safe way to use
`find_author_by_nickname`
+ * (without rewriting the revision traversal machinery) is to spawn a
+ * subprocess and do find_author_by_nickname() in it.
+ */
+const char *find_author_by_nickname(const char *name);
Thanks.
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
ZheNing Hu <adlternative@gmail.com> writes:
> Do you think this is appropriate?
>
> @@ -370,5 +370,15 @@ int parse_buffer_signed_by_header(const char *buffer,
> struct strbuf *payload,
> struct strbuf *signature,
> const struct git_hash_algo *algop);
> +/*
> + * Calling `find_author_by_nickname` to find the "author <email>" pair
> + * in the most recent commit which matches "--author=name".
> + *
> + * Note that `find_author_by_nickname` is not reusable, because it haven't
> + * reset flags for parsed objects. The only safe way to use
> `find_author_by_nickname`
> + * (without rewriting the revision traversal machinery) is to spawn a
> + * subprocess and do find_author_by_nickname() in it.
> + */
Telling people not to add any new caller is good, but everything
after "because" does not make sense to me.
I do not think calling find_author_by_nickname() in a subprocess
alone would not help somebody who wants to do this, either. We'd be
doing a moral equivalent of that call, but the result has to be
communicated back to the parent process,
In the longer term, we'd probably want to have a pre-computed table
of contributors, like we have precomputed files for reachability
bitmaps, commit DAG topology, and such, but that is obviously far
outside of the scope of this series.
> +const char *find_author_by_nickname(const char *name);
There was a problem hiding this comment.
On the Git mailing list, ZheNing Hu wrote (reply to this):
Junio C Hamano <gitster@pobox.com> 于2021年3月21日周日 下午9:57写道：
>
> ZheNing Hu <adlternative@gmail.com> writes:
>
> > Do you think this is appropriate?
> >
> > @@ -370,5 +370,15 @@ int parse_buffer_signed_by_header(const char *buffer,
> > struct strbuf *payload,
> > struct strbuf *signature,
> > const struct git_hash_algo *algop);
> > +/*
> > + * Calling `find_author_by_nickname` to find the "author <email>" pair
> > + * in the most recent commit which matches "--author=name".
> > + *
> > + * Note that `find_author_by_nickname` is not reusable, because it haven't
> > + * reset flags for parsed objects. The only safe way to use
> > `find_author_by_nickname`
> > + * (without rewriting the revision traversal machinery) is to spawn a
> > + * subprocess and do find_author_by_nickname() in it.
> > + */
>
> Telling people not to add any new caller is good, but everything
> after "because" does not make sense to me.
>
> I do not think calling find_author_by_nickname() in a subprocess
> alone would not help somebody who wants to do this, either. We'd be
> doing a moral equivalent of that call, but the result has to be
> communicated back to the parent process,
>
What I am thinking about here is that `commit --trailer` itself jumps to a
sub-process to do this, but this does depend on the fact that
`interpret-trailers`
itself does not have a traversal, and indeed should not be arbitrarily call it.
> In the longer term, we'd probably want to have a pre-computed table
> of contributors, like we have precomputed files for reachability
> bitmaps, commit DAG topology, and such, but that is obviously far
> outside of the scope of this series.
>
Indeed this will be a very big project. But `.mailmap` always makes me
feel similar.
> > +const char *find_author_by_nickname(const char *name);
The original `-trailer` adding some trailers like "Signed-off-by:C O <Mister@email.com>" is often too verbose and error-prone. Now add the syntax parse for the value of `--trailer`: e.g. "Signed-off-by:@junio", git will fuzzy search in the commit history to find the latest one commit which matches `--author=Junio`, and get the "author <email>" pair `Junio C Hamano <gitster@pobox.com>` as the value of `--trailer`, it will be a easy way to add trailers. git commit --trailer` can also benefit from this. Signed-off-by: ZheNing Hu <adlternative@gmail.com> Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: Jeff King <peff@peff.net>
adlternative
force-pushed
the
trailer-easy-ident
branch
from
ca521d3 to
8b8b236
Compare
|
/submit |
|
Submitted as pull.911.v2.git.1616317134.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
| each subsequent line starting with whitespace, like the "folding" in RFC 822. | ||
|
|
||
| Note that 'trailers' do not follow and are not intended to follow many | ||
| rules for RFC 822 headers. For example they do not follow |
There was a problem hiding this comment.
On the Git mailing list, Bagas Sanjaya wrote (reply to this):
On 21/03/21 15.58, ZheNing Hu via GitGitGadget wrote:
> +test_expect_success 'commit --trailer parse @nickname' '
> + echo "I love git" >file1 &&
> + git add file1 &&
> + git commit -m "yly" --author="batman <email1>" &&
> + echo "I love git" >file2 &&
> + git add file2 &&
> + git commit -m "yly" --author="jocker <email2>" &&
> + echo "I love git" >file3 &&
> + git add file3 &&
> + git commit -m "yly" \
> + --trailer "Reviewed-by:@bat" \
> + --trailer "Signed-off-by:@jock" \
> + --trailer "Helped-by:@email1" \
> + --trailer "Mentored-by:@email2" &&
> + git cat-file commit HEAD >commit.msg &&
> + sed -e "1,/^\$/d" commit.msg >actual &&
> + cat >expected <<-\EOF &&
> + yly
> +
> + Reviewed-by: batman <email1>
> + Signed-off-by: jocker <email2>
> + Helped-by: batman <email1>
> + Mentored-by: jocker <email2>
> + EOF
> + test_cmp expected actual
> +'
> +
> test_expect_success 'multiple -m' '
>
> >negative &&
> +test_expect_success 'trailer parse @nickname' '
> + echo "I love git" >file1 &&
> + git add file1 &&
> + git commit -m "yly" --author="batman <email1>" &&
> + echo "I love git" >file2 &&
> + git add file2 &&
> + git commit -m "yly" --author="jocker <email2>" &&
> + git interpret-trailers \
> + --trailer "Reviewed-by:@bat" \
> + --trailer "Signed-off-by:@jock" \
> + --trailer "Helped-by:@email1" \
> + --trailer "Mentored-by:@email2" \
> + empty >actual &&
> + cat >expected <<-\EOF &&
> +
> + Reviewed-by: batman <email1>
> + Signed-off-by: jocker <email2>
> + Helped-by: batman <email1>
> + Mentored-by: jocker <email2>
> + EOF
> + test_cmp expected actual
> +'
> +
> test_expect_success 'without config in another order' '
> sed -e "s/ Z\$/ /" >expected <<-\EOF &&
I think please consider this case: When I add --trailer "Reviewed-by:@bat", and there are two
identity pairs that match (`batman <email1>` and `batman <email2>`), I need to choose one that
will be in the trailer (for example because <email1> is primary email). So a disambiguation
prompt should be added, something like:
```
There are <N> identities that match, please choose one that will be added to the trailer:
1) batman <email1>
2) batman <email2>
...
n) batman <emailn>
```
The prompt can be repeated for each trailer values that are non-unique.
Thanks
--
An old man doll... just what I always wanted! - Clara
There was a problem hiding this comment.
On the Git mailing list, ZheNing Hu wrote (reply to this):
> I think please consider this case: When I add --trailer "Reviewed-by:@bat", and there are two
> identity pairs that match (`batman <email1>` and `batman <email2>`), I need to choose one that
> will be in the trailer (for example because <email1> is primary email). So a disambiguation
> prompt should be added, something like:
>
> ```
> There are <N> identities that match, please choose one that will be added to the trailer:
> 1) batman <email1>
> 2) batman <email2>
> ...
> n) batman <emailn>
> ```
>
Hi,
I think this kind of prompt will be beneficial to users, temporarily
adopt the method of
`find_author_by_nickname()` to get the last match commit "name <eamil>" pair.
If I want to find all these matched "name <eamil>" pairs and exclude
duplicates, I may need to
think about it for a while.
> The prompt can be repeated for each trailer values that are non-unique.
>
> Thanks
>
> --
> An old man doll... just what I always wanted! - Clara
Thanks.
--
ZheNing Hu
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Bagas Sanjaya <bagasdotme@gmail.com> writes:
> will be in the trailer (for example because <email1> is primary email). So a disambiguation
> prompt should be added, something like:
No, it just should error out without going interactive.
There was a problem hiding this comment.
On the Git mailing list, Bagas Sanjaya wrote (reply to this):
On 21/03/21 23.49, Junio C Hamano wrote:
> Bagas Sanjaya <bagasdotme@gmail.com> writes:
>
>> will be in the trailer (for example because <email1> is primary email). So a disambiguation
>> prompt should be added, something like:
>
> No, it just should error out without going interactive.
>
I mean the error should be:
```
error: @batman match multiple identities:
1) batman <email1>
2) batman <email2>
...
n) batman <emailn>
Please disambiguate by running "git commit --trailer" with full identity, like:
git commit --trailer="<someone> <<email>>" <options>...
```
--
An old man doll... just what I always wanted! - Clara
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Bagas Sanjaya <bagasdotme@gmail.com> writes:
> I mean the error should be:
>
> ```
> error: @batman match multiple identities:
> 1) batman <email1>
> 2) batman <email2>
> ...
> n) batman <emailn>
>
> Please disambiguate by running "git commit --trailer" with full identity, like:
> git commit --trailer="<someone> <<email>>" <options>...
>
> ```
Detecting error alone is probably prohibitively expensive, as it
takes you to scan _all_ commits down to the beginning of time to
pruve that a given pattern matches one and only one author ident.
Assuming that we would pay that cost (I doubt we would want to,
though), additional cost to show all possible hits would be small
and manageable, I think.
|
User |
| each subsequent line starting with whitespace, like the "folding" in RFC 822. | ||
|
|
||
| Note that 'trailers' do not follow and are not intended to follow many | ||
| rules for RFC 822 headers. For example they do not follow |
There was a problem hiding this comment.
On the Git mailing list, Christian Couder wrote (reply to this):
On Sun, Mar 21, 2021 at 9:58 AM ZheNing Hu via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: ZheNing Hu <adlternative@gmail.com>
>
> The original `-trailer` adding some trailers like
s/-trailer/--trailer/
> "Signed-off-by:C O <Mister@email.com>" is often too
> verbose and error-prone.
The 'trailer.<token>.command' config option can already be used to
help with that, for example:
-------
$ 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>'"
$ git interpret-trailers --trailer sign=Linus<<EOF
subject
body
EOF
subject
body
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
-------
So even without this patch, after your first patch that implements
`git commit --trailer ...`, it should be easy to setup something less
verbose and less error-prone.
There was a problem hiding this comment.
On the Git mailing list, ZheNing Hu wrote (reply to this):
Christian Couder <christian.couder@gmail.com> 于2021年3月21日周日 下午9:15写道：
>
> On Sun, Mar 21, 2021 at 9:58 AM ZheNing Hu via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: ZheNing Hu <adlternative@gmail.com>
> >
> > The original `-trailer` adding some trailers like
>
> s/-trailer/--trailer/
>
> > "Signed-off-by:C O <Mister@email.com>" is often too
> > verbose and error-prone.
>
> The 'trailer.<token>.command' config option can already be used to
> help with that, for example:
>
> -------
> $ 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>'"
> $ git interpret-trailers --trailer sign=Linus<<EOF
> subject
>
> body
> EOF
> subject
>
> body
>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> -------
>
> So even without this patch, after your first patch that implements
> `git commit --trailer ...`, it should be easy to setup something less
> verbose and less error-prone.
Hey, Christian,
`@nickname` can provides a "quick" way for those users
who haven't config as you mention. I hope I am not wrong.
--
ZheNing Hu
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Christian Couder <christian.couder@gmail.com> writes:
>> "Signed-off-by:C O <Mister@email.com>" is often too
>> verbose and error-prone.
>
> The 'trailer.<token>.command' config option can already be used to
> help with that, for example:
>
> -------
> $ 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>'"
> $ git interpret-trailers --trailer sign=Linus<<EOF
> subject
>
> body
> EOF
> subject
>
> body
>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> -------
>
> So even without this patch, after your first patch that implements
> `git commit --trailer ...`, it should be easy to setup something less
> verbose and less error-prone.
It is nice that it makes the complexity of 2/2 unnecessary ;-)
There was a problem hiding this comment.
On the Git mailing list, ZheNing Hu wrote (reply to this):
Junio C Hamano <gitster@pobox.com> 于2021年3月22日周一 上午12:52写道：
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> >> "Signed-off-by:C O <Mister@email.com>" is often too
> >> verbose and error-prone.
> >
> > The 'trailer.<token>.command' config option can already be used to
> > help with that, for example:
> >
> > -------
> > $ 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>'"
> > $ git interpret-trailers --trailer sign=Linus<<EOF
> > subject
> >
> > body
> > EOF
> > subject
> >
> > body
> >
> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> > -------
> >
> > So even without this patch, after your first patch that implements
> > `git commit --trailer ...`, it should be easy to setup something less
> > verbose and less error-prone.
>
> It is nice that it makes the complexity of 2/2 unnecessary ;-)
A little frustrated, both `--own-identity` and `@nickname` seem to be
rejected. I will roll back to the first patch.
There was a problem hiding this comment.
On the Git mailing list, Christian Couder wrote (reply to this):
On Mon, Mar 22, 2021 at 1:39 AM ZheNing Hu <adlternative@gmail.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> 于2021年3月22日周一 上午12:52写道：
> >
> > Christian Couder <christian.couder@gmail.com> writes:
> > > So even without this patch, after your first patch that implements
> > > `git commit --trailer ...`, it should be easy to setup something less
> > > verbose and less error-prone.
> >
> > It is nice that it makes the complexity of 2/2 unnecessary ;-)
>
> A little frustrated, both `--own-identity` and `@nickname` seem to be
> rejected. I will roll back to the first patch.
It's good to want to implement new features, and to come up quickly
with patches, but it's also good to give people time on this list to
discuss the goals you would like to achieve, and which features are
already available or should be implement to achieve the goals.
Thanks for working on this topic anyway!
There was a problem hiding this comment.
On the Git mailing list, ZheNing Hu wrote (reply to this):
Christian Couder <christian.couder@gmail.com> 于2021年3月22日周一 下午3:26写道：
>
> On Mon, Mar 22, 2021 at 1:39 AM ZheNing Hu <adlternative@gmail.com> wrote:
> >
> > Junio C Hamano <gitster@pobox.com> 于2021年3月22日周一 上午12:52写道：
> > >
> > > Christian Couder <christian.couder@gmail.com> writes:
>
> > > > So even without this patch, after your first patch that implements
> > > > `git commit --trailer ...`, it should be easy to setup something less
> > > > verbose and less error-prone.
> > >
> > > It is nice that it makes the complexity of 2/2 unnecessary ;-)
> >
> > A little frustrated, both `--own-identity` and `@nickname` seem to be
> > rejected. I will roll back to the first patch.
>
> It's good to want to implement new features, and to come up quickly
> with patches, but it's also good to give people time on this list to
> discuss the goals you would like to achieve, and which features are
> already available or should be implement to achieve the goals.
>
I basically started coding after receiving the first reviewer's comment.
It seems that I should be more patient.
> Thanks for working on this topic anyway!
Thanks, Christian.
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
ZheNing Hu <adlternative@gmail.com> writes:
> Junio C Hamano <gitster@pobox.com> 于2021年3月22日周一 上午12:52写道：
>>
>> Christian Couder <christian.couder@gmail.com> writes:
>> ...
>> > So even without this patch, after your first patch that implements
>> > `git commit --trailer ...`, it should be easy to setup something less
>> > verbose and less error-prone.
>>
>> It is nice that it makes the complexity of 2/2 unnecessary ;-)
>
> A little frustrated, both `--own-identity` and `@nickname` seem to be
> rejected. I will roll back to the first patch.
If existing functionality can be used to achieve the same end result
without any extra effort by end-users, that's a happy endgame, isn't
it? There isn't a reason to be frustrated---the users benefit with
the new --trailer option without adding (hence having to learn) new
extra features like --own-ident or @name, which is a big plus.
There was a problem hiding this comment.
On the Git mailing list, ZheNing Hu wrote (reply to this):
Junio C Hamano <gitster@pobox.com> 于2021年3月23日周二 上午1:07写道：
>
> ZheNing Hu <adlternative@gmail.com> writes:
>
> > Junio C Hamano <gitster@pobox.com> 于2021年3月22日周一 上午12:52写道：
> >>
> >> Christian Couder <christian.couder@gmail.com> writes:
> >> ...
> >> > So even without this patch, after your first patch that implements
> >> > `git commit --trailer ...`, it should be easy to setup something less
> >> > verbose and less error-prone.
> >>
> >> It is nice that it makes the complexity of 2/2 unnecessary ;-)
> >
> > A little frustrated, both `--own-identity` and `@nickname` seem to be
> > rejected. I will roll back to the first patch.
>
> If existing functionality can be used to achieve the same end result
> without any extra effort by end-users, that's a happy endgame, isn't
> it? There isn't a reason to be frustrated---the users benefit with
> the new --trailer option without adding (hence having to learn) new
> extra features like --own-ident or @name, which is a big plus.
>
>
Thanks, Junio, you are right :)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hope this can help
commit --trailermore useful!Base on #901,
The original patch series was too long so now split it into two.
Helped by Junio and Jeff, Thanks.
cc: Bradley M. Kuhn bkuhn@sfconservancy.org
cc: Junio C Hamano gitster@pobox.com
cc: Brandon Casey drafnel@gmail.com
cc: Shourya Shukla periperidip@gmail.com
cc: Christian Couder christian.couder@gmail.com
cc: Rafael Silva rafaeloliveira.cs@gmail.com
cc: Đoàn Trần Công Danh congdanhqx@gmail.com
cc: Jeff King peff@peff.net
cc: Ævar Arnfjörð Bjarmason avarab@gmail.com
cc: Bagas Sanjaya bagasdotme@gmail.com