git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [ANNOUNCE] Git v1.8.0-rc0
@ 2012-10-01 22:44 Junio C Hamano
  2012-10-03 20:18 ` grep.patternType (was: Re: [ANNOUNCE] Git v1.8.0-rc0) Junio C Hamano
  0 siblings, 1 reply; 23+ messages in thread
From: Junio C Hamano @ 2012-10-01 22:44 UTC (permalink / raw)
  To: git; +Cc: Linux Kernel

A release candidate Git v1.8.0-rc0 is now available for testing at
the usual places.  There are a couple of leftover features we might
merge before the final release, but other than that, this is meant
to be more or less feature-complete preview of the upcoming 1.8.0.

The release tarballs are found at:

    http://code.google.com/p/git-core/downloads/list

and their SHA-1 checksums are:

a5143163b9d17e1afd7e66d7c6e7457c2e09a022  git-1.8.0.rc0.tar.gz
679891dad4b0168ddd618c1de05978e35189c1bf  git-htmldocs-1.8.0.rc0.tar.gz
eab59abd44a941e382eca6ae5e1d357b337328d0  git-manpages-1.8.0.rc0.tar.gz

Also the following public repositories all have a copy of the v1.8.0-rc0
tag and the master branch that the tag points at:

  url = git://repo.or.cz/alt-git.git
  url = https://code.google.com/p/git-core/
  url = git://git.sourceforge.jp/gitroot/git-core/git.git
  url = git://git-core.git.sourceforge.net/gitroot/git-core/git-core
  url = https://github.com/gitster/git

Git v1.8.0 Release Notes (draft)
========================

Backward compatibility notes
----------------------------

In the next major release, we will change the behaviour of the "git
push" command.  When "git push [$there]" does not say what to push, we
have used the traditional "matching" semantics (all your branches were
sent to the remote as long as there already are branches of the same
name over there).  We will use the "simple" semantics, that pushes the
current branch to the branch with the same name only when the current
branch is set to integrate with that remote branch.  There is a user
preference configuration variable "push.default" to change this, and
"git push" will warn about the upcoming change until you set this
variable.

"git branch --set-upstream" is deprecated and may be removed in a
relatively distant future.  "git branch [-u|--set-upstream-to]" has
been introduced with a saner order of arguments.


Updates since v1.7.12
---------------------

UI, Workflows & Features

 * A credential helper for Win32 to allow access to the keychain of
   the logged-in user has been added.

 * An initial port to HP NonStop.

 * A credential helper to allow access to the Gnome keyring has been
   added.

 * When "git am" sanitizes the Subject: line, we strip the prefix from
   "Re: subject" and also from a less common "re: subject", but left
   even less common "RE: subject" intact.

 * It was tempting to say "git branch --set-upstream origin/master",
   but that tells Git to arrange the local branch "origin/master" to
   integrate with the currently checked out branch, which is highly
   unlikely what the user meant.  The option is deprecated; use the
   new "--set-upstream-to" (with a short-and-sweet "-u") option
   instead.

 * "git cherry-pick" learned the "--allow-empty-message" option to
   allow it to replay a commit without any log message.

 * After "git cherry-pick -s" gave control back to the user asking
   help to resolve conflicts, concluding "git commit" used to need to
   be run with "-s" if the user wants to sign it off; now the command
   leaves the sign-off line in the log template.

 * "git daemon" learned the "--access-hook" option to allow an
   external command to decline service based on the client address,
   repository path, etc.

 * "git difftool --dir-diff" learned to use symbolic links to prepare
   temporary copy of the working tree when available.

 * "git grep" learned to use a non-standard pattern type by default if
   a configuration variable tells it to.

 * "git merge-base" learned "--is-ancestor A B" option to tell if A is
   an ancestor of B.  The result is indicated by its exit status code.

 * "git mergetool" allows users to override the actual command used
   with the mergetool.$name.cmd configuration variable even for built-in
   mergetool backends.

 * The "-Xours" backend option to "git merge -s recursive" now takes
   effect even on binary files.

 * "git rebase -i" learned the "--edit-todo" option to open an editor
   to edit the insn sheet.


Foreign Interface

 * "git svn" has been updated to work with SVN 1.7.

 * "git p4" learned "--conflicts" option to specify what to do when
   encountering a conflict during "p4 submit".


Performance, Internal Implementation, etc. (please report possible regressions)

 * Git ships with a fall-back regexp implementation for platforms with
   buggy regexp library, but it was easy for people to keep using their
   platform regexp.  A new test has been added to check this.

 * The "check-docs" build target has been updated and greatly
   simplified.

 * The test suite is run under MALLOC_CHECK_ when running with glibc
   that supports the feature.

 * The documentation in the TeXinfo format was using indented output
   for materials meant to be examples that are better typeset in
   monospace.

 * Compatibility wrapper around some mkdir(2) implementations that
   reject parameter with trailing slash has been introduced.

 * Compatibility wrapper for systems that lack usable setitimer() has
   been added.

 * The option parsing of "git checkout" had error checking, dwim and
   defaulting missing options, all mixed in the code, and issuing an
   appropriate error message with useful context was getting harder.
   The code has been reorganized to allow giving a proper diagnosis
   when the user says "git checkout -b -t foo bar" (e.g. "-t" is not a
   good name for a branch).

 * Many internal uses of "git merge-base" equivalent were only to see
   if one commit fast-forwards to the other, which did not need the
   full set of merge bases to be computed. They have been updated to
   use less expensive checks.

 * The heuristics to detect and silently convert latin1 to utf8 when
   we were told to use utf-8 in the log message has been transplanted
   from "mailinfo" to "commit" and "commit-tree".

 * Messages given by "git <subcommand> -h" from many subcommands have
   been marked for translation.


Also contains minor documentation updates and code clean-ups.


Fixes since v1.7.12
-------------------

Unless otherwise noted, all the fixes since v1.7.12 in the
maintenance track are contained in this release (see release notes
to them for details).

 * The attribute system may be asked for a path that itself or its
   leading directories no longer exists in the working tree, and it is
   fine if we cannot open .gitattribute file in such a case.  Failure
   to open per-directory .gitattributes with error status other than
   ENOENT and ENOTDIR should be diagnosed, but it wasn't.

 * When looking for $HOME/.gitconfig etc., it is OK if we cannot read
   them because they do not exist, but we did not diagnose existing
   files that we cannot read.

 * When "git am" is fed an input that has multiple "Content-type: ..."
   header, it did not grok charset= attribute correctly.

 * "git blame MAKEFILE" run in a history that has "Makefile" but not
   "MAKEFILE" should say "No such file MAKEFILE in HEAD", but got
   confused on a case insensitive filesystem and failed to do so.

 * Even during a conflicted merge, "git blame $path" always meant to
   blame uncommitted changes to the "working tree" version; make it
   more useful by showing cleanly merged parts as coming from the other
   branch that is being merged.

 * It was unclear in the documentation for "git blame" that it is
   unnecessary for users to use the "--follow" option.
   (merge e5dce96 jc/blame-follows-renames later to maint).

 * Output from "git branch -v" contains "(no branch)" that could be
   localized, but the code to align it along with the names of
   branches were counting in bytes, not in display columns.

 * "git cherry-pick A C B" used to replay changes in A and then B and
   then C if these three commits had committer timestamps in that
   order, which is not what the user who said "A C B" naturally
   expects.

 * A repository created with "git clone --single" had its fetch
   refspecs set up just like a clone without "--single", leading the
   subsequent "git fetch" to slurp all the other branches, defeating
   the whole point of specifying "only this branch".
   (merge 31b808a rt/maint-clone-single later to maint).

 * Documentation talked about "first line of commit log" when it meant
   the title of the commit.  The description was clarified by defining
   how the title is decided and rewording the casual mention of "first
   line" to "title".

 * "git cvsimport" did not thoroughly cleanse tag names that it
   inferred from the names of the tags it obtained from CVS, which
   caused "git tag" to barf and stop the import in the middle.

 * Earlier we made the diffstat summary line that shows the number of
   lines added/deleted localizable, but it was found irritating having
   to see them in various languages on a list whose discussion language
   is English.

 * "git fetch --all", when passed "--no-tags", did not honor the
   "--no-tags" option while fetching from individual remotes (the same
   issue existed with "--tags", but combination "--all --tags" makes
   much less sense than "--all --no-tags").

 * "git fetch" over http had an old workaround for an unlikely server
   misconfiguration; it turns out that this hurts debuggability of the
   configuration in general, and has been reverted.
   (merge 6ac964a sp/maint-http-info-refs-no-retry later to maint).

 * "git fetch" over http advertised that it supports "deflate", which
   is much less common, and did not advertise more common "gzip" on
   its Accept-Encoding header.
   (merge aa90b96 sp/maint-http-enable-gzip later to maint).

 * After "gitk" showed the contents of a tag, neither "Reread
   references" nor "Reload" did not update what is shown as the
   contents of it, when the user overwrote the tag with "git tag -f".

 * "git log --all-match --grep=A --grep=B" ought to show commits that
   mention both A and B, but when these three options are used with
   --author or --committer, it showed commits that mention either A or
   B (or both) instead.

 * "git p4", when "--use-client-spec" and "--detect-branches" are used
   together, misdetected branches.

 * "git receive-pack" (the counterpart to "git push") did not give
   progress output while processing objects it received to the puser
   when run over the smart-http protocol.
   (merge 74eb32d jk/receive-pack-unpack-error-to-pusher later to maint).

 * When you misspell the command name you give to the "exec" action in
   the "git rebase -i" insn sheet, you are told that 'rebase' is not a
   git subcommand from "git rebase --continue".

 * The subcommand in "git remote" to remove a defined remote was
   "rm" and the command did not take a fully-spelled "remove".

 * The interactive prompt "git send-email" gives was error prone. It
   asked "What e-mail address do you want to use?" with the address it
   guessed (correctly) the user would want to use in its prompt,
   tempting the user to say "y". But the response was taken as "No,
   please use 'y' as the e-mail address instead", which is most
   certainly not what the user meant.

 * "git show --format='%ci'" did not give timestamp correctly for
   commits created without human readable name on "committer" line.

 * "git show --quiet" ought to be a synonym for "git show -s", but
   wasn't.

 * "git submodule frotz" was not diagnosed as "frotz" being an unknown
   subcommand to "git submodule"; the user instead got a complaint
   that "git submodule status" was run with an unknown path "frotz".
   (merge af9c9f9 rr/maint-submodule-unknown-cmd later to maint).

 * "git status" honored the ignore=dirty settings in .gitmodules but
   "git commit" didn't.
   (merge 8f6811e os/commit-submodule-ignore later to maint).

^ permalink raw reply	[flat|nested] 23+ messages in thread

* grep.patternType (was: Re: [ANNOUNCE] Git v1.8.0-rc0)
  2012-10-01 22:44 [ANNOUNCE] Git v1.8.0-rc0 Junio C Hamano
@ 2012-10-03 20:18 ` Junio C Hamano
  2012-10-03 22:14   ` grep.patternType Junio C Hamano
  2012-10-04  1:33   ` [PATCH 0/6] Tying loose ends of extended "grep" Junio C Hamano
  0 siblings, 2 replies; 23+ messages in thread
