[PATCH 0/5] New config option for git-grep to include untracked files

[PATCH 0/5] New config option for git-grep to include untracked files

[Dragan Simic]

This patch series introduces new config option grep.includeUntracked,
which makes the untracked files also searched by default when git-grep(1)
is invoked, in addition to searching the tracked files.  This is quite
handy when someone expects git-grep(1) to mimic grep(1) even better, when
it comes to the selection of searched files.

Setting grep.includeUntracked to true in one's git configuration should
have no ill effects to various scripts, which presumably shouldn't rely
on expensive operations such as git-grep(1).

This series also performs some related cleanups and small improvements,
which are extracted into separate patches.

Dragan Simic (5):
  grep: perform some minor code and comment cleanups
  grep docs: describe --recurse-submodules further and improve
    formatting a bit
  grep docs: describe --no-index further
  grep: introduce new config option to include untracked files
  grep docs: describe new config option to include untracked files

 Documentation/config/grep.txt |  8 +++++++-
 Documentation/git-grep.txt    | 19 +++++++++++--------
 builtin/grep.c                | 24 +++++++++++-------------
 t/t7810-grep.sh               |  9 +++++++++
 4 files changed, 38 insertions(+), 22 deletions(-)

[PATCH 1/5] grep: perform some minor code and comment cleanups

[Dragan Simic]

