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

2012-09-27 Thread Junio C Hamano
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:

 Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@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 ;-)
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2012-09-27 Thread Jeff King
On Thu, Sep 27, 2012 at 10:09:28AM -0700, Junio C Hamano wrote:

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

I also found the name confusing on first-read. While --author is an
example in one direction, the fact that --grep is not called --body
is a counter-example.

I'd much rather see it as --grep-reflog or something. You could also
do --grep-reflog-message, which would match a later
--grep-reflog-author, but I am not sure anybody would want the latter,
and it makes the current name a lot longer.

I actually think just checking the reflog when we call --grep would
the most common workflow, and requires no extra work from the user.  My
only hesitation is that if somebody _does_ want to distinguish, there's
no escape hatch. Of course, the reflog walker is already full of such
weird conflations (e.g., the rewriting of parent and date information).

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2012-09-27 Thread Junio C Hamano
Jeff King p...@peff.net writes:

 I actually think just checking the reflog when we call --grep would
 the most common workflow, and requires no extra work from the user.  My
 only hesitation is that if somebody _does_ want to distinguish, there's
 no escape hatch. Of course, the reflog walker is already full of such
 weird conflations (e.g., the rewriting of parent and date information).

Yes, that reasoning more-or-less matches the reason why I said we do
not necessarily want to go fancier.  But log -g output already
makes it fairly clear that the additional information is not part of
the regular log and is a some header-like thingy, so I actually am
OK with --reflog-grep and --grep being different.

The output from log --show-notes, on the other hand, is even more
conflated and a casual user would view it as part of the message, so
I would imagine that if we ever do the extention to cover notes
data, the normal --grep should apply to it.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html