threads / patch / 53548

patchgrep: add configuration variables for --heading

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

## tl;dr

6 messages between May 26, 2020 and Jun 8, 2020. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

Simon Ser· May 26, 2020, 08:37 UTC · lore

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

Documentation/git-grep.txt, grep.c

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
Eric Sunshine· May 26, 2020, 13:40 UTC · re: Simon Ser · lore

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

On Tue, May 26, 2020 at 4:37 AM Simon Ser <contact@emersion.fr> wrote:
Show 8 quoted lines
> 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.

Simon Ser· May 27, 2020, 15:45 UTC · re: Simon Ser · lore

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

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

Documentation/git-grep.txt, grep.c, t/t7810-grep.sh

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
Junio C Hamano· May 27, 2020, 16:45 UTC · re: Simon Ser · lore

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

Simon Ser <contact@emersion.fr> writes:
Show 24 quoted lines
> 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.

Show 13 quoted lines
> 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)
Simon Ser· Jun 8, 2020, 10:22 UTC · re: Junio C Hamano · lore

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

On Wednesday, May 27, 2020 6:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 7 quoted lines
> 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.

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

Phillip Wood· Jun 8, 2020, 14:02 UTC · re: Simon Ser · lore

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

Hi Simon
On 08/06/2020 11:22, Simon Ser wrote:
Show 13 quoted lines
> 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.

Show 19 quoted lines
>> 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

← back to recent threads