* [PATCH 1/6] grep: move configuration support to top-level grep.[ch]
2012-10-04 1:33 ` [PATCH 0/6] Tying loose ends of extended "grep" Junio C Hamano
@ 2012-10-04 1:33 ` Junio C Hamano
2012-10-04 1:33 ` [PATCH 2/6] grep: move pattern-type bits " Junio C Hamano
` (4 subsequent siblings)
5 siblings, 0 replies; 23+ messages in thread
From: Junio C Hamano @ 2012-10-04 1:33 UTC (permalink / raw)
To: git
As "git grep" will not stay to be the only command that will know
about the grep machinery, move these to a more appropriate place.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/grep.c | 67 ----------------------------------------------------------
grep.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
grep.h | 2 ++
3 files changed, 69 insertions(+), 67 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 82530a6..ce379d5 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -261,21 +261,6 @@ static int wait_all(void)
}
#endif
-static int parse_pattern_type_arg(const char *opt, const char *arg)
-{
- if (!strcmp(arg, "default"))
- return GREP_PATTERN_TYPE_UNSPECIFIED;
- else if (!strcmp(arg, "basic"))
- return GREP_PATTERN_TYPE_BRE;
- else if (!strcmp(arg, "extended"))
- return GREP_PATTERN_TYPE_ERE;
- else if (!strcmp(arg, "fixed"))
- return GREP_PATTERN_TYPE_FIXED;
- else if (!strcmp(arg, "perl"))
- return GREP_PATTERN_TYPE_PCRE;
- die("bad %s argument: %s", opt, arg);
-}
-
static void grep_pattern_type_options(const int pattern_type, struct grep_opt *opt)
{
switch (pattern_type) {
@@ -308,58 +293,6 @@ static void grep_pattern_type_options(const int pattern_type, struct grep_opt *o
}
}
-static int grep_config(const char *var, const char *value, void *cb)
-{
- struct grep_opt *opt = cb;
- char *color = NULL;
-
- if (userdiff_config(var, value) < 0)
- return -1;
-
- if (!strcmp(var, "grep.extendedregexp")) {
- if (git_config_bool(var, value))
- opt->extended_regexp_option = 1;
- else
- opt->extended_regexp_option = 0;
- return 0;
- }
-
- if (!strcmp(var, "grep.patterntype")) {
- opt->pattern_type_option = parse_pattern_type_arg(var, value);
- return 0;
- }
-
- if (!strcmp(var, "grep.linenumber")) {
- opt->linenum = git_config_bool(var, value);
- return 0;
- }
-
- if (!strcmp(var, "color.grep"))
- opt->color = git_config_colorbool(var, value);
- else if (!strcmp(var, "color.grep.context"))
- color = opt->color_context;
- else if (!strcmp(var, "color.grep.filename"))
- color = opt->color_filename;
- else if (!strcmp(var, "color.grep.function"))
- color = opt->color_function;
- else if (!strcmp(var, "color.grep.linenumber"))
- color = opt->color_lineno;
- else if (!strcmp(var, "color.grep.match"))
- color = opt->color_match;
- else if (!strcmp(var, "color.grep.selected"))
- color = opt->color_selected;
- else if (!strcmp(var, "color.grep.separator"))
- color = opt->color_sep;
- else
- return git_color_default_config(var, value, cb);
- if (color) {
- if (!value)
- return config_error_nonbool(var);
- color_parse(value, var, color);
- }
- return 0;
-}
-
static void *lock_and_read_sha1_file(const unsigned char *sha1, enum object_type *type, unsigned long *size)
{
void *data;
diff --git a/grep.c b/grep.c
index edc7776..551a2ed 100644
--- a/grep.c
+++ b/grep.c
@@ -1518,3 +1518,70 @@ static int grep_source_is_binary(struct grep_source *gs)
return 0;
}
+
+static int parse_pattern_type_arg(const char *opt, const char *arg)
+{
+ if (!strcmp(arg, "default"))
+ return GREP_PATTERN_TYPE_UNSPECIFIED;
+ else if (!strcmp(arg, "basic"))
+ return GREP_PATTERN_TYPE_BRE;
+ else if (!strcmp(arg, "extended"))
+ return GREP_PATTERN_TYPE_ERE;
+ else if (!strcmp(arg, "fixed"))
+ return GREP_PATTERN_TYPE_FIXED;
+ else if (!strcmp(arg, "perl"))
+ return GREP_PATTERN_TYPE_PCRE;
+ die("bad %s argument: %s", opt, arg);
+}
+
+int grep_config(const char *var, const char *value, void *cb)
+{
+ struct grep_opt *opt = cb;
+ char *color = NULL;
+
+ if (userdiff_config(var, value) < 0)
+ return -1;
+
+ if (!strcmp(var, "grep.extendedregexp")) {
+ if (git_config_bool(var, value))
+ opt->extended_regexp_option = 1;
+ else
+ opt->extended_regexp_option = 0;
+ return 0;
+ }
+
+ if (!strcmp(var, "grep.patterntype")) {
+ opt->pattern_type_option = parse_pattern_type_arg(var, value);
+ return 0;
+ }
+
+ if (!strcmp(var, "grep.linenumber")) {
+ opt->linenum = git_config_bool(var, value);
+ return 0;
+ }
+
+ if (!strcmp(var, "color.grep"))
+ opt->color = git_config_colorbool(var, value);
+ else if (!strcmp(var, "color.grep.context"))
+ color = opt->color_context;
+ else if (!strcmp(var, "color.grep.filename"))
+ color = opt->color_filename;
+ else if (!strcmp(var, "color.grep.function"))
+ color = opt->color_function;
+ else if (!strcmp(var, "color.grep.linenumber"))
+ color = opt->color_lineno;
+ else if (!strcmp(var, "color.grep.match"))
+ color = opt->color_match;
+ else if (!strcmp(var, "color.grep.selected"))
+ color = opt->color_selected;
+ else if (!strcmp(var, "color.grep.separator"))
+ color = opt->color_sep;
+ else
+ return git_color_default_config(var, value, cb);
+ if (color) {
+ if (!value)
+ return config_error_nonbool(var);
+ color_parse(value, var, color);
+ }
+ return 0;
+}
diff --git a/grep.h b/grep.h
index c256ac6..5381adc 100644
--- a/grep.h
+++ b/grep.h
@@ -145,6 +145,8 @@ extern void compile_grep_patterns(struct grep_opt *opt);
extern void free_grep_patterns(struct grep_opt *opt);
extern int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size);
+int grep_config(const char *var, const char *value, void *cb);
+
struct grep_source {
char *name;
--
1.8.0.rc0.57.g712528f
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 2/6] grep: move pattern-type bits support to top-level grep.[ch]
2012-10-04 1:33 ` [PATCH 0/6] Tying loose ends of extended "grep" Junio C Hamano
2012-10-04 1:33 ` [PATCH 1/6] grep: move configuration support to top-level grep.[ch] Junio C Hamano
@ 2012-10-04 1:33 ` Junio C Hamano
2012-10-04 1:33 ` [PATCH 3/6] log --grep: use the same helper to set -E/-F options as "git grep" Junio C Hamano
` (3 subsequent siblings)
5 siblings, 0 replies; 23+ messages in thread
From: Junio C Hamano @ 2012-10-04 1:33 UTC (permalink / raw)
To: git
Switching between -E/-G/-P/-F correctly needs a lot more than just
flipping opt->regflags bit these days, and we have a nice helper
function buried in builtin/grep.c for the sole use of "git grep".
Extract it so that "log --grep" family can also use it.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/grep.c | 38 +++-----------------------------------
grep.c | 32 ++++++++++++++++++++++++++++++++
grep.h | 1 +
3 files changed, 36 insertions(+), 35 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index ce379d5..2b14fee 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -261,38 +261,6 @@ static int wait_all(void)
}
#endif
-static void grep_pattern_type_options(const int pattern_type, struct grep_opt *opt)
-{
- switch (pattern_type) {
- case GREP_PATTERN_TYPE_UNSPECIFIED:
- /* fall through */
-
- case GREP_PATTERN_TYPE_BRE:
- opt->fixed = 0;
- opt->pcre = 0;
- opt->regflags &= ~REG_EXTENDED;
- break;
-
- case GREP_PATTERN_TYPE_ERE:
- opt->fixed = 0;
- opt->pcre = 0;
- opt->regflags |= REG_EXTENDED;
- break;
-
- case GREP_PATTERN_TYPE_FIXED:
- opt->fixed = 1;
- opt->pcre = 0;
- opt->regflags &= ~REG_EXTENDED;
- break;
-
- case GREP_PATTERN_TYPE_PCRE:
- opt->fixed = 0;
- opt->pcre = 1;
- opt->regflags &= ~REG_EXTENDED;
- break;
- }
-}
-
static void *lock_and_read_sha1_file(const unsigned char *sha1, enum object_type *type, unsigned long *size)
{
void *data;
@@ -810,11 +778,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
PARSE_OPT_NO_INTERNAL_HELP);
if (pattern_type_arg != GREP_PATTERN_TYPE_UNSPECIFIED)
- grep_pattern_type_options(pattern_type_arg, &opt);
+ grep_set_pattern_type_option(pattern_type_arg, &opt);
else if (opt.pattern_type_option != GREP_PATTERN_TYPE_UNSPECIFIED)
- grep_pattern_type_options(opt.pattern_type_option, &opt);
+ grep_set_pattern_type_option(opt.pattern_type_option, &opt);
else if (opt.extended_regexp_option)
- grep_pattern_type_options(GREP_PATTERN_TYPE_ERE, &opt);
+ grep_set_pattern_type_option(GREP_PATTERN_TYPE_ERE, &opt);
if (use_index && !startup_info->have_repository)
/* die the same way as if we did it at the beginning */
diff --git a/grep.c b/grep.c
index 551a2ed..0d8df65 100644
--- a/grep.c
+++ b/grep.c
@@ -1585,3 +1585,35 @@ int grep_config(const char *var, const char *value, void *cb)
}
return 0;
}
+
+void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)
+{
+ switch (pattern_type) {
+ case GREP_PATTERN_TYPE_UNSPECIFIED:
+ /* fall through */
+
+ case GREP_PATTERN_TYPE_BRE:
+ opt->fixed = 0;
+ opt->pcre = 0;
+ opt->regflags &= ~REG_EXTENDED;
+ break;
+
+ case GREP_PATTERN_TYPE_ERE:
+ opt->fixed = 0;
+ opt->pcre = 0;
+ opt->regflags |= REG_EXTENDED;
+ break;
+
+ case GREP_PATTERN_TYPE_FIXED:
+ opt->fixed = 1;
+ opt->pcre = 0;
+ opt->regflags &= ~REG_EXTENDED;
+ break;
+
+ case GREP_PATTERN_TYPE_PCRE:
+ opt->fixed = 0;
+ opt->pcre = 1;
+ opt->regflags &= ~REG_EXTENDED;
+ break;
+ }
+}
diff --git a/grep.h b/grep.h
index 5381adc..2f6aaa5 100644
--- a/grep.h
+++ b/grep.h
@@ -145,6 +145,7 @@ extern void compile_grep_patterns(struct grep_opt *opt);
extern void free_grep_patterns(struct grep_opt *opt);
extern int grep_buffer(struct grep_opt *opt, char *buf, unsigned long size);
+void grep_set_pattern_type_option(enum grep_pattern_type, struct grep_opt *opt);
int grep_config(const char *var, const char *value, void *cb);
struct grep_source {
--
1.8.0.rc0.57.g712528f
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 3/6] log --grep: use the same helper to set -E/-F options as "git grep"
2012-10-04 1:33 ` [PATCH 0/6] Tying loose ends of extended "grep" Junio C Hamano
2012-10-04 1:33 ` [PATCH 1/6] grep: move configuration support to top-level grep.[ch] Junio C Hamano
2012-10-04 1:33 ` [PATCH 2/6] grep: move pattern-type bits " Junio C Hamano
@ 2012-10-04 1:33 ` Junio C Hamano
2012-10-04 8:09 ` Jeff King
2012-10-04 1:33 ` [PATCH 4/6] log --grep: accept --basic-regexp and --perl-regexp Junio C Hamano
` (2 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: Junio C Hamano @ 2012-10-04 1:33 UTC (permalink / raw)
To: git
The command line option parser for "git log -F -E --grep='<ere>'"
did not flip the "fixed" bit, violating the general "last option
wins" principle among conflicting options.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
revision.c | 4 ++--
t/t4202-log.sh | 6 ++++++
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/revision.c b/revision.c
index a09e60b..7f5e53b 100644
--- a/revision.c
+++ b/revision.c
@@ -1604,12 +1604,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
} else if (!strcmp(arg, "--grep-debug")) {
revs->grep_filter.debug = 1;
} else if (!strcmp(arg, "--extended-regexp") || !strcmp(arg, "-E")) {
- revs->grep_filter.regflags |= REG_EXTENDED;
+ grep_set_pattern_type_option(GREP_PATTERN_TYPE_ERE, &revs->grep_filter);
} else if (!strcmp(arg, "--regexp-ignore-case") || !strcmp(arg, "-i")) {
revs->grep_filter.regflags |= REG_ICASE;
DIFF_OPT_SET(&revs->diffopt, PICKAXE_IGNORE_CASE);
} else if (!strcmp(arg, "--fixed-strings") || !strcmp(arg, "-F")) {
- revs->grep_filter.fixed = 1;
+ grep_set_pattern_type_option(GREP_PATTERN_TYPE_FIXED, &revs->grep_filter);
} else if (!strcmp(arg, "--all-match")) {
revs->grep_filter.all_match = 1;
} else if ((argcount = parse_long_opt("encoding", argv, &optarg))) {
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 924ba53..e6537ab 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -230,6 +230,12 @@ test_expect_success 'log --grep -i' '
test_cmp expect actual
'
+test_expect_success 'log -F -E --grep=<ere> uses ere' '
+ echo second >expect &&
+ git log -1 --pretty="tformat:%s" -F -E --grep=s.c.nd >actual &&
+ test_cmp expect actual
+'
+
cat > expect <<EOF
* Second
* sixth
--
1.8.0.rc0.57.g712528f
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 3/6] log --grep: use the same helper to set -E/-F options as "git grep"
2012-10-04 1:33 ` [PATCH 3/6] log --grep: use the same helper to set -E/-F options as "git grep" Junio C Hamano
@ 2012-10-04 8:09 ` Jeff King
0 siblings, 0 replies; 23+ messages in thread
From: Jeff King @ 2012-10-04 8:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Wed, Oct 03, 2012 at 06:33:36PM -0700, Junio C Hamano wrote:
> diff --git a/revision.c b/revision.c
> index a09e60b..7f5e53b 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1604,12 +1604,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
> } else if (!strcmp(arg, "--grep-debug")) {
> revs->grep_filter.debug = 1;
> } else if (!strcmp(arg, "--extended-regexp") || !strcmp(arg, "-E")) {
> - revs->grep_filter.regflags |= REG_EXTENDED;
> + grep_set_pattern_type_option(GREP_PATTERN_TYPE_ERE, &revs->grep_filter);
> } else if (!strcmp(arg, "--regexp-ignore-case") || !strcmp(arg, "-i")) {
> revs->grep_filter.regflags |= REG_ICASE;
> DIFF_OPT_SET(&revs->diffopt, PICKAXE_IGNORE_CASE);
> } else if (!strcmp(arg, "--fixed-strings") || !strcmp(arg, "-F")) {
> - revs->grep_filter.fixed = 1;
> + grep_set_pattern_type_option(GREP_PATTERN_TYPE_FIXED, &revs->grep_filter);
Very nice. After seeing the discussion on regexp types in your G+ feed,
I took a 5-minute look at this code last night and noticed the same
oddity. At which point I gave up looking at it for the evening, thinking
to come back to it later. And here my procrastination is rewarded. :)
-Peff
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 4/6] log --grep: accept --basic-regexp and --perl-regexp
2012-10-04 1:33 ` [PATCH 0/6] Tying loose ends of extended "grep" Junio C Hamano
` (2 preceding siblings ...)
2012-10-04 1:33 ` [PATCH 3/6] log --grep: use the same helper to set -E/-F options as "git grep" Junio C Hamano
@ 2012-10-04 1:33 ` Junio C Hamano
2012-10-04 8:12 ` Jeff King
2012-10-04 1:33 ` [PATCH 5/6] log: pass rev_info to git_log_config() Junio C Hamano
2012-10-04 1:33 ` [PATCH 6/6] log --grep: honor grep.patterntype etc. configuration variables Junio C Hamano
5 siblings, 1 reply; 23+ messages in thread
From: Junio C Hamano @ 2012-10-04 1:33 UTC (permalink / raw)
To: git
When we added the "--perl-regexp" option (or "-P") to "git grep", we
should have done the same for the commands in the "git log" family,
but somehow we forgot to do so. This corrects it.
Also introduce the "--basic-regexp" option for completeness, so that
the "last one wins" principle can be used to defeat an earlier -E
option, e.g. "git log -E --basic-regexp --grep='<bre>'". Note that
it cannot have the short "-G" option as the option is to grep in the
patch text in the context of "log" family.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
revision.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/revision.c b/revision.c
index 7f5e53b..0f73512 100644
--- a/revision.c
+++ b/revision.c
@@ -1603,6 +1603,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
return argcount;
} else if (!strcmp(arg, "--grep-debug")) {
revs->grep_filter.debug = 1;
+ } else if (!strcmp(arg, "--basic-regexp")) {
+ grep_set_pattern_type_option(GREP_PATTERN_TYPE_BRE, &revs->grep_filter);
} else if (!strcmp(arg, "--extended-regexp") || !strcmp(arg, "-E")) {
grep_set_pattern_type_option(GREP_PATTERN_TYPE_ERE, &revs->grep_filter);
} else if (!strcmp(arg, "--regexp-ignore-case") || !strcmp(arg, "-i")) {
@@ -1610,6 +1612,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
DIFF_OPT_SET(&revs->diffopt, PICKAXE_IGNORE_CASE);
} else if (!strcmp(arg, "--fixed-strings") || !strcmp(arg, "-F")) {
grep_set_pattern_type_option(GREP_PATTERN_TYPE_FIXED, &revs->grep_filter);
+ } else if (!strcmp(arg, "--perl-regexp") || !strcmp(arg, "-P")) {
+ grep_set_pattern_type_option(GREP_PATTERN_TYPE_PCRE, &revs->grep_filter);
} else if (!strcmp(arg, "--all-match")) {
revs->grep_filter.all_match = 1;
} else if ((argcount = parse_long_opt("encoding", argv, &optarg))) {
--
1.8.0.rc0.57.g712528f
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 4/6] log --grep: accept --basic-regexp and --perl-regexp
2012-10-04 1:33 ` [PATCH 4/6] log --grep: accept --basic-regexp and --perl-regexp Junio C Hamano
@ 2012-10-04 8:12 ` Jeff King
2012-10-04 16:44 ` Junio C Hamano
0 siblings, 1 reply; 23+ messages in thread
From: Jeff King @ 2012-10-04 8:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Wed, Oct 03, 2012 at 06:33:37PM -0700, Junio C Hamano wrote:
> When we added the "--perl-regexp" option (or "-P") to "git grep", we
> should have done the same for the commands in the "git log" family,
> but somehow we forgot to do so. This corrects it.
>
> Also introduce the "--basic-regexp" option for completeness, so that
> the "last one wins" principle can be used to defeat an earlier -E
> option, e.g. "git log -E --basic-regexp --grep='<bre>'". Note that
> it cannot have the short "-G" option as the option is to grep in the
> patch text in the context of "log" family.
Good, I think the addition of --basic-regexp is a nice touch.
> + } else if (!strcmp(arg, "--perl-regexp") || !strcmp(arg, "-P")) {
> + grep_set_pattern_type_option(GREP_PATTERN_TYPE_PCRE, &revs->grep_filter);
Do we want to yield short-and-sweet "-P" to perl-regexp? git-grep does
so to match GNU grep, but we are not matching anything here (except
ourselves in git-grep). I'd think most people who use it regularly would
just set grep.patternType.
I could go either way, though.
-Peff
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/6] log --grep: accept --basic-regexp and --perl-regexp
2012-10-04 8:12 ` Jeff King
@ 2012-10-04 16:44 ` Junio C Hamano
0 siblings, 0 replies; 23+ messages in thread
From: Junio C Hamano @ 2012-10-04 16:44 UTC (permalink / raw)
To: Jeff King; +Cc: git
Jeff King <peff@peff.net> writes:
>> + } else if (!strcmp(arg, "--perl-regexp") || !strcmp(arg, "-P")) {
>> + grep_set_pattern_type_option(GREP_PATTERN_TYPE_PCRE, &revs->grep_filter);
>
> Do we want to yield short-and-sweet "-P" to perl-regexp? git-grep does
> so to match GNU grep, but we are not matching anything here (except
> ourselves in git-grep). I'd think most people who use it regularly would
> just set grep.patternType.
My instinct always is that we should not to add short-and-sweet one
letter option until it is known to be necessary and useful; the
above was me typing without thinkng. And I agree grep.patternType
should be sufficient.
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 5/6] log: pass rev_info to git_log_config()
2012-10-04 1:33 ` [PATCH 0/6] Tying loose ends of extended "grep" Junio C Hamano
` (3 preceding siblings ...)
2012-10-04 1:33 ` [PATCH 4/6] log --grep: accept --basic-regexp and --perl-regexp Junio C Hamano
@ 2012-10-04 1:33 ` Junio C Hamano
2012-10-04 7:05 ` Junio C Hamano
2012-10-04 1:33 ` [PATCH 6/6] log --grep: honor grep.patterntype etc. configuration variables Junio C Hamano
5 siblings, 1 reply; 23+ messages in thread
From: Junio C Hamano @ 2012-10-04 1:33 UTC (permalink / raw)
To: git
Call init_revisions() first to prepare the revision traversal
parameters and pass it to git_log_config(), so that necessary bits
in the traversal parameters can be tweaked before we call the
command line parsing infrastructure setup_revisions() from
the cmd_log_init_finish() function.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* This is made separate from the next one that touches the contents
of "rev" to make sure the existing code does not depend on the
current initialization order. I do not think it does but better
be careful to keep the history easier to bisect, than be sorry
when an issue does appear.
builtin/log.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 09cf43e..07a0078 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -360,9 +360,8 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix)
struct rev_info rev;
struct setup_revision_opt opt;
- git_config(git_log_config, NULL);
-
init_revisions(&rev, prefix);
+ git_config(git_log_config, &rev);
rev.diff = 1;
rev.simplify_history = 0;
memset(&opt, 0, sizeof(opt));
@@ -450,10 +449,9 @@ int cmd_show(int argc, const char **argv, const char *prefix)
struct pathspec match_all;
int i, count, ret = 0;
- git_config(git_log_config, NULL);
-
init_pathspec(&match_all, NULL);
init_revisions(&rev, prefix);
+ git_config(git_log_config, &rev);
rev.diff = 1;
rev.always_show_header = 1;
rev.no_walk = REVISION_WALK_NO_WALK_SORTED;
@@ -530,9 +528,8 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix)
struct rev_info rev;
struct setup_revision_opt opt;
- git_config(git_log_config, NULL);
-
init_revisions(&rev, prefix);
+ git_config(git_log_config, &rev);
init_reflog_walk(&rev.reflog_info);
rev.verbose_header = 1;
memset(&opt, 0, sizeof(opt));
@@ -552,9 +549,8 @@ int cmd_log(int argc, const char **argv, const char *prefix)
struct rev_info rev;
struct setup_revision_opt opt;
- git_config(git_log_config, NULL);
-
init_revisions(&rev, prefix);
+ git_config(git_log_config, &rev);
rev.always_show_header = 1;
memset(&opt, 0, sizeof(opt));
opt.def = "HEAD";
@@ -1121,8 +1117,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
extra_hdr.strdup_strings = 1;
extra_to.strdup_strings = 1;
extra_cc.strdup_strings = 1;
- git_config(git_format_config, NULL);
init_revisions(&rev, prefix);
+ git_config(git_format_config, &rev);
rev.commit_format = CMIT_FMT_EMAIL;
rev.verbose_header = 1;
rev.diff = 1;
--
1.8.0.rc0.57.g712528f
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 5/6] log: pass rev_info to git_log_config()
2012-10-04 1:33 ` [PATCH 5/6] log: pass rev_info to git_log_config() Junio C Hamano
@ 2012-10-04 7:05 ` Junio C Hamano
2012-10-05 4:16 ` Junio C Hamano
0 siblings, 1 reply; 23+ messages in thread
From: Junio C Hamano @ 2012-10-04 7:05 UTC (permalink / raw)
To: git
Junio C Hamano <gitster@pobox.com> writes:
> Call init_revisions() first to prepare the revision traversal
> parameters and pass it to git_log_config(), so that necessary bits
> in the traversal parameters can be tweaked before we call the
> command line parsing infrastructure setup_revisions() from
> the cmd_log_init_finish() function.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
> * This is made separate from the next one that touches the contents
> of "rev" to make sure the existing code does not depend on the
> current initialization order. I do not think it does but better
> be careful to keep the history easier to bisect, than be sorry
> when an issue does appear.
And I was right X-<. This does break the assumption the recent
diff.context series makes.
What happens is that
- init_revisions() initializes revs->grep_filter; that is why this
patch wanted to call it first, so that it can futz with it
from git_config().
- however, init_revisions() also calls diff_setup(), and the
diff machinery initializes revs->diffopt->context from
diff_context_default. Compiled in default of this value is 3,
but the diff.context series wants to update this variable with
the configuration before this call happens.
So we would need to do something like:
- call git_log_config() first to let diff_context_default
updated from the configuration as before. find the values of
grep.* defaults at the same time, but stash it away in a
separate "struct grep_opt" (yuck);
- call init_revisions() and let it initialize revs->grep_filter
and revs->diffopt as before;
- copy the grep.* defaults we learned during git_log_config() to
revs->grep_filter.
which is a bit yucky, but survivable.
I'll fix these two patches up later.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/6] log: pass rev_info to git_log_config()
2012-10-04 7:05 ` Junio C Hamano
@ 2012-10-05 4:16 ` Junio C Hamano
2012-10-05 15:33 ` Jeff King
0 siblings, 1 reply; 23+ messages in thread
From: Junio C Hamano @ 2012-10-05 4:16 UTC (permalink / raw)
To: git
Junio C Hamano <gitster@pobox.com> writes:
> So we would need to do something like:
>
> - call git_log_config() first to let diff_context_default
> updated from the configuration as before. find the values of
> grep.* defaults at the same time, but stash it away in a
> separate "struct grep_opt" (yuck);
>
> - call init_revisions() and let it initialize revs->grep_filter
> and revs->diffopt as before;
>
> - copy the grep.* defaults we learned during git_log_config() to
> revs->grep_filter.
>
> which is a bit yucky, but survivable.
After thinking about it a bit more, I came to a conclusion that the
configuration handling lifted from builtin/grep.c needs a much
larger overhaul.
The grep_config() function takes one instance of grep_opt as a
callback parameter, and populates it by running git_config(). This
has three practical implications.
- You have to have an instance of grep_opt already when you call
the configuration. The codepath under discussion in this thread
is a prime example why that arrangement is not always possible.
- It is not easy to enhance grep_config() in such a way to make it
cascade to other callback functions to grab other variables in
one call of git_config(); grep_config() can be cascaded into from
other callbacks, but it has to be at the leaf level of a cascade.
- If you ever need to use more than one instance of grep_opt, you
will have to open and read the configuration file(s) every time
you initialize them.
The right way to arrange your configuration callback is probably to
model it after how diff configuration variables are handled. You
call git_config() once, and remember the values you read in set of
static variables. Later, whenever you need to instantiate a grep_opt,
you initialize it from these static variables.
All of the above did not matter back when the code in builtin/grep.c
was isolated and the configuration was never meant to be used by
other subsystems. But the last two patches in this series do want
to break that assumption, so grep_config() needs to be rethought.
Luckily, we don't have to have this in the upcoming 1.8.0 release
(it is is too late for any topic that is not a regression fix).
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/6] log: pass rev_info to git_log_config()
2012-10-05 4:16 ` Junio C Hamano
@ 2012-10-05 15:33 ` Jeff King
2012-10-05 19:07 ` Junio C Hamano
0 siblings, 1 reply; 23+ messages in thread
From: Jeff King @ 2012-10-05 15:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Thu, Oct 04, 2012 at 09:16:14PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > So we would need to do something like:
> >
> > - call git_log_config() first to let diff_context_default
> > updated from the configuration as before. find the values of
> > grep.* defaults at the same time, but stash it away in a
> > separate "struct grep_opt" (yuck);
> >
> > - call init_revisions() and let it initialize revs->grep_filter
> > and revs->diffopt as before;
> >
> > - copy the grep.* defaults we learned during git_log_config() to
> > revs->grep_filter.
> >
> > which is a bit yucky, but survivable.
>
> After thinking about it a bit more, I came to a conclusion that the
> configuration handling lifted from builtin/grep.c needs a much
> larger overhaul.
> [...]
> The right way to arrange your configuration callback is probably to
> model it after how diff configuration variables are handled. You
> call git_config() once, and remember the values you read in set of
> static variables. Later, whenever you need to instantiate a grep_opt,
> you initialize it from these static variables.
Agreed. Maybe the simplest thing would be to have grep_config fill in a
"static struct grep_opt grep_defaults", and then memcpy that into place
during init_revisions?
-Peff
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/6] log: pass rev_info to git_log_config()
2012-10-05 15:33 ` Jeff King
@ 2012-10-05 19:07 ` Junio C Hamano
0 siblings, 0 replies; 23+ messages in thread
From: Junio C Hamano @ 2012-10-05 19:07 UTC (permalink / raw)
To: Jeff King; +Cc: git
Jeff King <peff@peff.net> writes:
> Agreed. Maybe the simplest thing would be to have grep_config fill in a
> "static struct grep_opt grep_defaults", and then memcpy that into place
> during init_revisions?
Yes, I was doing that for a bit last night, but then realized that
the grep_config() should be split into two (grep specific part and
then the bits that cascade to others, which is "git grep" specific
requirement; it is far better to let other callers to arrange the
cascading themselves) before moving the grep specific bit to the
top-level, so that needs to come first.
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 6/6] log --grep: honor grep.patterntype etc. configuration variables
2012-10-04 1:33 ` [PATCH 0/6] Tying loose ends of extended "grep" Junio C Hamano
` (4 preceding siblings ...)
2012-10-04 1:33 ` [PATCH 5/6] log: pass rev_info to git_log_config() Junio C Hamano
@ 2012-10-04 1:33 ` Junio C Hamano
2012-10-04 8:17 ` Jeff King
5 siblings, 1 reply; 23+ messages in thread
From: Junio C Hamano @ 2012-10-04 1:33 UTC (permalink / raw)
To: git
Read grep.extendedregexp, grep.patterntype, etc. from the
configuration so that "log --grep='<pcre>'" honors the user
preference without an explicit -P from the command line.
Now that the callback parameter, which was so far unused, to
git_log_config() has to be of type "struct rev_info *", stop passing
it down to git_diff_ui_config(). The latter does not currently take
any callback parameter, and when it does, we would need to make a
structure that has rev info and that parameter and pass it to
git_log_config() anyway, and until that happens, passing NULL will
be less error prone.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/log.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/builtin/log.c b/builtin/log.c
index 07a0078..a38a6dd 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -329,6 +329,8 @@ static int cmd_log_walk(struct rev_info *rev)
static int git_log_config(const char *var, const char *value, void *cb)
{
+ struct rev_info *revs = cb;
+
if (!strcmp(var, "format.pretty"))
return git_config_string(&fmt_pretty, var, value);
if (!strcmp(var, "format.subjectprefix"))
@@ -352,7 +354,8 @@ static int git_log_config(const char *var, const char *value, void *cb)
if (!prefixcmp(var, "color.decorate."))
return parse_decorate_color_config(var, 15, value);
- return git_diff_ui_config(var, value, cb);
+ grep_config(var, value, &revs->grep_filter);
+ return git_diff_ui_config(var, value, NULL);
}
int cmd_whatchanged(int argc, const char **argv, const char *prefix)
--
1.8.0.rc0.57.g712528f
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 6/6] log --grep: honor grep.patterntype etc. configuration variables
2012-10-04 1:33 ` [PATCH 6/6] log --grep: honor grep.patterntype etc. configuration variables Junio C Hamano
@ 2012-10-04 8:17 ` Jeff King
2012-10-04 16:46 ` Junio C Hamano
0 siblings, 1 reply; 23+ messages in thread
From: Jeff King @ 2012-10-04 8:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Wed, Oct 03, 2012 at 06:33:39PM -0700, Junio C Hamano wrote:
> Read grep.extendedregexp, grep.patterntype, etc. from the
> configuration so that "log --grep='<pcre>'" honors the user
> preference without an explicit -P from the command line.
>
> Now that the callback parameter, which was so far unused, to
> git_log_config() has to be of type "struct rev_info *", stop passing
> it down to git_diff_ui_config(). The latter does not currently take
> any callback parameter, and when it does, we would need to make a
> structure that has rev info and that parameter and pass it to
> git_log_config() anyway, and until that happens, passing NULL will
> be less error prone.
Hmm. So I think this is a nice feature for some people, but I wonder if
we would run into any plumbing compatibility issues. People do tend to
use "log" as plumbing (since rev-list is not as capable). On the other
hand, I'd think most internal uses of "log --grep" would be passing
something along from the user, and the user would be happy to have it
interpreted by their chosen set of rules.
-Peff
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 6/6] log --grep: honor grep.patterntype etc. configuration variables
2012-10-04 8:17 ` Jeff King
@ 2012-10-04 16:46 ` Junio C Hamano
2012-10-04 18:01 ` Jeff King
0 siblings, 1 reply; 23+ messages in thread
From: Junio C Hamano @ 2012-10-04 16:46 UTC (permalink / raw)
To: Jeff King; +Cc: git
Jeff King <peff@peff.net> writes:
> Hmm. So I think this is a nice feature for some people, but I wonder if
> we would run into any plumbing compatibility issues. People do tend to
> use "log" as plumbing (since rev-list is not as capable). On the other
> hand, I'd think most internal uses of "log --grep" would be passing
> something along from the user, and the user would be happy to have it
> interpreted by their chosen set of rules.
This does make "rev-list --grep" aware of the configuration but at
the same time --basic-regexp and friends are also available to it.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 6/6] log --grep: honor grep.patterntype etc. configuration variables
2012-10-04 16:46 ` Junio C Hamano
@ 2012-10-04 18:01 ` Jeff King
2012-10-04 19:09 ` Junio C Hamano
0 siblings, 1 reply; 23+ messages in thread
From: Jeff King @ 2012-10-04 18:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Thu, Oct 04, 2012 at 09:46:42AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Hmm. So I think this is a nice feature for some people, but I wonder if
> > we would run into any plumbing compatibility issues. People do tend to
> > use "log" as plumbing (since rev-list is not as capable). On the other
> > hand, I'd think most internal uses of "log --grep" would be passing
> > something along from the user, and the user would be happy to have it
> > interpreted by their chosen set of rules.
>
> This does make "rev-list --grep" aware of the configuration but at
> the same time --basic-regexp and friends are also available to it.
Does it? I thought the patch only tweaked git_log_config. Am I
misreading?
Having --basic-regexp is a nice escape hatch, but it would be a
regression for older scripts which were written before --basic-regexp
existed (or was necessary).
-Peff
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 6/6] log --grep: honor grep.patterntype etc. configuration variables
2012-10-04 18:01 ` Jeff King
@ 2012-10-04 19:09 ` Junio C Hamano
0 siblings, 0 replies; 23+ messages in thread
From: Junio C Hamano @ 2012-10-04 19:09 UTC (permalink / raw)
To: Jeff King; +Cc: git
Jeff King <peff@peff.net> writes:
> On Thu, Oct 04, 2012 at 09:46:42AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > Hmm. So I think this is a nice feature for some people, but I wonder if
>> > we would run into any plumbing compatibility issues. People do tend to
>> > use "log" as plumbing (since rev-list is not as capable). On the other
>> > hand, I'd think most internal uses of "log --grep" would be passing
>> > something along from the user, and the user would be happy to have it
>> > interpreted by their chosen set of rules.
>>
>> This does make "rev-list --grep" aware of the configuration but at
>> the same time --basic-regexp and friends are also available to it.
>
> Does it?
Ah, it doesn't.
You can still say "rev-list --perl-regexp --grep=pcre" but that is
not what 6/6 does.
^ permalink raw reply [flat|nested] 23+ messages in thread