Re: [PATCH] git-log: added --invert-grep option

[Junio C Hamano <gitster@pobox.com>]

Christoph Junghans <ottxor@gentoo.org> writes:

> Implements a inverted match for "git log", like in the case of
> "git grep -v", which is useful from time to time to e.g. filter
> FIXUP message out of "git log".
>
> Internally, a new bol 'global_invert' has been introduces as
> revs->grep_filter.invert inverts the match line-wise, which cannot
> work as i.e. empty line always not match the pattern given.

While I am very sympathetic to those who feel the pain, i.e. the
need for something like this, I do not think this patch takes the
right approach.

The pain is that "git log --grep=..." can express only a limited
subset of what "git grep" can express.  The latter supports a very
rich set of logical operations, with operators like --and and --not,
and grouping, e.g. "git grep \( -e foo --or -e bar \) --and --not -e
baz" (i.e. "has either foo or bar but not baz").  It also can turn
the list of top-level predicates into "all of these predicates must
match somewhere in the entire file" with --all-match option.  None
of this richness is available to "git log --grep=...".

The root cause of the pain comes from the fact that it needs to
share the command line parsing with the revision list commands to
drive the underlying "grep" machinery, so you cannot say

    git log --not --grep=FIXUP

because "--not" is taken as "commits reachable from revs listed after
this point are to be excluded from the result" and not passed to the
underlying grep machinery.

The right way to do this is to somehow find a way to allow you to
express the full "grep" logical operations to the command line
parser that is used by the "log" family of commands.  That would
allow you to express something like "Show only commits that has
either foo or bar and does not have baz".  I am not going to
advocate this exact syntax, but to illustrate the idea, if you had
something like this supported:

    git log --grep-begin \
    	\( -e foo --or -e bar \) --and --not -e baz \
        --grep-end

by stopping the revision.c parser between --grep-{begin,end} and
instead feeding the arguments to the grep expression builder, we may
be able to get the full expressiveness of the logical operations
offered by the grep machinery.

And you shouldn't need to add any new field to grep_opt for that.
All you need is a design of a new syntax and a tweak to the revision
argument parser to understand the new syntax to redirect some
arguments to the grep command line parser.

A new option that allows you to _only_ negate without allowing you
to enable other richer logical operations of the underlying grep
machinery is going in a wrong direction, isn't it?

> Signed-off-by: Christoph Junghans <ottxor@gentoo.org>
> ---
>  Documentation/rev-list-options.txt     | 4 ++++
>  contrib/completion/git-completion.bash | 2 +-
>  grep.h                                 | 3 ++-
>  revision.c                             | 4 +++-
>  4 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index afccfdc..6d4671f 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -66,6 +66,10 @@ if it is part of the log message.
>  	Limit the commits output to ones that match all given `--grep`,
>  	instead of ones that match at least one.
>  
> +--invert-grep::
> +	Limit the commits output to ones with log message that do not
> +	match the pattern specified with `--grep=<pattern>`.
> +
>  -i::
>  --regexp-ignore-case::
>  	Match the regular expression limiting patterns without regard to letter
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 2fece98..914c317 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1425,7 +1425,7 @@ __git_log_gitk_options="
>  # Options that go well for log and shortlog (not gitk)
>  __git_log_shortlog_options="
>  	--author= --committer= --grep=
> -	--all-match
> +	--all-match --invert-grep
>  "
>  
>  __git_log_pretty_formats="oneline short medium full fuller email raw format:"
> diff --git a/grep.h b/grep.h
> index 95f197a..c137103 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -93,7 +93,8 @@ struct grep_opt {
>  	int prefix_length;
>  	regex_t regexp;
>  	int linenum;
> -	int invert;
> +	int invert; /** line-wise invert match */
> +	int global_invert; /** final global invert match */
>  	int ignore_case;
>  	int status_only;
>  	int name_only;
> diff --git a/revision.c b/revision.c
> index 75dda92..c8d4c49 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2011,6 +2011,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>  		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 (!strcmp(arg, "--invert-grep")) {
> +		revs->grep_filter.global_invert = 1;
>  	} else if ((argcount = parse_long_opt("encoding", argv, &optarg))) {
>  		if (strcmp(optarg, "none"))
>  			git_log_output_encoding = xstrdup(optarg);
> @@ -2909,7 +2911,7 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
>  				     (char *)message, strlen(message));
>  	strbuf_release(&buf);
>  	unuse_commit_buffer(commit, message);
> -	return retval;
> +	return opt->grep_filter.global_invert ? !retval : retval;
>  }
>  
>  static inline int want_ancestry(const struct rev_info *revs)