Re: [PATCH 2/3] revision: add --grep-reflog to filter commits by reflog messages
On Sat, Sep 29, 2012 at 12:54 PM, Junio C Hamano gits...@pobox.com wrote: I like how callers not doing a reflog walk do not have to pay the price to do the extra allocating. We could further limit it to only when --grep-reflog is in effect, but I guess that would mean wading through grep_filter's patterns, since it could be buried amidst ANDs and ORs? One alternative would be to set a bit in the grep_opt when we call append_header_grep_pattern. It feels a bit like a layering violation, though. I guess the bit could also go into rev_info. It may not even be a measurable slowdown, though. Premature optimization and all that. I do not think it is a layering violation. compile_grep_exp() should be aware of the short-cut possibilities and your our expression is interested in reflog so we need to read it is very similar in spirit to the existing opt-extended bit. It will obviously allow us to avoid reading reflog information unnecessarily here. I think it makes perfect sense. reflog, in terms of both the number of commits and message length, is usually short enough that slowdown does not really show, especially when used with git-log, an interactive command. Without the changes: $ time git log -g --grep . /dev/null real0m0.480s user0m0.451s sys 0m0.025s With the changes: $ time ./git log -g --grep . /dev/null real0m0.490s user0m0.471s sys 0m0.018s We may also want to flag the use of the --grep-reflog option when the --walk-reflogs option is not in effect in setup_revisions() as an error, or something. That's why I put Ignored unless --walk-reflogs is given in the document. But an error would be fine too. I suppose an error is preferable in case users do not read document carefully? -- Duy -- 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 2/3] revision: add --grep-reflog to filter commits by reflog messages
On Fri, Sep 28, 2012 at 10:54:29PM -0700, Junio C Hamano wrote:
Jeff King p...@peff.net writes:
+ if (opt-reflog_info) {
+ strbuf_addstr(buf, reflog );
+ get_reflog_message(buf, opt-reflog_info);
+ strbuf_addch(buf, '\n');
+ strbuf_addstr(buf, commit-buffer);
+ }
+ if (buf.len)
+ retval = grep_buffer(opt-grep_filter, buf.buf, buf.len);
+ else
+ retval = grep_buffer(opt-grep_filter,
+ commit-buffer, strlen(commit-buffer));
+ strbuf_release(buf);
+ return retval;
I like how callers not doing a reflog walk do not have to pay the price
to do the extra allocating. We could further limit it to only when
--grep-reflog is in effect, but I guess that would mean wading through
grep_filter's patterns, since it could be buried amidst ANDs and ORs?
One alternative would be to set a bit in the grep_opt when we call
append_header_grep_pattern. It feels a bit like a layering violation,
though. I guess the bit could also go into rev_info. It may not even be
a measurable slowdown, though. Premature optimization and all that.
I do not think it is a layering violation. compile_grep_exp()
should be aware of the short-cut possibilities and your our
expression is interested in reflog so we need to read it is very
similar in spirit to the existing opt-extended bit.
Hmm. Yeah, I guess so. I was thinking that the grep code did not know
there was a commit or reflog involved at all (we just pass it a buffer,
and how we prepare it is our business), but it does already know about
the magic GREP_HEADER_* variables, and this is definitely part of that.
We may also want to flag the use of the --grep-reflog option when
the --walk-reflogs option is not in effect in setup_revisions() as
an error, or something.
Good point. I think the docs in the patch just say it is ignored unless
walking, but it would be better to flag the error.
-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 2/3] revision: add --grep-reflog to filter commits by reflog messages
Nguyen Thai Ngoc Duy pclo...@gmail.com writes:
reflog, in terms of both the number of commits and message length, is
usually short enough that slowdown does not really show, especially
when used with git-log, an interactive command.
You shouldn't do things you can easily tell you do not need to,
especially when the effort to avoid doing unnecessary allocation,
copying and deallocation is not excessively larger than the saving.
I personally think that lack of perceived performance impact is not
an excuse to be sloppy. These things tend to add up.
That's why I put Ignored unless --walk-reflogs is given in the
document. But an error would be fine too. I suppose an error is
preferable in case users do not read document carefully?
A reader who reads documentation carefully will spot that it is a
sloppy coding that such a nonsense combination of two options are
not caught and diagnosed as an error, when accepting and silently
ignoring the option would not help anybody.
An alternative may be to turn -g on when --grep-reflog is given.
Starting from a version that forbids --grep-reflog without -g will
let us change the command line parser to do so in the future without
backward incompatibility, but you cannot do so if you start from a
version that accepts and silently ignores.
How about squashing this in? I've future-proofed commit_match() a
bit while at it; it would be cleaner to add new fake headers if the
function is done this way.
Documentation/rev-list-options.txt | 4 ++--
grep.c | 2 ++
grep.h | 1 +
revision.c | 13 +++--
t/t7810-grep.sh| 4
5 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/Documentation/rev-list-options.txt
b/Documentation/rev-list-options.txt
index aa7cd9d..ca22106 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -56,8 +56,8 @@ endif::git-rev-list[]
Limit the commits output to ones with reflog entries that
match the specified pattern (regular expression). With
more than one `--grep-reflog`, commits whose reflog message
- matches any of the given patterns are chosen. Ignored unless
- `--walk-reflogs` is given.
+ matches any of the given patterns are chosen. It is an
+ error to use this option unless `--walk-reflogs` is in use.
--grep=pattern::
diff --git a/grep.c b/grep.c
index d70dcdf..edc7776 100644
--- a/grep.c
+++ b/grep.c
@@ -64,6 +64,8 @@ void append_header_grep_pattern(struct grep_opt *opt,
{
struct grep_pat *p = create_grep_pat(pat, strlen(pat), header, 0,
GREP_PATTERN_HEAD, field);
+ if (field == GREP_HEADER_REFLOG)
+ opt-use_reflog_filter = 1;
do_append_grep_pat(opt-header_tail, p);
}
diff --git a/grep.h b/grep.h
index 6e78b96..c256ac6 100644
--- a/grep.h
+++ b/grep.h
@@ -107,6 +107,7 @@ struct grep_opt {
#define GREP_BINARY_TEXT 2
int binary;
int extended;
+ int use_reflog_filter;
int pcre;
int relative;
int pathname;
diff --git a/revision.c b/revision.c
index 109bec1..9ad72df 100644
--- a/revision.c
+++ b/revision.c
@@ -1908,6 +1908,8 @@ int setup_revisions(int argc, const char **argv, struct
rev_info *revs, struct s
if (revs-reflog_info revs-graph)
die(cannot combine --walk-reflogs with --graph);
+ if (!revs-reflog_info revs-grep_filter.use_reflog_filter)
+ die(cannot use --grep-reflog without --walk-reflogs);
return left;
}
@@ -2217,12 +2219,19 @@ static int commit_match(struct commit *commit, struct
rev_info *opt)
struct strbuf buf = STRBUF_INIT;
if (!opt-grep_filter.pattern_list !opt-grep_filter.header_list)
return 1;
- if (opt-reflog_info) {
+
+ /* Prepend fake headers as needed */
+ if (opt-grep_filter.use_reflog_filter) {
strbuf_addstr(buf, reflog );
get_reflog_message(buf, opt-reflog_info);
strbuf_addch(buf, '\n');
- strbuf_addstr(buf, commit-buffer);
}
+
+ /* Copy the commit to temporary if we are using fake headers */
+ if (buf.len)
+ strbuf_addstr(buf, commit-buffer);
+
+ /* Find either in the commit object, or in the temporary */
if (buf.len)
retval = grep_buffer(opt-grep_filter, buf.buf, buf.len);
else
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 3a5d0fd..f698001 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -572,6 +572,10 @@ test_expect_success 'log grep (9)' '
test_cmp expect actual
'
+test_expect_success 'log --grep-reflog can only be used under -g' '
+ test_must_fail git log --grep-reflog=commit: third
+'
+
test_expect_success 'log with multiple --grep uses union' '
git log --grep=i --grep=r
[PATCH 2/3] revision: add --grep-reflog to filter commits by reflog messages
Similar to --author/--committer which filters commits by author and
committer header fields. --grep-reflog adds a fake reflog header to
commit and a grep filter to search on that line.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
On Fri, Sep 28, 2012 at 12:09 AM, Junio C Hamano gits...@pobox.com
wrote:
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?
Fixed.
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 did not. I do now, tests are also added.
On Fri, Sep 28, 2012 at 12:28 AM, Jeff King p...@peff.net wrote:
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.
Changed it to --grep-reflog. I was tempted to add --grep-* but I
can't error out if user gives an invalid header. So --grep-reflog
only for now.
Documentation/rev-list-options.txt | 8
revision.c | 20 ++--
t/t7810-grep.sh| 26 ++
3 files changed, 52 insertions(+), 2 deletions(-)
diff --git a/Documentation/rev-list-options.txt
b/Documentation/rev-list-options.txt
index 1fc2a18..aa7cd9d 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -51,6 +51,14 @@ endif::git-rev-list[]
commits whose author matches any of the given patterns are
chosen (similarly for multiple `--committer=pattern`).
+--grep-reflog=pattern::
+
+ Limit the commits output to ones with reflog entries that
+ match the specified pattern (regular expression). With
+ more than one `--grep-reflog`, commits whose reflog message
+ matches any of the given patterns are chosen. Ignored unless
+ `--walk-reflogs` is given.
+
--grep=pattern::
Limit the commits output to ones with log message that
diff --git a/revision.c b/revision.c
index f7cf385..cfa0e2e 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, committer, optarg);
return argcount;
+ } else if ((argcount = parse_long_opt(grep-reflog, argv, optarg))) {
+ add_header_grep(revs, reflog, optarg);
+ return argcount;
} else if ((argcount = parse_long_opt(grep, argv, optarg))) {
add_message_grep(revs, optarg);
return argcount;
@@ -2210,10 +2213,23 @@ static int rewrite_parents(struct rev_info *revs,
struct commit *commit)
static int commit_match(struct commit *commit, struct rev_info *opt)
{
+ int retval;
+ struct strbuf buf = STRBUF_INIT;
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) {
+ strbuf_addstr(buf, reflog );
+ get_reflog_message(buf, opt-reflog_info);
+ strbuf_addch(buf, '\n');
+ strbuf_addstr(buf, commit-buffer);
+ }
+ if (buf.len)
+ retval = grep_buffer(opt-grep_filter, buf.buf, buf.len);
+ else
+ retval = grep_buffer(opt-grep_filter,
+commit-buffer, strlen(commit-buffer));
+ strbuf_release(buf);
+ return retval;
}
static inline int want_ancestry(struct rev_info *revs)
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 91db352..f42a605 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -546,6 +546,32 @@ test_expect_success 'log grep (6)' '
test_cmp expect actual
'
+test_expect_success 'log grep (7)' '
+ git log -g --grep-reflog=commit: third --pretty=tformat:%s actual
+ echo third expect
+ test_cmp expect actual
+'
+
+test_expect_success 'log grep (8)' '
+ git log -g --grep-reflog=commit:
Re: [PATCH 2/3] revision: add --grep-reflog to filter commits by reflog messages
On Sat, Sep 29, 2012 at 12:55 AM, Junio C Hamano gits...@pobox.com wrote: For that to happen, the code _must_ know what kind of headers we would support; discarding the existing enum is going in a wrong direction. Or what kind of manipulation is required for a header. The caller can decide if it wants such manipulation or not. Somebody might want to grep committer's date, for example. When we introduce git log --header=frotz:xyzzy option that looks for a generic frotz header and tries to see if xyzzy textually appears in the field, we may want to add a generic this is not a header that we have special treatment for enum to the mix. But for known kinds of headers, we would need a list of what each header is and what semantics it wants. So please reconsider undoing [1/3], and rerolling [2/3] that depends on it. Done. The enum is kept. I added a few tests about grepping timestamp in 1/3 to keep people (or myself) from making the same mistake I did. 3/3 is reposted for completeness, I don't care much about notes (so far). It's up to you to take or drop it. Nguyễn Thái Ngọc Duy (3): grep: prepare for new header field filter revision: add --grep-reflog to filter commits by reflog messages revision: make --grep search in notes too if shown Documentation/rev-list-options.txt | 8 grep.c | 10 +- grep.h | 7 +-- revision.c | 26 -- t/t7810-grep.sh| 38 ++ 5 files changed, 84 insertions(+), 5 deletions(-) -- 1.7.12.1.406.g6ab07c4 -- 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
[PATCH 2/3] revision: add --grep-reflog to filter commits by reflog messages
Similar to --author/--committer which filters commits by author and
committer header fields. --grep-reflog adds a fake reflog header to
commit and a grep filter to search on that line.
All rules to --author/--committer apply except no timestamp stripping.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
Documentation/rev-list-options.txt | 8
grep.c | 1 +
grep.h | 1 +
revision.c | 20 ++--
t/t7810-grep.sh| 26 ++
5 files changed, 54 insertions(+), 2 deletions(-)
diff --git a/Documentation/rev-list-options.txt
b/Documentation/rev-list-options.txt
index 1fc2a18..aa7cd9d 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -51,6 +51,14 @@ endif::git-rev-list[]
commits whose author matches any of the given patterns are
chosen (similarly for multiple `--committer=pattern`).
+--grep-reflog=pattern::
+
+ Limit the commits output to ones with reflog entries that
+ match the specified pattern (regular expression). With
+ more than one `--grep-reflog`, commits whose reflog message
+ matches any of the given patterns are chosen. Ignored unless
+ `--walk-reflogs` is given.
+
--grep=pattern::
Limit the commits output to ones with log message that
diff --git a/grep.c b/grep.c
index 8d73995..d70dcdf 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 d54adbe..6e78b96 100644
--- a/grep.h
+++ b/grep.h
@@ -30,6 +30,7 @@ enum grep_context {
enum grep_header_field {
GREP_HEADER_AUTHOR = 0,
GREP_HEADER_COMMITTER,
+ GREP_HEADER_REFLOG,
/* Must be at the end of the enum */
GREP_HEADER_FIELD_MAX
diff --git a/revision.c b/revision.c
index ae12e11..109bec1 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(grep-reflog, 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;
@@ -2210,10 +2213,23 @@ static int rewrite_parents(struct rev_info *revs,
struct commit *commit)
static int commit_match(struct commit *commit, struct rev_info *opt)
{
+ int retval;
+ struct strbuf buf = STRBUF_INIT;
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) {
+ strbuf_addstr(buf, reflog );
+ get_reflog_message(buf, opt-reflog_info);
+ strbuf_addch(buf, '\n');
+ strbuf_addstr(buf, commit-buffer);
+ }
+ if (buf.len)
+ retval = grep_buffer(opt-grep_filter, buf.buf, buf.len);
+ else
+ retval = grep_buffer(opt-grep_filter,
+commit-buffer, strlen(commit-buffer));
+ strbuf_release(buf);
+ return retval;
}
static inline int want_ancestry(struct rev_info *revs)
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 30eaa9a..3a5d0fd 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -546,6 +546,32 @@ test_expect_success 'log grep (6)' '
test_cmp expect actual
'
+test_expect_success 'log grep (7)' '
+ git log -g --grep-reflog=commit: third --pretty=tformat:%s actual
+ echo third expect
+ test_cmp expect actual
+'
+
+test_expect_success 'log grep (8)' '
+ git log -g --grep-reflog=commit: third --grep-reflog=commit: second
--pretty=tformat:%s actual
+ {
+ echo third echo second
+ } expect
+ test_cmp expect actual
+'
+
+test_expect_success 'log grep (9)' '
+ git log -g --grep-reflog=commit: third --author=Thor
--pretty=tformat:%s actual
+ echo third expect
+ test_cmp expect actual
+'
+
+test_expect_success 'log grep (9)' '
+ git log -g --grep-reflog=commit: third --author=non-existant
--pretty=tformat:%s actual
+ : expect
+ test_cmp expect actual
+'
+
test_expect_success 'log with multiple --grep uses union' '
git log --grep=i --grep=r --format=%s actual
{
--
1.7.12.1.406.g6ab07c4
--
To unsubscribe from this list: send the line
Re: [PATCH 2/3] revision: add --grep-reflog to filter commits by reflog messages
Jeff King p...@peff.net writes:
+if (opt-reflog_info) {
+strbuf_addstr(buf, reflog );
+get_reflog_message(buf, opt-reflog_info);
+strbuf_addch(buf, '\n');
+strbuf_addstr(buf, commit-buffer);
+}
+if (buf.len)
+retval = grep_buffer(opt-grep_filter, buf.buf, buf.len);
+else
+retval = grep_buffer(opt-grep_filter,
+ commit-buffer, strlen(commit-buffer));
+strbuf_release(buf);
+return retval;
I like how callers not doing a reflog walk do not have to pay the price
to do the extra allocating. We could further limit it to only when
--grep-reflog is in effect, but I guess that would mean wading through
grep_filter's patterns, since it could be buried amidst ANDs and ORs?
One alternative would be to set a bit in the grep_opt when we call
append_header_grep_pattern. It feels a bit like a layering violation,
though. I guess the bit could also go into rev_info. It may not even be
a measurable slowdown, though. Premature optimization and all that.
I do not think it is a layering violation. compile_grep_exp()
should be aware of the short-cut possibilities and your our
expression is interested in reflog so we need to read it is very
similar in spirit to the existing opt-extended bit.
It will obviously allow us to avoid reading reflog information
unnecessarily here. I think it makes perfect sense.
We may also want to flag the use of the --grep-reflog option when
the --walk-reflogs option is not in effect in setup_revisions() as
an error, or something.
--
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
