| Message ID | 9f70eeb4f04a874a2036e1d8c61f3b7ec130663a.1710781235.git.dsimic@manjaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | New config option for git-grep to include untracked files | expand |
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?
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/
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.
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.
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 ) '
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(+)