From: Junio C Hamano @ 2012-10-03 20:18 UTC (permalink / raw)
  To: git; +Cc: Michał Kiedrowicz, J Smith

Junio C Hamano <gitster@pobox.com> writes:

>  * "git grep" learned to use a non-standard pattern type by default if
>    a configuration variable tells it to.

This addition makes

    git grep -e "(integer|buffer)"

work as expected, when grep.patternType is set to "extended".

Should this

    git log --grep="(integer|buffer)"

also honor the same configuration variable?  If not, why not?

One more thing.  Currently you can say

    git log -E --grep="(integer|buffer)"

to ask for the ERE.  Should we also support -P to ask for pcre?  If
not, why not?

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: grep.patternType
  2012-10-03 20:18 ` grep.patternType (was: Re: [ANNOUNCE] Git v1.8.0-rc0) Junio C Hamano
@ 2012-10-03 22:14   ` Junio C Hamano
  2012-10-04  6:05     ` grep.patternType Michal Kiedrowicz
  2012-10-05  5:38     ` grep.patternType J Smith
  2012-10-04  1:33   ` [PATCH 0/6] Tying loose ends of extended "grep" Junio C Hamano
  1 sibling, 2 replies; 23+ messages in thread
From: Junio C Hamano @ 2012-10-03 22:14 UTC (permalink / raw)
  To: git; +Cc: Michał Kiedrowicz, J Smith

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>>  * "git grep" learned to use a non-standard pattern type by default if
>>    a configuration variable tells it to.
>
> This addition makes
>
>     git grep -e "(integer|buffer)"
>
> work as expected, when grep.patternType is set to "extended".
>
> Should this
>
>     git log --grep="(integer|buffer)"
>
> also honor the same configuration variable?  If not, why not?
>
> One more thing.  Currently you can say
>
>     git log -E --grep="(integer|buffer)"
>
> to ask for the ERE.  Should we also support -P to ask for pcre?  If
> not, why not?

Answering to myself who has been in tying-loose-ends mode.

My answers to these questions are both yes, and I have a neatly
lined up series that begins with a small bugfix and then
enhancement, but I do not think these do not deserve to in the
upcoming release.  The topic came too late, and even the fix is
for a bug that has been with us for a long time.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 0/6] Tying loose ends of extended "grep"
  2012-10-03 20:18 ` grep.patternType (was: Re: [ANNOUNCE] Git v1.8.0-rc0) Junio C Hamano
  2012-10-03 22:14   ` grep.patternType Junio C Hamano
@ 2012-10-04  1:33   ` Junio C Hamano
  2012-10-04  1:33     ` [PATCH 1/6] grep: move configuration support to top-level grep.[ch] Junio C Hamano
                       ` (5 more replies)
  1 sibling, 6 replies; 23+ messages in thread
