patch, 8 partsgrep.h: remove unused "regex_t regexp" from grep_opt

I'd like to use the grep API for more things in some upcoming optimization patches, and to do that I need to use grep_init(), grep_config() etc.

These APIs have a very unusual API, and we've got quite a bit of code in grep.c to support an obscure edge case in how "grep.extendedRegexp" behaves when clashing with a "grep.patternType" variable present in the same config space.

This series is an opinionated change of that behavior, and resulting large deletion of code.

The series starts out by deleting some unused code in grep.c, and moving a bit of related builtin/grep.c-specific code to that file, and out of grep.c.

Ævar Arnfjörð Bjarmason (8):
  grep.h: remove unused "regex_t regexp" from grep_opt
  git.c & grep.c: assert that "prefix" is NULL or non-zero string
  grep: remove unused "prefix_length" member
  grep.c: move "prefix" out of "struct grep_opt"
  log tests: check if grep_config() is called by "log"-like cmds
  grep API: call grep_config() after grep_init()
  grep: simplify config parsing, change grep.<rx config> interaction
  grep: make "extendedRegexp=true" the same as "patternType=extended"

 Documentation/config/grep.txt |   4 +-
 Documentation/git-grep.txt    |   4 +-
 builtin/grep.c                |  31 +++++----
 builtin/log.c                 |  13 +++-
 git.c                         |   4 +-
 grep.c                        | 118 ++++------------------------------
 grep.h                        |  34 ++++++----
 revision.c                    |   4 +-
 t/t4202-log.sh                |  16 +++++
 t/t7810-grep.sh               |   4 +-
 10 files changed, 89 insertions(+), 143 deletions(-)

This "regex_t" in grep_opt has not been used since f9b9faf6f8a (builtin-grep: allow more than one patterns., 2006-05-02), we still use a "regex_t" for compiling regexes, but that's in the "grep_pat" struct".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.h | 1 -
 1 file changed, 1 deletion(-)

The "prefix" we get from setup.c is either going to be NULL or a string of length >0, never "". So let's drop the "prefix && *prefix" check in grep.c added in 0d042fecf2f (git-grep: show pathnames relative to the current directory, 2006-08-11).

As seen in code in revision.c that was added in cd676a51367 (diff --relative: output paths as relative to the current subdirectory, 2008-02-12) we have existing code that does away with this assertion.

This makes it easier to reason about a subsequent change to the "prefix_length" code in grep.c in a subsequent commit, and since we're going to the trouble of doing that let's leave behind an assert() to promise this to any future callers.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git.c  | 4 ++--
 grep.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

On Sat, Nov 06, 2021 at 10:10:48PM +0100, Ævar Arnfjörð Bjarmason wrote:

Small nit, but the check to `prefix` (in `prefix && *prefix`) is redundant with the left-hand side of the or.

Looking around, ls-tree's initialization includes a conditional of the form:

    if (prefix && *prefix)
      chomp_prefix = strlen(prefix);

So that could be cleaned up too. But honestly, the pre-image of this patch (and the spot in ls-tree) doesn't make a lot of sense to me to begin with.

Even if prefix were the empty string, calling strlen() on it will just give us zero. So there is no difference between assigning `str && *str ? strlen(str) : 0` and `str ? strlen(str) : 0`.

So I am confused why this needs hardening with an assertion when it seems like the checks before calling strlen() were overly restrictive to begin with. In other words: why not only include this hunk (either in this patch, or squashed into another patch later on)?

Thanks, Taylor

Remove the "prefix_length" member, which we compute with a strlen() on the "prefix" argument to grep_init(), but whose strlen() hasn't been used since 493b7a08d80 (grep: accept relative paths outside current working directory, 2009-09-05).

When this code was added in 0d042fecf2f (git-grep: show pathnames relative to the current directory, 2006-08-11) we used the length, but since 493b7a08d80 we haven't used it for anything except a boolean check that we could have done on the "prefix" member itself.

Before a preceding commit we also used to guard the strlen() with "prefix && *prefix", but as that commit notes the RHS of that && chain was also redundant.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c | 4 ++--
 grep.c         | 1 -
 grep.h         | 1 -
 3 files changed, 2 insertions(+), 4 deletions(-)

On Sat, Nov 06, 2021 at 10:10:49PM +0100, Ævar Arnfjörð Bjarmason wrote:

OK, so now we *are* relying on the assumption that prefix is either NULL or a non-empty string.

I assume that the last patch was along the lines of "let's clean up this redundant check before calling strlen()" and "prepare to not call strlen() at all and just check the string itself for NULL". To be honest, I imagine that it would have been much easier to review if these two had been squashed into one, since I was a little surprised to see the line I had just been commenting on in the previous patch removed.

Perhaps I should have looked a little further in the series before commenting there, but I think it would have been even easier for reviewers to see these two patches together.

