Re: [PATCH] revision: add --reflog-message to grep reflog messages

[Junio C Hamano <gitster@pobox.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.

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

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

> 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 ;-)