From: Junio C Hamano @ 2012-10-04  1:33 UTC (permalink / raw)
  To: git; +Cc: H. Peter Anvin

Over time we have added a few things to our "git grep" front-end,
such as

 - grep.extendedregexp configuration (v1.7.5)
 - use of pcre (v1.7.6)
 - grep.patterntype configuration (v1.8.0)

But all the time, we forgot that "git log --grep" would need to
honor them.

The first three patches should be uncontroversial.  We move helpers
out of builtin/grep.c to a more generic place, and fix a bug in the
command line parser for "git log -F -E --grep='<ere>'" (this did not
correctly enable regular expression).

The fourth patch adds "git log --perl-regexp --grep='<pcre>'".

The last two teaches "log --grep" to honor the same grep.*
configuration variables.

color.grep and grep.linenumber should not matter, as the use of grep
mechanism in "log --grep" is about boolean result "do we have hits?"
and not about actually showing the hits in the output, but the users
would expect that grep.extendedregexp and its more generalized
version grep.patterntype are honored, which was not the case.

Junio C Hamano (6):
  grep: move configuration support to top-level grep.[ch]
  grep: move pattern-type bits support to top-level grep.[ch]
  log --grep: use the same helper to set -E/-F options as "git grep"
  log --grep: accept --basic-regexp and --perl-regexp
  log: pass rev_info to git_log_config()
  log --grep: honor grep.patterntype etc. configuration variables

 builtin/grep.c | 105 ++-------------------------------------------------------
 builtin/log.c  |  19 +++++------
 grep.c         |  99 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 grep.h         |   3 ++
 revision.c     |   8 +++--
 t/t4202-log.sh |   6 ++++
 6 files changed, 126 insertions(+), 114 deletions(-)

