Re: [PATCH] revision: add --reflog-message to grep reflog messages
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 27, 2012, 17:09 UTC
- Message-ID
- <7v4nmjs88n.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1348745786-27197-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > ---
Plase explain yourself in the space above.
Show 13 quoted lines
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt > index 1fc2a18..aeaa58c 100644 > --- a/Documentation/rev-list-options.txt > +++ b/Documentation/rev-list-options.txt > @@ -51,6 +51,11 @@ endif::git-rev-list[] > commits whose author matches any of the given patterns are > chosen (similarly for multiple `--committer=<pattern>`). > > +--reflog-message=<pattern>:: > + Limit the commits output to ones with reflog entries that > + match the specified pattern (regular expression). Ignored unless > + --walk-reflogs is given. > +
I am debating myself if it is sane for this option to have no hint that it is about "limiting" in its name. "--author/--committer" don't and it is clear from the context of the command that they are not about setting author/committer, so "--reflog-message" may be interpreted the same, perhaps.
The entry in the context above talks about multiple occurrence of that option. Shouldn't this new one also say what happens when it is given twice?
Show 26 quoted lines
> diff --git a/grep.c b/grep.c
> index 898be6e..72ac1bf 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -697,6 +697,7 @@ static struct {
> } header_field[] = {
> { "author ", 7 },
> { "committer ", 10 },
> + { "reflog ", 7 },
> };
>
> static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,
> diff --git a/grep.h b/grep.h
> index 8a28a67..1416ad7 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -29,9 +29,10 @@ enum grep_context {
>
> enum grep_header_field {
> GREP_HEADER_AUTHOR = 0,
> - GREP_HEADER_COMMITTER
> + GREP_HEADER_COMMITTER,
> + GREP_HEADER_REFLOG,
> + GREP_HEADER_FIELD_MAX
> };
> -#define GREP_HEADER_FIELD_MAX (GREP_HEADER_COMMITTER + 1)Please add comment to ensure that FIELD_MAX stays at the end; if you ensure that, the result is much better than the original "we know committer is at the end so add one".
I think I wrote prep_header_patterns() and compile_grep_patterns() carefully enough not to assume the headers are only the author and committer names, so the various combinations i.e. all-match, author(s), committer(s), grep(s), and reflog-message(s), should work out of the box, but have you actually tested them?
I do not know offhand the matching side is prepared to take random garbage fields. IIRC, we strip the trailing timestamp from committer and author header lines when we match, and a new code needs to be added to control when that stripping should / should not kick in depending on the header.
Show 35 quoted lines
> diff --git a/revision.c b/revision.c
> index ae12e11..837051c 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1595,6 +1595,9 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
> } else if ((argcount = parse_long_opt("committer", argv, &optarg))) {
> add_header_grep(revs, GREP_HEADER_COMMITTER, optarg);
> return argcount;
> + } else if ((argcount = parse_long_opt("reflog-message", argv, &optarg))) {
> + add_header_grep(revs, GREP_HEADER_REFLOG, optarg);
> + return argcount;
> } else if ((argcount = parse_long_opt("grep", argv, &optarg))) {
> add_message_grep(revs, optarg);
> return argcount;
> @@ -2212,8 +2215,20 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
> {
> if (!opt->grep_filter.pattern_list && !opt->grep_filter.header_list)
> return 1;
> - return grep_buffer(&opt->grep_filter,
> - commit->buffer, strlen(commit->buffer));
> + if (opt->reflog_info) {
> + int retval;
> + struct strbuf buf = STRBUF_INIT;
> + strbuf_addstr(&buf, "reflog ");
> + get_reflog_message(&buf, opt->reflog_info);
> + strbuf_addch(&buf, '\n');
> + strbuf_addstr(&buf, commit->buffer);
> + retval = grep_buffer(&opt->grep_filter, buf.buf, buf.len);
> + strbuf_release(&buf);
> + return retval;
> + } else {
> + return grep_buffer(&opt->grep_filter,
> + commit->buffer, strlen(commit->buffer));
> + }
> }This part looks familiar and smells sane ;-)