Everything in this patch looks fine to me, assuming that prefix is indeed always NULL or non-empty (which I haven't verified myself).

Thanks, Taylor

The "struct grep_opt" is a mixture of things that would be needed by all callers of the grep.c API, and quite a few things that only the builtin/grep.c needs.

Since we got rid of "prefix_length" in the previous commit, let's move the "prefix" variable over to "builtin/grep.c" where it's used. To do this let's create a "struct grep_cmd_opt", which we'll have a pointer to in a new "caller_priv" member in "struct grep_opt" (the existing "priv" is used by the top-level "grep.c" itself).

We might eventually need to have grep_opt_dup() learn about this new member, but since the prefix can be a "const char *const" (i.e. we never change it in any way) let's leave that aside for now, in any case "builtin/grep.c" is the only user of grep_opt_dup(), even though it lives in the top-level "grep.c".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c | 17 ++++++++++++-----
 grep.c         |  3 +--
 grep.h         |  4 ++--
 revision.c     |  2 +-
 4 files changed, 16 insertions(+), 10 deletions(-)

On Sat, Nov 06, 2021 at 10:10:50PM +0100, Ævar Arnfjörð Bjarmason wrote:

I'm definitely in favor of removing specialized, caller-specific bits from an internal API. But I'm not sure why grep.c needs to keep track o this new "caller_priv" field at all.

Among the uses of `prefix` in builtin/grep.c, I see grep_source_name, and the call to run_pager(). Would it be more straightforward to pass down prefix from cmd_grep down to its use in grep_source_name?

There are quite a few intermediary functions that we go through to get from cmd_grep() down to grep_source_name(). For instance, we could reach it through:

    cmd_grep -> grep_tree -> grep_submodule -> grep_cache -> grep_oid ->
    grep_source_name

But passing prefix from cmd_grep down to grep_source_name without relying on the internals of grep.c seems like a good direction to me. I could even buy that (ab)using a static variable in builtin/grep.c to keep track of a constant prefix value would save you some plumbing (though I'd rather see the usage spelled out more explicitly).

All of that is to say that I share your motivation for this patch and think that the direction is good, but I would have preferred to do it without the caller_priv variable (unless there is something that I am missing here).

Thanks, Taylor

On Mon, Nov 08 2021, Taylor Blau wrote:

Yes, that would make much more sense. I just had tunnel vision while writing this, evidently. Yeah, just either passing it or using a static variable in grep.c would make more sense.

Arguably we could even have it be read only (const char *const) and have a global "the_prefix" or something, but it's probably not widely used enough for that to make sense.

On Tue, Nov 09, 2021 at 03:10:31AM +0100, Ævar Arnfjörð Bjarmason wrote:

Either of the first two seem fine to me, but introducing `the_prefix` seems both over-specified, and too big a step for this series alone.

Thanks, Taylor

Extend the tests added in my 9df46763ef1 (log: add exhaustive tests for pattern style options & config, 2017-05-20) to check not only whether "git log" handles "grep.patternType", but also "git show" etc.

It's sufficient to check whether a PCRE regex matches for the purposes of this test, we otherwise assume that it's running the same code as "git log", whose behavior is tested more exhaustively by test added in 9df46763ef1e.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4202-log.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

The grep_init() function used the odd pattern of initializing the passed-in "struct grep_opt" with a statically defined "grep_defaults" struct, which would be modified in-place when we invoked grep_config().

So we effectively (b) initialized config, (a) then defaults, (c) followed by user options. Usually those are ordered as "a", "b" and "c" instead.

As the comments being removed here show the previous behavior needed to be carefully explained as we'd potentially share the populated configuration among different instances of grep_init(). In practice we didn't do that, but now that it can't be a concern anymore let's remove those comments.

See 6ba9bb76e02 (grep: copy struct in one fell swoop, 2020-11-29) and 7687a0541e0 (grep: move the configuration parsing logic to grep.[ch], 2012-10-09) for the commits that added the comments.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c |  4 ++--
 builtin/log.c  | 13 +++++++++++--
 grep.c         | 39 +++------------------------------------
 grep.h         | 21 +++++++++++++++++++++
 4 files changed, 37 insertions(+), 40 deletions(-)

On Sat, Nov 06, 2021 at 10:10:52PM +0100, Ævar Arnfjörð Bjarmason wrote:

Do we risk changing any user-visible behavior here? Based on my reading of grep.c before and after this patch, I think the answer is "no", but I wasn't sure if you had done a similar analysis.

In any case, I think the "bring your own structure" instead of getting one copied around is much easier to reason about. Even if we weren't accidentally stomping on ownership of the struct before, not having to reason about it is a nice benefit.

Makes sense, I agree.

This doesn't appear strictly related to the rest of your changes, but only serves to prevent the caller-provided data from being sent down to git_color_default_config().

It didn't matter before because (a) the caller doesn't specify any data to begin with, and git_color_default_config() (or the functions that it calls) don't do anything with the extra pointer. Now cmd_grep() is going to start passing around a pointer to a struct grep_opt.

But git_color_default_config() still doesn't do anything with the pointer it receives, and passing that pointer around is standard practice among config.c code. So I don't think that this hunk is strictly necessary, and it's somewhat different than the pattern established within config.c.

I wouldn't be sad to see this hunk dropped (and in fact have a slight preference leaning this way), but I don't mind keeping it around, either.

I'm nit-picking, but creating a throwaway struct for the convenience of using designated initialization (at the cost of having to memcpy an entire struct around) seems like overkill.

Especially since we're just going to write into the other fields of the the target struct anyway, I'd probably rather have seen everything written out explicitly without the throwaway or memcpy.

Thanks, Taylor

On Mon, Nov 08 2021, Taylor Blau wrote:

I don't think we're changing any behavior except the one noted in this series.

We only set a few config variables, so I thought that was fairly easy to trace...

Will either split it up or drop it.

It's a widely used pattern in the codebase at this point, see 5726a6b4012 (*.c *_init(): define in terms of corresponding *_INIT macro, 2021-07-01) (mine, but I stole it from Jeff King).

As his linked-to compiler test shows the memcpy() is optimized away, so modern compilers will treat these idioms the same way.

There was a suggestions somewhere that we should prorably move to that "*<x> = <y>" or whatever it was briefer C99 (I think) syntax across the board, it would be less verbose. But I haven't tested if it's as widely supported, so I've just been sticking with that blank/memcpy() pattern for "do init in terms of macro".

On Tue, Nov 09, 2021 at 03:06:22AM +0100, Ævar Arnfjörð Bjarmason wrote:

I do at least prefer memcpy() over *<x> = <y> when x and y are structures. But I wasn't aware that this was common in our codebase. Anyway, my suggestion was only along the lines of "you're already writing individual fields below, so why not just do that throughout instead of memcpy()-ing some of them via a macro which expands to a designated initializer?"

But this is a cosmetic point, so whatever you feel fits in most with the surrounding style (so long as the pattern we're propagating isn't terrible, which is the case here) then I'm OK with it.

Thanks, Taylor

Change the interaction between "grep.patternType=default" and "grep.extendedRegexp=true" to make setting "grep.extendedRegexp=true" synonymous with setting "grep.patternType=extended".

This changes our existing config parsing behavior as detailed below, but in a way that's consistent with how we parse other configuration.

Pedantically speaking we're probably breaking past promises here, but I doubt that this will impact anyone in practice. The reduction in complexity and resulting consistency with other default config behavior is worth it.

When "grep.patternType" was introduced in 84befcd0a4a (grep: add a grep.patternType configuration setting, 2012-08-03) we made two seemingly contradictory promises:

 1. You can set "grep.patternType", and "[setting it to] 'default'
    will return to the default matching behavior".

 2. Support the existing "grep.extendedRegexp" option, but ignore it
    when the new "grep.patternType" is set, *except* "when the
    `grep.patternType` option is set. to a value other than 'default'".

I think that 84befcd0a4a probably didn't intend this behavior, but instead ended up conflating our internal "unspecified" state with a user's explicit desire to set the configuration back to the default.

I.e. a user would correctly expect this to keep working:

    # ERE grep
    git -c grep.extendedRegexp=true grep <pattern>

And likewise for "grep.patternType=default" to take precedence over the disfavored "grep.extendedRegexp" option, i.e. the usual "last set wins" semantics.

    # BRE grep
    git -c grep.extendedRegexp=true -c grep.patternType=basic grep <pattern>

But probably not for this to ignore the new "grep.patternType" option entirely, say if /etc/gitconfig was still setting "grep.extendedRegexp", but "~/.gitconfig" used the new "grep.patternType" (and wanted to use the "default" value):

    # Was ERE, now BRE
    git -c grep.extendedRegexp=true grep.patternType=default grep <pattern>

I think that in practice nobody or almost nobody is going to be relying on this obscure interaction, and as shown here it makes the config parsing much simpler. We no longer have to carry a complex state machine in "grep_commit_pattern_type()" and "grep_set_pattern_type_option()".

We can also do away with the "int fixed" and "int pcre2" members in favor of using "pattern_type_option" directly in "grep.c", as well as dropping the "pattern_type_arg" variable in "builtin/grep.c" in favor of using the "pattern_type_option" member directly.

See my 07a3d411739 (grep: remove regflags from the public grep_opt API, 2017-06-29) for addition of the two comments being removed here, i.e. the complexity noted in that commit is now going away.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config/grep.txt |  3 +-
 Documentation/git-grep.txt    |  3 +-
 builtin/grep.c                | 10 ++---
 grep.c                        | 71 +++++------------------------------
 grep.h                        |  6 +--
 revision.c                    |  2 -
 t/t7810-grep.sh               |  2 +-
 7 files changed, 17 insertions(+), 80 deletions(-)

On Sat, Nov 06, 2021 at 10:10:53PM +0100, Ævar Arnfjörð Bjarmason wrote:

OK, so this is the case that we'd be "breaking". And I think that the new behavior you're outlining here (where a higher-precedence grep.patternType=default overrides a lower-precedence grep.extendedRegexp=true, resulting in using BRE over ERE) makes more sense.

At least, it makes more sense if your expectation of "default" is "the default matching behavior", not "fallthrough to grep.extendedRegexp".

In any case, I am sensitive to breaking existing user workflows, but this seems so obscure to me that I have a hard time expecting that m(any?) users will even notice this at all.

The situation I'm most concerned about is having grep.extendedRegexp set in, say, /etc/gitconfig and grep.patternType=default set at a higher-precedence level.

Not the fault of your patch, but these two are annoyingly (and subtly) different from one another. Could we clean this up and put everything in Documentation/config/grep.txt (and then include that in the CONFIGURATION section of Documentation/git-grep.txt)?

Makes sense, and matches your description.

And here's our "as long as we haven't set the pattern type already via grep.patternType, allow grep.extendedRegexp to set it". But the same "only set when unspecified" condition *isn't* in place for grep.patternType, which is what makes us prefer values from that configuration over the other. Makes sense.

Everything else looks good here, too.

Thanks, Taylor

On Mon, Nov 08 2021, Taylor Blau wrote:

*nod*, but the only user who'd end up with that is someone who's trying to override grep.extendedRegexp but failing to do it, so this would help.

Or someone who'd read the docs, understood that we promised that would do nothing, and inserted that just to test us, but that seems unlikely :)

Or, I suppose someone who's entirely confused, and will continue being even more confused now that behavior changes on a git upgrade from ERE to BRE.

I'm hoping the last two paragraphs describe no-one & that this is safe to do.

I've got a large series to do that for all of these, but opted to skip that particular digression here (even for just grep.txt it's a bit distracting).

On Tue, Nov 09, 2021 at 03:01:12AM +0100, Ævar Arnfjörð Bjarmason wrote:

Yeah, I agree that it seems very unlikely that anybody would actually be affected here, so I'm comfortable with the change.

I think that you could include a preparatory clean-up in this series to make the Documentation/config/grep.txt included by Documentation/git-grep.txt without being too distracting. But if punting on it now works better for you, I don't think either makes a reviewer's job substantially different.

Thanks, Taylor

In the preceding commit we changed how a "grep.patternType=default" set after "grep.extendedRegexp=true" would be handled so that the last set would win, but a "grep.extendedRegexp=true" would only be used if "grep.patternType" was set to a value other than "default".

Thus a user who had old config and set "grep.extendedRegexp=true" in their ~/.gitconfig expecting ERE behavior would be opted-in to say "perl" regexes if a system "/etc/gitconfig" started setting "grep.patternType=perl".

These funny semantics of only paying attention to a set if another key is not set to a given value aren't how we treat other config keys, so let's do away with this caveat for consistency.

The new semantics are simple, a "grep.extendedRegexp=true" is an exact synonym for specifying "grep.patternType=extended" in the config. We'll keep ignoring ""grep.extendedRegexp=false", although arguably we could treat it as a "grep.patternType=basic".

As argued in the preceding commit I think this behavior came about because we were conflating the state of our code's own internal "default" value with what we found in explicit user config. See 84befcd0a4a (grep: add a grep.patternType configuration setting, 2012-08-03) for that past behavior.

Let's further change the documentation to note that "grep.extendedRegexp" is a deprecated synonym, perhaps we'll be able to remove it at some point in the future and do away with this special-case entirely.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config/grep.txt | 3 +--
 Documentation/git-grep.txt    | 3 +--
 grep.c                        | 8 +++-----
 grep.h                        | 5 +----
 t/t7810-grep.sh               | 2 +-
 5 files changed, 7 insertions(+), 14 deletions(-)

This series changes the behavior of an obscure interaction between "grep.extendedRegexp=true" and "grep.patternType=default". See 7-8/8. Along the way we can delete a lot of code that was needed to support the previous behavior.

For v1 and a more extensive summary see [1]. Thanks a lot Taylor for the detailed review on v1!

Hopefully this v1 addresses all the feedback on one way or another, it's still 8 patches, but much of the early part of v1 is squashed together & re-done as suggested.

Then there's a mid-series de-duplication of the grep config documentation. I ended up keeping the change to not needlessly pass "cb" around in grep_cmd_config(), but that's now also in its own patch.

1. https://lore.kernel.org/git/cover-0.8-00000000000-20211106T210711Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (8):
  grep.h: remove unused "regex_t regexp" from grep_opt
  built-ins: trust the "prefix" from run_builtin()
  log tests: check if grep_config() is called by "log"-like cmds
  grep docs: de-duplicate configuration sections
  grep.c: don't pass along NULL callback value
  grep API: call grep_config() after grep_init()
  grep: simplify config parsing, change grep.<rx config> interaction
  grep: make "extendedRegexp=true" the same as "patternType=extended"

 Documentation/config/grep.txt |  11 ++--
 Documentation/git-grep.txt    |  30 +--------
 builtin/grep.c                |  27 ++++----
 builtin/log.c                 |  13 +++-
 builtin/ls-tree.c             |   9 ++-
 git.c                         |   4 +-
 grep.c                        | 118 ++++------------------------------
 grep.h                        |  35 ++++++----
 revision.c                    |   4 +-
 t/t4202-log.sh                |  16 +++++
 t/t7810-grep.sh               |   4 +-
 11 files changed, 97 insertions(+), 174 deletions(-)

Range-diff against v1:
1:  412b8b65266 = 1:  1435db727ef grep.h: remove unused "regex_t regexp" from grep_opt
2:  244715e3497 < -:  ----------- git.c & grep.c: assert that "prefix" is NULL or non-zero string
3:  3338cc95b81 < -:  ----------- grep: remove unused "prefix_length" member
4:  78298657d69 < -:  ----------- grep.c: move "prefix" out of "struct grep_opt"
-:  ----------- > 2:  63cf2fe266d built-ins: trust the "prefix" from run_builtin()
5:  ba9be0b9283 = 3:  41e38ebb32c log tests: check if grep_config() is called by "log"-like cmds
-:  ----------- > 4:  efe95397d72 grep docs: de-duplicate configuration sections
-:  ----------- > 5:  d0f0ac6c7ae grep.c: don't pass along NULL callback value
6:  933ac853bca ! 6:  917944f79a5 grep API: call grep_config() after grep_init()
    @@ Commit message
         didn't do that, but now that it can't be a concern anymore let's
         remove those comments.
     
    +    This does not change the behavior of any of the configuration
    +    variables or options. That would have been the case if we didn't move
    +    around the grep_config() call in "builtin/log.c". But now that we call
    +    "grep_config" after "git_log_config" and "git_format_config" we'll
    +    need to pass in the already initialized "struct grep_opt *".
    +
         See 6ba9bb76e02 (grep: copy struct in one fell swoop, 2020-11-29) and
         7687a0541e0 (grep: move the configuration parsing logic to grep.[ch],
         2012-10-09) for the commits that added the comments.
     
    +    The memcpy() pattern here will be optimized away and follows the
    +    convention of other *_init() functions. See 5726a6b4012 (*.c *_init():
    +    define in terms of corresponding *_INIT macro, 2021-07-01).
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/grep.c ##
     @@ builtin/grep.c: static int wait_all(void)
    + 
      static int grep_cmd_config(const char *var, const char *value, void *cb)
      {
    - 	int st = grep_config(var, value, cb);
    --	if (git_color_default_config(var, value, cb) < 0)
    -+	if (git_color_default_config(var, value, NULL) < 0)
    +-	int st = grep_config(var, value, NULL);
    ++	int st = grep_config(var, value, cb);
    + 	if (git_color_default_config(var, value, NULL) < 0)
      		st = -1;
      
    - 	if (!strcmp(var, "grep.threads")) {
     @@ builtin/grep.c: int cmd_grep(int argc, const char **argv, const char *prefix)
    - 		OPT_END()
      	};
    + 	grep_prefix = prefix;
      
     -	git_config(grep_cmd_config, NULL);
      	grep_init(&opt, the_repository);
     +	git_config(grep_cmd_config, &opt);
    - 	opt.caller_priv = &opt_cmd;
      
      	/*
    + 	 * If there is no -- then the paths must exist in the working
     
      ## builtin/log.c ##
     @@ builtin/log.c: static int git_log_config(const char *var, const char *value, void *cb)
    @@ grep.c: int grep_config(const char *var, const char *value, void *cb)
     
      ## grep.h ##
     @@ grep.h: struct grep_opt {
    + 	int show_hunk_mark;
    + 	int file_break;
    + 	int heading;
    ++	void *caller_priv;
    + 	void *priv;
    + 
    + 	void (*output)(struct grep_opt *opt, const void *data, size_t size);
      	void *output_priv;
      };
      
7:  677a8f8520f ! 7:  140a7416223 grep: simplify config parsing, change grep.<rx config> interaction
    @@ Commit message
         but in a way that's consistent with how we parse other
         configuration.
     
    -    Pedantically speaking we're probably breaking past promises here, but
    -    I doubt that this will impact anyone in practice. The reduction in
    -    complexity and resulting consistency with other default config
    -    behavior is worth it.
    +    We are breaking past promises here, but I doubt that this will impact
    +    anyone in practice. The reduction in complexity and resulting
    +    consistency with other default config behavior is worth it.
     
         When "grep.patternType" was introduced in 84befcd0a4a (grep: add a
         grep.patternType configuration setting, 2012-08-03) we made two
    @@ Commit message
             # BRE grep
             git -c grep.extendedRegexp=true -c grep.patternType=basic grep <pattern>
     
    -    But probably not for this to ignore the new "grep.patternType" option
    -    entirely, say if /etc/gitconfig was still setting
    +    But probably not for this to ignore the favored "grep.patternType"
    +    option entirely, say if /etc/gitconfig was still setting
         "grep.extendedRegexp", but "~/.gitconfig" used the new
         "grep.patternType" (and wanted to use the "default" value):
     
    @@ Documentation/config/grep.txt: grep.patternType::
      	If set to true, enable `--extended-regexp` option by default. This
     -	option is ignored when the `grep.patternType` option is set to a value
     -	other than 'default'.
    -+	option is ignored when the `grep.patternType` option is set.
    - 
    - grep.threads::
    - 	Number of grep worker threads to use.
    -
    - ## Documentation/git-grep.txt ##
    -@@ Documentation/git-grep.txt: grep.patternType::
    - 
    - grep.extendedRegexp::
    - 	If set to true, enable `--extended-regexp` option by default. This
    --	option is ignored when the `grep.patternType` option is set to a value
    --	other than 'default'.
     +	option is ignored when the `grep.patternType` option is set.
      
      grep.threads::
8:  dadd5dff77a ! 8:  cc904d93b26 grep: make "extendedRegexp=true" the same as "patternType=extended"
    @@ Documentation/config/grep.txt: grep.patternType::
      grep.extendedRegexp::
     -	If set to true, enable `--extended-regexp` option by default. This
     -	option is ignored when the `grep.patternType` option is set.
    -+	Deprecated synonym for 'grep.patternType=extended`.
    - 
    - grep.threads::
    - 	Number of grep worker threads to use.
    -
    - ## Documentation/git-grep.txt ##
    -@@ Documentation/git-grep.txt: grep.patternType::
    - 	value 'default' will return to the default matching behavior.
    - 
    - grep.extendedRegexp::
    --	If set to true, enable `--extended-regexp` option by default. This
    --	option is ignored when the `grep.patternType` option is set.
     +	Deprecated synonym for 'grep.patternType=extended`.
      
      grep.threads::
    @@ grep.h: struct grep_opt {
      	.colors = { \
      		[GREP_COLOR_CONTEXT] = "", \
      		[GREP_COLOR_FILENAME] = "", \
    -@@ grep.h: struct grep_opt {
    - 		[GREP_COLOR_SELECTED] = "", \
    - 		[GREP_COLOR_SEP] = GIT_COLOR_CYAN, \
    - 	}, \
    --	.only_matching = 0, \
    - 	.color = -1, \
    - 	.output = std_output, \
    - }
     
      ## t/t7810-grep.sh ##
     @@ t/t7810-grep.sh: do

This "regex_t" in grep_opt has not been used since f9b9faf6f8a (builtin-grep: allow more than one patterns., 2006-05-02), we still use a "regex_t" for compiling regexes, but that's in the "grep_pat" struct".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.h | 1 -
 1 file changed, 1 deletion(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

I would have expected "this used to be used but no longer; only initialization of and assignment to it remain"; I am somewhat surprised to see there is no mention to it anywhere in the code ;-)

Good find.  

Change code in "builtin/grep.c" and "builtin/ls-tree.c" to trust the "prefix" passed from "run_builtin()". The "prefix" we get from setup.c is either going to be NULL or a string of length >0, never "".

So we can drop the "prefix && *prefix" checks added for "builtin/grep.c" in 0d042fecf2f (git-grep: show pathnames relative to the current directory, 2006-08-11), and for "builtin/ls-tree.c" in a69dd585fca (ls-tree: chomp leading directories when run from a subdirectory, 2005-12-23).

As seen in code in revision.c that was added in cd676a51367 (diff --relative: output paths as relative to the current subdirectory, 2008-02-12) we already have existing code that does away with this assertion.

This makes it easier to reason about a subsequent change to the "prefix_length" code in grep.c in a subsequent commit, and since we're going to the trouble of doing that let's leave behind an assert() to promise this to any future callers.

For "builtin/grep.c" it would be painful to pass the "prefix" down the callchain of:

    cmd_grep -> grep_tree -> grep_submodule -> grep_cache -> grep_oid ->
    grep_source_name

So for the code that needs it in grep_source_name() let's add a "grep_prefix" variable similar to the existing "ls_tree_prefix".

While at it let's move the code in cmd_ls_tree() around so that we assign to the "ls_tree_prefix" right after declaring the variables, and stop assigning to "prefix". We only subsequently used that variable later in the function after clobbering it. Let's just use our own "grep_prefix" instead.

Let's also add an assert() in git.c, so that we'll make this promise about the "prefix" to any current and future callers, as well as to any readers of the code.

Code history:

 * The strlen() in "grep.c" hasn't been used since 493b7a08d80 (grep:
   accept relative paths outside current working directory, 2009-09-05).

   When that code was added in 0d042fecf2f (git-grep: show pathnames
   relative to the current directory, 2006-08-11) we used the length.

   But since 493b7a08d80 we haven't used it for anything except a
   boolean check that we could have done on the "prefix" member
   itself.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c    | 13 ++++++++-----
 builtin/ls-tree.c |  9 ++++-----
 git.c             |  4 ++--
 grep.c            |  4 +---
 grep.h            |  4 +---
 revision.c        |  2 +-
 6 files changed, 17 insertions(+), 19 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

See below.

We now assume a non-NULL prefix means a non-NUL *prefix, so this change is understandable.

This moving down of git_config() call is not. A necessary change, or an unrelated churn? If necessary, why? By not checking if prefix[0] is NUL, we now need to delay reading the configuration, because ...?

The above two are unnecessary changes.

It was not like the introduction of ls_tree_prefix was made in order to get rid of "prefix" altogether. We still have and use prefix, but we have ls_tree_prefix to expose the value of it to other functions as a way to "cheat", without having to pass it through as a parameter (and cheating is OK as its scope is limited to the file).

Perhaps make a conscious effort to refrain from making such unnecessary changes, especially when working on a multi-patch series, which may avoid wearing down reviewers?

Likewise.  Little cuts accumulate.

Good.

So, builtin/grep.c is the only user of the low-level grep machinery that needs to touch these two, and we can lose these members now that builtin/grep.c relies on the file-scope global instead. OK.

Extend the tests added in my 9df46763ef1 (log: add exhaustive tests for pattern style options & config, 2017-05-20) to check not only whether "git log" handles "grep.patternType", but also "git show" etc.

It's sufficient to check whether a PCRE regex matches for the purposes of this test, we otherwise assume that it's running the same code as "git log", whose behavior is tested more exhaustively by test added in 9df46763ef1e.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4202-log.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

I do agree with the reasoning that it is sufficient to check with only one kind, but it is unclear if PCRE is so special and if so why. Wouldn't testing with, say grep.patternType=extended, alone also be sufficient? The reason I am asking it is mostly because the above description is insufficient to answer these questions, but also because we can lose the prerequisite and gain a bit wider test coverage, if we use a pattern that is not PCRE.

Knowing that "show", "whatchanged", "reflog", and "format-patch" all share the same backend in builtin/log.c, I am not sure how much value the new tests add. Testing the "--grep-reflog" option and the "git rev-list" command may exercise a bit different code path, though.

I prefer to send the output that ought to contain the grep hits consistently to "actual", i.e.

	case "$cmd" in
	format-patch) myarg="--stdout -1" ;;
	*) myarg= ;;
	esac	

instead of sending only the filenames.

I also prefer "case/esac" as it would be more concise when we need to tweak myarg per command later.

Thanks.

Include the "config/grep.txt" file in "git-grep.txt", instead of repeating an almost identical description of the "grep" configuration variables in two places. In a subsequent commit we'll amend this documentation, and can now do so in one place instead of two.

Let's also add a short blurb at the top indicating that this is included documentation, so users won't think that they need to read the two versions and compare them.

That wording is copy/pasted from the change I made in b6a8d09f6d8 (gc docs: include the "gc.*" section from "config" in "gc", 2019-04-07), eventually we'll want to include this via template, and indeed this change is extracted from a WIP series that fixes all these "CONFIGURATION" includes which does that. But doing that would require build system changes, so let's punt on it for now.

There is no loss of information here that isn't shown in the addition to "grep.txt". This change was made by copying the contents of "git-grep.txt"'s version over the "grep.txt" version. Aside from the change "grep.txt" being made here the two were identical.

This documentation started being copy/pasted around in b22520a37c8 (grep: allow -E and -n to be turned on by default via configuration, 2011-03-30). After that in e.g. 6453f7b3486 (grep: add grep.fullName config variable, 2014-03-17) they started drifting apart, with only grep.fullName being described in the command documentation.

In 434e6e753fe (config.txt: move grep.* to a separate file, 2018-10-27) we gained the include, but didn't do this next step, let's do it now.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config/grep.txt |  7 +++++--
 Documentation/git-grep.txt    | 30 +++---------------------------
 2 files changed, 8 insertions(+), 29 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Good find. They are indeed almost identical. I am not sure about the value of ...

... when everybody becomes consistent, but in the meantime, while some documentation pages are consistent while others are not, I can see how it might help.

The patch looks good.

Thanks.

Change grep_cmd_config() top stop passing around the always-NULL "cb" value. When this code was added in 7e8f59d577e (grep: color patterns in output, 2009-03-07) it was non-NULL, but when that changed in 15fabd1bbd4 (builtin/grep.c: make configuration callback more reusable, 2012-10-09) this code was left behind.

In a subsequent change I'll start using the "cb" value, this will make it clear which functions we call need it, and which don't.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

"Change X top stop passing"? I cannot guess so I will not say "I'll fix it to X, no need to resend".

The change itself does seem sensible.

Thanks.

The grep_init() function used the odd pattern of initializing the passed-in "struct grep_opt" with a statically defined "grep_defaults" struct, which would be modified in-place when we invoked grep_config().

So we effectively (b) initialized config, (a) then defaults, (c) followed by user options. Usually those are ordered as "a", "b" and "c" instead.

As the comments being removed here show the previous behavior needed to be carefully explained as we'd potentially share the populated configuration among different instances of grep_init(). In practice we didn't do that, but now that it can't be a concern anymore let's remove those comments.

This does not change the behavior of any of the configuration variables or options. That would have been the case if we didn't move around the grep_config() call in "builtin/log.c". But now that we call "grep_config" after "git_log_config" and "git_format_config" we'll need to pass in the already initialized "struct grep_opt *".

See 6ba9bb76e02 (grep: copy struct in one fell swoop, 2020-11-29) and 7687a0541e0 (grep: move the configuration parsing logic to grep.[ch], 2012-10-09) for the commits that added the comments.

The memcpy() pattern here will be optimized away and follows the convention of other *_init() functions. See 5726a6b4012 (*.c *_init(): define in terms of corresponding *_INIT macro, 2021-07-01).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c |  4 ++--
 builtin/log.c  | 13 +++++++++++--
 grep.c         | 39 +++------------------------------------
 grep.h         | 22 ++++++++++++++++++++++
 4 files changed, 38 insertions(+), 40 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

OK, so we did this because we wanted to be able to

   1. call grep_config() only once to populate the template;

   2. call grep_init() more than once, and match the grep_opt to
      what the config wanted, without having to call grep_config()
      once per grep_init() invocation.

   3. each invocation of grep_init() in 2. may be followed by
      parse_options() to further tweak grep_opt.

And now we instead have to do

   1. call grep_init()
   2. call grep_config()
   3. parse_options() to tweak

for each instance of grep_opt, which is much more common.

OK.

This used to tweak the "default template", which we no longer use, so can go? And in its place ...

... each command in the "log" family tweaks the grep_opt used for real from the configuration.

Ditto.  OK, the new pattern makes sense.

This is unrelated and unexplained change, isn't it?

Other than the mysterious caller_priv bit, the change makes sense to me.

Thanks.

Change the interaction between "grep.patternType=default" and "grep.extendedRegexp=true" to make setting "grep.extendedRegexp=true" synonymous with setting "grep.patternType=extended".

This changes our existing config parsing behavior as detailed below, but in a way that's consistent with how we parse other configuration.

We are breaking past promises here, but I doubt that this will impact anyone in practice. The reduction in complexity and resulting consistency with other default config behavior is worth it.

When "grep.patternType" was introduced in 84befcd0a4a (grep: add a grep.patternType configuration setting, 2012-08-03) we made two seemingly contradictory promises:

 1. You can set "grep.patternType", and "[setting it to] 'default'
    will return to the default matching behavior".

 2. Support the existing "grep.extendedRegexp" option, but ignore it
    when the new "grep.patternType" is set, *except* "when the
    `grep.patternType` option is set. to a value other than 'default'".

I think that 84befcd0a4a probably didn't intend this behavior, but instead ended up conflating our internal "unspecified" state with a user's explicit desire to set the configuration back to the default.

I.e. a user would correctly expect this to keep working:

    # ERE grep
    git -c grep.extendedRegexp=true grep <pattern>

And likewise for "grep.patternType=default" to take precedence over the disfavored "grep.extendedRegexp" option, i.e. the usual "last set wins" semantics.

    # BRE grep
    git -c grep.extendedRegexp=true -c grep.patternType=basic grep <pattern>

But probably not for this to ignore the favored "grep.patternType" option entirely, say if /etc/gitconfig was still setting "grep.extendedRegexp", but "~/.gitconfig" used the new "grep.patternType" (and wanted to use the "default" value):

    # Was ERE, now BRE
    git -c grep.extendedRegexp=true grep.patternType=default grep <pattern>

I think that in practice nobody or almost nobody is going to be relying on this obscure interaction, and as shown here it makes the config parsing much simpler. We no longer have to carry a complex state machine in "grep_commit_pattern_type()" and "grep_set_pattern_type_option()".

We can also do away with the "int fixed" and "int pcre2" members in favor of using "pattern_type_option" directly in "grep.c", as well as dropping the "pattern_type_arg" variable in "builtin/grep.c" in favor of using the "pattern_type_option" member directly.

See my 07a3d411739 (grep: remove regflags from the public grep_opt API, 2017-06-29) for addition of the two comments being removed here, i.e. the complexity noted in that commit is now going away.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config/grep.txt |  3 +-
 builtin/grep.c                | 10 ++---
 grep.c                        | 71 +++++------------------------------
 grep.h                        |  6 +--
 revision.c                    |  2 -
 t/t7810-grep.sh               |  2 +-
 6 files changed, 16 insertions(+), 78 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

This description alone is not quite understandable. It is not saying much more than the single line title, and presense of it does not seem to improve the understanding by the readers.

OK, so setting grep.patternType=default makes grep.extendedRegexp to be taken into account. By grep.patternType to something else, the other one is ignored. 2. is a very explicit way to say so. Where did you get 1. from? If you have this paragraph in the log message in mind, I agree that it is less than ideally phrased, but ...

    Rather than adding an additional setting for grep.fooRegexp for
    current and future pattern matching options, add a
    grep.patternType setting that can accept appropriate values for
    modifying the default grep pattern matching behavior. The
    current values are "basic", "extended", "fixed", "perl" and
    "default" for setting -G, -E, -F, -P and the default behavior
    respectively.

... with the understanding of 2. (which is in what the commit adds to Documentation/config.txt), it is reasonable to understand that "the default behaviour" is "use BRE or ERE, depending on the setting of grep.extendedRegexp".

Doesn't the code behave that way? I think the above is exactly how the commit wanted to make the code behave.

I am not sure where that comes from, but if I imagine somebody confuses between "default" and "basic" and considers "default" a synonym for "basic", I can sort-of understand it. Is it what is happening here?

But it is not what the original .patternType patch wanted to do back then, and it is not what we want to see now.

This makes sense.

This makes sense, too.

Do either of the above two not work as you expect (i.e. the first use ERE and the second use BRE)?

What I have trouble with is that it is unclear if you are describing what should happen (in the above, I said "makes sense", to show my agreement, assuming that it is the case), or if you are describing what does happen that you disagree with.

Another thing I have trouble with is your mention of "keep working". Are you proposing to deliberately break what is working as users correctly expect? Why?

I do not quite get your "Was X, now Y" label. What did you want to say with that?

Also I am not sure what you exactly mean when you say "and wanted to use the 'default' value". There is no single "THE" default value. If patternType=default is the last patterntype (it may be set in many places, but the last one should win), the user is telling that the last-one-wins setting of extendedRegexp is to be honored. So, if grep.extendedRegexp in /etc/gitconfig is the last one defined, we would choose between BRE and ERE depending on that setting.

Isn't that what is happening in the current code?

Or are all the above explanation result of simple misunderstanding that setting to "default" means setting to "basic"?

On Fri, Nov 12 2021, Junio C Hamano wrote:

I'll try to reply to all the rest of the feedback, just really quick on this, because I think it might represent a bit of a gordian knot.

Yes, I'd like to change the behavior, because it makes the grep API much easier to deal with, and beacuse I think it impacts nobody in practice.

The real goal for this series is that I've got pending patches to speedup diffcore-pickaxe massively by moving it over to PCRE & drop the kwset.c code. An old perf test I dug up for that is in [1].

To do that I needed to re-use the bits of grep.c machinery that deal with setting up patterns, dealing with BRE,ERE,PCRE etc. elsewhere.

I *can* do that in a different way, but it's going to be much easier if we can gradually evolve the already working grep API to become an internal textual pattern matching API. Eventually I'd like to move all of regcomp()/regexec() over to such a thing, because we can for any other ranodm thing we use regexes for get speedups by using PCRE (and optionally use its interface to understand BRE/ERE syntax).

The alternative is to split that part off from grep.c, which is a bit more painful, or to have the init bits etc. take some "no config doesn't go first", "no it goes first" flags just to support this one API user.

So it would be generally useful to know if you're at all open to that. Reading between the lines in some other comments I fear that it may be a "no" except if we mark it as deprecated, wait some years, maybe remove/change it then etc.

1.

    GIT_TEST_LONG= GIT_PERF_REPEAT_COUNT=10 GIT_PERF_MAKE_OPTS='-j8 USE_LIBPCRE=1 CFLAGS=-O3 LIBPCREDIR=/home/avar/g/pcre2/inst' ./run origin/next HEAD -- p4209-pickaxe.sh
    Test                                                                      origin/next       HEAD
    ------------------------------------------------------------------------------------------------------------------
    4209.1: git log -S'int main' <limit-rev>..                                0.38(0.36+0.01)   0.37(0.33+0.04) -2.6%
    4209.2: git log -S'æ' <limit-rev>..                                       0.51(0.47+0.04)   0.32(0.27+0.05) -37.3%
    4209.3: git log --pickaxe-regex -S'(int|void|null)' <limit-rev>..         0.72(0.68+0.03)   0.57(0.54+0.03) -20.8%
    4209.4: git log --pickaxe-regex -S'if *\([^ ]+ & ' <limit-rev>..          0.60(0.55+0.02)   0.39(0.34+0.05) -35.0%
    4209.5: git log --pickaxe-regex -S'[àáâãäåæñøùúûüýþ]' <limit-rev>..       0.43(0.40+0.03)   0.50(0.44+0.06) +16.3%
    4209.6: git log -G'(int|void|null)' <limit-rev>..                         0.64(0.55+0.09)   0.63(0.56+0.05) -1.6%
    4209.7: git log -G'if *\([^ ]+ & ' <limit-rev>..                          0.64(0.59+0.05)   0.63(0.56+0.06) -1.6%
    4209.8: git log -G'[àáâãäåæñøùúûüýþ]' <limit-rev>..                       0.63(0.54+0.08)   0.62(0.55+0.06) -1.6%
    4209.9: git log -i -S'int main' <limit-rev>..                             0.39(0.35+0.03)   0.38(0.35+0.02) -2.6%
    4209.10: git log -i -S'æ' <limit-rev>..                                   0.39(0.33+0.06)   0.32(0.28+0.04) -17.9%
    4209.11: git log -i --pickaxe-regex -S'(int|void|null)' <limit-rev>..     0.90(0.84+0.05)   0.58(0.53+0.04) -35.6%
    4209.12: git log -i --pickaxe-regex -S'if *\([^ ]+ & ' <limit-rev>..      0.71(0.64+0.06)   0.40(0.37+0.03) -43.7%
    4209.13: git log -i --pickaxe-regex -S'[àáâãäåæñøùúûüýþ]' <limit-rev>..   0.43(0.40+0.03)   0.50(0.46+0.04) +16.3%
    4209.14: git log -i -G'(int|void|null)' <limit-rev>..                     0.64(0.57+0.06)   0.62(0.56+0.05) -3.1%
    4209.15: git log -i -G'if *\([^ ]+ & ' <limit-rev>..                      0.65(0.59+0.06)   0.63(0.54+0.08) -3.1%
    4209.16: git log -i -G'[àáâãäåæñøùúûüýþ]' <limit-rev>..                   0.63(0.55+0.08)   0.62(0.56+0.05) -1.6%

In the preceding commit we changed how a "grep.patternType=default" set after "grep.extendedRegexp=true" would be handled so that the last set would win, but a "grep.extendedRegexp=true" would only be used if "grep.patternType" was set to a value other than "default".

Thus a user who had old config and set "grep.extendedRegexp=true" in their ~/.gitconfig expecting ERE behavior would be opted-in to say "perl" regexes if a system "/etc/gitconfig" started setting "grep.patternType=perl".

These funny semantics of only paying attention to a set if another key is not set to a given value aren't how we treat other config keys, so let's do away with this caveat for consistency.

The new semantics are simple, a "grep.extendedRegexp=true" is an exact synonym for specifying "grep.patternType=extended" in the config. We'll keep ignoring ""grep.extendedRegexp=false", although arguably we could treat it as a "grep.patternType=basic".

As argued in the preceding commit I think this behavior came about because we were conflating the state of our code's own internal "default" value with what we found in explicit user config. See 84befcd0a4a (grep: add a grep.patternType configuration setting, 2012-08-03) for that past behavior.

Let's further change the documentation to note that "grep.extendedRegexp" is a deprecated synonym, perhaps we'll be able to remove it at some point in the future and do away with this special-case entirely.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config/grep.txt | 3 +--
 grep.c                        | 8 +++-----
 grep.h                        | 4 +---
 t/t7810-grep.sh               | 2 +-
 4 files changed, 6 insertions(+), 11 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

I am getting the feeling that my suspicion about your confusion in 7/8 is right.

The grep.patternType=default was merely a backward compatibility measure for those who were used to grep.extendedRegexp=true/false way of doing things, and "default" never meant to mean "basic". It merely was an instruction to "honor the old extendedRegexp variable for this old timer". The intention all along was that patternType was a more flexible single true way to set the type to supersede extendedRegexp but we couldn't discontinue the support for the latter all of a sudden, and the "default" was invented as the transition mechanism, but it ended up as the mechanism to choose either new (setting patternType to some other value) or old (patternType is set to default, making the value given to extendedRegexp is the only thing that matters) world to live in.

If you want to change anything in this area, the right thing to do is rather to _deprecate_ extendedRegexp and eventually remove extendedRegexp together with the "default" setting to patternType, I would think, not to change the semantics of extendedRegexp in any way.

Thanks.

On Wed, Nov 10, 2021 at 02:43:42AM +0100, Ævar Arnfjörð Bjarmason wrote:

Thanks. I'm going to put this version on my review backlog for now if you don't mind, since I think it's more pertinent for us to focus on the upcoming release.

But I look forward to getting back to this once the release has stabilized a little bit.

Thanks, Taylor

In v1 and v2[1] of this series more code in grep.c was deleted by changing what I think is a really obscure interaction between "grep.extendedRegexp=true" and "grep.patternType".

Junio preferred having a deprecation period[2], so here's a re-roll that preserves all existing behavior, at the cost of bit less code deletion & simplification (from "97 insertions(+), 174 deletions(-)" to "106 insertions(+), 131 deletions(-)").

Notes on individual patches below. This re-roll should address all outstanding feedback on v2.

1. https://lore.kernel.org/git/cover-v2-0.8-00000000000-20211110T013632Z-avarab@gmail.com/
2. https://lore.kernel.org/git/xmqqk0hdqifg.fsf@gitster.g/

Ævar Arnfjörð Bjarmason (7):
  grep.h: remove unused "regex_t regexp" from grep_opt

No change.

  log tests: check if grep_config() is called by "log"-like cmds

Simplified the test, and it no longer depends (optionally) PCRE. We just test BRE v.s. ERE instead.

  grep tests: add missing "grep.patternType" config test

A new test to assert existing behavior, we had a blindspot in this area (could have hidden a possible regression).

  built-ins: trust the "prefix" from run_builtin()

Substantially the same, but I made some edits as requested to minimize the diff size / skip any cleanups-while-at it.

  grep.c: don't pass along NULL callback value

Trivial commit message typo fix.

  grep API: call grep_config() after grep_init()

Removed stray leftover WIP code (an unused "caller_priv" struct member), oops.

  grep: simplify config parsing and option parsing

Mostly new, replaces the now-ejected two last commits of v2 that changed obscure behavior. We still delete most of the code involved in this part of grep initialization and config handling, but now with no changes in existing behavior.

This is now also ejected:

  grep docs: de-duplicate configuration sections
  (https://lore.kernel.org/git/patch-v2-4.8-efe95397d72-20211110T013632Z-avarab@gmail.com/)

Since I don't need to change any documentation anymore, moving around the grep docs to live in one place isn't within the scope of this series.

 builtin/grep.c    |  27 +++++-----
 builtin/log.c     |  13 ++++-
 builtin/ls-tree.c |   2 +-
 git.c             |   1 +
 grep.c            | 124 ++++++++--------------------------------------
 grep.h            |  33 ++++++++----
 revision.c        |   4 +-
 t/t4202-log.sh    |  24 +++++++++
 t/t7810-grep.sh   |   9 ++++
 9 files changed, 106 insertions(+), 131 deletions(-)

Range-diff against v2:
1:  1435db727ef = 1:  71ff51cb3c9 grep.h: remove unused "regex_t regexp" from grep_opt
3:  41e38ebb32c ! 2:  ec8e42ced1a log tests: check if grep_config() is called by "log"-like cmds
    @@ Commit message
         whether "git log" handles "grep.patternType", but also "git show"
         etc.
     
    -    It's sufficient to check whether a PCRE regex matches for the purposes
    -    of this test, we otherwise assume that it's running the same code as
    -    "git log", whose behavior is tested more exhaustively by test added in
    -    9df46763ef1e.
    +    It's sufficient to check whether we match a "fixed" or a "basic" regex
    +    here to see if these codepaths correctly invoked grep_config(). We
    +    don't need to check the details of their regular expression matching
    +    as the "log" test does.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ t/t4202-log.sh: test_expect_success !FAIL_PREREQS 'log with various grep.pattern
      
     +for cmd in show whatchanged reflog format-patch
     +do
    -+	myarg=
    -+	if test "$cmd" = "format-patch"
    -+	then
    -+		myarg="HEAD~.."
    -+	fi
    ++	case "$cmd" in
    ++	format-patch) myarg="HEAD~.." ;;
    ++	*) myarg= ;;
    ++	esac
     +
    -+	test_expect_success PCRE "$cmd: understands grep.patternType=perl, like 'log'" '
    -+		git -c grep.patternType=fixed -C pattern-type $cmd --grep="1(?=\|2)" $myarg >actual &&
    -+		test_must_be_empty actual &&
    -+		git -c grep.patternType=perl -C pattern-type $cmd --grep="1(?=\|2)" $myarg >actual &&
    -+		test_file_not_empty actual
    ++	test_expect_success "$cmd: understands grep.patternType, like 'log'" '
    ++		git init "pattern-type-$cmd" &&
    ++		(
    ++			cd "pattern-type-$cmd" &&
    ++			test_commit 1 file A &&
    ++			test_commit "(1|2)" file B 2 &&
    ++
    ++			git -c grep.patternType=fixed $cmd --grep="..." $myarg >actual &&
    ++			test_must_be_empty actual &&
    ++
    ++			git -c grep.patternType=basic $cmd --grep="..." $myarg >actual &&
    ++			test_file_not_empty actual
    ++		)
     +	'
     +done
    ++test_done
     +
      test_expect_success 'log --author' '
      	cat >expect <<-\EOF &&
8:  cc904d93b26 ! 3:  fcad1b1664b grep: make "extendedRegexp=true" the same as "patternType=extended"
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    grep: make "extendedRegexp=true" the same as "patternType=extended"
    +    grep tests: add missing "grep.patternType" config test
     
    -    In the preceding commit we changed how a "grep.patternType=default"
    -    set after "grep.extendedRegexp=true" would be handled so that the last
    -    set would win, but a "grep.extendedRegexp=true" would only be used if
    -    "grep.patternType" was set to a value other than "default".
    -
    -    Thus a user who had old config and set "grep.extendedRegexp=true" in
    -    their ~/.gitconfig expecting ERE behavior would be opted-in to say
    -    "perl" regexes if a system "/etc/gitconfig" started setting
    -    "grep.patternType=perl".
    -
    -    These funny semantics of only paying attention to a set if another key
    -    is not set to a given value aren't how we treat other config keys, so
    -    let's do away with this caveat for consistency.
    -
    -    The new semantics are simple, a "grep.extendedRegexp=true" is an exact
    -    synonym for specifying "grep.patternType=extended" in the
    -    config. We'll keep ignoring ""grep.extendedRegexp=false", although
    -    arguably we could treat it as a "grep.patternType=basic".
    -
    -    As argued in the preceding commit I think this behavior came about
    -    because we were conflating the state of our code's own internal
    -    "default" value with what we found in explicit user config. See
    -    84befcd0a4a (grep: add a grep.patternType configuration setting,
    -    2012-08-03) for that past behavior.
    -
    -    Let's further change the documentation to note that
    -    "grep.extendedRegexp" is a deprecated synonym, perhaps we'll be able
    -    to remove it at some point in the future and do away with this
    -    special-case entirely.
    +    Extend the grep tests to assert that setting
    +    "grep.patternType=extended" followed by "grep.patternType=default"
    +    will behave as if "--extended-regexp" was provided, and not as
    +    "--basic-regexp". In a subsequent commit we'll need to treat
    +    "grep.patternType=default" as a special-case, but let's make sure we
    +    don't ignore it if "grep.patternType" was set to a non-"default" value
    +    before.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    - ## Documentation/config/grep.txt ##
    -@@ Documentation/config/grep.txt: grep.patternType::
    - 	value 'default' will return to the default matching behavior.
    - 
    - grep.extendedRegexp::
    --	If set to true, enable `--extended-regexp` option by default. This
    --	option is ignored when the `grep.patternType` option is set.
    -+	Deprecated synonym for 'grep.patternType=extended`.
    - 
    - grep.threads::
    - 	Number of grep worker threads to use. If unset (or set to 0), Git will
    -
    - ## grep.c ##
    -@@ grep.c: static const char *color_grep_slots[] = {
    - 
    - 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"))
    -+	if (!strcmp(arg, "basic") ||
    -+	    !strcmp(arg, "default"))
    - 		return GREP_PATTERN_TYPE_BRE;
    - 	else if (!strcmp(arg, "extended"))
    - 		return GREP_PATTERN_TYPE_ERE;
    -@@ grep.c: int grep_config(const char *var, const char *value, void *cb)
    - 	if (userdiff_config(var, value) < 0)
    - 		return -1;
    - 
    --	if (opt->pattern_type_option == GREP_PATTERN_TYPE_UNSPECIFIED &&
    --	    !strcmp(var, "grep.extendedregexp") &&
    -+	if (!strcmp(var, "grep.extendedregexp") &&
    - 	    git_config_bool(var, value)) {
    - 		opt->pattern_type_option = GREP_PATTERN_TYPE_ERE;
    - 		return 0;
    -
    - ## grep.h ##
    -@@ grep.h: enum grep_expr_node {
    - };
    - 
    - enum grep_pattern_type {
    --	GREP_PATTERN_TYPE_UNSPECIFIED = 0,
    --	GREP_PATTERN_TYPE_BRE,
    -+	GREP_PATTERN_TYPE_BRE = 0,
    - 	GREP_PATTERN_TYPE_ERE,
    - 	GREP_PATTERN_TYPE_FIXED,
    - 	GREP_PATTERN_TYPE_PCRE
    -@@ grep.h: struct grep_opt {
    - 	.relative = 1, \
    - 	.pathname = 1, \
    - 	.max_depth = -1, \
    --	.pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED, \
    - 	.colors = { \
    - 		[GREP_COLOR_CONTEXT] = "", \
    - 		[GREP_COLOR_FILENAME] = "", \
    -
      ## t/t7810-grep.sh ##
     @@ t/t7810-grep.sh: do
    + 		test_cmp expected actual
      	'
      
    - 	test_expect_success "grep $L with grep.patternType=basic and grep.extendedRegexp=true" '
    --		echo "${HC}ab:a+bc" >expected &&
    -+		echo "${HC}ab:abc" >expected &&
    ++	test_expect_success "grep $L with grep.patternType=extended and grep.patternType=default" '
    ++		echo "${HC}ab:a+bc" >expected &&
    ++		git \
    ++			-c grep.patternType=extended \
    ++			-c grep.patternType=default \
    ++			grep "a+b*c" $H ab >actual &&
    ++		test_cmp expected actual
    ++	'
    ++
    + 	test_expect_success "grep $L with grep.patternType=extended and grep.extendedRegexp=false" '
    + 		echo "${HC}ab:abc" >expected &&
      		git \
    - 			-c grep.patternType=basic \
    - 			-c grep.extendedRegexp=true \
2:  63cf2fe266d ! 4:  854ffe8d0b9 built-ins: trust the "prefix" from run_builtin()
    @@ builtin/grep.c: int cmd_grep(int argc, const char **argv, const char *prefix)
     
      ## builtin/ls-tree.c ##
     @@ builtin/ls-tree.c: int cmd_ls_tree(int argc, const char **argv, const char *prefix)
    - 		OPT__ABBREV(&abbrev),
    - 		OPT_END()
    - 	};
    --
    --	git_config(git_default_config, NULL);
    + 
    + 	git_config(git_default_config, NULL);
      	ls_tree_prefix = prefix;
     -	if (prefix && *prefix)
     +	if (prefix)
      		chomp_prefix = strlen(prefix);
      
    -+	git_config(git_default_config, NULL);
      	argc = parse_options(argc, argv, prefix, ls_tree_options,
    - 			     ls_tree_usage, 0);
    - 	if (full_tree) {
    --		ls_tree_prefix = prefix = NULL;
    -+		ls_tree_prefix = NULL;
    - 		chomp_prefix = 0;
    - 	}
    - 	/* -d -r should imply -t, but -d by itself should not have to. */
    -@@ builtin/ls-tree.c: int cmd_ls_tree(int argc, const char **argv, const char *prefix)
    - 	parse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC &
    - 				  ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
    - 		       PATHSPEC_PREFER_CWD,
    --		       prefix, argv + 1);
    -+		       ls_tree_prefix, argv + 1);
    - 	for (i = 0; i < pathspec.nr; i++)
    - 		pathspec.items[i].nowildcard_len = pathspec.items[i].len;
    - 	pathspec.has_wildcard = 0;
     
      ## git.c ##
    -@@ git.c: static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
    - {
    - 	int status, help;
    - 	struct stat st;
    --	const char *prefix;
    -+	const char *prefix = NULL;
    - 
    --	prefix = NULL;
    - 	help = argc == 2 && !strcmp(argv[1], "-h");
    - 	if (!help) {
    - 		if (p->option & RUN_SETUP)
     @@ git.c: static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
      			int nongit_ok;
      			prefix = setup_git_directory_gently(&nongit_ok);
4:  efe95397d72 < -:  ----------- grep docs: de-duplicate configuration sections
5:  d0f0ac6c7ae ! 5:  2536eae2c32 grep.c: don't pass along NULL callback value
    @@ Metadata
      ## Commit message ##
         grep.c: don't pass along NULL callback value
     
    -    Change grep_cmd_config() top stop passing around the always-NULL "cb"
    +    Change grep_cmd_config() to stop passing around the always-NULL "cb"
         value. When this code was added in 7e8f59d577e (grep: color patterns
         in output, 2009-03-07) it was non-NULL, but when that changed in
         15fabd1bbd4 (builtin/grep.c: make configuration callback more
6:  917944f79a5 ! 6:  4e1be7c165b grep API: call grep_config() after grep_init()
    @@ grep.c: int grep_config(const char *var, const char *value, void *cb)
     
      ## grep.h ##
     @@ grep.h: struct grep_opt {
    - 	int show_hunk_mark;
    - 	int file_break;
    - 	int heading;
    -+	void *caller_priv;
    - 	void *priv;
    - 
    - 	void (*output)(struct grep_opt *opt, const void *data, size_t size);
      	void *output_priv;
      };
      
7:  140a7416223 ! 7:  f40ab932cb1 grep: simplify config parsing, change grep.<rx config> interaction
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    grep: simplify config parsing, change grep.<rx config> interaction
    +    grep: simplify config parsing and option parsing
     
    -    Change the interaction between "grep.patternType=default" and
    -    "grep.extendedRegexp=true" to make setting "grep.extendedRegexp=true"
    -    synonymous with setting "grep.patternType=extended".
    -
    -    This changes our existing config parsing behavior as detailed below,
    -    but in a way that's consistent with how we parse other
    -    configuration.
    -
    -    We are breaking past promises here, but I doubt that this will impact
    -    anyone in practice. The reduction in complexity and resulting
    -    consistency with other default config behavior is worth it.
    +    Simplify the parsing of "grep.patternType" and
    +    "grep.extendedRegexp". This changes no behavior, but gets rid of
    +    complex parsing logic that isn't needed anymore.
     
         When "grep.patternType" was introduced in 84befcd0a4a (grep: add a
    -    grep.patternType configuration setting, 2012-08-03) we made two
    -    seemingly contradictory promises:
    +    grep.patternType configuration setting, 2012-08-03) we promised that:
     
          1. You can set "grep.patternType", and "[setting it to] 'default'
             will return to the default matching behavior".
     
    -     2. Support the existing "grep.extendedRegexp" option, but ignore it
    -        when the new "grep.patternType" is set, *except* "when the
    -        `grep.patternType` option is set. to a value other than 'default'".
    -
    -    I think that 84befcd0a4a probably didn't intend this behavior, but
    -    instead ended up conflating our internal "unspecified" state with a
    -    user's explicit desire to set the configuration back to the
    -    default.
    -
    -    I.e. a user would correctly expect this to keep working:
    -
    -        # ERE grep
    -        git -c grep.extendedRegexp=true grep <pattern>
    -
    -    And likewise for "grep.patternType=default" to take precedence over
    -    the disfavored "grep.extendedRegexp" option, i.e. the usual "last set
    -    wins" semantics.
    +     2. We'd support the existing "grep.extendedRegexp" option, but ignore
    +        it when the new "grep.patternType" option is set. We said we'd
    +        only ignore the older "grep.extendedRegexp" option "when the
    +        `grep.patternType` option is set. to a value other than
    +        'default'".
     
    -        # BRE grep
    -        git -c grep.extendedRegexp=true -c grep.patternType=basic grep <pattern>
    +    In a preceding commit we changed grep_config() to be called after
    +    grep_init(), which means that much of the complexity here can go
    +    away.
     
    -    But probably not for this to ignore the favored "grep.patternType"
    -    option entirely, say if /etc/gitconfig was still setting
    -    "grep.extendedRegexp", but "~/.gitconfig" used the new
    -    "grep.patternType" (and wanted to use the "default" value):
    -
    -        # Was ERE, now BRE
    -        git -c grep.extendedRegexp=true grep.patternType=default grep <pattern>
    -
    -    I think that in practice nobody or almost nobody is going to be
    -    relying on this obscure interaction, and as shown here it makes the
    -    config parsing much simpler. We no longer have to carry a complex
    -    state machine in "grep_commit_pattern_type()" and
    -    "grep_set_pattern_type_option()".
    -
    -    We can also do away with the "int fixed" and "int pcre2" members in
    -    favor of using "pattern_type_option" directly in "grep.c", as well as
    -    dropping the "pattern_type_arg" variable in "builtin/grep.c" in favor
    -    of using the "pattern_type_option" member directly.
    +    Now as before when we only understand a "grep.extendedRegexp" setting
    +    of "true", and if "grep.patterntype=default" is set we'll interpret it
    +    as "grep.patterntype=basic", except if we previously saw a
    +    "grep.extendedRegexp", then it's interpreted as
    +    "grep.patterntype=extended".
     
         See my 07a3d411739 (grep: remove regflags from the public grep_opt
         API, 2017-06-29) for addition of the two comments being removed here,
         i.e. the complexity noted in that commit is now going away.
     
    -    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    +    We don't need grep_commit_pattern_type() anymore, we can instead have
    +    OPT_SET_INT() in "builtin/grep.c" manipulate the "pattern_type_option"
    +    member in "struct grep_opt" directly.
     
    - ## Documentation/config/grep.txt ##
    -@@ Documentation/config/grep.txt: grep.patternType::
    - 
    - grep.extendedRegexp::
    - 	If set to true, enable `--extended-regexp` option by default. This
    --	option is ignored when the `grep.patternType` option is set to a value
    --	other than 'default'.
    -+	option is ignored when the `grep.patternType` option is set.
    - 
    - grep.threads::
    - 	Number of grep worker threads to use. If unset (or set to 0), Git will
    +    We can also do away with the indirection of the "int fixed" and "int
    +    pcre2" members in favor of using "pattern_type_option" directly in
    +    "grep.c".
    +
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/grep.c ##
     @@ builtin/grep.c: int cmd_grep(int argc, const char **argv, const char *prefix)
    @@ builtin/grep.c: int cmd_grep(int argc, const char **argv, const char *prefix)
      		int fallback = 0;
     
      ## grep.c ##
    +@@ grep.c: static const char *color_grep_slots[] = {
    + 
    + 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"))
    ++	if (!strcmp(arg, "basic"))
    + 		return GREP_PATTERN_TYPE_BRE;
    + 	else if (!strcmp(arg, "extended"))
    + 		return GREP_PATTERN_TYPE_ERE;
     @@ grep.c: int grep_config(const char *var, const char *value, void *cb)
    - 	if (userdiff_config(var, value) < 0)
      		return -1;
      
    --	if (!strcmp(var, "grep.extendedregexp")) {
    --		opt->extended_regexp_option = git_config_bool(var, value);
    -+	if (opt->pattern_type_option == GREP_PATTERN_TYPE_UNSPECIFIED &&
    -+	    !strcmp(var, "grep.extendedregexp") &&
    -+	    git_config_bool(var, value)) {
    -+		opt->pattern_type_option = GREP_PATTERN_TYPE_ERE;
    + 	if (!strcmp(var, "grep.extendedregexp")) {
    ++		if (opt->extended_regexp_option)
    ++			return 0;
    + 		opt->extended_regexp_option = git_config_bool(var, value);
    ++		if (opt->extended_regexp_option)
    ++			opt->pattern_type_option = GREP_PATTERN_TYPE_ERE;
    ++		return 0;
    ++	}
    ++
    ++	if (!strcmp(var, "grep.patterntype") &&
    ++	    !strcmp(value, "default")) {
    ++		opt->pattern_type_option = opt->extended_regexp_option == 1
    ++			? GREP_PATTERN_TYPE_ERE : GREP_PATTERN_TYPE_BRE;
      		return 0;
      	}
      
    + 	if (!strcmp(var, "grep.patterntype")) {
    ++		opt->extended_regexp_option = -1; /* ignore */
    + 		opt->pattern_type_option = parse_pattern_type_arg(var, value);
    + 		return 0;
    + 	}
     @@ grep.c: void grep_init(struct grep_opt *opt, struct repository *repo)
      	opt->header_tail = &opt->header_list;
      }
    @@ grep.c: static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
      	if (err) {
     
      ## grep.h ##
    +@@ grep.h: enum grep_expr_node {
    + };
    + 
    + enum grep_pattern_type {
    +-	GREP_PATTERN_TYPE_UNSPECIFIED = 0,
    +-	GREP_PATTERN_TYPE_BRE,
    ++	GREP_PATTERN_TYPE_BRE = 0,
    + 	GREP_PATTERN_TYPE_ERE,
    + 	GREP_PATTERN_TYPE_FIXED,
    + 	GREP_PATTERN_TYPE_PCRE
     @@ grep.h: struct grep_opt {
      	int unmatch_name_only;
      	int count;
    @@ grep.h: struct grep_opt {
      	int pathname;
      	int null_following_name;
     @@ grep.h: struct grep_opt {
    - 	int max_depth;
      	int funcname;
      	int funcbody;
    --	int extended_regexp_option;
    + 	int extended_regexp_option;
     -	int pattern_type_option;
     +	enum grep_pattern_type pattern_type_option;
      	int ignore_locale;
      	char colors[NR_GREP_COLORS][COLOR_MAXLEN];
      	unsigned pre_context;
     @@ grep.h: struct grep_opt {
    + 	.relative = 1, \
    + 	.pathname = 1, \
    + 	.max_depth = -1, \
    +-	.pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED, \
    + 	.colors = { \
    + 		[GREP_COLOR_CONTEXT] = "", \
    + 		[GREP_COLOR_FILENAME] = "", \
    +@@ grep.h: struct grep_opt {
      
      int grep_config(const char *var, const char *value, void *);
      void grep_init(struct grep_opt *, struct repository *repo);
    @@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *re
      	if (!is_encoding_utf8(get_log_output_encoding()))
      		revs->grep_filter.ignore_locale = 1;
      	compile_grep_patterns(&revs->grep_filter);
    -
    - ## t/t7810-grep.sh ##
    -@@ t/t7810-grep.sh: do
    - 	'
    - 
    - 	test_expect_success "grep $L with grep.extendedRegexp=true and grep.patternType=default" '
    --		echo "${HC}ab:abc" >expected &&
    -+		echo "${HC}ab:a+bc" >expected &&
    - 		git \
    - 			-c grep.extendedRegexp=true \
    - 			-c grep.patternType=default \

This "regex_t" in grep_opt has not been used since f9b9faf6f8a (builtin-grep: allow more than one patterns., 2006-05-02), we still use a "regex_t" for compiling regexes, but that's in the "grep_pat" struct".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.h | 1 -
 1 file changed, 1 deletion(-)

Extend the tests added in my 9df46763ef1 (log: add exhaustive tests for pattern style options & config, 2017-05-20) to check not only whether "git log" handles "grep.patternType", but also "git show" etc.

It's sufficient to check whether we match a "fixed" or a "basic" regex here to see if these codepaths correctly invoked grep_config(). We don't need to check the details of their regular expression matching as the "log" test does.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4202-log.sh | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

On 29.11.2021 15:50, Ævar Arnfjörð Bjarmason wrote:

After rebasing my work from <20220302090250.590450-1-fs@gigacodes.de> on master I was a bit confused as to why my tests in t4202 were no longer executing and none of my changes did anything about it. I suppose this `test_done` is left over from testing and slipped into master?

Fix a regression in ff37a60c369 (log tests: check if grep_config() is called by "log"-like cmds, 2022-02-16), a "test_done" command used during development made it into a submitted patch causing tests 41-136 in t/t4202-log.sh to be skipped.

Reported-by: Fabian Stelzer <fs@gigacodes.de>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Fri, Mar 04 2022, Fabian Stelzer wrote:

Ouch! Yes, that's a rather obvious stupid mistake of mine that shouldn't have escaped the lab. Sorry!

FWIW since we haven't run these tests in a while (not too long though!) it's conceivable that CI would fail on them, but in addition to passing locally here's passing CI for this change: https://github.com/avar/git/runs/5420131756

So we should be OK with this change.

 t/t4202-log.sh | 1 -
 1 file changed, 1 deletion(-)

Change code in "builtin/grep.c" and "builtin/ls-tree.c" to trust the "prefix" passed from "run_builtin()". The "prefix" we get from setup.c is either going to be NULL or a string of length >0, never "".

So we can drop the "prefix && *prefix" checks added for "builtin/grep.c" in 0d042fecf2f (git-grep: show pathnames relative to the current directory, 2006-08-11), and for "builtin/ls-tree.c" in a69dd585fca (ls-tree: chomp leading directories when run from a subdirectory, 2005-12-23).

As seen in code in revision.c that was added in cd676a51367 (diff --relative: output paths as relative to the current subdirectory, 2008-02-12) we already have existing code that does away with this assertion.

This makes it easier to reason about a subsequent change to the "prefix_length" code in grep.c in a subsequent commit, and since we're going to the trouble of doing that let's leave behind an assert() to promise this to any future callers.

For "builtin/grep.c" it would be painful to pass the "prefix" down the callchain of:

    cmd_grep -> grep_tree -> grep_submodule -> grep_cache -> grep_oid ->
    grep_source_name

So for the code that needs it in grep_source_name() let's add a "grep_prefix" variable similar to the existing "ls_tree_prefix".

While at it let's move the code in cmd_ls_tree() around so that we assign to the "ls_tree_prefix" right after declaring the variables, and stop assigning to "prefix". We only subsequently used that variable later in the function after clobbering it. Let's just use our own "grep_prefix" instead.

Let's also add an assert() in git.c, so that we'll make this promise about the "prefix" to any current and future callers, as well as to any readers of the code.

Code history:

 * The strlen() in "grep.c" hasn't been used since 493b7a08d80 (grep:
   accept relative paths outside current working directory, 2009-09-05).

   When that code was added in 0d042fecf2f (git-grep: show pathnames
   relative to the current directory, 2006-08-11) we used the length.

   But since 493b7a08d80 we haven't used it for anything except a
   boolean check that we could have done on the "prefix" member
   itself.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c    | 13 ++++++++-----
 builtin/ls-tree.c |  2 +-
 git.c             |  1 +
 grep.c            |  4 +---
 grep.h            |  4 +---
 revision.c        |  2 +-
 6 files changed, 13 insertions(+), 13 deletions(-)

Extend the grep tests to assert that setting "grep.patternType=extended" followed by "grep.patternType=default" will behave as if "--extended-regexp" was provided, and not as "--basic-regexp". In a subsequent commit we'll need to treat "grep.patternType=default" as a special-case, but let's make sure we don't ignore it if "grep.patternType" was set to a non-"default" value before.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t7810-grep.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

grep.patternType is the usual "last-one wins". If the last value set to patternType is the default, the setting to grep.extendedRegexp should take effect (so if it is set to true, we'd see ERE behavour).

Back in the days when the "return to the default matching behavior" part was written in 84befcd0 (grep: add a grep.patternType configuration setting, 2012-08-03), grep.extendedRegexp was the only way to configure the behaviour since b22520a3 (grep: allow -E and -n to be turned on by default via configuration, 2011-03-30). It was understandable that we referred to the behaviour that honors the older configuration variable as "the default matching" behaviour. It is fairly clear in its log message:

    When grep.patternType is set to a value other than "default", the
    grep.extendedRegexp setting is ignored. The value of "default" restores
    the current default behavior, including the grep.extendedRegexp
    behavior.

So, unless your description is a typo, I am somewhat surprised by your findings that =default that comes later does not defeat an earlier =extended.

It should just clear that earlier extended set by grep.patternType and only pay attention to grep.extendedRegexp variable. Doing anything else is a bug, I think.

Thanks.

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

So, let's see how 

  $ git -c grep.patternType=extended \
	-c grep.patternType=default \
	grep foo

works today.

We start from builtin/grep.c::cmd_grep(), which calls git_config(grep_cmd_config). grep_cmd_config() farms out most of the work to grep.c::grep_config(), which populates the grep_defaults structure. grep_defaults.pattern_type_option first becomes GREP_PATTERN_TYPE_ERE and then it gets overwritten to GREP_PATTERN_TYPE_UNSPECIFIED.

Then grep.c::grep_init() copies that grep_defaults to the per-invocation "struct grep_opt opt" that is on-stack in builtin/grep.c::cmd_grep().

opt.patternType becomes GREP_PATTERN_TYPE_UNSPECIFIED; opt.extendedRegexp in the same structure is 0, because nobody has touched the corresponding member in grep_defaults in grep_cmd_config().

Then parse_options() gets its turn to futz with members in "opt". -E/-G/-F/-P would be parsed into a separate variable "pattern_type"; in this case, there is no command line option, so the pattern_type variable has GREP_PATTERN_TYPE_UNSPECIFIED.

And finally grep.c::grep_commit_pattern_type() is called to combine what is in "pattern_type" and "opt".

It calls grep_set_pattern_type_option() to futz with members in opt that is what determines the final choice.

 - If pattern_type is not UNSPECIFIED, use that;

 - Otherwise, if opt->pattern_type_option is not UNSPECIFIED, use that;

 - Otherwise, i.e. if pattern_type from the command line and
   opt->pattern_type_option from the configuration are both
   UNSPECIFIED, then check if opt->extended_regexp_option (which is
   set from the config via grep.extendedRegexp) is set.  If so, call
   grep_set_pattern_type_option() to use ERE.

Now, what does grep_set_pattern_type_option() do?

The first thing it does is when pattern_type given is not ERE, drop the opt->extended_regexp_option bit (which may have been set by having grep.extendedRegexp configuration set to true). This is because, just like 'fixed' and 'pcre2', the runtime after the opt structure is set up, the code does not look at a single "type" member that enumerates BRE, ERE, FIXED, PCRE to determine the type of the pattern, and the 'extended_regexp_option' member, after grep_commit_pattern_type() finishes its processing, is used to signal that ERE is in effect. But as we've seen in the design goal of the earlier change 84befcd0 (grep: add a grep.patternType configuration setting, 2012-08-03), the bit obtained from the grep.extendedRegexp configuration variable is only valid when grep.patternType is set to UNSPECIFIED (aka default), so there needs some dropping of this bit happen.

But with two grep.patternType configuration, I do not think the bug will trigger. As we traced above, we just get UNSPECIFIED in grep_defaults.pattern_type_option, that is copied to cmd_grep()::opt, and it gets combined with UNSPECIFIED in cmd_grep()::pattern_type in grep_commit_pattern_type(). But the three-step logic in the "commit" will not do anything in this case. So, I do not see any code that makes this behave as if "git grep -E foo" was given.

I suspect that if you do

  $ git -c grep.extendedRegexp=true \
	-c grep.patternType=default \
	grep foo

it should set the .extended_regexp_option member to true and the .pattern_type_option member to UNSPECIFIED in grep_defaults, copy it to cmd_grep()::opt, and grep_commit_pattern_type() will try to combine that "opt" with pattern_type==UNSPECIFIED. The third "both pattern_type and opt.pattern_type_option are UNSPECIFIED" case triggers, and grep_set_pattern_type_option() would be called, with its pattern_type parameter explicitly set to ERE.

The logic to combine these two are convoluted and I sense that it could be simplified without breaking the established semantics, but so far I am not seeing how the code can break in such a way that

this claim holds.

So,... after spending too much time following the code, I went back to the actual test added by the code and see this:

+ test_expect_success "grep $L with grep.patternType=extended and grep.patternType=default" ' + echo "${HC}ab:a+bc" >expected && + git \ + -c grep.patternType=extended \ + -c grep.patternType=default \ + grep "a+b*c" $H ab >actual && + test_cmp expected actual + '

Here, file "ac" has three lines

        a+b*c
        a+bc
        abc

and "a+b*c" pattern is designed to hit the first line with -F, the second one with -G (because + is literal in BRE so it must exist literally in the haystack, b* matches single b but not literal b* in the haystack), the last one with -E (because neither + or * is literal, so the first two lines do not match, but the last one matches). The expectation in the code, unlike what is in the log message, is that this should match as if -G was given.

So, I guess there is no bug (other than the alarming false report in the log message).

The grep_init() function used the odd pattern of initializing the passed-in "struct grep_opt" with a statically defined "grep_defaults" struct, which would be modified in-place when we invoked grep_config().

So we effectively (b) initialized config, (a) then defaults, (c) followed by user options. Usually those are ordered as "a", "b" and "c" instead.

As the comments being removed here show the previous behavior needed to be carefully explained as we'd potentially share the populated configuration among different instances of grep_init(). In practice we didn't do that, but now that it can't be a concern anymore let's remove those comments.

This does not change the behavior of any of the configuration variables or options. That would have been the case if we didn't move around the grep_config() call in "builtin/log.c". But now that we call "grep_config" after "git_log_config" and "git_format_config" we'll need to pass in the already initialized "struct grep_opt *".

See 6ba9bb76e02 (grep: copy struct in one fell swoop, 2020-11-29) and 7687a0541e0 (grep: move the configuration parsing logic to grep.[ch], 2012-10-09) for the commits that added the comments.

The memcpy() pattern here will be optimized away and follows the convention of other *_init() functions. See 5726a6b4012 (*.c *_init(): define in terms of corresponding *_INIT macro, 2021-07-01).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c |  4 ++--
 builtin/log.c  | 13 +++++++++++--
 grep.c         | 39 +++------------------------------------
 grep.h         | 21 +++++++++++++++++++++
 4 files changed, 37 insertions(+), 40 deletions(-)

Change grep_cmd_config() to stop passing around the always-NULL "cb" value. When this code was added in 7e8f59d577e (grep: color patterns in output, 2009-03-07) it was non-NULL, but when that changed in 15fabd1bbd4 (builtin/grep.c: make configuration callback more reusable, 2012-10-09) this code was left behind.

In a subsequent change I'll start using the "cb" value, this will make it clear which functions we call need it, and which don't.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Simplify the parsing of "grep.patternType" and "grep.extendedRegexp". This changes no behavior, but gets rid of complex parsing logic that isn't needed anymore.

When "grep.patternType" was introduced in 84befcd0a4a (grep: add a grep.patternType configuration setting, 2012-08-03) we promised that:

 1. You can set "grep.patternType", and "[setting it to] 'default'
    will return to the default matching behavior".

 2. We'd support the existing "grep.extendedRegexp" option, but ignore
    it when the new "grep.patternType" option is set. We said we'd
    only ignore the older "grep.extendedRegexp" option "when the
    `grep.patternType` option is set. to a value other than
    'default'".

In a preceding commit we changed grep_config() to be called after grep_init(), which means that much of the complexity here can go away.

Now as before when we only understand a "grep.extendedRegexp" setting of "true", and if "grep.patterntype=default" is set we'll interpret it as "grep.patterntype=basic", except if we previously saw a "grep.extendedRegexp", then it's interpreted as "grep.patterntype=extended".

See my 07a3d411739 (grep: remove regflags from the public grep_opt API, 2017-06-29) for addition of the two comments being removed here, i.e. the complexity noted in that commit is now going away.

We don't need grep_commit_pattern_type() anymore, we can instead have OPT_SET_INT() in "builtin/grep.c" manipulate the "pattern_type_option" member in "struct grep_opt" directly.

We can also do away with the indirection of the "int fixed" and "int pcre2" members in favor of using "pattern_type_option" directly in "grep.c".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c | 10 +++----
 grep.c         | 81 +++++++++++---------------------------------------
 grep.h         |  9 ++----
 revision.c     |  2 --
 4 files changed, 24 insertions(+), 78 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Deprecating grep.extendedRegexp and forcing users to only use the grep.patterntype would be a lot more sensible way forward than giving it a new meaning and letting these two variables interact with each other.

Depending on how cleanly the internal code can become, with the former variable still supported for backward compatibility, we might not need to break working set-up existing end-users have, though.

We'll see.

Thanks.

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Hmph.  I see tests between fixed vs basic, though.

A simplification and code deletion of the overly complex setup for the grep API, no behavior changes. For v3 see: https://lore.kernel.org/git/cover-v3-0.7-00000000000-20211129T143956Z-avarab@gmail.com/

As Junio notes in https://lore.kernel.org/git/xmqqbl22634q.fsf@gitster.g/ my previous 3/7 had entirely the wrong commit message for the change it was making, i.e. the code itself was correct, but I somehow got things reversed in my head when writing the explanation.

That's fixed here, and for good measure I added another related test to the same commit for the same existing behavior.

Ævar Arnfjörð Bjarmason (7):
  grep.h: remove unused "regex_t regexp" from grep_opt
  log tests: check if grep_config() is called by "log"-like cmds
  grep tests: add missing "grep.patternType" config test
  built-ins: trust the "prefix" from run_builtin()
  grep.c: don't pass along NULL callback value
  grep API: call grep_config() after grep_init()
  grep: simplify config parsing and option parsing

 builtin/grep.c    |  27 +++++-----
 builtin/log.c     |  13 ++++-
 builtin/ls-tree.c |   2 +-
 git.c             |   1 +
 grep.c            | 124 ++++++++--------------------------------------
 grep.h            |  33 ++++++++----
 revision.c        |   4 +-
 t/t4202-log.sh    |  24 +++++++++
 t/t7810-grep.sh   |  19 +++++++
 9 files changed, 116 insertions(+), 131 deletions(-)

Range-diff against v3:
1:  71ff51cb3c9 = 1:  d7d232b2b52 grep.h: remove unused "regex_t regexp" from grep_opt
2:  ec8e42ced1a = 2:  f853d669682 log tests: check if grep_config() is called by "log"-like cmds
3:  fcad1b1664b ! 3:  a97b7de3a3c grep tests: add missing "grep.patternType" config test
    @@ Commit message
     
         Extend the grep tests to assert that setting
         "grep.patternType=extended" followed by "grep.patternType=default"
    -    will behave as if "--extended-regexp" was provided, and not as
    -    "--basic-regexp". In a subsequent commit we'll need to treat
    +    will behave as if "--basic-regexp" was provided, and not as
    +    "--extended-regexp". In a subsequent commit we'll need to treat
         "grep.patternType=default" as a special-case, but let's make sure we
    -    don't ignore it if "grep.patternType" was set to a non-"default" value
    -    before.
    +    ignore it if it's being set to "default" following an earlier
    +    non-"default" "grep.patternType" setting.
    +
    +    Let's also test what happens when we have a sequence of "extended"
    +    followed by "default" and "fixed". In that case the "fixed" should
    +    prevail.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ t/t7810-grep.sh: do
     +			grep "a+b*c" $H ab >actual &&
     +		test_cmp expected actual
     +	'
    ++
    ++	test_expect_success "grep $L with grep.patternType=[extended -> default -> fixed]" '
    ++		echo "${HC}ab:a+b*c" >expected &&
    ++		git \
    ++			-c grep.patternType=extended \
    ++			-c grep.patternType=default \
    ++			-c grep.patternType=fixed \
    ++			grep "a+b*c" $H ab >actual &&
    ++		test_cmp expected actual
    ++	'
     +
      	test_expect_success "grep $L with grep.patternType=extended and grep.extendedRegexp=false" '
      		echo "${HC}ab:abc" >expected &&
4:  854ffe8d0b9 = 4:  f7d995a5a80 built-ins: trust the "prefix" from run_builtin()
5:  2536eae2c32 = 5:  ab1685f0dad grep.c: don't pass along NULL callback value
6:  4e1be7c165b = 6:  8ffa22df8c7 grep API: call grep_config() after grep_init()
7:  f40ab932cb1 = 7:  efbd1c50b43 grep: simplify config parsing and option parsing

This "regex_t" in grep_opt has not been used since f9b9faf6f8a (builtin-grep: allow more than one patterns., 2006-05-02), we still use a "regex_t" for compiling regexes, but that's in the "grep_pat" struct".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.h | 1 -
 1 file changed, 1 deletion(-)

Extend the tests added in my 9df46763ef1 (log: add exhaustive tests for pattern style options & config, 2017-05-20) to check not only whether "git log" handles "grep.patternType", but also "git show" etc.

It's sufficient to check whether we match a "fixed" or a "basic" regex here to see if these codepaths correctly invoked grep_config(). We don't need to check the details of their regular expression matching as the "log" test does.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4202-log.sh | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Extend the grep tests to assert that setting "grep.patternType=extended" followed by "grep.patternType=default" will behave as if "--basic-regexp" was provided, and not as "--extended-regexp". In a subsequent commit we'll need to treat "grep.patternType=default" as a special-case, but let's make sure we ignore it if it's being set to "default" following an earlier non-"default" "grep.patternType" setting.

Let's also test what happens when we have a sequence of "extended" followed by "default" and "fixed". In that case the "fixed" should prevail.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t7810-grep.sh | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Change code in "builtin/grep.c" and "builtin/ls-tree.c" to trust the "prefix" passed from "run_builtin()". The "prefix" we get from setup.c is either going to be NULL or a string of length >0, never "".

So we can drop the "prefix && *prefix" checks added for "builtin/grep.c" in 0d042fecf2f (git-grep: show pathnames relative to the current directory, 2006-08-11), and for "builtin/ls-tree.c" in a69dd585fca (ls-tree: chomp leading directories when run from a subdirectory, 2005-12-23).

As seen in code in revision.c that was added in cd676a51367 (diff --relative: output paths as relative to the current subdirectory, 2008-02-12) we already have existing code that does away with this assertion.

This makes it easier to reason about a subsequent change to the "prefix_length" code in grep.c in a subsequent commit, and since we're going to the trouble of doing that let's leave behind an assert() to promise this to any future callers.

For "builtin/grep.c" it would be painful to pass the "prefix" down the callchain of:

    cmd_grep -> grep_tree -> grep_submodule -> grep_cache -> grep_oid ->
    grep_source_name

So for the code that needs it in grep_source_name() let's add a "grep_prefix" variable similar to the existing "ls_tree_prefix".

While at it let's move the code in cmd_ls_tree() around so that we assign to the "ls_tree_prefix" right after declaring the variables, and stop assigning to "prefix". We only subsequently used that variable later in the function after clobbering it. Let's just use our own "grep_prefix" instead.

Let's also add an assert() in git.c, so that we'll make this promise about the "prefix" to any current and future callers, as well as to any readers of the code.

Code history:

 * The strlen() in "grep.c" hasn't been used since 493b7a08d80 (grep:
   accept relative paths outside current working directory, 2009-09-05).

   When that code was added in 0d042fecf2f (git-grep: show pathnames
   relative to the current directory, 2006-08-11) we used the length.

   But since 493b7a08d80 we haven't used it for anything except a
   boolean check that we could have done on the "prefix" member
   itself.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c    | 13 ++++++++-----
 builtin/ls-tree.c |  2 +-
 git.c             |  1 +
 grep.c            |  4 +---
 grep.h            |  4 +---
 revision.c        |  2 +-
 6 files changed, 13 insertions(+), 13 deletions(-)

Change grep_cmd_config() to stop passing around the always-NULL "cb" value. When this code was added in 7e8f59d577e (grep: color patterns in output, 2009-03-07) it was non-NULL, but when that changed in 15fabd1bbd4 (builtin/grep.c: make configuration callback more reusable, 2012-10-09) this code was left behind.

In a subsequent change I'll start using the "cb" value, this will make it clear which functions we call need it, and which don't.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

The grep_init() function used the odd pattern of initializing the passed-in "struct grep_opt" with a statically defined "grep_defaults" struct, which would be modified in-place when we invoked grep_config().

So we effectively (b) initialized config, (a) then defaults, (c) followed by user options. Usually those are ordered as "a", "b" and "c" instead.

As the comments being removed here show the previous behavior needed to be carefully explained as we'd potentially share the populated configuration among different instances of grep_init(). In practice we didn't do that, but now that it can't be a concern anymore let's remove those comments.

This does not change the behavior of any of the configuration variables or options. That would have been the case if we didn't move around the grep_config() call in "builtin/log.c". But now that we call "grep_config" after "git_log_config" and "git_format_config" we'll need to pass in the already initialized "struct grep_opt *".

See 6ba9bb76e02 (grep: copy struct in one fell swoop, 2020-11-29) and 7687a0541e0 (grep: move the configuration parsing logic to grep.[ch], 2012-10-09) for the commits that added the comments.

The memcpy() pattern here will be optimized away and follows the convention of other *_init() functions. See 5726a6b4012 (*.c *_init(): define in terms of corresponding *_INIT macro, 2021-07-01).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c |  4 ++--
 builtin/log.c  | 13 +++++++++++--
 grep.c         | 39 +++------------------------------------
 grep.h         | 21 +++++++++++++++++++++
 4 files changed, 37 insertions(+), 40 deletions(-)

Simplify the parsing of "grep.patternType" and "grep.extendedRegexp". This changes no behavior, but gets rid of complex parsing logic that isn't needed anymore.

When "grep.patternType" was introduced in 84befcd0a4a (grep: add a grep.patternType configuration setting, 2012-08-03) we promised that:

 1. You can set "grep.patternType", and "[setting it to] 'default'
    will return to the default matching behavior".

 2. We'd support the existing "grep.extendedRegexp" option, but ignore
    it when the new "grep.patternType" option is set. We said we'd
    only ignore the older "grep.extendedRegexp" option "when the
    `grep.patternType` option is set. to a value other than
    'default'".

In a preceding commit we changed grep_config() to be called after grep_init(), which means that much of the complexity here can go away.

Now as before when we only understand a "grep.extendedRegexp" setting of "true", and if "grep.patterntype=default" is set we'll interpret it as "grep.patterntype=basic", except if we previously saw a "grep.extendedRegexp", then it's interpreted as "grep.patterntype=extended".

See my 07a3d411739 (grep: remove regflags from the public grep_opt API, 2017-06-29) for addition of the two comments being removed here, i.e. the complexity noted in that commit is now going away.

We don't need grep_commit_pattern_type() anymore, we can instead have OPT_SET_INT() in "builtin/grep.c" manipulate the "pattern_type_option" member in "struct grep_opt" directly.

We can also do away with the indirection of the "int fixed" and "int pcre2" members in favor of using "pattern_type_option" directly in "grep.c".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c | 10 +++----
 grep.c         | 81 +++++++++++---------------------------------------
 grep.h         |  9 ++----
 revision.c     |  2 --
 4 files changed, 24 insertions(+), 78 deletions(-)

A simplification and code deletion of the overly complex setup for the grep API, no behavior changes. For v4 see: https://lore.kernel.org/git/cover-v4-0.7-00000000000-20211203T101348Z-avarab@gmail.com/

This re-roll is rebased on the latest push-out to "master", now-landed topic had a minor conflict with it in git.c.

Ævar Arnfjörð Bjarmason (7):
  grep.h: remove unused "regex_t regexp" from grep_opt
  log tests: check if grep_config() is called by "log"-like cmds
  grep tests: add missing "grep.patternType" config test
  built-ins: trust the "prefix" from run_builtin()
  grep.c: don't pass along NULL callback value
  grep API: call grep_config() after grep_init()
  grep: simplify config parsing and option parsing

 builtin/grep.c    |  27 +++++-----
 builtin/log.c     |  13 ++++-
 builtin/ls-tree.c |   2 +-
 git.c             |   1 +
 grep.c            | 124 ++++++++--------------------------------------
 grep.h            |  33 ++++++++----
 revision.c        |   4 +-
 t/t4202-log.sh    |  24 +++++++++
 t/t7810-grep.sh   |  19 +++++++
 9 files changed, 116 insertions(+), 131 deletions(-)

Range-diff against v4:
1:  d7d232b2b52 = 1:  b6a3e0e2e08 grep.h: remove unused "regex_t regexp" from grep_opt
2:  f853d669682 = 2:  c0d77b2683f log tests: check if grep_config() is called by "log"-like cmds
3:  a97b7de3a3c = 3:  f02f246aa23 grep tests: add missing "grep.patternType" config test
4:  f7d995a5a80 ! 4:  a542a352eab built-ins: trust the "prefix" from run_builtin()
    @@ builtin/ls-tree.c: int cmd_ls_tree(int argc, const char **argv, const char *pref
     
      ## git.c ##
     @@ git.c: static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
    - 			int nongit_ok;
    - 			prefix = setup_git_directory_gently(&nongit_ok);
    - 		}
    -+		assert(!prefix || *prefix);
    - 		precompose_argv_prefix(argc, argv, NULL);
    - 		if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&
    - 		    !(p->option & DELAY_PAGER_CONFIG))
    + 	} else {
    + 		prefix = NULL;
    + 	}
    ++	assert(!prefix || *prefix);
    + 	precompose_argv_prefix(argc, argv, NULL);
    + 	if (use_pager == -1 && run_setup &&
    + 		!(p->option & DELAY_PAGER_CONFIG))
     
      ## grep.c ##
     @@ grep.c: int grep_config(const char *var, const char *value, void *cb)
5:  ab1685f0dad = 5:  a33b00a247e grep.c: don't pass along NULL callback value
6:  8ffa22df8c7 = 6:  92b1c3958fa grep API: call grep_config() after grep_init()
7:  efbd1c50b43 = 7:  63de643ebc2 grep: simplify config parsing and option parsing

This "regex_t" in grep_opt has not been used since f9b9faf6f8a (builtin-grep: allow more than one patterns., 2006-05-02), we still use a "regex_t" for compiling regexes, but that's in the "grep_pat" struct".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.h | 1 -
 1 file changed, 1 deletion(-)

Extend the tests added in my 9df46763ef1 (log: add exhaustive tests for pattern style options & config, 2017-05-20) to check not only whether "git log" handles "grep.patternType", but also "git show" etc.

It's sufficient to check whether we match a "fixed" or a "basic" regex here to see if these codepaths correctly invoked grep_config(). We don't need to check the details of their regular expression matching as the "log" test does.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4202-log.sh | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Extend the grep tests to assert that setting "grep.patternType=extended" followed by "grep.patternType=default" will behave as if "--basic-regexp" was provided, and not as "--extended-regexp". In a subsequent commit we'll need to treat "grep.patternType=default" as a special-case, but let's make sure we ignore it if it's being set to "default" following an earlier non-"default" "grep.patternType" setting.

Let's also test what happens when we have a sequence of "extended" followed by "default" and "fixed". In that case the "fixed" should prevail.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t7810-grep.sh | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

The grep.patternType configuration variable has the "last one wins" semantics just all the usual configuration variable, but the meaning of the variable when it is set to "default" depends on the value set to grep.extendedRegexp variable.

If you rewrite with the above understanding, what you wrote will become a lot more concise.

    Extend the grep tests to assert that grep.patternType is the
    usual "last one wins" variable, and specifically, setting it to
    "default" has the same meaning as setting it to "basic" when
    grep.extendedRegexp is not set (or set to false).

And from that point of view, I think the second new test has much less value than a possible alternative to ensure grep.patternType set to fixed and then default behaves like setting it to extended when grep.extendedRegexp is set to true. As written, this is just testing that the variable we designed and know to be "last one wins" behaves as one once more.

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

Also, it is probably a good idea to strees that grep.extendedRegexp is also "last one wins", so as I wrote in a separate message, Git finds the last one for grep.extendedRegexp and grep.patternType independently and combines these last values to come up with the pattern type it uses.

I'll tentatively queue the following patch between your 3/7 and 4/7, but it probably is a good idea to squash it into 3/7, as it belongs to the same theme: clarify how these two variables are meant to interact with each other.

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----
Subject: [PATCH] t7810: make sure grep.extendedRegexp is also last-one-wins

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t7810-grep.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

We made a mistake to add grep.extendedRegexp configuration variable long time ago, and made things even worse by introducing an even more generalized grep.patternType configuration variable.

This was mostly because interactive users were lazy and wanted a way to declare "I do not live in the ancient age, and my regexps are always extended" and write "git grep" without having to type three more letters " -E" on the command line.

But this in turn forces script writers to always specify what kind of patterns they are writing, because without such command line override, the interpretation of the patterns they write in their scripts will be affected by the configuration variables of the user who is running their script.

Introduce GIT_DISABLE_GREP_PATTERN_CONFIG environment variable that script writers can set to "true" and export at the very beginning of their script to defeat grep.extendedRegexp and grep.patternType configuration variables.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This is merely a weather balloon without proper documentation and
   test.  It might be even better idea to make such an environment
   variable to _specify_ what kind of pattern the script uses,
   instead of "we defeat end-user configuration and now we are
   forced to write in basic or have to write -E/-P etc.", which is
   what this patch does.

 grep.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

On Fri, Dec 24 2021, Junio C Hamano wrote:

You note the lack of documentation. I do think anything in this direction would do well to:

 * Specify what it is we're promising now exactly. The git-grep
   command is in "main porcelain" now, this change sounds like we're
   promising to make its output more plumbing-like.

 * As an aside I think a good follow-up to my series would be to
   just start warning() and eventually die()-ing on grep.extendedRegexp
   which would make this a bit simpler.

 * A "GIT_DISABLE_GREP_PATTERN_CONFIG" seems overly narrow. Just a few lines
   from the code being patched here we read the grep.lineNumber config, which is
   similarly annoying if you're parsing the "git grep" output, so at least a
   "GIT_DISABLE_GREP_CONFIG" would be handy.

 * But more generally we've had discussions on and off on-list about supporting
   a generic way to disable reading the config. Supporting e.g. "git --no-config" or
   a "GIT_NO_CONFIG" would be handy, even if all it did for now (and we could document
   it as such) would be to change the behavior of grep.
 

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

Just as I suspected earlier, up to the [PATCH 6/7] of the series passes this test and this test reveals the breakage in [PATCH 7/7].

In the review of that step, I said "I am not sure how any change that says we do not need the "commit" phase can be correct." but come to think of it, it was a bit too strong. It is possible to implement correct semantics without the "commit" phase, as long as we do not try to decide between basic and extended too hastily when we see grep.patternType=default, before we are sure that we will not see any new definition of grep.extendedRegexp [*]. In the extreme, we could keep it as 'default' until the time just before we compile the regexp and consult the final value of grep.extendedRegexp to decide between the two, and such an implementation would still give correct results without having to have a "commit" phase.

[Footnote]

* The story is the same for the `--patternType=default` command line
  option, but by the time we read the command line option, we should
  be already done with the configuration, and there is no command
  line option to override the grep.extendedRegexp configuration
  variable, so we do not have to worry about that case.  When we
  read --patternType=default from the command line, we can safely
  use the value we know about grep.extendedRegexp and decide to use
  either basic or extended.

  But that is not true for grep.patternType configuration varialble,
  as the additional test I am responding to shows.

Change code in "builtin/grep.c" and "builtin/ls-tree.c" to trust the "prefix" passed from "run_builtin()". The "prefix" we get from setup.c is either going to be NULL or a string of length >0, never "".

So we can drop the "prefix && *prefix" checks added for "builtin/grep.c" in 0d042fecf2f (git-grep: show pathnames relative to the current directory, 2006-08-11), and for "builtin/ls-tree.c" in a69dd585fca (ls-tree: chomp leading directories when run from a subdirectory, 2005-12-23).

As seen in code in revision.c that was added in cd676a51367 (diff --relative: output paths as relative to the current subdirectory, 2008-02-12) we already have existing code that does away with this assertion.

This makes it easier to reason about a subsequent change to the "prefix_length" code in grep.c in a subsequent commit, and since we're going to the trouble of doing that let's leave behind an assert() to promise this to any future callers.

For "builtin/grep.c" it would be painful to pass the "prefix" down the callchain of:

    cmd_grep -> grep_tree -> grep_submodule -> grep_cache -> grep_oid ->
    grep_source_name

So for the code that needs it in grep_source_name() let's add a "grep_prefix" variable similar to the existing "ls_tree_prefix".

While at it let's move the code in cmd_ls_tree() around so that we assign to the "ls_tree_prefix" right after declaring the variables, and stop assigning to "prefix". We only subsequently used that variable later in the function after clobbering it. Let's just use our own "grep_prefix" instead.

Let's also add an assert() in git.c, so that we'll make this promise about the "prefix" to any current and future callers, as well as to any readers of the code.

Code history:

 * The strlen() in "grep.c" hasn't been used since 493b7a08d80 (grep:
   accept relative paths outside current working directory, 2009-09-05).

   When that code was added in 0d042fecf2f (git-grep: show pathnames
   relative to the current directory, 2006-08-11) we used the length.

   But since 493b7a08d80 we haven't used it for anything except a
   boolean check that we could have done on the "prefix" member
   itself.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c    | 13 ++++++++-----
 builtin/ls-tree.c |  2 +-
 git.c             |  1 +
 grep.c            |  4 +---
 grep.h            |  4 +---
 revision.c        |  2 +-
 6 files changed, 13 insertions(+), 13 deletions(-)

Change grep_cmd_config() to stop passing around the always-NULL "cb" value. When this code was added in 7e8f59d577e (grep: color patterns in output, 2009-03-07) it was non-NULL, but when that changed in 15fabd1bbd4 (builtin/grep.c: make configuration callback more reusable, 2012-10-09) this code was left behind.

In a subsequent change I'll start using the "cb" value, this will make it clear which functions we call need it, and which don't.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

The grep_init() function used the odd pattern of initializing the passed-in "struct grep_opt" with a statically defined "grep_defaults" struct, which would be modified in-place when we invoked grep_config().

So we effectively (b) initialized config, (a) then defaults, (c) followed by user options. Usually those are ordered as "a", "b" and "c" instead.

As the comments being removed here show the previous behavior needed to be carefully explained as we'd potentially share the populated configuration among different instances of grep_init(). In practice we didn't do that, but now that it can't be a concern anymore let's remove those comments.

This does not change the behavior of any of the configuration variables or options. That would have been the case if we didn't move around the grep_config() call in "builtin/log.c". But now that we call "grep_config" after "git_log_config" and "git_format_config" we'll need to pass in the already initialized "struct grep_opt *".

See 6ba9bb76e02 (grep: copy struct in one fell swoop, 2020-11-29) and 7687a0541e0 (grep: move the configuration parsing logic to grep.[ch], 2012-10-09) for the commits that added the comments.

The memcpy() pattern here will be optimized away and follows the convention of other *_init() functions. See 5726a6b4012 (*.c *_init(): define in terms of corresponding *_INIT macro, 2021-07-01).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c |  4 ++--
 builtin/log.c  | 13 +++++++++++--
 grep.c         | 39 +++------------------------------------
 grep.h         | 21 +++++++++++++++++++++
 4 files changed, 37 insertions(+), 40 deletions(-)

Simplify the parsing of "grep.patternType" and "grep.extendedRegexp". This changes no behavior, but gets rid of complex parsing logic that isn't needed anymore.

When "grep.patternType" was introduced in 84befcd0a4a (grep: add a grep.patternType configuration setting, 2012-08-03) we promised that:

 1. You can set "grep.patternType", and "[setting it to] 'default'
    will return to the default matching behavior".

 2. We'd support the existing "grep.extendedRegexp" option, but ignore
    it when the new "grep.patternType" option is set. We said we'd
    only ignore the older "grep.extendedRegexp" option "when the
    `grep.patternType` option is set. to a value other than
    'default'".

In a preceding commit we changed grep_config() to be called after grep_init(), which means that much of the complexity here can go away.

Now as before when we only understand a "grep.extendedRegexp" setting of "true", and if "grep.patterntype=default" is set we'll interpret it as "grep.patterntype=basic", except if we previously saw a "grep.extendedRegexp", then it's interpreted as "grep.patterntype=extended".

See my 07a3d411739 (grep: remove regflags from the public grep_opt API, 2017-06-29) for addition of the two comments being removed here, i.e. the complexity noted in that commit is now going away.

We don't need grep_commit_pattern_type() anymore, we can instead have OPT_SET_INT() in "builtin/grep.c" manipulate the "pattern_type_option" member in "struct grep_opt" directly.

We can also do away with the indirection of the "int fixed" and "int pcre2" members in favor of using "pattern_type_option" directly in "grep.c".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c | 10 +++----
 grep.c         | 81 +++++++++++---------------------------------------
 grep.h         |  9 ++----
 revision.c     |  2 --
 4 files changed, 24 insertions(+), 78 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

You need to call it into readers' attention that back then the author of that commit meant by "the default" to mean "whatever the configuration system specified before the patch applied, i.e. with grep.extendedRegexp".

Yes, in short, you can think of it this way.

 - We use grep.patternType as the source of truth.  It is a usual
   last-one-wins variable, with values like fixed, basic, pcre,
   etc.

 - There however is a special value 'default', which may mean
   'basic' or 'extended', depending on what grep.extendedRegexp is
   set to.

Is that a typo? If extendedRegexp is set to 'true', then the 'default' would mean 'extended', so I would expect that we'd see it as the same as setting it 'grep.patternType=extended'.

I am not sure what this means. grep.extendedRegexp is also a usual last-one-wins variable. If you had this series:

    git \
    -c grep.extendedRegexp = false \
    -c grep.extendedRegexp = true \
    -c grep.patternType = default \
    some-command-that-take-regexp

the last grep.extendedRegexp is true, and the last grep.patternType is default, so the command sould work on extended. It is also true if you had

    git \
    -c grep.extendedRegexp = false \
    -c grep.patternType = default \
    -c grep.extendedRegexp = true \
    some-command-that-take-regexp

That is why it is important to remember the fact that patternType was give as "default" before you finish reading the configuration and you are sure you know the last value of grep.extendedRegexp. Only after that, you can resolve what that "default" means between "basic" and "extended". Trying to interpret "default" as soon as you see it in grep.patternType and trying to make it into either "basic" or "extended" will not work unless you know you have the final value of grep.extendedRegexp.

And I think that is what this function wanted to say: "we now have seen all necessary, so we can commit what pattern type we are going to use; before this point, we couldn't tell what 'default' meant".

So I am not sure how any change that says we do not need the "commit" phase can be correct.

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

I understand that this has no changes other than the rebase to adjust for the "even when we are running 'git cmd -h', make sure we try to find where the git repository is (gently)". Am I correct?

I am not complaining for lack of improvements or anything. I am only making sure that I am applying the right version to a right base.

THanks.

A simplification and code deletion of the overly complex setup for the grep API, no behavior changes. For v5 see: https://lore.kernel.org/git/cover-v5-0.7-00000000000-20211222T025214Z-avarab@gmail.com

Changes since v4:

 * As Junio pointed out there were behavior changes in the v4. I
   integrated/squashed the change he added to the
   gitster/ab/grep-patterntype branch to add the tests, and a fixed
   7/7 correctly handles the case of a flip-flopping
   grep.extendedRegexp now.

 * Some commit message additions/rewording that I hope will address
   relevant comments from Junio.

Ævar Arnfjörð Bjarmason (7):
  grep.h: remove unused "regex_t regexp" from grep_opt
  log tests: check if grep_config() is called by "log"-like cmds
  grep tests: add missing "grep.patternType" config tests
  built-ins: trust the "prefix" from run_builtin()
  grep.c: don't pass along NULL callback value
  grep API: call grep_config() after grep_init()
  grep: simplify config parsing and option parsing

 builtin/grep.c    |  27 +++++-----
 builtin/log.c     |  13 ++++-
 builtin/ls-tree.c |   2 +-
 git.c             |   1 +
 grep.c            | 126 +++++++++-------------------------------------
 grep.h            |  33 ++++++++----
 revision.c        |   4 +-
 t/t4202-log.sh    |  24 +++++++++
 t/t7810-grep.sh   |  39 ++++++++++++++
 9 files changed, 138 insertions(+), 131 deletions(-)

Range-diff against v5:
1:  b6a3e0e2e08 = 1:  b62e6b6162a grep.h: remove unused "regex_t regexp" from grep_opt
2:  c0d77b2683f = 2:  0edcdb50afd log tests: check if grep_config() is called by "log"-like cmds
3:  f02f246aa23 ! 3:  1b724d5e2e9 grep tests: add missing "grep.patternType" config test
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    grep tests: add missing "grep.patternType" config test
    +    grep tests: add missing "grep.patternType" config tests
     
         Extend the grep tests to assert that setting
         "grep.patternType=extended" followed by "grep.patternType=default"
    @@ Commit message
     
         Let's also test what happens when we have a sequence of "extended"
         followed by "default" and "fixed". In that case the "fixed" should
    -    prevail.
    +    prevail, as well as tests to check that a "grep.extendedRegexp=true"
    +    followed by a "grep.extendedRegexp=false" behaves as though
    +    "grep.extendedRegexp" wasn't provided.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    +    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## t/t7810-grep.sh ##
     @@ t/t7810-grep.sh: do
      		test_cmp expected actual
      	'
      
    ++	test_expect_success "grep $L with grep.extendedRegexp is last-one-wins" '
    ++		echo "${HC}ab:a+bc" >expected &&
    ++		git \
    ++			-c grep.extendedRegexp=true \
    ++			-c grep.patternType=basic \
    ++			-c grep.extendedRegexp=false \
    ++			grep "a+b*c" $H ab >actual &&
    ++		test_cmp expected actual
    ++	'
    ++
    ++	test_expect_success "grep $L with grep.extendedRegexp is last-one-wins & defers to grep.patternType" '
    ++		echo "${HC}ab:abc" >expected &&
    ++		git \
    ++			-c grep.extendedRegexp=true \
    ++			-c grep.patternType=extended \
    ++			-c grep.extendedRegexp=false \
    ++			grep "a+b*c" $H ab >actual &&
    ++		test_cmp expected actual
    ++	'
    ++
     +	test_expect_success "grep $L with grep.patternType=extended and grep.patternType=default" '
     +		echo "${HC}ab:a+bc" >expected &&
     +		git \
4:  a542a352eab = 4:  f4876552771 built-ins: trust the "prefix" from run_builtin()
5:  a33b00a247e = 5:  069b0339146 grep.c: don't pass along NULL callback value
6:  92b1c3958fa = 6:  e38eca56959 grep API: call grep_config() after grep_init()
7:  63de643ebc2 ! 7:  88dfd40bf9e grep: simplify config parsing and option parsing
    @@ Commit message
          1. You can set "grep.patternType", and "[setting it to] 'default'
             will return to the default matching behavior".
     
    +        In that context "the default" meant whatever the configuration
    +        system specified before that change, i.e. via grep.extendedRegexp.
    +
          2. We'd support the existing "grep.extendedRegexp" option, but ignore
             it when the new "grep.patternType" option is set. We said we'd
             only ignore the older "grep.extendedRegexp" option "when the
    @@ Commit message
         grep_init(), which means that much of the complexity here can go
         away.
     
    -    Now as before when we only understand a "grep.extendedRegexp" setting
    -    of "true", and if "grep.patterntype=default" is set we'll interpret it
    -    as "grep.patterntype=basic", except if we previously saw a
    -    "grep.extendedRegexp", then it's interpreted as
    -    "grep.patterntype=extended".
    +    As before "grep.extendedRegexp" is a last-one-wins variable. We need
    +    to maintain state inside parse_pattern_type_arg() to ignore it if a
    +    non-"default" grep.patternType was seen, but otherwise flip between
    +    BRE and ERE for "grep.extendedRegexp=[false|true]".
     
         See my 07a3d411739 (grep: remove regflags from the public grep_opt
         API, 2017-06-29) for addition of the two comments being removed here,
    @@ grep.c: int grep_config(const char *var, const char *value, void *cb)
      		return -1;
      
      	if (!strcmp(var, "grep.extendedregexp")) {
    -+		if (opt->extended_regexp_option)
    ++		if (opt->extended_regexp_option == -1)
     +			return 0;
      		opt->extended_regexp_option = git_config_bool(var, value);
     +		if (opt->extended_regexp_option)
     +			opt->pattern_type_option = GREP_PATTERN_TYPE_ERE;
    ++		else
    ++			opt->pattern_type_option = GREP_PATTERN_TYPE_BRE;
     +		return 0;
     +	}
     +

This "regex_t" in grep_opt has not been used since f9b9faf6f8a (builtin-grep: allow more than one patterns., 2006-05-02), we still use a "regex_t" for compiling regexes, but that's in the "grep_pat" struct".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.h | 1 -
 1 file changed, 1 deletion(-)

Extend the tests added in my 9df46763ef1 (log: add exhaustive tests for pattern style options & config, 2017-05-20) to check not only whether "git log" handles "grep.patternType", but also "git show" etc.

It's sufficient to check whether we match a "fixed" or a "basic" regex here to see if these codepaths correctly invoked grep_config(). We don't need to check the details of their regular expression matching as the "log" test does.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4202-log.sh | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Extend the grep tests to assert that setting "grep.patternType=extended" followed by "grep.patternType=default" will behave as if "--basic-regexp" was provided, and not as "--extended-regexp". In a subsequent commit we'll need to treat "grep.patternType=default" as a special-case, but let's make sure we ignore it if it's being set to "default" following an earlier non-"default" "grep.patternType" setting.

Let's also test what happens when we have a sequence of "extended" followed by "default" and "fixed". In that case the "fixed" should prevail, as well as tests to check that a "grep.extendedRegexp=true" followed by a "grep.extendedRegexp=false" behaves as though "grep.extendedRegexp" wasn't provided.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t7810-grep.sh | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

Change code in "builtin/grep.c" and "builtin/ls-tree.c" to trust the "prefix" passed from "run_builtin()". The "prefix" we get from setup.c is either going to be NULL or a string of length >0, never "".

So we can drop the "prefix && *prefix" checks added for "builtin/grep.c" in 0d042fecf2f (git-grep: show pathnames relative to the current directory, 2006-08-11), and for "builtin/ls-tree.c" in a69dd585fca (ls-tree: chomp leading directories when run from a subdirectory, 2005-12-23).

As seen in code in revision.c that was added in cd676a51367 (diff --relative: output paths as relative to the current subdirectory, 2008-02-12) we already have existing code that does away with this assertion.

This makes it easier to reason about a subsequent change to the "prefix_length" code in grep.c in a subsequent commit, and since we're going to the trouble of doing that let's leave behind an assert() to promise this to any future callers.

For "builtin/grep.c" it would be painful to pass the "prefix" down the callchain of:

    cmd_grep -> grep_tree -> grep_submodule -> grep_cache -> grep_oid ->
    grep_source_name

So for the code that needs it in grep_source_name() let's add a "grep_prefix" variable similar to the existing "ls_tree_prefix".

While at it let's move the code in cmd_ls_tree() around so that we assign to the "ls_tree_prefix" right after declaring the variables, and stop assigning to "prefix". We only subsequently used that variable later in the function after clobbering it. Let's just use our own "grep_prefix" instead.

Let's also add an assert() in git.c, so that we'll make this promise about the "prefix" to any current and future callers, as well as to any readers of the code.

Code history:

 * The strlen() in "grep.c" hasn't been used since 493b7a08d80 (grep:
   accept relative paths outside current working directory, 2009-09-05).

   When that code was added in 0d042fecf2f (git-grep: show pathnames
   relative to the current directory, 2006-08-11) we used the length.

   But since 493b7a08d80 we haven't used it for anything except a
   boolean check that we could have done on the "prefix" member
   itself.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c    | 13 ++++++++-----
 builtin/ls-tree.c |  2 +-
 git.c             |  1 +
 grep.c            |  4 +---
 grep.h            |  4 +---
 revision.c        |  2 +-
 6 files changed, 13 insertions(+), 13 deletions(-)

Change grep_cmd_config() to stop passing around the always-NULL "cb" value. When this code was added in 7e8f59d577e (grep: color patterns in output, 2009-03-07) it was non-NULL, but when that changed in 15fabd1bbd4 (builtin/grep.c: make configuration callback more reusable, 2012-10-09) this code was left behind.

In a subsequent change I'll start using the "cb" value, this will make it clear which functions we call need it, and which don't.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

The grep_init() function used the odd pattern of initializing the passed-in "struct grep_opt" with a statically defined "grep_defaults" struct, which would be modified in-place when we invoked grep_config().

So we effectively (b) initialized config, (a) then defaults, (c) followed by user options. Usually those are ordered as "a", "b" and "c" instead.

As the comments being removed here show the previous behavior needed to be carefully explained as we'd potentially share the populated configuration among different instances of grep_init(). In practice we didn't do that, but now that it can't be a concern anymore let's remove those comments.

This does not change the behavior of any of the configuration variables or options. That would have been the case if we didn't move around the grep_config() call in "builtin/log.c". But now that we call "grep_config" after "git_log_config" and "git_format_config" we'll need to pass in the already initialized "struct grep_opt *".

See 6ba9bb76e02 (grep: copy struct in one fell swoop, 2020-11-29) and 7687a0541e0 (grep: move the configuration parsing logic to grep.[ch], 2012-10-09) for the commits that added the comments.

The memcpy() pattern here will be optimized away and follows the convention of other *_init() functions. See 5726a6b4012 (*.c *_init(): define in terms of corresponding *_INIT macro, 2021-07-01).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c |  4 ++--
 builtin/log.c  | 13 +++++++++++--
 grep.c         | 39 +++------------------------------------
 grep.h         | 21 +++++++++++++++++++++
 4 files changed, 37 insertions(+), 40 deletions(-)

Simplify the parsing of "grep.patternType" and "grep.extendedRegexp". This changes no behavior, but gets rid of complex parsing logic that isn't needed anymore.

When "grep.patternType" was introduced in 84befcd0a4a (grep: add a grep.patternType configuration setting, 2012-08-03) we promised that:

 1. You can set "grep.patternType", and "[setting it to] 'default'
    will return to the default matching behavior".

    In that context "the default" meant whatever the configuration
    system specified before that change, i.e. via grep.extendedRegexp.

 2. We'd support the existing "grep.extendedRegexp" option, but ignore
    it when the new "grep.patternType" option is set. We said we'd
    only ignore the older "grep.extendedRegexp" option "when the
    `grep.patternType` option is set. to a value other than
    'default'".

In a preceding commit we changed grep_config() to be called after grep_init(), which means that much of the complexity here can go away.

As before "grep.extendedRegexp" is a last-one-wins variable. We need to maintain state inside parse_pattern_type_arg() to ignore it if a non-"default" grep.patternType was seen, but otherwise flip between BRE and ERE for "grep.extendedRegexp=[false|true]".

See my 07a3d411739 (grep: remove regflags from the public grep_opt API, 2017-06-29) for addition of the two comments being removed here, i.e. the complexity noted in that commit is now going away.

We don't need grep_commit_pattern_type() anymore, we can instead have OPT_SET_INT() in "builtin/grep.c" manipulate the "pattern_type_option" member in "struct grep_opt" directly.

We can also do away with the indirection of the "int fixed" and "int pcre2" members in favor of using "pattern_type_option" directly in "grep.c".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c | 10 +++---
 grep.c         | 83 ++++++++++++--------------------------------------
 grep.h         |  9 ++----
 revision.c     |  2 --
 4 files changed, 26 insertions(+), 78 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

I very much like the lossage of redundant fixed and pcre2 members.

As I kept telling you, we still need a separate bit to keep track of the last value of grep.extendedRegexp, but the primary mechanism to determine what pattern type to use should be a single enum that is pattern_type. When we see "fixed", "pcre", "-G", etc. from grep.patternType config or from command line, we can stuff their enum values in pattern_type member of this struct, and when we see "default", we need to leave "default" in pattern_type member until we see the last definition of grep.extendedRegexp, at which time we can turn it into either "basic" or "extended".

So having only two members is absolutely the right thing to do.

But this part convinces me that whatever this patch does, it will not possible be capable of doing the right thing. You cannot implement "we have to remember that the last grep.patternType we saw was DEFAULT and in that case we cannot decide the real pattern type until we see the last definition of grep.extendedRegexp, which may be well after we saw the last grep.patternType definition" without a value in this enum to express that the last value we saw was DEFAULT.

In other words, this lossage is likely wrong. Let's keep reading and see how well the config reader in this patch does.

The above does not look correct at all.

What happens when the configuration parser sees these configuration variables in this sequence:

 - grep.patternType set to say "pcre" (or anything not "default").
 - grep.extendedRegexp set to "true".
 - grep.patternType set to "default".

After these three variable definitions with the usual "last one wins" (for each variable independently), the last value for the grep.patternType variable is "default", and the last value for the grep.extendedRegexp variable is "true". The user wants to use the ERE patterns.

The way the above code would work on this three variable definition sequence, as far as I read it, would however not give us the desired behaviour. First we drop extended_regexp_option member to -1 while setting PCRE to attern_type_option member, and then grep.extendedRegexp is totally ignored, and then we see patterntype set to default and notice extended_regexp_option is *NOT* 1 (because you ignored it and left it to -1), and end up using BRE, no?

I agree 100% with the direction that .fixed and .pcre2 members that were added over time to the struct are redundant and it is a very good idea to get rid of them. But we need to keep track of two configuration variables separately to allow them the "last one wins" semantics independently, and for that, you cannot lose the "default" value from the enum. It is impossible not to store the fact that "default" was the last value so far we saw for grep.patternType because you do not know, at the point of seeing "default", what the final value for grep.extendedRegexp will be. If you want to correctly implement the interaction between two variables without regression, that is.

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

By the way, the example I gave you for the previous round, and similarly the one in the message I am responding to were all written to help you realize that it is simply a broken approach if we do not keep "default" as default and instead resolve it to either "basic" or "extended" too early. The goal of these examples was *NOT* to tell you "this single thing is broken with the code in this round so let's fix it".

It seems I am not succeeding in conveying that point, and specially I smell that in the change between v5 and v6.

So let me try to be a bit more explicit. Let's not do another round of "I think this is a moral equivalent of what you want, even though it is not done the way you suggested." I think we wasted a reroll or three with that attitude in changes leading to v6 already, after I gave my review to v5, and I think the v5 review essentially was a repeat of my review for v3's 3/7, so if I conveyed the point clearly enough back then, perhaps we didn't have to waste your time on v4 and v5, either. Sorry about that.

So, here is what this step of the series SHOULD do:

 * Use two members to keep track of the final configuration value we
   saw for grep.patternTYpe and grep.extendedRegexp independently.
   The existing .fixed and .pcre2 fields are superfluous.  But no
   more "ah, we see patternType so let's ignore extendedRegexp"
   games.

 * When parsing the command line options -G, -E, etc., update the
   .patternType member with the value found.  We do not want to and
   need to touch .extendedRegexp member, whose SOLE purpose should
   be to keep track of "what the last value we saw for
   grep.extendedRegexp configuration variable".

 * Do ALL THE ABOVE while keeping "default" in the .patternType
   member as "default" as-is given by the user; do not turn it into
   "basic" or "extended" in config callback at all.

 * At some point of your choice between the time we finished parsing
   both configuration variables and command line options and the
   time we compile the pattern string to regexp objects of various
   types, look at the .patternType member and resolve it into
   basic/extended IFF it is set to default, using .extendedRegexp
   member (for this to work correctly, it is important not to let
   -E/-G command like options to touch .extendedRegexp member---it
   should be used ONLY to keep track of "what the last value we saw
   for grep.extendedRegexp configuration variable").

 * After the above step is done, .extendedRegexp member is no longer
   needed and we can compile the pattern using only the value in
   .patternType member.

The penultimate bullet point gives us a wiggle room to lose the "commit" thing and delay it until the very last moment, the function that decides to call which regexp engine's regcomp. The important thing is that we cannot lose the value "default" from .patternType field or lose the last value given to .extendedRegexp field too early, namely, before we have read all the configurtion streams and know the last value for these two variables.

Thanks.  Hopefully I was clear enough this time.

----- >8 ---- ----- >8 ---- ----- >8 ---- ----- >8 ---- ----- >8 -----
Subject: [PATCH] fixup! grep tests: add missing "grep.patternType" config tests

---
 t/t7810-grep.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

A re-roll that should *hopefully* address the comments Junio had in https://lore.kernel.org/git/xmqq4k6tyj8r.fsf@gitster.g/; To quote those:

Yes, the v6 was broken. Sorry about that. Between re-rolling some other things and coming back to this series I managed to miss some of those subtleties.

So, at the risk of another wasted re-roll this doesn't exactly implement what you suggested, but I think it'll finally cover all the basis of being bug-for-bug compatible with the old implementation now.

I started this re-roll by implementing it exactly as you suggested, along with new tests (both your fixup, and more), but as the updated commit message in 09/10 details found that I could convert it back to a state machine around a static variable in "grep_config()", which has the advantage of getting rid of more "struct grep_opt" fields annd narrowing the behavior to just that function.

More comments on the range-diff below:

Ævar Arnfjörð Bjarmason (10):
  grep.h: remove unused "regex_t regexp" from grep_opt
  log tests: check if grep_config() is called by "log"-like cmds

No changes.

  grep tests: add missing "grep.patternType" config tests

A couple of new tests.

  built-ins: trust the "prefix" from run_builtin()
  grep.c: don't pass along NULL callback value
  grep API: call grep_config() after grep_init()

No changes.

  grep.h: make "grep_opt.pattern_type_option" use its enum
  grep.c: do "if (bool && memchr())" not "if (memchr() && bool)"

I split these two off from the previous tip commit to make the diff for the next one smaller.

  grep: simplify config parsing and option parsing

Changed, as noted above.

  grep.[ch]: remove GREP_PATTERN_TYPE_UNSPECIFIED

With that new behavior we can also get rid of GREP_PATTERN_TYPE_UNSPECIFIED. It's still implicitly there as == 0, but for a non-switch'd enum type treating these as "flags" makes more sense, and makes the code in grep_config() more straightforward & brief.

 builtin/grep.c    |  27 +++++-----
 builtin/log.c     |  13 ++++-
 builtin/ls-tree.c |   2 +-
 git.c             |   1 +
 grep.c            | 126 +++++++++-------------------------------------
 grep.h            |  34 +++++++++----
 revision.c        |   4 +-
 t/t4202-log.sh    |  24 +++++++++
 t/t7810-grep.sh   |  68 +++++++++++++++++++++++++
 9 files changed, 168 insertions(+), 131 deletions(-)

Range-diff against v6:
 1:  b62e6b6162a =  1:  b62e6b6162a grep.h: remove unused "regex_t regexp" from grep_opt
 2:  0edcdb50afd =  2:  0edcdb50afd log tests: check if grep_config() is called by "log"-like cmds
 3:  1b724d5e2e9 !  3:  e1b4b5b77e0 grep tests: add missing "grep.patternType" config tests
    @@ t/t7810-grep.sh: do
     +		test_cmp expected actual
     +	'
     +
    ++	test_expect_success "grep $L with grep.extendedRegexp and grep.patternType are both last-one-wins independently (BRE)" '
    ++		echo "${HC}ab:abc" >expected &&
    ++		git \
    ++			-c grep.patternType=fixed \
    ++			-c grep.extendedRegexp=true \
    ++			-c grep.patternType=default \
    ++			grep "a+b*c" $H ab >actual &&
    ++		test_cmp expected actual
    ++	'
    ++
    ++	test_expect_success "grep $L with grep.extendedRegexp and grep.patternType are both last-one-wins independently (ERE)" '
    ++		echo "${HC}ab:a+bc" >expected &&
    ++		git \
    ++			-c grep.patternType=default \
    ++			-c grep.extendedRegexp=true \
    ++			-c grep.patternType=basic \
    ++			grep "a+b*c" $H ab >actual &&
    ++		test_cmp expected actual
    ++	'
    ++
     +	test_expect_success "grep $L with grep.patternType=extended and grep.patternType=default" '
     +		echo "${HC}ab:a+bc" >expected &&
     +		git \
    @@ t/t7810-grep.sh: do
      	test_expect_success "grep $L with grep.patternType=extended and grep.extendedRegexp=false" '
      		echo "${HC}ab:abc" >expected &&
      		git \
    +@@ t/t7810-grep.sh: do
    + 		test_cmp expected actual
    + 	'
    + 
    ++	test_expect_success "grep $L with grep.extendedRegexp=false and grep.patternType=default" '
    ++		echo "${HC}ab:abc" >expected &&
    ++		git \
    ++			-c grep.extendedRegexp=false \
    ++			-c grep.patternType=extended \
    ++			grep "a+b*c" $H ab >actual &&
    ++		test_cmp expected actual
    ++	'
    ++
    + 	test_expect_success "grep $L with grep.extendedRegexp=true and grep.patternType=basic" '
    + 		echo "${HC}ab:a+bc" >expected &&
    + 		git \
 4:  f4876552771 =  4:  6d91a765fd7 built-ins: trust the "prefix" from run_builtin()
 5:  069b0339146 =  5:  844b4727ca3 grep.c: don't pass along NULL callback value
 6:  e38eca56959 =  6:  d9cf9bf5e37 grep API: call grep_config() after grep_init()
 -:  ----------- >  7:  57ecc5c0d65 grep.h: make "grep_opt.pattern_type_option" use its enum
 -:  ----------- >  8:  7dbeafde26b grep.c: do "if (bool && memchr())" not "if (memchr() && bool)"
 7:  88dfd40bf9e !  9:  c6ca39b4554 grep: simplify config parsing and option parsing
    @@ Commit message
         grep_init(), which means that much of the complexity here can go
         away.
     
    -    As before "grep.extendedRegexp" is a last-one-wins variable. We need
    -    to maintain state inside parse_pattern_type_arg() to ignore it if a
    -    non-"default" grep.patternType was seen, but otherwise flip between
    -    BRE and ERE for "grep.extendedRegexp=[false|true]".
    +    As before both "grep.patternType" and "grep.extendedRegexp" are
    +    last-one-wins variable, with "grep.extendedRegexp" yielding to
    +    "grep.patternType", except when "grep.patternType=default".
    +
    +    Note that this applies as we parse the config, i.e. a sequence of:
    +
    +        -c grep.patternType=perl
    +        -c grep.extendedRegexp=true \
    +        -c grep.patternType=default
    +
    +    Should select ERE due to "grep.extendedRegexp=true and
    +    grep.extendedRegexp=default", not BRE, even though that's the
    +    "default" patternType. We can determine this as we parse the config,
    +    because:
    +
    +     * If we see "grep.extendedRegexp" we set the internal "ero" to its
    +       boolean value.
    +
    +     * If we see "grep.extendedRegexp" but
    +       "grep.patternType=[default|<unset>]" is in effect we *don't* set
    +       the internal "pattern_type_option" to update the pattern type.
    +
    +     * If we see "grep.patternType!=default" we can set our internal
    +       "pattern_type_option" directly, it doesn't matter what the state of
    +       "grep.extendedRegexp" is, but we don't forget what it was, in case
    +       we see a "grep.patternType=default" again.
    +
    +     * If we see a "grep.patternType=default" we can set the pattern to
    +       ERE or BRE depending on whether we last saw a
    +       "grep.extendedRegexp=true" or
    +       "grep.extendedRegexp=[false|<unset>]".
    +
    +    We could equally call this new adjust_pattern_type() in
    +    compile_regexp(), i.e. this fixup on top of this passes all our
    +    tests (with -U0 for brevity):
    +
    +        @@ -60,0 +61 @@ static void adjust_pattern_type(enum grep_pattern_type *pto, const int ero)
    +        +static int ero = -1;
    +        @@ -65 +65,0 @@ int grep_config(const char *var, const char *value, void *cb)
    +        -       static int ero = -1;
    +        @@ -72 +71,0 @@ int grep_config(const char *var, const char *value, void *cb)
    +        -               adjust_pattern_type(&opt->pattern_type_option, ero);
    +        @@ -80 +78,0 @@ int grep_config(const char *var, const char *value, void *cb)
    +        -               adjust_pattern_type(&opt->pattern_type_option, ero);
    +        @@ -445,0 +444,2 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
    +        +       if (ero != -1)
    +        +               adjust_pattern_type(&opt->pattern_type_option, ero);
    +
    +    But doing it as we stream the git_config() makes it
    +    clear that we can determine the interplay between these two variables
    +    as we go. We don't need to wait until we see the last value of the two
    +    configuration variables.
    +
    +    This is true because of the rationale above, and because the
    +    subsequent code in compile_regexp() treats
    +    "pattern_type_option=GREP_PATTERN_TYPE_{UNSPECIFIED,BRE}"
    +    equally. I.e. we'll end up with different internal
    +    ""pattern_type_option" values there for:
    +
    +        # UNSPECIFIED
    +        -c grep.patternType=default
    +        # BRE
    +        -c grep.extendedRegexp=false -c grep.patternType=default
    +
    +    But the difference won't matter, which simplifies some of this logic,
    +    we never need to adjust a "grep.patternType" if we didn't see a
    +    "grep.extendedRegexp" before. We can also remove the
    +    "extended_regexp_option" member from "struct grep_opt" in favor of a
    +    static variable in grep_config().
    +
    +    The command-line parsing in cmd_grep() can then completely ignore
    +    "grep.extendedRegexp". Whatever effect it had before that step won't
    +    matter if we see -G, -E, -P etc.
     
         See my 07a3d411739 (grep: remove regflags from the public grep_opt
         API, 2017-06-29) for addition of the two comments being removed here,
         i.e. the complexity noted in that commit is now going away.
     
    -    We don't need grep_commit_pattern_type() anymore, we can instead have
    -    OPT_SET_INT() in "builtin/grep.c" manipulate the "pattern_type_option"
    -    member in "struct grep_opt" directly.
    -
    -    We can also do away with the indirection of the "int fixed" and "int
    -    pcre2" members in favor of using "pattern_type_option" directly in
    -    "grep.c".
    -
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/grep.c ##
    @@ builtin/grep.c: int cmd_grep(int argc, const char **argv, const char *prefix)
      		int fallback = 0;
     
      ## grep.c ##
    -@@ grep.c: static const char *color_grep_slots[] = {
    +@@ grep.c: static int parse_pattern_type_arg(const char *opt, const char *arg)
      
    - 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"))
    -+	if (!strcmp(arg, "basic"))
    - 		return GREP_PATTERN_TYPE_BRE;
    - 	else if (!strcmp(arg, "extended"))
    - 		return GREP_PATTERN_TYPE_ERE;
    + define_list_config_array_extra(color_grep_slots, {"match"});
    + 
    ++static void adjust_pattern_type(enum grep_pattern_type *pto, const int ero)
    ++{
    ++	if (*pto == GREP_PATTERN_TYPE_UNSPECIFIED)
    ++		*pto = ero ? GREP_PATTERN_TYPE_ERE : GREP_PATTERN_TYPE_BRE;
    ++}
    ++
    + /*
    +  * Read the configuration file once and store it in
    +  * the grep_defaults template.
     @@ grep.c: int grep_config(const char *var, const char *value, void *cb)
    + {
    + 	struct grep_opt *opt = cb;
    + 	const char *slot;
    ++	static int ero = -1;
    + 
    + 	if (userdiff_config(var, value) < 0)
      		return -1;
      
      	if (!strcmp(var, "grep.extendedregexp")) {
    -+		if (opt->extended_regexp_option == -1)
    -+			return 0;
    - 		opt->extended_regexp_option = git_config_bool(var, value);
    -+		if (opt->extended_regexp_option)
    -+			opt->pattern_type_option = GREP_PATTERN_TYPE_ERE;
    -+		else
    -+			opt->pattern_type_option = GREP_PATTERN_TYPE_BRE;
    -+		return 0;
    -+	}
    -+
    -+	if (!strcmp(var, "grep.patterntype") &&
    -+	    !strcmp(value, "default")) {
    -+		opt->pattern_type_option = opt->extended_regexp_option == 1
    -+			? GREP_PATTERN_TYPE_ERE : GREP_PATTERN_TYPE_BRE;
    +-		opt->extended_regexp_option = git_config_bool(var, value);
    ++		ero = git_config_bool(var, value);
    ++		adjust_pattern_type(&opt->pattern_type_option, ero);
      		return 0;
      	}
      
      	if (!strcmp(var, "grep.patterntype")) {
    -+		opt->extended_regexp_option = -1; /* ignore */
      		opt->pattern_type_option = parse_pattern_type_arg(var, value);
    ++		if (ero == -1)
    ++			return 0;
    ++		adjust_pattern_type(&opt->pattern_type_option, ero);
      		return 0;
      	}
    + 
     @@ grep.c: void grep_init(struct grep_opt *opt, struct repository *repo)
      	opt->header_tail = &opt->header_list;
      }
    @@ grep.c: static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
     -	p->fixed = opt->fixed;
     +	p->fixed = opt->pattern_type_option == GREP_PATTERN_TYPE_FIXED;
      
    --	if (memchr(p->pattern, 0, p->patternlen) && !opt->pcre2)
    +-	if (!opt->pcre2 &&
     +	if (opt->pattern_type_option != GREP_PATTERN_TYPE_PCRE &&
    -+	    memchr(p->pattern, 0, p->patternlen))
    + 	    memchr(p->pattern, 0, p->patternlen))
      		die(_("given pattern contains NULL byte (via -f <file>). This is only supported with -P under PCRE v2"));
      
    - 	p->is_fixed = is_fixed(p->pattern, p->patternlen);
     @@ grep.c: static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
      		return;
      	}
    @@ grep.c: static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
      	if (err) {
     
      ## grep.h ##
    -@@ grep.h: enum grep_expr_node {
    - };
    - 
    - enum grep_pattern_type {
    --	GREP_PATTERN_TYPE_UNSPECIFIED = 0,
    --	GREP_PATTERN_TYPE_BRE,
    -+	GREP_PATTERN_TYPE_BRE = 0,
    - 	GREP_PATTERN_TYPE_ERE,
    - 	GREP_PATTERN_TYPE_FIXED,
    - 	GREP_PATTERN_TYPE_PCRE
     @@ grep.h: struct grep_opt {
      	int unmatch_name_only;
      	int count;
    @@ grep.h: struct grep_opt {
      	int pathname;
      	int null_following_name;
     @@ grep.h: struct grep_opt {
    + 	int max_depth;
      	int funcname;
      	int funcbody;
    - 	int extended_regexp_option;
    --	int pattern_type_option;
    -+	enum grep_pattern_type pattern_type_option;
    +-	int extended_regexp_option;
    + 	enum grep_pattern_type pattern_type_option;
      	int ignore_locale;
      	char colors[NR_GREP_COLORS][COLOR_MAXLEN];
    - 	unsigned pre_context;
    -@@ grep.h: struct grep_opt {
    - 	.relative = 1, \
    - 	.pathname = 1, \
    - 	.max_depth = -1, \
    --	.pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED, \
    - 	.colors = { \
    - 		[GREP_COLOR_CONTEXT] = "", \
    - 		[GREP_COLOR_FILENAME] = "", \
     @@ grep.h: struct grep_opt {
      
      int grep_config(const char *var, const char *value, void *);
 -:  ----------- > 10:  b764c09d2b7 grep.[ch]: remove GREP_PATTERN_TYPE_UNSPECIFIED

This "regex_t" in grep_opt has not been used since f9b9faf6f8a (builtin-grep: allow more than one patterns., 2006-05-02), we still use a "regex_t" for compiling regexes, but that's in the "grep_pat" struct".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.h | 1 -
 1 file changed, 1 deletion(-)

Extend the tests added in my 9df46763ef1 (log: add exhaustive tests for pattern style options & config, 2017-05-20) to check not only whether "git log" handles "grep.patternType", but also "git show" etc.

It's sufficient to check whether we match a "fixed" or a "basic" regex here to see if these codepaths correctly invoked grep_config(). We don't need to check the details of their regular expression matching as the "log" test does.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4202-log.sh | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Extend the grep tests to assert that setting "grep.patternType=extended" followed by "grep.patternType=default" will behave as if "--basic-regexp" was provided, and not as "--extended-regexp". In a subsequent commit we'll need to treat "grep.patternType=default" as a special-case, but let's make sure we ignore it if it's being set to "default" following an earlier non-"default" "grep.patternType" setting.

Let's also test what happens when we have a sequence of "extended" followed by "default" and "fixed". In that case the "fixed" should prevail, as well as tests to check that a "grep.extendedRegexp=true" followed by a "grep.extendedRegexp=false" behaves as though "grep.extendedRegexp" wasn't provided.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t7810-grep.sh | 68 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

Change code in "builtin/grep.c" and "builtin/ls-tree.c" to trust the "prefix" passed from "run_builtin()". The "prefix" we get from setup.c is either going to be NULL or a string of length >0, never "".

So we can drop the "prefix && *prefix" checks added for "builtin/grep.c" in 0d042fecf2f (git-grep: show pathnames relative to the current directory, 2006-08-11), and for "builtin/ls-tree.c" in a69dd585fca (ls-tree: chomp leading directories when run from a subdirectory, 2005-12-23).

As seen in code in revision.c that was added in cd676a51367 (diff --relative: output paths as relative to the current subdirectory, 2008-02-12) we already have existing code that does away with this assertion.

This makes it easier to reason about a subsequent change to the "prefix_length" code in grep.c in a subsequent commit, and since we're going to the trouble of doing that let's leave behind an assert() to promise this to any future callers.

For "builtin/grep.c" it would be painful to pass the "prefix" down the callchain of:

    cmd_grep -> grep_tree -> grep_submodule -> grep_cache -> grep_oid ->
    grep_source_name

So for the code that needs it in grep_source_name() let's add a "grep_prefix" variable similar to the existing "ls_tree_prefix".

While at it let's move the code in cmd_ls_tree() around so that we assign to the "ls_tree_prefix" right after declaring the variables, and stop assigning to "prefix". We only subsequently used that variable later in the function after clobbering it. Let's just use our own "grep_prefix" instead.

Let's also add an assert() in git.c, so that we'll make this promise about the "prefix" to any current and future callers, as well as to any readers of the code.

Code history:

 * The strlen() in "grep.c" hasn't been used since 493b7a08d80 (grep:
   accept relative paths outside current working directory, 2009-09-05).

   When that code was added in 0d042fecf2f (git-grep: show pathnames
   relative to the current directory, 2006-08-11) we used the length.

   But since 493b7a08d80 we haven't used it for anything except a
   boolean check that we could have done on the "prefix" member
   itself.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c    | 13 ++++++++-----
 builtin/ls-tree.c |  2 +-
 git.c             |  1 +
 grep.c            |  4 +---
 grep.h            |  4 +---
 revision.c        |  2 +-
 6 files changed, 13 insertions(+), 13 deletions(-)

Change grep_cmd_config() to stop passing around the always-NULL "cb" value. When this code was added in 7e8f59d577e (grep: color patterns in output, 2009-03-07) it was non-NULL, but when that changed in 15fabd1bbd4 (builtin/grep.c: make configuration callback more reusable, 2012-10-09) this code was left behind.

In a subsequent change I'll start using the "cb" value, this will make it clear which functions we call need it, and which don't.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

The grep_init() function used the odd pattern of initializing the passed-in "struct grep_opt" with a statically defined "grep_defaults" struct, which would be modified in-place when we invoked grep_config().

So we effectively (b) initialized config, (a) then defaults, (c) followed by user options. Usually those are ordered as "a", "b" and "c" instead.

As the comments being removed here show the previous behavior needed to be carefully explained as we'd potentially share the populated configuration among different instances of grep_init(). In practice we didn't do that, but now that it can't be a concern anymore let's remove those comments.

This does not change the behavior of any of the configuration variables or options. That would have been the case if we didn't move around the grep_config() call in "builtin/log.c". But now that we call "grep_config" after "git_log_config" and "git_format_config" we'll need to pass in the already initialized "struct grep_opt *".

See 6ba9bb76e02 (grep: copy struct in one fell swoop, 2020-11-29) and 7687a0541e0 (grep: move the configuration parsing logic to grep.[ch], 2012-10-09) for the commits that added the comments.

The memcpy() pattern here will be optimized away and follows the convention of other *_init() functions. See 5726a6b4012 (*.c *_init(): define in terms of corresponding *_INIT macro, 2021-07-01).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c |  4 ++--
 builtin/log.c  | 13 +++++++++++--
 grep.c         | 39 +++------------------------------------
 grep.h         | 21 +++++++++++++++++++++
 4 files changed, 37 insertions(+), 40 deletions(-)

Remove the need for "GREP_PATTERN_TYPE_UNSPECIFIED" in favor of having the users of the "pattern_type_option" member check whether that member is set or not.

The "UNSPECIFIED" case was already handled implicitly in compile_regexp(), and we don't use this "enum" in a "switch" statement, so let's not explicitly name the "GREP_PATTERN_TYPE_UNSPECIFIED = 0" case. It is still important that "GREP_PATTERN_TYPE_BRE != 0", as can be seen in failing tests if the parsing for "basic" in parse_pattern_type_arg() is made to "return 0".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.c | 9 ++++++---
 grep.h | 4 +---
 2 files changed, 7 insertions(+), 6 deletions(-)

Simplify the parsing of "grep.patternType" and "grep.extendedRegexp". This changes no behavior, but gets rid of complex parsing logic that isn't needed anymore.

When "grep.patternType" was introduced in 84befcd0a4a (grep: add a grep.patternType configuration setting, 2012-08-03) we promised that:

 1. You can set "grep.patternType", and "[setting it to] 'default'
    will return to the default matching behavior".

    In that context "the default" meant whatever the configuration
    system specified before that change, i.e. via grep.extendedRegexp.

 2. We'd support the existing "grep.extendedRegexp" option, but ignore
    it when the new "grep.patternType" option is set. We said we'd
    only ignore the older "grep.extendedRegexp" option "when the
    `grep.patternType` option is set. to a value other than
    'default'".

In a preceding commit we changed grep_config() to be called after grep_init(), which means that much of the complexity here can go away.

As before both "grep.patternType" and "grep.extendedRegexp" are last-one-wins variable, with "grep.extendedRegexp" yielding to "grep.patternType", except when "grep.patternType=default".

Note that this applies as we parse the config, i.e. a sequence of:

    -c grep.patternType=perl
    -c grep.extendedRegexp=true \
    -c grep.patternType=default

Should select ERE due to "grep.extendedRegexp=true and grep.extendedRegexp=default", not BRE, even though that's the "default" patternType. We can determine this as we parse the config, because:

 * If we see "grep.extendedRegexp" we set the internal "ero" to its
   boolean value.

 * If we see "grep.extendedRegexp" but
   "grep.patternType=[default|<unset>]" is in effect we *don't* set
   the internal "pattern_type_option" to update the pattern type.

 * If we see "grep.patternType!=default" we can set our internal
   "pattern_type_option" directly, it doesn't matter what the state of
   "grep.extendedRegexp" is, but we don't forget what it was, in case
   we see a "grep.patternType=default" again.

 * If we see a "grep.patternType=default" we can set the pattern to
   ERE or BRE depending on whether we last saw a
   "grep.extendedRegexp=true" or
   "grep.extendedRegexp=[false|<unset>]".

We could equally call this new adjust_pattern_type() in compile_regexp(), i.e. this fixup on top of this passes all our tests (with -U0 for brevity):

    @@ -60,0 +61 @@ static void adjust_pattern_type(enum grep_pattern_type *pto, const int ero)
    +static int ero = -1;
    @@ -65 +65,0 @@ int grep_config(const char *var, const char *value, void *cb)
    -       static int ero = -1;
    @@ -72 +71,0 @@ int grep_config(const char *var, const char *value, void *cb)
    -               adjust_pattern_type(&opt->pattern_type_option, ero);
    @@ -80 +78,0 @@ int grep_config(const char *var, const char *value, void *cb)
    -               adjust_pattern_type(&opt->pattern_type_option, ero);
    @@ -445,0 +444,2 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
    +       if (ero != -1)
    +               adjust_pattern_type(&opt->pattern_type_option, ero);

But doing it as we stream the git_config() makes it clear that we can determine the interplay between these two variables as we go. We don't need to wait until we see the last value of the two configuration variables.

This is true because of the rationale above, and because the subsequent code in compile_regexp() treats "pattern_type_option=GREP_PATTERN_TYPE_{UNSPECIFIED,BRE}" equally. I.e. we'll end up with different internal ""pattern_type_option" values there for:

    # UNSPECIFIED
    -c grep.patternType=default
    # BRE
    -c grep.extendedRegexp=false -c grep.patternType=default

But the difference won't matter, which simplifies some of this logic, we never need to adjust a "grep.patternType" if we didn't see a "grep.extendedRegexp" before. We can also remove the "extended_regexp_option" member from "struct grep_opt" in favor of a static variable in grep_config().

The command-line parsing in cmd_grep() can then completely ignore "grep.extendedRegexp". Whatever effect it had before that step won't matter if we see -G, -E, -P etc.

See my 07a3d411739 (grep: remove regflags from the public grep_opt API, 2017-06-29) for addition of the two comments being removed here, i.e. the complexity noted in that commit is now going away.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c | 10 +++----
 grep.c         | 77 +++++++++++---------------------------------------
 grep.h         |  4 ---
 revision.c     |  2 --
 4 files changed, 20 insertions(+), 73 deletions(-)

Change the "pattern_type_option" member of "struct grep_opt" to use the enum type we use for it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Change code in compile_regexp() to check the cheaper boolean "!opt->pcre2" condition before the "memchr()" search.

This doesn't noticeably optimize anything, but makes the code more obvious and conventional. The line wrapping being added here also makes a subsequent commit smaller.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

A v8 re-roll of this series. For context and v7 see: https://lore.kernel.org/git/cover-v7-00.10-00000000000-20211228T004707Z-avarab@gmail.com

The v7 has not been picked up yet. The ab/grep-patterntype in Junio's tree is the v6. This v8 is rebased on "master" for a merge conflict with the now-merged lh/use-gnu-color-in-grep.

Ævar Arnfjörð Bjarmason (10):
  grep.h: remove unused "regex_t regexp" from grep_opt
  log tests: check if grep_config() is called by "log"-like cmds
  grep tests: add missing "grep.patternType" config tests
  built-ins: trust the "prefix" from run_builtin()
  grep.c: don't pass along NULL callback value
  grep API: call grep_config() after grep_init()
  grep.h: make "grep_opt.pattern_type_option" use its enum
  grep.c: do "if (bool && memchr())" not "if (memchr() && bool)"
  grep: simplify config parsing and option parsing
  grep.[ch]: remove GREP_PATTERN_TYPE_UNSPECIFIED

 builtin/grep.c    |  27 +++++-----
 builtin/log.c     |  13 ++++-
 builtin/ls-tree.c |   2 +-
 git.c             |   1 +
 grep.c            | 126 +++++++++-------------------------------------
 grep.h            |  34 +++++++++----
 revision.c        |   4 +-
 t/t4202-log.sh    |  24 +++++++++
 t/t7810-grep.sh   |  68 +++++++++++++++++++++++++
 9 files changed, 168 insertions(+), 131 deletions(-)

Range-diff against v7:
 1:  b62e6b6162a =  1:  010a2066656 grep.h: remove unused "regex_t regexp" from grep_opt
 2:  0edcdb50afd =  2:  e4981fa3417 log tests: check if grep_config() is called by "log"-like cmds
 3:  e1b4b5b77e0 =  3:  59092169e55 grep tests: add missing "grep.patternType" config tests
 4:  6d91a765fd7 =  4:  331c9019a0e built-ins: trust the "prefix" from run_builtin()
 5:  844b4727ca3 =  5:  25dd327b653 grep.c: don't pass along NULL callback value
 6:  d9cf9bf5e37 !  6:  3c559ad006a grep API: call grep_config() after grep_init()
    @@ grep.c: static void std_output(struct grep_opt *opt, const void *buf, size_t siz
     -	.pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED,
     -	.colors = {
     -		[GREP_COLOR_CONTEXT] = "",
    --		[GREP_COLOR_FILENAME] = "",
    +-		[GREP_COLOR_FILENAME] = GIT_COLOR_MAGENTA,
     -		[GREP_COLOR_FUNCTION] = "",
    --		[GREP_COLOR_LINENO] = "",
    --		[GREP_COLOR_COLUMNNO] = "",
    +-		[GREP_COLOR_LINENO] = GIT_COLOR_GREEN,
    +-		[GREP_COLOR_COLUMNNO] = GIT_COLOR_GREEN,
     -		[GREP_COLOR_MATCH_CONTEXT] = GIT_COLOR_BOLD_RED,
     -		[GREP_COLOR_MATCH_SELECTED] = GIT_COLOR_BOLD_RED,
     -		[GREP_COLOR_SELECTED] = "",
    @@ grep.h: struct grep_opt {
     +	.pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED, \
     +	.colors = { \
     +		[GREP_COLOR_CONTEXT] = "", \
    -+		[GREP_COLOR_FILENAME] = "", \
    ++		[GREP_COLOR_FILENAME] = GIT_COLOR_MAGENTA, \
     +		[GREP_COLOR_FUNCTION] = "", \
    -+		[GREP_COLOR_LINENO] = "", \
    -+		[GREP_COLOR_COLUMNNO] = "", \
    ++		[GREP_COLOR_LINENO] = GIT_COLOR_GREEN, \
    ++		[GREP_COLOR_COLUMNNO] = GIT_COLOR_GREEN, \
     +		[GREP_COLOR_MATCH_CONTEXT] = GIT_COLOR_BOLD_RED, \
     +		[GREP_COLOR_MATCH_SELECTED] = GIT_COLOR_BOLD_RED, \
     +		[GREP_COLOR_SELECTED] = "", \
 7:  57ecc5c0d65 =  7:  daf873899c1 grep.h: make "grep_opt.pattern_type_option" use its enum
 8:  7dbeafde26b =  8:  62650a78ea9 grep.c: do "if (bool && memchr())" not "if (memchr() && bool)"
 9:  c6ca39b4554 !  9:  c211bb0c69d grep: simplify config parsing and option parsing
    @@ grep.h: struct grep_opt {
      	int word_regexp;
     -	int fixed;
      	int all_match;
    - #define GREP_BINARY_DEFAULT	0
    - #define GREP_BINARY_NOMATCH	1
    + 	int no_body_match;
    + 	int body_hit;
     @@ grep.h: struct grep_opt {
      	int allow_textconv;
      	int extended;
10:  b764c09d2b7 ! 10:  b52a0c11fa9 grep.[ch]: remove GREP_PATTERN_TYPE_UNSPECIFIED
    @@ grep.h: struct grep_opt {
     -	.pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED, \
      	.colors = { \
      		[GREP_COLOR_CONTEXT] = "", \
    - 		[GREP_COLOR_FILENAME] = "", \
    + 		[GREP_COLOR_FILENAME] = GIT_COLOR_MAGENTA, \

This "regex_t" in grep_opt has not been used since f9b9faf6f8a (builtin-grep: allow more than one patterns., 2006-05-02), we still use a "regex_t" for compiling regexes, but that's in the "grep_pat" struct".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.h | 1 -
 1 file changed, 1 deletion(-)

Extend the tests added in my 9df46763ef1 (log: add exhaustive tests for pattern style options & config, 2017-05-20) to check not only whether "git log" handles "grep.patternType", but also "git show" etc.

It's sufficient to check whether we match a "fixed" or a "basic" regex here to see if these codepaths correctly invoked grep_config(). We don't need to check the details of their regular expression matching as the "log" test does.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t4202-log.sh | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Extend the grep tests to assert that setting "grep.patternType=extended" followed by "grep.patternType=default" will behave as if "--basic-regexp" was provided, and not as "--extended-regexp". In a subsequent commit we'll need to treat "grep.patternType=default" as a special-case, but let's make sure we ignore it if it's being set to "default" following an earlier non-"default" "grep.patternType" setting.

Let's also test what happens when we have a sequence of "extended" followed by "default" and "fixed". In that case the "fixed" should prevail, as well as tests to check that a "grep.extendedRegexp=true" followed by a "grep.extendedRegexp=false" behaves as though "grep.extendedRegexp" wasn't provided.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t7810-grep.sh | 68 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

Change code in "builtin/grep.c" and "builtin/ls-tree.c" to trust the "prefix" passed from "run_builtin()". The "prefix" we get from setup.c is either going to be NULL or a string of length >0, never "".

So we can drop the "prefix && *prefix" checks added for "builtin/grep.c" in 0d042fecf2f (git-grep: show pathnames relative to the current directory, 2006-08-11), and for "builtin/ls-tree.c" in a69dd585fca (ls-tree: chomp leading directories when run from a subdirectory, 2005-12-23).

As seen in code in revision.c that was added in cd676a51367 (diff --relative: output paths as relative to the current subdirectory, 2008-02-12) we already have existing code that does away with this assertion.

This makes it easier to reason about a subsequent change to the "prefix_length" code in grep.c in a subsequent commit, and since we're going to the trouble of doing that let's leave behind an assert() to promise this to any future callers.

For "builtin/grep.c" it would be painful to pass the "prefix" down the callchain of:

    cmd_grep -> grep_tree -> grep_submodule -> grep_cache -> grep_oid ->
    grep_source_name

So for the code that needs it in grep_source_name() let's add a "grep_prefix" variable similar to the existing "ls_tree_prefix".

While at it let's move the code in cmd_ls_tree() around so that we assign to the "ls_tree_prefix" right after declaring the variables, and stop assigning to "prefix". We only subsequently used that variable later in the function after clobbering it. Let's just use our own "grep_prefix" instead.

Let's also add an assert() in git.c, so that we'll make this promise about the "prefix" to any current and future callers, as well as to any readers of the code.

Code history:

 * The strlen() in "grep.c" hasn't been used since 493b7a08d80 (grep:
   accept relative paths outside current working directory, 2009-09-05).

   When that code was added in 0d042fecf2f (git-grep: show pathnames
   relative to the current directory, 2006-08-11) we used the length.

   But since 493b7a08d80 we haven't used it for anything except a
   boolean check that we could have done on the "prefix" member
   itself.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/grep.c    | 13 ++++++++-----
 builtin/ls-tree.c |  2 +-
 git.c             |  1 +
 grep.c            |  4 +---
 grep.h            |  4 +---
 revision.c        |  2 +-
 6 files changed, 13 insertions(+), 13 deletions(-)