-- 
1.8.0.rc0.57.g712528f

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [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	[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	[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	[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	[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	[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	[flat|nested] 23+ messages in thread

* Re: grep.patternType
  2012-10-03 22:14   ` grep.patternType Junio C Hamano
@ 2012-10-04  6:05     ` Michal Kiedrowicz
  2012-10-05  5:38     ` grep.patternType J Smith
  1 sibling, 0 replies; 23+ messages in thread
From: Michal Kiedrowicz @ 2012-10-04  6:05 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, J Smith

Junio C Hamano <gitster@pobox.com> wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Junio C Hamano <gitster@pobox.com> writes:
> >
> >>  * "git grep" learned to use a non-standard pattern type by
> >> default if a configuration variable tells it to.
> >
> > This addition makes
> >
> >     git grep -e "(integer|buffer)"
> >
> > work as expected, when grep.patternType is set to "extended".
> >
> > Should this
> >
> >     git log --grep="(integer|buffer)"
> >
> > also honor the same configuration variable?  If not, why not?

I think this should respect grep.patternType.

> >
> > One more thing.  Currently you can say
> >
> >     git log -E --grep="(integer|buffer)"
> >
> > to ask for the ERE.  Should we also support -P to ask for pcre?  If
> > not, why not?

This also.

> 
> Answering to myself who has been in tying-loose-ends mode.
> 
> My answers to these questions are both yes, and I have a neatly
> lined up series that begins with a small bugfix and then
> enhancement, but I do not think these do not deserve to in the
> upcoming release.  The topic came too late, and even the fix is
> for a bug that has been with us for a long time.

I think I am the one to blame for this inconsistency.  When I
implemented "git-grep -P" I was thinking about making it work for all
regex operations in git but since I'm mostly using regexes with
git-grep I was too lazy to make it work with git-log.

^ permalink raw reply	[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 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

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

* 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

* 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: grep.patternType
  2012-10-03 22:14   ` grep.patternType Junio C Hamano
  2012-10-04  6:05     ` grep.patternType Michal Kiedrowicz
@ 2012-10-05  5:38     ` J Smith
  1 sibling, 0 replies; 23+ messages in thread
From: J Smith @ 2012-10-05  5:38 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Michał Kiedrowicz

On Wed, Oct 3, 2012 at 6:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>>  * "git grep" learned to use a non-standard pattern type by default if
>>>    a configuration variable tells it to.
>>
>> This addition makes
>>
>>     git grep -e "(integer|buffer)"
>>
>> work as expected, when grep.patternType is set to "extended".
>>
>> Should this
>>
>>     git log --grep="(integer|buffer)"
>>
>> also honor the same configuration variable?  If not, why not?
>>
>> One more thing.  Currently you can say
>>
>>     git log -E --grep="(integer|buffer)"
>>
>> to ask for the ERE.  Should we also support -P to ask for pcre?  If
>> not, why not?
>
> Answering to myself who has been in tying-loose-ends mode.
>
> My answers to these questions are both yes, and I have a neatly
> lined up series that begins with a small bugfix and then
> enhancement, but I do not think these do not deserve to in the
> upcoming release.  The topic came too late, and even the fix is
> for a bug that has been with us for a long time.

Yeah, I think that could be useful and consistent. I took a look at
the grep situation in the log command briefly when writing the
original patch but ended up leaving it as-is as for the time being due
to time constraints and the like. But yeah, that behaviour is
definitely desirable. I think any commands that work with grep should
probably follow suit, for that matter. (Are there others other than
log and grep itself...?)

^ 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

end of thread, other threads:[~2012-10-05 19:08 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-01 22:44 [ANNOUNCE] Git v1.8.0-rc0 Junio C Hamano
2012-10-03 20:18 ` grep.patternType (was: Re: [ANNOUNCE] Git v1.8.0-rc0) Junio C Hamano
2012-10-03 22:14   ` grep.patternType Junio C Hamano
2012-10-04  6:05     ` grep.patternType Michal Kiedrowicz
2012-10-05  5:38     ` grep.patternType J Smith
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     ` [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
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
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
2012-10-05 15:33           ` Jeff King
2012-10-05 19:07             ` Junio C Hamano
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
2012-10-04 18:01           ` Jeff King
2012-10-04 19:09             ` Junio C Hamano

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).