[PATCH] grep: add configuration variables for --heading

[PATCH] grep: add configuration variables for --heading

[Simon Ser]

There are already configuration variables for -n and --column. Add one
for --heading, allowing users to customize the default behaviour.

Signed-off-by: Simon Ser <contact@emersion.fr>
---
 Documentation/git-grep.txt | 3 +++
 grep.c                     | 5 +++++
 2 files changed, 8 insertions(+)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index a7f9bc99eaf1..ed4f05d885a2 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -47,6 +47,9 @@ grep.lineNumber::
 grep.column::
 	If set to true, enable the `--column` option by default.
 
+grep.heading::
+	If set to true, enable the `--heading` option by default.
+
 grep.patternType::
 	Set the default matching behavior. Using a value of 'basic', 'extended',
 	'fixed', or 'perl' will enable the `--basic-regexp`, `--extended-regexp`,
diff --git a/grep.c b/grep.c
index 13232a904aca..4549bc722650 100644
--- a/grep.c
+++ b/grep.c
@@ -133,6 +133,10 @@ int grep_config(const char *var, const char *value, void *cb)
 		opt->columnnum = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "grep.heading")) {
+		opt->heading = git_config_bool(var, value);
+		return 0;
+	}
 
 	if (!strcmp(var, "grep.fullname")) {
 		opt->relative = !git_config_bool(var, value);
@@ -199,6 +203,7 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
 	opt->pattern_type_option = def->pattern_type_option;
 	opt->linenum = def->linenum;
 	opt->columnnum = def->columnnum;
+	opt->heading = def->heading;
 	opt->max_depth = def->max_depth;
 	opt->pathname = def->pathname;
 	opt->relative = def->relative;
-- 
2.26.2

Re: [PATCH] grep: add configuration variables for --heading

[Eric Sunshine]

On Tue, May 26, 2020 at 4:37 AM Simon Ser <contact@emersion.fr> wrote:
> There are already configuration variables for -n and --column. Add one
> for --heading, allowing users to customize the default behaviour.
>
> Signed-off-by: Simon Ser <contact@emersion.fr>
> ---
>  Documentation/git-grep.txt | 3 +++
>  grep.c                     | 5 +++++
>  2 files changed, 8 insertions(+)

To round this out, adding a test -- probably in t7810-grep.sh -- would
be a good idea.

[PATCH v2] grep: add configuration variables for --heading

[Simon Ser]

There are already configuration variables for -n and --column. Add one
for --heading, allowing users to customize the default behaviour.

Signed-off-by: Simon Ser <contact@emersion.fr>
---

v2: add a test

 Documentation/git-grep.txt | 3 +++
 grep.c                     | 5 +++++
 t/t7810-grep.sh            | 5 +++++
 3 files changed, 13 insertions(+)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index a7f9bc99eaf1..ed4f05d885a2 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -47,6 +47,9 @@ grep.lineNumber::
 grep.column::
 	If set to true, enable the `--column` option by default.
 
+grep.heading::
+	If set to true, enable the `--heading` option by default.
+
 grep.patternType::
 	Set the default matching behavior. Using a value of 'basic', 'extended',
 	'fixed', or 'perl' will enable the `--basic-regexp`, `--extended-regexp`,
diff --git a/grep.c b/grep.c
index 13232a904aca..4549bc722650 100644
--- a/grep.c
+++ b/grep.c
@@ -133,6 +133,10 @@ int grep_config(const char *var, const char *value, void *cb)
 		opt->columnnum = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "grep.heading")) {
+		opt->heading = git_config_bool(var, value);
+		return 0;
+	}
 
 	if (!strcmp(var, "grep.fullname")) {
 		opt->relative = !git_config_bool(var, value);
@@ -199,6 +203,7 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
 	opt->pattern_type_option = def->pattern_type_option;
 	opt->linenum = def->linenum;
 	opt->columnnum = def->columnnum;
+	opt->heading = def->heading;
 	opt->max_depth = def->max_depth;
 	opt->pathname = def->pathname;
 	opt->relative = def->relative;
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 991d5bd9c03f..1acc6fe89c51 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -1556,6 +1556,11 @@ test_expect_success 'grep --heading' '
 	test_cmp expected actual
 '
 
+test_expect_success 'grep with grep.heading=true' '
+	git -c grep.heading=true grep -e char -e lo_w hello.c hello_world >actual &&
+	test_cmp expected actual
+'
+
 cat >expected <<EOF
 <BOLD;GREEN>hello.c<RESET>
 4:int main(int argc, const <BLACK;BYELLOW>char<RESET> **argv)
-- 
2.26.2

Re: [PATCH v2] grep: add configuration variables for --heading

[Junio C Hamano]

Simon Ser <contact@emersion.fr> writes:

> There are already configuration variables for -n and --column. Add one
> for --heading, allowing users to customize the default behaviour.
>
> Signed-off-by: Simon Ser <contact@emersion.fr>
> ---
>
> v2: add a test
>
>  Documentation/git-grep.txt | 3 +++
>  grep.c                     | 5 +++++
>  t/t7810-grep.sh            | 5 +++++
>  3 files changed, 13 insertions(+)
>
> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> index a7f9bc99eaf1..ed4f05d885a2 100644
> --- a/Documentation/git-grep.txt
> +++ b/Documentation/git-grep.txt
> @@ -47,6 +47,9 @@ grep.lineNumber::
>  grep.column::
>  	If set to true, enable the `--column` option by default.
>  
> +grep.heading::
> +	If set to true, enable the `--heading` option by default.
> +

OK.

Naturally it follows that a command-line option

	$ git config grep.heading yes
	$ git grep --no-heading -e pattern

is a way to countermand the configured default per invocation
basis.

> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 991d5bd9c03f..1acc6fe89c51 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -1556,6 +1556,11 @@ test_expect_success 'grep --heading' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'grep with grep.heading=true' '
> +	git -c grep.heading=true grep -e char -e lo_w hello.c hello_world >actual &&
> +	test_cmp expected actual
> +'
> +

When writing new tests, we often get too excited and stop at showing
off how well the shiny new feature works, but we should make sure
that we test the "negative" case, too, i.e. that the "feature" can
be disabled when the user does not want to trigger it, and that the
"feature" notices incorrect invocations and fails appropriately.

E.g.

	git -c grep.heading=yes grep --no-heading ...

should not leave the opt->heading true, and

	git -c grep.heading=nonsense grep ...

should fail, saying "grep.heading must be a bool" (or something
along that line).

Thanks.

>  cat >expected <<EOF
>  <BOLD;GREEN>hello.c<RESET>
>  4:int main(int argc, const <BLACK;BYELLOW>char<RESET> **argv)

Re: [PATCH v2] grep: add configuration variables for --heading

[Simon Ser]

On Wednesday, May 27, 2020 6:45 PM, Junio C Hamano <gitster@pobox.com> wrote:

> Naturally it follows that a command-line option
>
> $ git config grep.heading yes
> $ git grep --no-heading -e pattern
>
> is a way to countermand the configured default per invocation
> basis.

Is see where you're getting at, but this is missing for a handful of
options, like grep.lineNumber and grep.column. I'd rather not create an
inconsistency here.

> When writing new tests, we often get too excited and stop at showing
> off how well the shiny new feature works, but we should make sure
> that we test the "negative" case, too, i.e. that the "feature" can
> be disabled when the user does not want to trigger it, and that the
> "feature" notices incorrect invocations and fails appropriately.
>
> E.g.
>
> git -c grep.heading=yes grep --no-heading ...
>
> should not leave the opt->heading true, and
>
> git -c grep.heading=nonsense grep ...
>
> should fail, saying "grep.heading must be a bool" (or something
> along that line).

Note, these new tests are only required if --no-heading is added to the
patch.

Re: [PATCH v2] grep: add configuration variables for --heading

[Phillip Wood]

Hi Simon

On 08/06/2020 11:22, Simon Ser wrote:
> On Wednesday, May 27, 2020 6:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 
>> Naturally it follows that a command-line option
>>
>> $ git config grep.heading yes
>> $ git grep --no-heading -e pattern
>>
>> is a way to countermand the configured default per invocation
>> basis.
> 
> Is see where you're getting at, but this is missing for a handful of
> options, like grep.lineNumber and grep.column. I'd rather not create an
> inconsistency here.

Those options do exist - try
   git -c grep.column=true grep --no-column ...
and you will see that the column number is not printed. This is because 
OPT_BOOL() without the PARSE_OPT_NONEG flag creates as --no- option.

If these existing options are not documented or tested perhaps you would 
be able to add a second patch to fix that.

>> When writing new tests, we often get too excited and stop at showing
>> off how well the shiny new feature works, but we should make sure
>> that we test the "negative" case, too, i.e. that the "feature" can
>> be disabled when the user does not want to trigger it, and that the
>> "feature" notices incorrect invocations and fails appropriately.
>>
>> E.g.
>>
>> git -c grep.heading=yes grep --no-heading ...
>>
>> should not leave the opt->heading true, and
>>
>> git -c grep.heading=nonsense grep ...
>>
>> should fail, saying "grep.heading must be a bool" (or something
>> along that line).
> 
> Note, these new tests are only required if --no-heading is added to the
> patch.

That is not correct in the case of `git grep -c grep.heading=nonsense 
grep ...`. Anyway --no-heading does exist.

Best Wishes

Phillip