Move some variable definitions around, and reflow one comment block, to
make the code a bit neater after spotting those slightly unpolished areas.
There are no functional changes to the source code.

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 builtin/grep.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 982bcfc4b1df..af89c8b5cb19 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -623,13 +623,13 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
 		     struct tree_desc *tree, struct strbuf *base, int tn_len,
 		     int check_attr)
 {
-	struct repository *repo = opt->repo;
-	int hit = 0;
+	int hit = 0, name_base_len = 0;
+	int old_baselen = base->len;
 	enum interesting match = entry_not_interesting;
+	struct repository *repo = opt->repo;
 	struct name_entry entry;
-	int old_baselen = base->len;
 	struct strbuf name = STRBUF_INIT;
-	int name_base_len = 0;
+
 	if (repo->submodule_prefix) {
 		strbuf_addstr(&name, repo->submodule_prefix);
 		name_base_len = name.len;
@@ -890,19 +890,15 @@ static int pattern_callback(const struct option *opt, const char *arg,
 
 int cmd_grep(int argc, const char **argv, const char *prefix)
 {
-	int hit = 0;
+	int hit = 0, seen_dashdash = 0, use_index = 1;
 	int cached = 0, untracked = 0, opt_exclude = -1;
-	int seen_dashdash = 0;
 	int external_grep_allowed__ignored;
+	int i, dummy, allow_revs;
 	const char *show_in_pager = NULL, *default_pager = "dummy";
 	struct grep_opt opt;
 	struct object_array list = OBJECT_ARRAY_INIT;
 	struct pathspec pathspec;
 	struct string_list path_list = STRING_LIST_INIT_DUP;
-	int i;
-	int dummy;
-	int use_index = 1;
-	int allow_revs;
 
 	struct option options[] = {
 		OPT_BOOL(0, "cached", &cached,
@@ -1059,9 +1055,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		recurse_submodules = 0;
 
 	/*
-	 * skip a -- separator; we know it cannot be
-	 * separating revisions from pathnames if
-	 * we haven't even had any patterns yet
+	 * skip a -- separator; we know it cannot be separating revisions
+	 * from pathnames if we haven't even had any patterns yet
 	 */
 	if (argc > 0 && !opt.pattern_list && !strcmp(argv[0], "--")) {
 		argv++;

[PATCH 2/5] grep docs: describe --recurse-submodules further and improve formatting a bit

[Dragan Simic]

Clarify that --recurse-submodules cannot be used together with --untracked,
and improve the formatting in a couple of places, to make it visually clear
that those are the commands or the names of configuration options.

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 Documentation/config/grep.txt | 2 +-
 Documentation/git-grep.txt    | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/config/grep.txt b/Documentation/config/grep.txt
index e521f20390ce..10041f27b0c8 100644
--- a/Documentation/config/grep.txt
+++ b/Documentation/config/grep.txt
@@ -24,5 +24,5 @@ grep.fullName::
 	If set to true, enable `--full-name` option by default.
 
 grep.fallbackToNoIndex::
-	If set to true, fall back to git grep --no-index if git grep
+	If set to true, fall back to `git grep --no-index` if `git grep`
 	is executed outside of a git repository.  Defaults to false.
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 0d0103c780af..ade69f00ebdd 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -65,8 +65,8 @@ OPTIONS
 	Recursively search in each submodule that is active and
 	checked out in the repository.  When used in combination with the
 	<tree> option the prefix of all submodule output will be the name of
-	the parent project's <tree> object. This option has no effect
-	if `--no-index` is given.
+	the parent project's <tree> object.  This option cannot be used together
+	with `--untracked`, and it has no effect if `--no-index` is specified.
 
 -a::
 --text::
@@ -178,7 +178,7 @@ providing this option will cause it to die.
 	Use \0 as the delimiter for pathnames in the output, and print
 	them verbatim. Without this option, pathnames with "unusual"
 	characters are quoted as explained for the configuration
-	variable core.quotePath (see linkgit:git-config[1]).
+	variable `core.quotePath` (see linkgit:git-config[1]).
 
 -o::
 --only-matching::

[PATCH 3/5] grep docs: describe --no-index further

[Dragan Simic]

Describe the dependency between --no-index and either of the --cached and
--untracked options, which cannot be used together.

As part of that, shuffle a couple of the options, to make the documentation
flow a bit better;  it makes more sense to describe first the options that
have something in common, and to after that describe an option that has some
dependency on the already described options.

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 Documentation/git-grep.txt | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index ade69f00ebdd..b377523381bb 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -28,7 +28,7 @@ SYNOPSIS
 	   [-f <file>] [-e] <pattern>
 	   [--and|--or|--not|(|)|-e <pattern>...]
 	   [--recurse-submodules] [--parent-basename <basename>]
-	   [ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | <tree>...]
+	   [ [--[no-]exclude-standard] [--cached | --untracked | --no-index] | <tree>...]
 	   [--] [<pathspec>...]
 
 DESCRIPTION
@@ -45,13 +45,15 @@ OPTIONS
 	Instead of searching tracked files in the working tree, search
 	blobs registered in the index file.
 
---no-index::
-	Search files in the current directory that is not managed by Git.
-
 --untracked::
 	In addition to searching in the tracked files in the working
 	tree, search also in untracked files.
 
+--no-index::
+	Search files in the current directory that is not managed by Git.
+	This option cannot be used together with `--cached` or `--untracked`.
+	See also `grep.fallbackToNoIndex` in CONFIGURATION below.
+
 --no-exclude-standard::
 	Also search in ignored files by not honoring the `.gitignore`
 	mechanism. Only useful with `--untracked`.

[PATCH 4/5] grep: introduce new config option to include untracked files

[Dragan Simic]

Add new configuration option grep.includeUntracked that enables --untracked
option by default.  This pretty much follows the logic established by the
already existing configuration option grep.fallbackToNoIndex, while also
respecting the dependencies of the --untracked option.

Also add a few automated tests to the t7810, to cover the new configuration
option by replicating the already existing tests for --untracked.

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 builtin/grep.c  | 3 +++
 t/t7810-grep.sh | 9 +++++++++
 2 files changed, 12 insertions(+)

diff --git a/builtin/grep.c b/builtin/grep.c
index af89c8b5cb19..71d94126fb6e 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1041,6 +1041,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		the_repository->settings.command_requires_full_index = 0;
 	}
 
+	if (use_index && !cached)
+		git_config_get_bool("grep.includeuntracked", &untracked);
+
 	if (use_index && !startup_info->have_repository) {
 		int fallback = 0;
 		git_config_get_bool("grep.fallbacktonoindex", &fallback);
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 875dcfd98f3a..de93936d585f 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -1298,20 +1298,29 @@ test_expect_success 'inside git repository but with --no-index' '
 		git grep --untracked o >../actual.unignored &&
 		test_cmp ../expect.unignored ../actual.unignored &&
 
+		git -c grep.includeUntracked=true grep o >../actual.unignored &&
+		test_cmp ../expect.unignored ../actual.unignored &&
+
 		git grep --no-index o >../actual.full &&
 		test_cmp ../expect.full ../actual.full &&
 
+		git -c grep.includeUntracked=true grep --no-index o >../actual.full &&
+		test_cmp ../expect.full ../actual.full &&
+
 		git grep --no-index --exclude-standard o >../actual.unignored &&
 		test_cmp ../expect.unignored ../actual.unignored &&
 
 		cd sub &&
 		test_must_fail git grep o >../../actual.sub &&
 		test_must_be_empty ../../actual.sub &&
 
 		git grep --no-index o >../../actual.sub &&
 		test_cmp ../../expect.sub ../../actual.sub &&
 
 		git grep --untracked o >../../actual.sub &&
+		test_cmp ../../expect.sub ../../actual.sub &&
+
+		git -c grep.includeUntracked=true grep o >../../actual.sub &&
 		test_cmp ../../expect.sub ../../actual.sub
 	)
 '

[PATCH 5/5] grep docs: describe new config option to include untracked files

[Dragan Simic]

Describe the new configuration option grep.includeUntracked, including the
dependencies with the already existing options, and the conditions that
make this option ignored, which allows other options to be used while the
new option is enabled in one's git configuration file.

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 Documentation/config/grep.txt | 6 ++++++
 Documentation/git-grep.txt    | 3 ++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/config/grep.txt b/Documentation/config/grep.txt
index 10041f27b0c8..32f086997596 100644
--- a/Documentation/config/grep.txt
+++ b/Documentation/config/grep.txt
@@ -23,6 +23,12 @@ grep.threads::
 grep.fullName::
 	If set to true, enable `--full-name` option by default.
 
+grep.includeUntracked::
+	If set to true, enable `--untracked` option by default, to search also
+	in untracked files, in addition to searching in the tracked files in the
+	working tree.  If `--cached` or `--no-index` is specified, this option is
+	ignored.  Also, it cannot be enabled together with `submodule.recurse`.
+
 grep.fallbackToNoIndex::
 	If set to true, fall back to `git grep --no-index` if `git grep`
 	is executed outside of a git repository.  Defaults to false.
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index b377523381bb..af5f6572df16 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -47,7 +47,8 @@ OPTIONS
 
 --untracked::
 	In addition to searching in the tracked files in the working
-	tree, search also in untracked files.
+	tree, search also in untracked files.  See also `grep.includeUntracked`
+	in CONFIGURATION below.
 
 --no-index::
 	Search files in the current directory that is not managed by Git.

Re: [PATCH 1/5] grep: perform some minor code and comment cleanups

[Eric Sunshine]

On Mon, Mar 18, 2024 at 1:04 PM Dragan Simic <dsimic@manjaro.org> wrote:
> Move some variable definitions around, and reflow one comment block, to
> make the code a bit neater after spotting those slightly unpolished areas.
> There are no functional changes to the source code.
>
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
> diff --git a/builtin/grep.c b/builtin/grep.c
> @@ -623,13 +623,13 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
> -       struct repository *repo = opt->repo;
> -       int hit = 0;
> +       int hit = 0, name_base_len = 0;
> +       int old_baselen = base->len;
>         enum interesting match = entry_not_interesting;
> +       struct repository *repo = opt->repo;
>         struct name_entry entry;
> -       int old_baselen = base->len;
>         struct strbuf name = STRBUF_INIT;
> -       int name_base_len = 0;
> @@ -890,19 +890,15 @@ static int pattern_callback(const struct option *opt, const char *arg,
> -       int hit = 0;
> +       int hit = 0, seen_dashdash = 0, use_index = 1;
>         int cached = 0, untracked = 0, opt_exclude = -1;
> -       int seen_dashdash = 0;
>         int external_grep_allowed__ignored;
> +       int i, dummy, allow_revs;
>         const char *show_in_pager = NULL, *default_pager = "dummy";
>         struct grep_opt opt;
>         struct object_array list = OBJECT_ARRAY_INIT;
>         struct pathspec pathspec;
>         struct string_list path_list = STRING_LIST_INIT_DUP;
> -       int i;
> -       int dummy;
> -       int use_index = 1;
> -       int allow_revs;

It's entirely subjective, of course, so no right-or-wrong answer, but
I personally do not find that this change improves code quality or
readability.

With my reviewer hat on, I spent an inordinate amount of time staring
at this change trying to locate each variable's new location to verify
that no initializers were dropped and that the declared type hadn't
changed. Taking into consideration that reviewers are a limited
resource on this project, I'd probably have skipped this patch
altogether if I were doing this series unless these changes concretely
help a subsequent patch.

Re: [PATCH 2/5] grep docs: describe --recurse-submodules further and improve formatting a bit

[Eric Sunshine]

On Mon, Mar 18, 2024 at 1:04 PM Dragan Simic <dsimic@manjaro.org> wrote:
> Clarify that --recurse-submodules cannot be used together with --untracked,
> and improve the formatting in a couple of places, to make it visually clear
> that those are the commands or the names of configuration options.
>
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
> diff --git a/Documentation/config/grep.txt b/Documentation/config/grep.txt
> @@ -24,5 +24,5 @@ grep.fullName::
>  grep.fallbackToNoIndex::
> -       If set to true, fall back to git grep --no-index if git grep
> +       If set to true, fall back to `git grep --no-index` if `git grep`

Good.

> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> @@ -65,8 +65,8 @@ OPTIONS
>         <tree> option the prefix of all submodule output will be the name of
> -       the parent project's <tree> object. This option has no effect
> -       if `--no-index` is given.
> +       the parent project's <tree> object.  This option cannot be used together
> +       with `--untracked`, and it has no effect if `--no-index` is specified.

I believe that there is a patch series currently in-flight which is
re-styling in-prose <foo> placeholders as _<foo>_, so you may want to
make that change as well while you're touching this.

Re: [PATCH 1/5] grep: perform some minor code and comment cleanups

[Dragan Simic]

On 2024-03-18 20:59, Eric Sunshine wrote:
> On Mon, Mar 18, 2024 at 1:04 PM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> Move some variable definitions around, and reflow one comment block, 
>> to
>> make the code a bit neater after spotting those slightly unpolished 
>> areas.
>> There are no functional changes to the source code.
>> 
>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>> ---
>> diff --git a/builtin/grep.c b/builtin/grep.c
>> @@ -623,13 +623,13 @@ static int grep_tree(struct grep_opt *opt, const 
>> struct pathspec *pathspec,
>> -       struct repository *repo = opt->repo;
>> -       int hit = 0;
>> +       int hit = 0, name_base_len = 0;
>> +       int old_baselen = base->len;
>>         enum interesting match = entry_not_interesting;
>> +       struct repository *repo = opt->repo;
>>         struct name_entry entry;
>> -       int old_baselen = base->len;
>>         struct strbuf name = STRBUF_INIT;
>> -       int name_base_len = 0;
>> @@ -890,19 +890,15 @@ static int pattern_callback(const struct option 
>> *opt, const char *arg,
>> -       int hit = 0;
>> +       int hit = 0, seen_dashdash = 0, use_index = 1;
>>         int cached = 0, untracked = 0, opt_exclude = -1;
>> -       int seen_dashdash = 0;
>>         int external_grep_allowed__ignored;
>> +       int i, dummy, allow_revs;
>>         const char *show_in_pager = NULL, *default_pager = "dummy";
>>         struct grep_opt opt;
>>         struct object_array list = OBJECT_ARRAY_INIT;
>>         struct pathspec pathspec;
>>         struct string_list path_list = STRING_LIST_INIT_DUP;
>> -       int i;
>> -       int dummy;
>> -       int use_index = 1;
>> -       int allow_revs;
> 
> It's entirely subjective, of course, so no right-or-wrong answer, but
> I personally do not find that this change improves code quality or
> readability.
> 
> With my reviewer hat on, I spent an inordinate amount of time staring
> at this change trying to locate each variable's new location to verify
> that no initializers were dropped and that the declared type hadn't
> changed. Taking into consideration that reviewers are a limited
> resource on this project, I'd probably have skipped this patch
> altogether if I were doing this series unless these changes concretely
> help a subsequent patch.

Oh, I'm fully aware that the reviewers are a limited resource, and I do
agree that all this is subjective.  Though, I believe it makes the code
look nicer, which is the only reason why I performed and submitted those
changes in the first place.

Though, maybe it would've been better if I submitted these changes as
a separate patch, instead as part of this series.

Re: [PATCH 2/5] grep docs: describe --recurse-submodules further and improve formatting a bit

[Dragan Simic]

On 2024-03-18 21:02, Eric Sunshine wrote:
> On Mon, Mar 18, 2024 at 1:04 PM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> Clarify that --recurse-submodules cannot be used together with 
>> --untracked,
>> and improve the formatting in a couple of places, to make it visually 
>> clear
>> that those are the commands or the names of configuration options.
>> 
>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>> ---
>> diff --git a/Documentation/config/grep.txt 
>> b/Documentation/config/grep.txt
>> @@ -24,5 +24,5 @@ grep.fullName::
>>  grep.fallbackToNoIndex::
>> -       If set to true, fall back to git grep --no-index if git grep
>> +       If set to true, fall back to `git grep --no-index` if `git 
>> grep`
> 
> Good.
> 
>> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
>> @@ -65,8 +65,8 @@ OPTIONS
>>         <tree> option the prefix of all submodule output will be the 
>> name of
>> -       the parent project's <tree> object. This option has no effect
>> -       if `--no-index` is given.
>> +       the parent project's <tree> object.  This option cannot be 
>> used together
>> +       with `--untracked`, and it has no effect if `--no-index` is 
>> specified.
> 
> I believe that there is a patch series currently in-flight which is
> re-styling in-prose <foo> placeholders as _<foo>_, so you may want to
> make that change as well while you're touching this.

Thanks for the notice.  I'll add that to the v2 of this series, if
there will be other reasons for the v2.

Re: [PATCH 0/5] New config option for git-grep to include untracked files

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

> This patch series introduces new config option grep.includeUntracked,
> which makes the untracked files also searched by default when git-grep(1)
> is invoked, in addition to searching the tracked files.

Hmph.  I am moderately negative on any configuration that screws
with the default haystack from which needle is sought for.

I may often do "git grep --cached" but that does not mean I would
welcome an configuration option to make "git grep" search in the
index even when the request by the user does not have "--cached".

Inclusion of untracked sources in a sense is even worse, especially
when an unsuspecting "git grep" user (or a script) fully expects
that any paths found in the output are to be found in "git ls-files
-s" output but when you stray into a repository with the
configuration set, that expectation suddelnly gets broken.

So, I dunno.

Re: [PATCH 1/5] grep: perform some minor code and comment cleanups

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> It's entirely subjective, of course, so no right-or-wrong answer, but
> I personally do not find that this change improves code quality or
> readability.

I agree that this is entirely subjective.  To those who wrote these
variable decls and inits, what they wrote was the most readable,
wasn't it?  It probably falls into the "to some readers the existing
code may not be perfect, but once it is written, it is not worth a
patch noise to fix it" category.

> With my reviewer hat on, I spent an inordinate amount of time staring
> at this change trying to locate each variable's new location to verify
> that no initializers were dropped and that the declared type hadn't
> changed.

It is true that "cleaning up, no behaviour changes intended" patches
are unpleasant to review.  They are boring to read, and the risk of
breakage due to mistake is unnecessary and severe.

But if the result is objectively better, such a one-time cost may be
worth it.  We are investing into the better future.  For example, we
may have an unsorted mess of an enum definition, and we do
appreciate in the longer run, such a definition were "more or less"
sorted within the constraint of some other criteria (like, "errors
get negative value").  If the enum is a huge one, it may need some
careful reviewing to verify such a change that turns the unsorted
mess into a sorted nice list, but the cost of doing so may be
justified.

Does the change in this patch qualify as "objectively better"?  I
dunno.

Thanks.

Re: [PATCH 3/5] grep docs: describe --no-index further

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

> ---no-index::
> -	Search files in the current directory that is not managed by Git.
> -
>  --untracked::
>  	In addition to searching in the tracked files in the working
>  	tree, search also in untracked files.
>  
> +--no-index::
> +	Search files in the current directory that is not managed by Git.
> +	This option cannot be used together with `--cached` or `--untracked`.
> +	See also `grep.fallbackToNoIndex` in CONFIGURATION below.

Hmph, this is not the fault of this patch, but the description is
iffy.  You can run "git grep --no-index" inside a directory that is
managed by Git, and it behaves as if you gave --untracked, if I am
not mistaken.

What "--no-index" does is to pretend that there is no system called
Git and work as if it were a strange implementation of "grep -r".
The reader should be taught to understand the mode as such, because
that understanding will apply whether the current directory happens
to be part of a working tree managed by git, or not under control by
git repository anywhere.

There is no tracked or untracked or managed or anything like that,
as we are pretending that there is no git, so it falls naturally
that --cached or --untracked would not work.

And from that point of view, swapping the order of "--no-index" and
"--untracked" in this patch does make sense.  All other options that
specify which haystack to find the needle in are all about git;
"--no-index" truly is an oddball that pretends that we live in a
world without git, and as an oddball, we should move the description
out from where it does not belong.  It might also make sense to
rethink where `--recurse-submodules` sits in the list of options
while at it, as it also is an option that affects which haystack the
search for the needle is carried out.

>  --no-exclude-standard::
>  	Also search in ignored files by not honoring the `.gitignore`
>  	mechanism. Only useful with `--untracked`.

Re: [PATCH 4/5] grep: introduce new config option to include untracked files

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

> Add new configuration option grep.includeUntracked that enables --untracked
> option by default.  This pretty much follows the logic established by the
> already existing configuration option grep.fallbackToNoIndex, while also
> respecting the dependencies of the --untracked option.
>
> Also add a few automated tests to the t7810, to cover the new configuration

Do we have any non-automated tests in t7810?

> option by replicating the already existing tests for --untracked.
>
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
>  builtin/grep.c  | 3 +++
>  t/t7810-grep.sh | 9 +++++++++
>  2 files changed, 12 insertions(+)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index af89c8b5cb19..71d94126fb6e 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -1041,6 +1041,9 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  		the_repository->settings.command_requires_full_index = 0;
>  	}
>  
> +	if (use_index && !cached)
> +		git_config_get_bool("grep.includeuntracked", &untracked);

Can this ever return an error?  E.g.

	[grep] includeuntracked = "not really"

How badly would setting this configuration variable break third
party tools that assume their "git grep" invocation without the
"--untracked" option would not yield hits from untracked files?

Re: [PATCH 0/5] New config option for git-grep to include untracked files

[Dragan Simic]

Hello Junio,

On 2024-03-19 01:21, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>> This patch series introduces new config option grep.includeUntracked,
>> which makes the untracked files also searched by default when 
>> git-grep(1)
>> is invoked, in addition to searching the tracked files.
> 
> Hmph.  I am moderately negative on any configuration that screws
> with the default haystack from which needle is sought for.
> 
> I may often do "git grep --cached" but that does not mean I would
> welcome an configuration option to make "git grep" search in the
> index even when the request by the user does not have "--cached".
> 
> Inclusion of untracked sources in a sense is even worse, especially
> when an unsuspecting "git grep" user (or a script) fully expects
> that any paths found in the output are to be found in "git ls-files
> -s" output but when you stray into a repository with the
> configuration set, that expectation suddelnly gets broken.

Hmm...  Those are valid concerns.  For example, I'd also be against
another configuration option that would make it possible to enable
--cached by default, for example, because it could easily mess the
things up really bad.

However, I think that the usability of this new configuration option
outweighs the possible issues it could cause to some users.  For
example, I quite often find myself in need to specify --untracked
while running git-grep(1), to see what's found in the code changes
I haven't staged yet, so I find this new configuration very useful.

Of course, I already have a few aliases defined for grep operations,
but defining another one for "grep --untracked" simply doesn't fit
into my alias scheme.  Obviously, that's why I implemented this new
option, :) hoping that it would be usable to other people, too.

I'm not sure how many users could be affected by the possible negative
effects of this configuration option, by using a specific mix of git
operations, but I think it would be quite useful.

Re: [PATCH 1/5] grep: perform some minor code and comment cleanups

[Dragan Simic]

On 2024-03-19 01:32, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
>> It's entirely subjective, of course, so no right-or-wrong answer, but
>> I personally do not find that this change improves code quality or
>> readability.
> 
> I agree that this is entirely subjective.  To those who wrote these
> variable decls and inits, what they wrote was the most readable,
> wasn't it?  It probably falls into the "to some readers the existing
> code may not be perfect, but once it is written, it is not worth a
> patch noise to fix it" category.

There's no doubt that it was the most readable form to the people who
wrote the code, which was some time ago, but the time inevitably passes
and the surrounding code changes over time, maybe even some new coding
guidelines become introduced, etc.

Things inevitably change, that's all I'm trying to say.

>> With my reviewer hat on, I spent an inordinate amount of time staring
>> at this change trying to locate each variable's new location to verify
>> that no initializers were dropped and that the declared type hadn't
>> changed.
> 
> It is true that "cleaning up, no behaviour changes intended" patches
> are unpleasant to review.  They are boring to read, and the risk of
> breakage due to mistake is unnecessary and severe.
> 
> But if the result is objectively better, such a one-time cost may be
> worth it.  We are investing into the better future.  For example, we
> may have an unsorted mess of an enum definition, and we do
> appreciate in the longer run, such a definition were "more or less"
> sorted within the constraint of some other criteria (like, "errors
> get negative value").  If the enum is a huge one, it may need some
> careful reviewing to verify such a change that turns the unsorted
> mess into a sorted nice list, but the cost of doing so may be
> justified.

I fully agree that reviewing code-cleanup patches it usually boring
and often taxing.  I mean, why change something that has served us
well for years, just to make it look nicer in someone's eyes?  What
does even "nicer" mean to everyone?

Well, I sometimes look at the code as if it were a beautiful painting.
To some people, it doesn't matter if a painting has some rough areas,
as long as it can be hung on a wall.  To them, it's just a square thing.
Though, to some other people, spotting such rough areas is what makes
the painting less beautiful to them.  Paintings usually cannot be fixed
easily, but fixing the code is much easier.

Of course, "nicer" is very hard to define, but I believe that, in the
domain of computing, it could be partially defined as "more consistent
and flowing better".  That's what I tried to achieve with this patch,
and I hope I've managed to convey my viewpoint.

> Does the change in this patch qualify as "objectively better"?  I
> dunno.

I'd say that these changes qualify as "semi-objectively nicer", but
surely not as "objectively better".  For any changes to be objectively
better, they'd need to introduce some functional changes to the code,
while a well-performed code cleanup actually introduces no functional
changes.

Re: [PATCH 3/5] grep docs: describe --no-index further

[Dragan Simic]

On 2024-03-19 01:55, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>> ---no-index::
>> -	Search files in the current directory that is not managed by Git.
>> -
>>  --untracked::
>>  	In addition to searching in the tracked files in the working
>>  	tree, search also in untracked files.
>> 
>> +--no-index::
>> +	Search files in the current directory that is not managed by Git.
>> +	This option cannot be used together with `--cached` or 
>> `--untracked`.
>> +	See also `grep.fallbackToNoIndex` in CONFIGURATION below.
> 
> Hmph, this is not the fault of this patch, but the description is
> iffy.  You can run "git grep --no-index" inside a directory that is
> managed by Git, and it behaves as if you gave --untracked, if I am
> not mistaken.
> 
> What "--no-index" does is to pretend that there is no system called
> Git and work as if it were a strange implementation of "grep -r".
> The reader should be taught to understand the mode as such, because
> that understanding will apply whether the current directory happens
> to be part of a working tree managed by git, or not under control by
> git repository anywhere.
> 
> There is no tracked or untracked or managed or anything like that,
> as we are pretending that there is no git, so it falls naturally
> that --cached or --untracked would not work.
> 
> And from that point of view, swapping the order of "--no-index" and
> "--untracked" in this patch does make sense.  All other options that
> specify which haystack to find the needle in are all about git;
> "--no-index" truly is an oddball that pretends that we live in a
> world without git, and as an oddball, we should move the description
> out from where it does not belong.  It might also make sense to
> rethink where `--recurse-submodules` sits in the list of options
> while at it, as it also is an option that affects which haystack the
> search for the needle is carried out.

Thank you very much for your detailed review, as always.  I'll try to
incorporate all your remarks into the next version of this patch.

Re: [PATCH 4/5] grep: introduce new config option to include untracked files

[Dragan Simic]

On 2024-03-19 01:58, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
>> Add new configuration option grep.includeUntracked that enables 
>> --untracked
>> option by default.  This pretty much follows the logic established by 
>> the
>> already existing configuration option grep.fallbackToNoIndex, while 
>> also
>> respecting the dependencies of the --untracked option.
>> 
>> Also add a few automated tests to the t7810, to cover the new 
>> configuration
> 
> Do we have any non-automated tests in t7810?

Good point, will be removed in the v2, if we get there.  Tying 
"automated"
to "test" is just an old habit of mine.

>> option by replicating the already existing tests for --untracked.
>> 
>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>> ---
>>  builtin/grep.c  | 3 +++
>>  t/t7810-grep.sh | 9 +++++++++
>>  2 files changed, 12 insertions(+)
>> 
>> diff --git a/builtin/grep.c b/builtin/grep.c
>> index af89c8b5cb19..71d94126fb6e 100644
>> --- a/builtin/grep.c
>> +++ b/builtin/grep.c
>> @@ -1041,6 +1041,9 @@ int cmd_grep(int argc, const char **argv, const 
>> char *prefix)
>>  		the_repository->settings.command_requires_full_index = 0;
>>  	}
>> 
>> +	if (use_index && !cached)
>> +		git_config_get_bool("grep.includeuntracked", &untracked);
> 
> Can this ever return an error?  E.g.
> 
> 	[grep] includeuntracked = "not really"
> 
> How badly would setting this configuration variable break third
> party tools that assume their "git grep" invocation without the
> "--untracked" option would not yield hits from untracked files?

After a brief inspection of the code in cache.c, git_config_get_bool()
always returns either 0 or 1, so we should be fine.  Thus, any
strangeness in a configuration file would end up not enabling
this option.

As I already explained in my earlier response, [1] I think that
the usability of this option outweighs the possible issues it may
cause to some users.

[1] 
https://lore.kernel.org/git/c68a6d94bb02e5d9aa2f81bee022baa8@manjaro.org/

Re: [PATCH 4/5] grep: introduce new config option to include untracked files

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

>>> +	if (use_index && !cached)
>>> +		git_config_get_bool("grep.includeuntracked", &untracked);
>> Can this ever return an error?  E.g.
>> 	[grep] includeuntracked = "not really"
>
> After a brief inspection of the code in cache.c, git_config_get_bool()
> always returns either 0 or 1, so we should be fine.  Thus, any
> strangeness in a configuration file would end up not enabling
> this option.

If that were the case, then it is not "fine".

When the user triggered an operation which *requires* us to parse
and interpret the meaning of an entry in their configuration file
correctly in order to carry it out, and if that entry has a value
that we do not consider valid, we should notice and complain, before
saying "Nah, this I do not understand, so I'll do one of the two
things I would have done if the value were understandable and would
not tell the user which one I did".

What makes it fine int his case is that git_config_get_bool() dies
when the given value is not a Boolean ;-).  The returned value from
the function can be used to tell if the variable does not exist and
the caller should decide to stuff some default value to &untracked
but in this case you do not need to.

Re: [PATCH 4/5] grep: introduce new config option to include untracked files

[Dragan Simic]

On 2024-03-19 15:32, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>>>> +	if (use_index && !cached)
>>>> +		git_config_get_bool("grep.includeuntracked", &untracked);
>>> Can this ever return an error?  E.g.
>>> 	[grep] includeuntracked = "not really"
>> 
>> After a brief inspection of the code in cache.c, git_config_get_bool()
>> always returns either 0 or 1, so we should be fine.  Thus, any
>> strangeness in a configuration file would end up not enabling
>> this option.
> 
> If that were the case, then it is not "fine".
> 
> When the user triggered an operation which *requires* us to parse
> and interpret the meaning of an entry in their configuration file
> correctly in order to carry it out, and if that entry has a value
> that we do not consider valid, we should notice and complain, before
> saying "Nah, this I do not understand, so I'll do one of the two
> things I would have done if the value were understandable and would
> not tell the user which one I did".
> 
> What makes it fine int his case is that git_config_get_bool() dies
> when the given value is not a Boolean ;-).  The returned value from
> the function can be used to tell if the variable does not exist and
> the caller should decide to stuff some default value to &untracked
> but in this case you do not need to.

I'm sorry for not being meticulous enough in my previous response.
What made me not pay enough attention is that it should all be already
covered properly with the already existing mechanisms for parsing the
git configuration files.

In other words, if invoking git_config_get_bool() over a user's
garbled git configuration file could cause any issues, that would've
been another, pretty much unrelated bug.

Re: [PATCH 0/5] New config option for git-grep to include untracked files

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

> However, I think that the usability of this new configuration option
> outweighs the possible issues it could cause to some users.

"Screw others who share my preference and want to set this variable,
but use third-party tools and scripts that will get broken with this
change, because I do not use them" is what we try not to say around
here.

> For example, I quite often find myself in need to specify
> --untracked while running git-grep(1), to see what's found in the
> code changes I haven't staged yet, so I find this new
> configuration very useful.

The feature (not the command line option, but the behaviour that is
triggered by it) surely may be useful.  Otherwise we wouldn't have
added it.  As I already said, I often find myself doing "--cached".
But that does not make it a good idea to invent a new configuration
variable and set "grep.source=cached".

Re: [PATCH 0/5] New config option for git-grep to include untracked files

[Dragan Simic]

On 2024-03-19 18:43, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>> However, I think that the usability of this new configuration option
>> outweighs the possible issues it could cause to some users.
> 
> "Screw others who share my preference and want to set this variable,
> but use third-party tools and scripts that will get broken with this
> change, because I do not use them" is what we try not to say around
> here.

Well, I didn't mean it that way, but I get your point.  Basically,
this series cannot be merged, so I'll extract the cleanup patches
from it, improve them according to the earlier discussions, and
submit them separately.

>> For example, I quite often find myself in need to specify
>> --untracked while running git-grep(1), to see what's found in the
>> code changes I haven't staged yet, so I find this new
>> configuration very useful.
> 
> The feature (not the command line option, but the behaviour that is
> triggered by it) surely may be useful.  Otherwise we wouldn't have
> added it.  As I already said, I often find myself doing "--cached".
> But that does not make it a good idea to invent a new configuration
> variable and set "grep.source=cached".

I see.