threads / patch / 61147

patch, 5 partsgrep docs: describe --no-index further

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

## tl;dr

22 messages between Mar 18, 2024 and Mar 19, 2024. Diffs are folded; open one to read it.

replies: 21people: 3as markdown or json

Dragan Simic· Mar 18, 2024, 17:03 UTC · lore

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

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(-)
Dragan Simic· Mar 18, 2024, 17:03 UTC · re: Dragan Simic · lore

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(-)
Show changes to Documentation/git-grep.txt +6 −3
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`.
Junio C Hamano· Mar 19, 2024, 00:55 UTC · re: Dragan Simic · lore

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

Dragan Simic <dsimic@manjaro.org> writes:
Show 11 quoted lines
> ---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`.
Dragan Simic· Mar 19, 2024, 05:37 UTC · re: Junio C Hamano · lore

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

On 2024-03-19 01:55, Junio C Hamano wrote:
Show 40 quoted lines
> 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.

Dragan Simic· Mar 18, 2024, 17:03 UTC · re: Dragan Simic · lore

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

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(+)
Show changes to 2 files +12 −0

builtin/grep.c, t/t7810-grep.sh

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
 	)
 '
Junio C Hamano· Mar 19, 2024, 00:58 UTC · re: Dragan Simic · lore

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

Dragan Simic <dsimic@manjaro.org> writes:
Show 6 quoted lines
> 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?
Show 18 quoted lines
> 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?

Dragan Simic· Mar 19, 2024, 05:47 UTC · re: Junio C Hamano · lore

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

On 2024-03-19 01:58, Junio C Hamano wrote:
Show 13 quoted lines
> 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.

Show 27 quoted lines
>> 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/

Junio C Hamano· Mar 19, 2024, 14:32 UTC · re: Dragan Simic · lore

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

Dragan Simic <dsimic@manjaro.org> writes:
Show 9 quoted lines
>>> +	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.

Dragan Simic· Mar 19, 2024, 14:52 UTC · re: Junio C Hamano · lore

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

On 2024-03-19 15:32, Junio C Hamano wrote:
Show 27 quoted lines
> 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.

Dragan Simic· Mar 18, 2024, 17:03 UTC · re: Dragan Simic · lore

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

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(-)
Show changes to 2 files +4 −4

Documentation/config/grep.txt, Documentation/git-grep.txt

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::
Eric Sunshine· Mar 18, 2024, 20:02 UTC · re: Dragan Simic · lore

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

On Mon, Mar 18, 2024 at 1:04 PM Dragan Simic <dsimic@manjaro.org> wrote:
Show 11 quoted lines
> 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.
Show 7 quoted lines
> 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.

Dragan Simic· Mar 18, 2024, 22:14 UTC · re: Eric Sunshine · lore

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

On 2024-03-18 21:02, Eric Sunshine wrote:
Show 34 quoted lines
> 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.

Dragan Simic· Mar 18, 2024, 17:03 UTC · re: Dragan Simic · lore

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

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(-)
Show changes to builtin/grep.c +8 −13
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++;
Eric Sunshine· Mar 18, 2024, 19:59 UTC · re: Dragan Simic · lore

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

On Mon, Mar 18, 2024 at 1:04 PM Dragan Simic <dsimic@manjaro.org> wrote:
Show 34 quoted lines
> 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.

Dragan Simic· Mar 18, 2024, 22:03 UTC · re: Eric Sunshine · lore

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

On 2024-03-18 20:59, Eric Sunshine wrote:
Show 52 quoted lines
> 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.

Junio C Hamano· Mar 19, 2024, 00:32 UTC · re: Eric Sunshine · lore

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

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.
Dragan Simic· Mar 19, 2024, 05:33 UTC · re: Junio C Hamano · lore

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

On 2024-03-19 01:32, Junio C Hamano wrote:
Show 11 quoted lines
> 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.
Show 18 quoted lines
>> 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.

Dragan Simic· Mar 18, 2024, 17:03 UTC · re: Dragan Simic · lore

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

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(-)
Show changes to 2 files +8 −1

Documentation/config/grep.txt, Documentation/git-grep.txt

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.
Junio C Hamano· Mar 19, 2024, 00:21 UTC · re: Dragan Simic · lore

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

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.
Dragan Simic· Mar 19, 2024, 05:09 UTC · re: Junio C Hamano · lore

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

Hello Junio,
On 2024-03-19 01:21, Junio C Hamano wrote:
Show 19 quoted lines
> 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.

Junio C Hamano· Mar 19, 2024, 17:43 UTC · re: Dragan Simic · lore

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

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".

Dragan Simic· Mar 19, 2024, 17:48 UTC · re: Junio C Hamano · lore

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

On 2024-03-19 18:43, Junio C Hamano wrote:
Show 9 quoted lines
> 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.

Show 10 quoted lines
>> 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.

← back to recent threads