[PATCH 0/2] clean leftover calls to string_list_remove_duplicates

[PATCH 0/2] clean leftover calls to string_list_remove_duplicates

[Amisha Chhajed]

replace calls to string_list_remove_duplicates with string_list_sort_u.

sorted behavior of &keys_uniq depends on the call to string_list_sort on
&keys before &keys is processed to form &keys_uniq, this introduces an
edge case where &keys_uniq will not be sorted as expected, follow [1]
to reproduce.
add string_list_sort_u to &keys_uniq to fix this edge case and ensure
future enhancements to the processing logic does not introduce regressions.

[1]
run command:
cat <<'EOF' >> Documentation/config/add.adoc
aa*.b::
aa.b::
EOF
from git/ and then make test, this sees one failure when the test compared
actual output with sort -u output.

Amisha Chhajed (2):
  sparse-checkout: use string_list_sort_u
  help: ensure &keys_uniq follows sort -u

 builtin/help.c            | 2 +-
 builtin/sparse-checkout.c | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

-- 
2.52.0

[PATCH 1/2] sparse-checkout: use string_list_sort_u

[Amisha Chhajed]

From: Amisha Chhajed <136238836+amishhaa@users.noreply.github.com>

sparse_checkout_list() uses string_list_sort and
string_list_remove_duplicates instead of string_list_sort_u.

use string_list_sort_u at that place.

Signed-off-by: Amisha Chhajed <136238836+amishhaa@users.noreply.github.com>
---
 builtin/sparse-checkout.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index cccf630331..34e965bfa6 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -94,8 +94,7 @@ static int sparse_checkout_list(int argc, const char **argv, const char *prefix,
 			string_list_append(&sl, pe->pattern + 1);
 		}
 
-		string_list_sort(&sl);
-		string_list_remove_duplicates(&sl, 0);
+		string_list_sort_u(&sl, 0);
 
 		for (i = 0; i < sl.nr; i++) {
 			quote_c_style(sl.items[i].string, NULL, stdout, 0);
-- 
2.52.0

[PATCH 2/2] help: ensure &keys_uniq follows sort -u

[Amisha Chhajed]

From: Amisha Chhajed <136238836+amishhaa@users.noreply.github.com>

uniqueness operation of &keys_uniq depends on the sort operation executed
for &keys this might introduce regressions in future when the logic of
forming &keys_uniq from &keys is changed.

add string_list_sort_u operation for &keys_uniq after the processing of
&keys so it follows the expected sort -u behaviour.

Signed-off-by: Amisha Chhajed <136238836+amishhaa@users.noreply.github.com>
---
 builtin/help.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/help.c b/builtin/help.c
index c09cbc8912..0c9c007214 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -196,7 +196,7 @@ static void list_config_help(enum show_config_type type)
 
 	}
 	string_list_clear(&keys, 0);
-	string_list_remove_duplicates(&keys_uniq, 0);
+	string_list_sort_u(&keys_uniq, 0);
 	for_each_string_list_item(item, &keys_uniq)
 		puts(item->string);
 	string_list_clear(&keys_uniq, 0);
-- 
2.52.0

Re: [PATCH 1/2] sparse-checkout: use string_list_sort_u

[Junio C Hamano]

Amisha Chhajed <amishhhaaaa@gmail.com> writes:

> From: Amisha Chhajed <136238836+amishhaa@users.noreply.github.com>
>
> sparse_checkout_list() uses string_list_sort and
> string_list_remove_duplicates instead of string_list_sort_u.
>
> use string_list_sort_u at that place.
>
> Signed-off-by: Amisha Chhajed <136238836+amishhaa@users.noreply.github.com>
> ---
>  builtin/sparse-checkout.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index cccf630331..34e965bfa6 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -94,8 +94,7 @@ static int sparse_checkout_list(int argc, const char **argv, const char *prefix,
>  			string_list_append(&sl, pe->pattern + 1);
>  		}
>  
> -		string_list_sort(&sl);
> -		string_list_remove_duplicates(&sl, 0);
> +		string_list_sort_u(&sl, 0);
>  
>  		for (i = 0; i < sl.nr; i++) {
>  			quote_c_style(sl.items[i].string, NULL, stdout, 0);

Obviously correct.  Will queue.  Thanks.

Re: [PATCH 2/2] help: ensure &keys_uniq follows sort -u

[Junio C Hamano]

Amisha Chhajed <amishhhaaaa@gmail.com> writes:

> From: Amisha Chhajed <136238836+amishhaa@users.noreply.github.com>
>
> uniqueness operation of &keys_uniq depends on the sort operation executed
> for &keys this might introduce regressions in future when the logic of
> forming &keys_uniq from &keys is changed.
>
> add string_list_sort_u operation for &keys_uniq after the processing of
> &keys so it follows the expected sort -u behaviour.

I am not sure the above reasoning is sound.  With the original code,
we

 - prepare empty keys_uniq
 - collect keys
 - sort keys
 - iterate over keys
   - add either the whole "section[.subsection].key" or "section" to keys_uniq

before we call remove_duplicates.  keys_uniq would have duplicates,
but because keys is sorted upfront, wouldn't the contents of
keys_uniq be collected in sorted order anyway?

This is not a performance critical part of the system, so it is OK
as a future-proof measure to sort keys_uniq immediately before we
start doing something that we _care_ about its sortedness (e.g.,
presenting the final output to the user), even if keys_uniq is known
to be already sorted with the current code.  Using sort_u here would
allow us not to worry about how keys_uniq is constructed in that
ugly loop.

Yes, this function, especially the loop before the part you are
touching, _is_ ugly.  What drug the authors of it were under when it
was written, I have to wonder X-<.  For example, wouldn't readers
wonder why CONFIG_HUMAN output mode does puts() right in the middle
of the loop over keys string list, while the other two does not
puts() and have a separate loop over keys_uniq instead?

I suspect that making a switch(type) that calls one of three helper
functions for the three different output types after keys has been
populated in the earlier part of this function, but immediately
before it is sorted with string_list_sort(&keys), would be a
low-hanging fruit clean-up that makes the result far easier to
follow than the current code.  The helper function to handle
CONFIG_HUMAN mode may need to sort keys, but other two helper
functions do not have to and iterate over unsorted keys to construct
their output list, on which they can do sort_u before they output.

Thanks.

> Signed-off-by: Amisha Chhajed <136238836+amishhaa@users.noreply.github.com>
> ---
>  builtin/help.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/help.c b/builtin/help.c
> index c09cbc8912..0c9c007214 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -196,7 +196,7 @@ static void list_config_help(enum show_config_type type)
>  
>  	}
>  	string_list_clear(&keys, 0);
> -	string_list_remove_duplicates(&keys_uniq, 0);
> +	string_list_sort_u(&keys_uniq, 0);
>  	for_each_string_list_item(item, &keys_uniq)
>  		puts(item->string);
>  	string_list_clear(&keys_uniq, 0);

Re: [PATCH 2/2] help: ensure &keys_uniq follows sort -u

[Amisha Chhajed]

On Fri, 13 Feb 2026 at 01:28, Junio C Hamano <gitster@pobox.com> wrote:
>
> Amisha Chhajed <amishhhaaaa@gmail.com> writes:
>
> > From: Amisha Chhajed <136238836+amishhaa@users.noreply.github.com>
> >
> > uniqueness operation of &keys_uniq depends on the sort operation executed
> > for &keys this might introduce regressions in future when the logic of
> > forming &keys_uniq from &keys is changed.
> >
> > add string_list_sort_u operation for &keys_uniq after the processing of
> > &keys so it follows the expected sort -u behaviour.
>
> I am not sure the above reasoning is sound.  With the original code,
> we
>
>  - prepare empty keys_uniq
>  - collect keys
>  - sort keys
>  - iterate over keys
>    - add either the whole "section[.subsection].key" or "section" to keys_uniq
>
> before we call remove_duplicates.  keys_uniq would have duplicates,
> but because keys is sorted upfront, wouldn't the contents of
> keys_uniq be collected in sorted order anyway?

No, there is a case where it would not be sorted(keys_uniq won't be sorted
even though keys is), more details on the case[0] and steps to reproduce[1].
[0] https://lore.kernel.org/git/CAPvEtrfEZXHxcDf=z60ODfUA8cS81rhF1y7KEZApEBby7aCa1A@mail.gmail.com/
[1] https://lore.kernel.org/git/20260212041017.91370-1-amishhhaaaa@gmail.com/T/#m64880c5cd0d36e35bc78692757cf206b13496aea
only reason it is not causing a problem now is because we do not have
this edge case appearing git documentation(from where the keys are built)
but if someday a case like this appears there then it would cause problems.

> This is not a performance critical part of the system, so it is OK
> as a future-proof measure to sort keys_uniq immediately before we
> start doing something that we _care_ about its sortedness (e.g.,
> presenting the final output to the user), even if keys_uniq is known
> to be already sorted with the current code.  Using sort_u here would
> allow us not to worry about how keys_uniq is constructed in that
> ugly loop.

Agreed, we do not need to sort it twice if we decouple CONFIG_HUMAN
from the rest of the switch case, that is a great way to go about it,
thank you!.
I will work on it.

Re: [PATCH 2/2] help: ensure &keys_uniq follows sort -u

[Junio C Hamano]

Amisha Chhajed <amishhhaaaa@gmail.com> writes:

> No, there is a case where it would not be sorted(keys_uniq won't be sorted
> even though keys is), more details on the case[0] and steps to reproduce[1].
> [0] https://lore.kernel.org/git/CAPvEtrfEZXHxcDf=z60ODfUA8cS81rhF1y7KEZApEBby7aCa1A@mail.gmail.com/
> [1] https://lore.kernel.org/git/20260212041017.91370-1-amishhhaaaa@gmail.com/T/#m64880c5cd0d36e35bc78692757cf206b13496aea
> only reason it is not causing a problem now is because we do not have
> this edge case appearing git documentation(from where the keys are built)
> but if someday a case like this appears there then it would cause problems.

Ah, if you already have a reproduction case , it would have been
very good to add it as a new test.  That way, we can (1) apply the
patch, (2) tentatively revert only the code change, (3) build and
run test to see that the test breaks, demonstrating an existing
breakage, (4) restore the code change we tentatively reverted, (5)
build and run test again to see that the existing breakage is now
gone.

>> This is not a performance critical part of the system, so it is OK
>> as a future-proof measure to sort keys_uniq immediately before we
>> start doing something that we _care_ about its sortedness (e.g.,
>> presenting the final output to the user), even if keys_uniq is known
>> to be already sorted with the current code.  Using sort_u here would
>> allow us not to worry about how keys_uniq is constructed in that
>> ugly loop.
>
> Agreed, we do not need to sort it twice if we decouple CONFIG_HUMAN
> from the rest of the switch case, that is a great way to go about it,
> thank you!.
> I will work on it.

Thanks.

[PATCH v2 1/2] sparse-checkout: use string_list_sort_u

[Amisha Chhajed]

From: Amisha Chhajed <136238836+amishhaa@users.noreply.github.com>

sparse_checkout_list() uses string_list_sort and
string_list_remove_duplicates instead of string_list_sort_u.

use string_list_sort_u at that place.

Signed-off-by: Amisha Chhajed <136238836+amishhaa@users.noreply.github.com>
---
 builtin/sparse-checkout.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index cccf630331..34e965bfa6 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -94,8 +94,7 @@ static int sparse_checkout_list(int argc, const char **argv, const char *prefix,
 			string_list_append(&sl, pe->pattern + 1);
 		}
 
-		string_list_sort(&sl);
-		string_list_remove_duplicates(&sl, 0);
+		string_list_sort_u(&sl, 0);
 
 		for (i = 0; i < sl.nr; i++) {
 			quote_c_style(sl.items[i].string, NULL, stdout, 0);
-- 
2.52.0

[PATCH v2 2/2] help: cleanup the contruction of keys_uniq

[Amisha Chhajed]

From: Amisha Chhajed <136238836+amishhaa@users.noreply.github.com>

uniqueness property of keys_uniq depends on the sort operation executed
for keys, sorted property of keys does not gurantee sorted property of
keys_uniq due to processing keys, this might also introduce regressions
in future when the logic of forming keys_uniq from keys is changed.

add string_list_sort_u operation for keys_uniq and refactor the
processing code to simplify it, add test that demonstrates that sorted
property of keys does not gurantee sorted property of keys_uniq.

Signed-off-by: Amisha Chhajed <136238836+amishhaa@users.noreply.github.com>
---
 builtin/help.c  | 71 +++++++++++++++++++++++++------------------------
 t/t0012-help.sh | 18 +++++++++++++
 2 files changed, 54 insertions(+), 35 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index c09cbc8912..c278d7ffcb 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -156,47 +156,48 @@ static void list_config_help(enum show_config_type type)
 			BUG("slot_expansion %s.%s is not used",
 			    e->prefix, e->placeholder);
 
-	string_list_sort(&keys);
-	for (size_t i = 0; i < keys.nr; i++) {
-		const char *var = keys.items[i].string;
-		const char *wildcard, *tag, *cut;
-		const char *dot = NULL;
-		struct strbuf sb = STRBUF_INIT;
-
-		switch (type) {
-		case SHOW_CONFIG_HUMAN:
+	if (type == SHOW_CONFIG_HUMAN) {
+		string_list_sort(&keys);
+		for (size_t i = 0; i < keys.nr; i++) {
+			const char *var = keys.items[i].string;
 			puts(var);
-			continue;
-		case SHOW_CONFIG_SECTIONS:
-			dot = strchr(var, '.');
-			break;
-		case SHOW_CONFIG_VARS:
-			break;
-		}
-		wildcard = strchr(var, '*');
-		tag = strchr(var, '<');
-
-		if (!dot && !wildcard && !tag) {
-			string_list_append(&keys_uniq, var);
-			continue;
 		}
+	}
+	else{
+		for (size_t i = 0; i < keys.nr; i++) {
+			const char *var = keys.items[i].string;
+			const char *wildcard, *tag, *cut;
+			const char *dot = NULL;
+			struct strbuf sb = STRBUF_INIT;
+
+			if (type == SHOW_CONFIG_SECTIONS) {
+				dot = strchr(var, '.');
+			}
+			wildcard = strchr(var, '*');
+			tag = strchr(var, '<');
 
-		if (dot)
-			cut = dot;
-		else if (wildcard && !tag)
-			cut = wildcard;
-		else if (!wildcard && tag)
-			cut = tag;
-		else
-			cut = wildcard < tag ? wildcard : tag;
-
-		strbuf_add(&sb, var, cut - var);
-		string_list_append(&keys_uniq, sb.buf);
-		strbuf_release(&sb);
+			if (!dot && !wildcard && !tag) {
+				string_list_append(&keys_uniq, var);
+				continue;
+			}
 
+			if (dot)
+				cut = dot;
+			else if (wildcard && !tag)
+				cut = wildcard;
+			else if (!wildcard && tag)
+				cut = tag;
+			else
+				cut = wildcard < tag ? wildcard : tag;
+
+			strbuf_add(&sb, var, cut - var);
+			string_list_append(&keys_uniq, sb.buf);
+			strbuf_release(&sb);
+		}
 	}
+
 	string_list_clear(&keys, 0);
-	string_list_remove_duplicates(&keys_uniq, 0);
+	string_list_sort_u(&keys_uniq, 0);
 	for_each_string_list_item(item, &keys_uniq)
 		puts(item->string);
 	string_list_clear(&keys_uniq, 0);
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index d3a0967e9d..0dbe6dd46f 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -160,6 +160,24 @@ test_expect_success 'git help --config-for-completion' '
 	test_cmp human.munged vars
 '
 
+test_expect_success 'git help --config-for-completion' '
+	file="$GIT_SOURCE_DIR/Documentation/config/add.adoc" &&
+	test_when_finished "git -C \"$GIT_SOURCE_DIR\" checkout -- Documentation/config/add.adoc" &&
+	cat <<-\EOF >>"$file" &&
+	aa*.b::
+	aa.b::
+	EOF
+	git help -c >human &&
+	grep -E \
+	     -e "^[^.]+\.[^.]+$" \
+	     -e "^[^.]+\.[^.]+\.[^.]+$" human |
+	     sed -e "s/\*.*//" -e "s/<.*//" |
+	     sort -u >human.munged &&
+
+	git help --config-for-completion >vars &&
+	test_cmp human.munged vars
+'
+
 test_expect_success 'git help --config-sections-for-completion' '
 	git help -c >human &&
 	grep -E \
-- 
2.52.0

Re: [PATCH v2 2/2] help: cleanup the contruction of keys_uniq

[Junio C Hamano]

Amisha Chhajed <amishhhaaaa@gmail.com> writes:

> +	}
> +	else{

Style:

	} else {

> +		for (size_t i = 0; i < keys.nr; i++) {
> +			const char *var = keys.items[i].string;
> +			const char *wildcard, *tag, *cut;
> +			const char *dot = NULL;
> +			struct strbuf sb = STRBUF_INIT;
> +
> +			if (type == SHOW_CONFIG_SECTIONS) {
> +				dot = strchr(var, '.');
> +			}

No {braces} around a single-statement block.  Wouldn't it easier to
follow if we do not rely on the initialization?  I.e.,

			if (type == SHOW_CONFIG_SECTIONS)
				dot = strchr(var, '.');
			else /* SHOW_CONFIG_VARS */
				dot = NULL;

or even

   			switch (type) {
                        case SHOW_CONFIG_SECTIONS:
				dot = strchr(var, '.');
				break;
			case SHOW_CONFIG_VARS:
				dot = NULL;
				break;
			default:
				BUG("%d: unexpected type", type);
			}

?

But all of the above might become a moot point; see below.

> +			wildcard = strchr(var, '*');
> +			tag = strchr(var, '<');
>  
> +			if (!dot && !wildcard && !tag) {
> +				string_list_append(&keys_uniq, var);
> +				continue;
> +			}
>  
> +			if (dot)
> +				cut = dot;
> +			else if (wildcard && !tag)
> +				cut = wildcard;
> +			else if (!wildcard && tag)
> +				cut = tag;
> +			else
> +				cut = wildcard < tag ? wildcard : tag;

How much are you saving by conflating SHOW_CONFIG_SECTIONS and
SHOW_CONFIG_VARS into this same "else" block?  In CONFIG_VARS mode,
dot is always NULL, and when dot is not NULL, neither wildcard or
tag affect the final output at all.  Would the logic become clearer
if you split these two mode into two, I have to wonder?

> +			strbuf_add(&sb, var, cut - var);
> +			string_list_append(&keys_uniq, sb.buf);
> +			strbuf_release(&sb);
> +		}
>  	}
> +
>  	string_list_clear(&keys, 0);
> -	string_list_remove_duplicates(&keys_uniq, 0);
> +	string_list_sort_u(&keys_uniq, 0);
>  	for_each_string_list_item(item, &keys_uniq)
>  		puts(item->string);

You inherited the source of ugliness from the original.  Even in
HUMAN mode, you sort-u keys_uniq and run puts(), and the only thing
that makes it a no-op is the fact that in the if/else above, keys_uniq
is left untouched.

I wonder if the above should look more like this:

	switch (type) {
	case SHOW_CONFIG_HUMAN:
		show_config_human(&keys);
		break;
	case SHOW_CONFIG_SECTIONS:
		show_config_sections(&keys);
		break;
	case SHOW_CONFIG_VARS:
		show_config_vars(&keys);
		break;
	default:
		BUG("%d: unexpected type", type);
	}
	string_list_clear(&keys);
        return;

without keys_uniq string list in this function (it would be an
implementation detail in show_config_sections() and _vars().

q> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
> index d3a0967e9d..0dbe6dd46f 100755
> --- a/t/t0012-help.sh
> +++ b/t/t0012-help.sh
> @@ -160,6 +160,24 @@ test_expect_success 'git help --config-for-completion' '
>  	test_cmp human.munged vars
>  '
>  
> +test_expect_success 'git help --config-for-completion' '
> +	file="$GIT_SOURCE_DIR/Documentation/config/add.adoc" &&
> +	test_when_finished "git -C \"$GIT_SOURCE_DIR\" checkout -- Documentation/config/add.adoc" &&
> +	cat <<-\EOF >>"$file" &&
> +	aa*.b::
> +	aa.b::
> +	EOF
> +	git help -c >human &&
> +	grep -E \
> +	     -e "^[^.]+\.[^.]+$" \
> +	     -e "^[^.]+\.[^.]+\.[^.]+$" human |
> +	     sed -e "s/\*.*//" -e "s/<.*//" |
> +	     sort -u >human.munged &&

Dedent "sed" and "sort" to the same level as "grep -E".

> +	git help --config-for-completion >vars &&
> +	test_cmp human.munged vars
> +'
> +
>  test_expect_success 'git help --config-sections-for-completion' '
>  	git help -c >human &&
>  	grep -E \

Re: [PATCH v2 2/2] help: cleanup the contruction of keys_uniq

[Eric Sunshine]

On Thu, Feb 12, 2026 at 11:30 PM Junio C Hamano <gitster@pobox.com> wrote:
> Amisha Chhajed <amishhhaaaa@gmail.com> writes:
> > +test_expect_success 'git help --config-for-completion' '
> > +     file="$GIT_SOURCE_DIR/Documentation/config/add.adoc" &&
> > +     test_when_finished "git -C \"$GIT_SOURCE_DIR\" checkout -- Documentation/config/add.adoc" &&
> > +     cat <<-\EOF >>"$file" &&
> > +     aa*.b::
> > +     aa.b::
> > +     EOF
> > +     git help -c >human &&
> > +     grep -E \
> > +          -e "^[^.]+\.[^.]+$" \
> > +          -e "^[^.]+\.[^.]+\.[^.]+$" human |
> > +          sed -e "s/\*.*//" -e "s/<.*//" |
> > +          sort -u >human.munged &&
>
> Dedent "sed" and "sort" to the same level as "grep -E".

Also, don't we usually avoid having both `grep` and `sed` in the same
pipeline like this, considering that `sed` alone should be able to
handle the job itself?

Re: [PATCH v2 2/2] help: cleanup the contruction of keys_uniq

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Thu, Feb 12, 2026 at 11:30 PM Junio C Hamano <gitster@pobox.com> wrote:
>> Amisha Chhajed <amishhhaaaa@gmail.com> writes:
>> > +test_expect_success 'git help --config-for-completion' '
>> > +     file="$GIT_SOURCE_DIR/Documentation/config/add.adoc" &&
>> > +     test_when_finished "git -C \"$GIT_SOURCE_DIR\" checkout -- Documentation/config/add.adoc" &&
>> > +     cat <<-\EOF >>"$file" &&
>> > +     aa*.b::
>> > +     aa.b::
>> > +     EOF
>> > +     git help -c >human &&
>> > +     grep -E \
>> > +          -e "^[^.]+\.[^.]+$" \
>> > +          -e "^[^.]+\.[^.]+\.[^.]+$" human |
>> > +          sed -e "s/\*.*//" -e "s/<.*//" |
>> > +          sort -u >human.munged &&
>>
>> Dedent "sed" and "sort" to the same level as "grep -E".
>
> Also, don't we usually avoid having both `grep` and `sed` in the same
> pipeline like this, considering that `sed` alone should be able to
> handle the job itself?

Yes, we often say "do not pipe output of grep or awk to sed".  I did
not want to burden a bit too much on a contributor who is relatively
new to the list.

Thanks.

[PATCH v3 1/2] sparse-checkout: use string_list_sort_u

[Amisha Chhajed]

From: Amisha Chhajed <136238836+amishhaa@users.noreply.github.com>

sparse_checkout_list() uses string_list_sort and
string_list_remove_duplicates instead of string_list_sort_u.

use string_list_sort_u at that place.

Signed-off-by: Amisha Chhajed <136238836+amishhaa@users.noreply.github.com>
---
 builtin/sparse-checkout.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index cccf630331..34e965bfa6 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -94,8 +94,7 @@ static int sparse_checkout_list(int argc, const char **argv, const char *prefix,
 			string_list_append(&sl, pe->pattern + 1);
 		}
 
-		string_list_sort(&sl);
-		string_list_remove_duplicates(&sl, 0);
+		string_list_sort_u(&sl, 0);
 
 		for (i = 0; i < sl.nr; i++) {
 			quote_c_style(sl.items[i].string, NULL, stdout, 0);
-- 
2.52.0

[PATCH v3 2/2] help: cleanup the contruction of keys_uniq

[Amisha Chhajed]

From: Amisha Chhajed <136238836+amishhaa@users.noreply.github.com>

uniqueness property of keys_uniq depends on the sort operation executed
for keys, sorted property of keys does not gurantee sorted property of
keys_uniq due to processing keys, this might also introduce regressions
in future when the logic of forming keys_uniq from keys is changed.

add string_list_sort_u operation for keys_uniq and refactor the
processing code to simplify it.

Signed-off-by: Amisha Chhajed <136238836+amishhaa@users.noreply.github.com>
---
 builtin/help.c | 134 +++++++++++++++++++++++++++++++++----------------
 1 file changed, 90 insertions(+), 44 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index c09cbc8912..b70de09864 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -111,6 +111,84 @@ struct slot_expansion {
 	int found;
 };
 
+static void show_config_human(struct string_list *keys)
+{
+	string_list_sort(keys);
+	for (size_t i = 0; i < keys->nr; i++) {
+		const char *var = keys->items[i].string;
+		puts(var);
+	}
+}
+
+static void show_config_sections(struct string_list *keys)
+{
+	struct string_list keys_uniq = STRING_LIST_INIT_DUP;
+	struct strbuf sb = STRBUF_INIT;
+	struct string_list_item *item;
+
+	for (size_t i = 0; i < keys->nr; i++) {
+		const char *var = keys->items[i].string;
+		const char *dot = strchr(var, '.');
+		const char *wildcard = strchr(var, '*');
+		const char *tag = strchr(var, '<');
+		const char *cut;
+
+		if (dot)
+			cut = dot;
+		else if (wildcard && tag)
+			cut = wildcard < tag ? wildcard : tag;
+		else if (wildcard)
+			cut = wildcard;
+		else if (tag)
+			cut = tag;
+		else {
+			string_list_append(&keys_uniq, var);
+			continue;
+		}
+
+		strbuf_add(&sb, var, cut - var);
+		string_list_append(&keys_uniq, sb.buf);
+		strbuf_release(&sb);
+	}
+	string_list_sort_u(&keys_uniq, 0);
+	for_each_string_list_item(item, &keys_uniq)
+		puts(item->string);
+	string_list_clear(&keys_uniq, 0);
+}
+
+static void show_config_vars(struct string_list *keys)
+{
+	struct string_list keys_uniq = STRING_LIST_INIT_DUP;
+	struct strbuf sb = STRBUF_INIT;
+	struct string_list_item *item;
+
+	for (size_t i = 0; i < keys->nr; i++) {
+		const char *var = keys->items[i].string;
+		const char *wildcard = strchr(var, '*');
+		const char *tag = strchr(var, '<');
+		const char *cut;
+
+		if (wildcard && tag)
+			cut = wildcard < tag ? wildcard : tag;
+		else if (wildcard)
+			cut = wildcard;
+		else if (tag)
+			cut = tag;
+		else {
+			string_list_append(&keys_uniq, var);
+			continue;
+		}
+
+		strbuf_add(&sb, var, cut - var);
+		string_list_append(&keys_uniq, sb.buf);
+		strbuf_release(&sb);
+	}
+	string_list_sort_u(&keys_uniq, 0);
+	for_each_string_list_item(item, &keys_uniq)
+		puts(item->string);
+	string_list_clear(&keys_uniq, 0);
+}
+
 static void list_config_help(enum show_config_type type)
 {
 	struct slot_expansion slot_expansions[] = {
@@ -129,8 +207,6 @@ static void list_config_help(enum show_config_type type)
 	const char **p;
 	struct slot_expansion *e;
 	struct string_list keys = STRING_LIST_INIT_DUP;
-	struct string_list keys_uniq = STRING_LIST_INIT_DUP;
-	struct string_list_item *item;
 
 	for (p = config_name_list; *p; p++) {
 		const char *var = *p;
@@ -156,50 +232,20 @@ static void list_config_help(enum show_config_type type)
 			BUG("slot_expansion %s.%s is not used",
 			    e->prefix, e->placeholder);
 
-	string_list_sort(&keys);
-	for (size_t i = 0; i < keys.nr; i++) {
-		const char *var = keys.items[i].string;
-		const char *wildcard, *tag, *cut;
-		const char *dot = NULL;
-		struct strbuf sb = STRBUF_INIT;
-
-		switch (type) {
-		case SHOW_CONFIG_HUMAN:
-			puts(var);
-			continue;
-		case SHOW_CONFIG_SECTIONS:
-			dot = strchr(var, '.');
-			break;
-		case SHOW_CONFIG_VARS:
-			break;
-		}
-		wildcard = strchr(var, '*');
-		tag = strchr(var, '<');
-
-		if (!dot && !wildcard && !tag) {
-			string_list_append(&keys_uniq, var);
-			continue;
-		}
-
-		if (dot)
-			cut = dot;
-		else if (wildcard && !tag)
-			cut = wildcard;
-		else if (!wildcard && tag)
-			cut = tag;
-		else
-			cut = wildcard < tag ? wildcard : tag;
-
-		strbuf_add(&sb, var, cut - var);
-		string_list_append(&keys_uniq, sb.buf);
-		strbuf_release(&sb);
-
+	switch (type) {
+	case SHOW_CONFIG_HUMAN:
+		show_config_human(&keys);
+		break;
+	case SHOW_CONFIG_SECTIONS:
+		show_config_sections(&keys);
+		break;
+	case SHOW_CONFIG_VARS:
+		show_config_vars(&keys);
+		break;
+	default:
+		BUG("%d: unexpected type", type);
 	}
 	string_list_clear(&keys, 0);
-	string_list_remove_duplicates(&keys_uniq, 0);
-	for_each_string_list_item(item, &keys_uniq)
-		puts(item->string);
-	string_list_clear(&keys_uniq, 0);
 }
 
 static enum help_format parse_help_format(const char *format)
-- 
2.52.0

Re: [PATCH v2 2/2] help: cleanup the contruction of keys_uniq

[Amisha Chhajed]

>
> q> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
> > index d3a0967e9d..0dbe6dd46f 100755
> > --- a/t/t0012-help.sh
> > +++ b/t/t0012-help.sh
> > @@ -160,6 +160,24 @@ test_expect_success 'git help --config-for-completion' '
> >       test_cmp human.munged vars
> >  '
> >
> > +test_expect_success 'git help --config-for-completion' '
> > +     file="$GIT_SOURCE_DIR/Documentation/config/add.adoc" &&
> > +     test_when_finished "git -C \"$GIT_SOURCE_DIR\" checkout -- Documentation/config/add.adoc" &&
> > +     cat <<-\EOF >>"$file" &&
> > +     aa*.b::
> > +     aa.b::
> > +     EOF
> > +     git help -c >human &&
> > +     grep -E \
> > +          -e "^[^.]+\.[^.]+$" \
> > +          -e "^[^.]+\.[^.]+\.[^.]+$" human |
> > +          sed -e "s/\*.*//" -e "s/<.*//" |
> > +          sort -u >human.munged &&
>
> Dedent "sed" and "sort" to the same level as "grep -E".
>
> > +     git help --config-for-completion >vars &&
> > +     test_cmp human.munged vars
> > +'
> > +
> >  test_expect_success 'git help --config-sections-for-completion' '
> >       git help -c >human &&
> >       grep -E \


had to drop this test, as it was working for a while locally on my
machine because i had manually
added the case in documentation then didn't make clean so the binary
had it. Unfortunately i wasn't
able to find a way that rebuilds config-list.h to test this, noticed
that this topic is actively being worked on
in https://lore.kernel.org/git/9cdcc9de04f0f8fff657f0474b31c063466ed808.1771280837.git.ben.knoble+github@gmail.com/T/#me826da3b6a128e1ceb7215d64328b7d6aa2b211e

Re: [PATCH v3 1/2] sparse-checkout: use string_list_sort_u

[Junio C Hamano]

Amisha Chhajed <amishhhaaaa@gmail.com> writes:

> From: Amisha Chhajed <136238836+amishhaa@users.noreply.github.com>
>
> sparse_checkout_list() uses string_list_sort and
> string_list_remove_duplicates instead of string_list_sort_u.
>
> use string_list_sort_u at that place.
>
> Signed-off-by: Amisha Chhajed <136238836+amishhaa@users.noreply.github.com>
> ---
>  builtin/sparse-checkout.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

An exact copy of this patch is already in 'next' since Feb 17th, if
I am not mistaken.

> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index cccf630331..34e965bfa6 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -94,8 +94,7 @@ static int sparse_checkout_list(int argc, const char **argv, const char *prefix,
>  			string_list_append(&sl, pe->pattern + 1);
>  		}
>  
> -		string_list_sort(&sl);
> -		string_list_remove_duplicates(&sl, 0);
> +		string_list_sort_u(&sl, 0);
>  
>  		for (i = 0; i < sl.nr; i++) {
>  			quote_c_style(sl.items[i].string, NULL, stdout, 0);

Re: [PATCH v3 2/2] help: cleanup the contruction of keys_uniq

[Junio C Hamano]

Amisha Chhajed <amishhhaaaa@gmail.com> writes:

> From: Amisha Chhajed <136238836+amishhaa@users.noreply.github.com>
>
> +static void show_config_sections(struct string_list *keys)
> +{
> ...
> +}
> +
> +static void show_config_vars(struct string_list *keys)
> +{
> ...
> +}

The striking similarity of the body of the loops in these two
functions bothered me enough to try writing this; the result does
not look too bad, I think.

By the way, I'd really prefer to see contributors *NOT* to use
undeliverable and/or bouncing e-mail addresses when working on this
project, as I'd always have to edit the Cc: list to avoid getting
bounces.

Thanks.

 builtin/help.c | 72 ++++++++++++++++++++++------------------------------------
 1 file changed, 27 insertions(+), 45 deletions(-)

diff --git c/builtin/help.c w/builtin/help.c
index b70de09864..bc5c5a556c 100644
--- c/builtin/help.c
+++ w/builtin/help.c
@@ -120,36 +120,37 @@ static void show_config_human(struct string_list *keys)
 	}
 }
 
-static void show_config_sections(struct string_list *keys)
+static void grab_leading_part(struct string_list *keys, const char *var, int use_dot)
 {
-	struct string_list keys_uniq = STRING_LIST_INIT_DUP;
-	struct strbuf sb = STRBUF_INIT;
-	struct string_list_item *item;
+	const char *cut = NULL;
 
-	for (size_t i = 0; i < keys->nr; i++) {
-		const char *var = keys->items[i].string;
-		const char *dot = strchr(var, '.');
-		const char *wildcard = strchr(var, '*');
-		const char *tag = strchr(var, '<');
-		const char *cut;
-
-		if (dot)
-			cut = dot;
-		else if (wildcard && tag)
-			cut = wildcard < tag ? wildcard : tag;
-		else if (wildcard)
-			cut = wildcard;
-		else if (tag)
-			cut = tag;
-		else {
-			string_list_append(&keys_uniq, var);
-			continue;
-		}
+	if (use_dot)
+		cut = strchr(var, use_dot);
 
+	if (!cut) {
+		size_t prefix_len = strcspn(var, "*<");
+		if (var[prefix_len])
+			cut = var + prefix_len;
+	}
+
+	if (!cut)
+		string_list_append(keys, var);
+	else {
+		struct strbuf sb = STRBUF_INIT;
 		strbuf_add(&sb, var, cut - var);
-		string_list_append(&keys_uniq, sb.buf);
+		string_list_append(keys, sb.buf);
 		strbuf_release(&sb);
 	}
+}
+
+static void show_config_sections(struct string_list *keys)
+{
+	struct string_list keys_uniq = STRING_LIST_INIT_DUP;
+	struct string_list_item *item;
+
+	for (size_t i = 0; i < keys->nr; i++)
+		grab_leading_part(&keys_uniq, keys->items[i].string, '.');
+
 	string_list_sort_u(&keys_uniq, 0);
 	for_each_string_list_item(item, &keys_uniq)
 		puts(item->string);
@@ -159,30 +160,11 @@ static void show_config_sections(struct string_list *keys)
 static void show_config_vars(struct string_list *keys)
 {
 	struct string_list keys_uniq = STRING_LIST_INIT_DUP;
-	struct strbuf sb = STRBUF_INIT;
 	struct string_list_item *item;
 
-	for (size_t i = 0; i < keys->nr; i++) {
-		const char *var = keys->items[i].string;
-		const char *wildcard = strchr(var, '*');
-		const char *tag = strchr(var, '<');
-		const char *cut;
-
-		if (wildcard && tag)
-			cut = wildcard < tag ? wildcard : tag;
-		else if (wildcard)
-			cut = wildcard;
-		else if (tag)
-			cut = tag;
-		else {
-			string_list_append(&keys_uniq, var);
-			continue;
-		}
+	for (size_t i = 0; i < keys->nr; i++)
+		grab_leading_part(&keys_uniq, keys->items[i].string, '\0');
 
-		strbuf_add(&sb, var, cut - var);
-		string_list_append(&keys_uniq, sb.buf);
-		strbuf_release(&sb);
-	}
 	string_list_sort_u(&keys_uniq, 0);
 	for_each_string_list_item(item, &keys_uniq)
 		puts(item->string);

Re: [PATCH v3 2/2] help: cleanup the contruction of keys_uniq

[Amisha Chhajed]

>
> The striking similarity of the body of the loops in these two
> functions bothered me enough to try writing this; the result does
> not look too bad, I think.


Agreed, I was also not very happy with the similarity present at these
two places,
especially the wildcard and tag part, tried to convulse them into something
singular. It again started to look like the original so ultimately
kept it like this.

>
> By the way, I'd really prefer to see contributors *NOT* to use
> undeliverable and/or bouncing e-mail addresses when working on this
> project, as I'd always have to edit the Cc: list to avoid getting
> bounces.
>
> Thanks.
>

Thanks, I will take care.

Re: [PATCH v3 2/2] help: cleanup the contruction of keys_uniq

[Junio C Hamano]

Amisha Chhajed <amishhhaaaa@gmail.com> writes:

>>
>> The striking similarity of the body of the loops in these two
>> functions bothered me enough to try writing this; the result does
>> not look too bad, I think.
>
>
> Agreed, I was also not very happy with the similarity present at these
> two places,
> especially the wildcard and tag part, tried to convulse them into something
> singular. It again started to look like the original so ultimately
> kept it like this.
>
>>
>> By the way, I'd really prefer to see contributors *NOT* to use
>> undeliverable and/or bouncing e-mail addresses when working on this
>> project, as I'd always have to edit the Cc: list to avoid getting
>> bounces.
>>
>> Thanks.
>>
>
> Thanks, I will take care.

Thanks.

[PATCH v4 0/1] Make keys_uniq stop depending on sort of keys_uniq

[Amisha Chhajed]

While its problematic for keys_uniq to depend on sort operation on keys,
reproduction of the issue as a test is complex, when we add something to the
documentation we will need to re-build git so the array(config-list.h)
is also rebuilt with our new test case so the functions which fail on the test
can catch it during runtime.

The details for the test are as follows:
the case[0] and steps to reproduce[1].
[0] https://lore.kernel.org/git/CAPvEtrfEZXHxcDf=z60ODfUA8cS81rhF1y7KEZApEBby7aCa1A@mail.gmail.com/
[1] https://lore.kernel.org/git/20260212041017.91370-1-amishhhaaaa@gmail.com/T/#m64880c5cd0d36e35bc78692757cf206b13496aea

Community help is appreciated as i tried various ways to reproduce the issue
in a test, including cat to config-list.h, cat to documentation config,
applied patch
https://lore.kernel.org/git/9cdcc9de04f0f8fff657f0474b31c063466ed808.1771280837.git.ben.knoble+github@gmail.com/T/#me826da3b6a128e1ceb7215d64328b7d6aa2b211e
as well, but unfortunately, I couldn't get the test in config-list during
test runtime as that array is baked into the binary and needs rebuild to show.

Thanks.

Amisha Chhajed (1):
  help: cleanup the contruction of keys_uniq

 builtin/help.c  | 84 ++++++++++++++++++++++++++++++-------------------
 t/t0012-help.sh | 26 +++++++--------
 2 files changed, 65 insertions(+), 45 deletions(-)

-- 
2.52.0

[PATCH v4 1/1] help: cleanup the contruction of keys_uniq

[Amisha Chhajed]

From: Amisha Chhajed <136238836+amishhaa@users.noreply.github.com>

construction of keys_uniq depends on sort operation
executed on keys before processing, which does not
gurantee that keys_uniq will be sorted.

refactor the code to shift the sort operation after
the processing to remove dependency on key's sort operation
and strictly maintain the sorted order of keys_uniq.
dedent sort -u and sed in tests and replace grep with sed.

Signed-off-by: Amisha Chhajed <136238836+amishhaa@users.noreply.github.com>
---
 builtin/help.c  | 84 ++++++++++++++++++++++++++++++-------------------
 t/t0012-help.sh | 26 +++++++--------
 2 files changed, 65 insertions(+), 45 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index c09cbc8912..3658836d23 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -111,6 +111,49 @@ struct slot_expansion {
 	int found;
 };
 
+static void set_config_vars(struct string_list *keys_uniq, struct string_list_item *var)
+{
+	struct strbuf sb = STRBUF_INIT;
+	const char *str = var->string;
+	const char *wildcard = strchr(str, '*');
+	const char *tag = strchr(str, '<');
+	const char *cut;
+
+	if (wildcard && tag)
+		cut = wildcard < tag ? wildcard : tag;
+	else if (wildcard)
+		cut = wildcard;
+	else if (tag)
+		cut = tag;
+	else {
+		string_list_append(keys_uniq, str);
+		return;
+	}
+
+	strbuf_add(&sb, str, cut - str);
+	string_list_append(keys_uniq, sb.buf);
+	strbuf_release(&sb);
+}
+
+static void set_config_sections(struct string_list *keys_uniq, struct string_list_item *var)
+{
+	struct strbuf sb = STRBUF_INIT;
+	const char *str = var->string;
+	const char *dot = strchr(str, '.');
+	const char *cut;
+
+	if (dot)
+		cut = dot;
+	else {
+		set_config_vars(keys_uniq, var);
+		return;
+	}
+
+	strbuf_add(&sb, str, cut - str);
+	string_list_append(keys_uniq, sb.buf);
+	strbuf_release(&sb);
+}
+
 static void list_config_help(enum show_config_type type)
 {
 	struct slot_expansion slot_expansions[] = {
@@ -156,50 +199,27 @@ static void list_config_help(enum show_config_type type)
 			BUG("slot_expansion %s.%s is not used",
 			    e->prefix, e->placeholder);
 
-	string_list_sort(&keys);
 	for (size_t i = 0; i < keys.nr; i++) {
-		const char *var = keys.items[i].string;
-		const char *wildcard, *tag, *cut;
-		const char *dot = NULL;
-		struct strbuf sb = STRBUF_INIT;
-
 		switch (type) {
 		case SHOW_CONFIG_HUMAN:
-			puts(var);
-			continue;
+			string_list_append(&keys_uniq, keys.items[i].string);
+			break;
 		case SHOW_CONFIG_SECTIONS:
-			dot = strchr(var, '.');
+			set_config_sections(&keys_uniq, &keys.items[i]);
 			break;
 		case SHOW_CONFIG_VARS:
+			set_config_vars(&keys_uniq, &keys.items[i]);
 			break;
+		default:
+			BUG("%d: unexpected type", type);
 		}
-		wildcard = strchr(var, '*');
-		tag = strchr(var, '<');
-
-		if (!dot && !wildcard && !tag) {
-			string_list_append(&keys_uniq, var);
-			continue;
-		}
-
-		if (dot)
-			cut = dot;
-		else if (wildcard && !tag)
-			cut = wildcard;
-		else if (!wildcard && tag)
-			cut = tag;
-		else
-			cut = wildcard < tag ? wildcard : tag;
-
-		strbuf_add(&sb, var, cut - var);
-		string_list_append(&keys_uniq, sb.buf);
-		strbuf_release(&sb);
-
 	}
-	string_list_clear(&keys, 0);
-	string_list_remove_duplicates(&keys_uniq, 0);
+
+	string_list_sort_u(&keys_uniq, 0);
 	for_each_string_list_item(item, &keys_uniq)
 		puts(item->string);
 	string_list_clear(&keys_uniq, 0);
+	string_list_clear(&keys, 0);
 }
 
 static enum help_format parse_help_format(const char *format)
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index d3a0967e9d..03104b3bf4 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -141,20 +141,20 @@ test_expect_success 'git help -c' '
 
 	'\''git help config'\'' for more information
 	EOF
-	grep -v -E \
-		-e "^[^.]+\.[^.]+$" \
-		-e "^[^.]+\.[^.]+\.[^.]+$" \
+	sed \
+		-e "/^[^.]*\.[^.]*$/d" \
+		-e "/^[^.]*\.[^.]*\.[^.]*$/d" \
 		help.output >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'git help --config-for-completion' '
 	git help -c >human &&
-	grep -E \
-	     -e "^[^.]+\.[^.]+$" \
-	     -e "^[^.]+\.[^.]+\.[^.]+$" human |
-	     sed -e "s/\*.*//" -e "s/<.*//" |
-	     sort -u >human.munged &&
+	sed -n \
+	     -e "/^[^.]*\.[^.]*$/p" \
+	     -e "/^[^.]*\.[^.]*\.[^.]*$/p" human |
+	sed -e "s/\*.*//" -e "s/<.*//" |
+	sort -u >human.munged &&
 
 	git help --config-for-completion >vars &&
 	test_cmp human.munged vars
@@ -162,11 +162,11 @@ test_expect_success 'git help --config-for-completion' '
 
 test_expect_success 'git help --config-sections-for-completion' '
 	git help -c >human &&
-	grep -E \
-	     -e "^[^.]+\.[^.]+$" \
-	     -e "^[^.]+\.[^.]+\.[^.]+$" human |
-	     sed -e "s/\..*//" |
-	     sort -u >human.munged &&
+	sed -n \
+	     -e "/^[^.]*\.[^.]*$/p" \
+	     -e "/^[^.]*\.[^.]*\.[^.]*$/p" human |
+	sed -e "s/\..*//" |
+	sort -u >human.munged &&
 
 	git help --config-sections-for-completion >sections &&
 	test_cmp human.munged sections
-- 
2.52.0

Re: [PATCH v3 2/2] help: cleanup the contruction of keys_uniq

[Amisha Chhajed]

Incredibly sorry for the bouncing mail once again, I will fix it locally.


On Thu, 26 Feb 2026 at 22:15, Junio C Hamano <gitster@pobox.com> wrote:
>
> Amisha Chhajed <amishhhaaaa@gmail.com> writes:
>
> >>
> >> The striking similarity of the body of the loops in these two
> >> functions bothered me enough to try writing this; the result does
> >> not look too bad, I think.
> >
> >
> > Agreed, I was also not very happy with the similarity present at these
> > two places,
> > especially the wildcard and tag part, tried to convulse them into something
> > singular. It again started to look like the original so ultimately
> > kept it like this.
> >
> >>
> >> By the way, I'd really prefer to see contributors *NOT* to use
> >> undeliverable and/or bouncing e-mail addresses when working on this
> >> project, as I'd always have to edit the Cc: list to avoid getting
> >> bounces.
> >>
> >> Thanks.
> >>
> >
> > Thanks, I will take care.
>
> Thanks.



-- 
Thanks,
Amisha

Re: [PATCH v4 1/1] help: cleanup the contruction of keys_uniq

[Junio C Hamano]

Amisha Chhajed <amishhhaaaa@gmail.com> writes:

> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
> index d3a0967e9d..03104b3bf4 100755
> --- a/t/t0012-help.sh
> +++ b/t/t0012-help.sh
> @@ -141,20 +141,20 @@ test_expect_success 'git help -c' '
>  
>  	'\''git help config'\'' for more information
>  	EOF
> -	grep -v -E \
> -		-e "^[^.]+\.[^.]+$" \
> -		-e "^[^.]+\.[^.]+\.[^.]+$" \
> +	sed \
> +		-e "/^[^.]*\.[^.]*$/d" \
> +		-e "/^[^.]*\.[^.]*\.[^.]*$/d" \
>  		help.output >actual &&

We used to require at least one non-dot byte between each dot in the
original, but now we do not.  Is this change in semantics intended?

You could fix it with "sed -E" and keeping the ERE in the original,
I guess?

It was in a distant past when I tried benchmarking them for the last
time, but I recall "sed" was a lot slower than "grep" on a "match
and print" job that "grep" could be an alternative.  So I am not
sure what the point of the change in this hunk is.

>  	test_cmp expect actual
>  '
>  
>  test_expect_success 'git help --config-for-completion' '
>  	git help -c >human &&
> -	grep -E \
> -	     -e "^[^.]+\.[^.]+$" \
> -	     -e "^[^.]+\.[^.]+\.[^.]+$" human |
> -	     sed -e "s/\*.*//" -e "s/<.*//" |
> -	     sort -u >human.munged &&
> +	sed -n \
> +	     -e "/^[^.]*\.[^.]*$/p" \
> +	     -e "/^[^.]*\.[^.]*\.[^.]*$/p" human |
> +	sed -e "s/\*.*//" -e "s/<.*//" |

Ditto.

>  test_expect_success 'git help --config-sections-for-completion' '
>  	git help -c >human &&
> -	grep -E \
> -	     -e "^[^.]+\.[^.]+$" \
> -	     -e "^[^.]+\.[^.]+\.[^.]+$" human |
> -	     sed -e "s/\..*//" |
> -	     sort -u >human.munged &&
> +	sed -n \
> +	     -e "/^[^.]*\.[^.]*$/p" \
> +	     -e "/^[^.]*\.[^.]*\.[^.]*$/p" human |
> +	sed -e "s/\..*//" |

Ditto.

Just like piping "grep" output to "sed" is an anti-pattern, piping
"sed" output to an invocation of "sed" is often an anti-pattern.

Perhaps something like this would replace the original "grep | sed"
pipeline?

	sed -E -e "
		/^[^.]+\.[^.]+$/b out
		/^[^.]+\.[^.]+\.[^.]+$/b out
		d
		: out
		s/\..*//
	" human |
	sort -u

Re: [PATCH v3 2/2] help: cleanup the contruction of keys_uniq

[Junio C Hamano]

Amisha Chhajed <amishhhaaaa@gmail.com> writes:

> Incredibly sorry for the bouncing mail once again, I will fix it locally.

Thanks for noticing.  Your v4 has the same address.

[PATCH v5] help: cleanup the contruction of keys_uniq

[Amisha Chhajed]

From: Amisha Chhajed <136238836+amishhaa@users.noreply.github.com>

construction of keys_uniq depends on sort operation
executed on keys before processing, which does not
gurantee that keys_uniq will be sorted.

refactor the code to shift the sort operation after
the processing to remove dependency on key's sort operation
and strictly maintain the sorted order of keys_uniq.

move strbuf init and release out of loop to reuse same buffer.

dedent sort -u and sed in tests and replace grep with sed, to
avoid piping grep's output to sed.

Suggested-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
Signed-off-by: Amisha Chhajed <amishhhaaaa@gmail.com>
---
 builtin/help.c  | 91 ++++++++++++++++++++++++++++++-------------------
 t/t0012-help.sh | 39 ++++++++++++---------
 2 files changed, 78 insertions(+), 52 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index c09cbc8912..daadc61c70 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -111,6 +111,49 @@ struct slot_expansion {
 	int found;
 };
 
+static void set_config_vars(struct string_list *keys_uniq, struct string_list_item *var)
+{
+	struct strbuf sb = STRBUF_INIT;
+	const char *str = var->string;
+	const char *wildcard = strchr(str, '*');
+	const char *tag = strchr(str, '<');
+	const char *cut;
+
+	if (wildcard && tag)
+		cut = wildcard < tag ? wildcard : tag;
+	else if (wildcard)
+		cut = wildcard;
+	else if (tag)
+		cut = tag;
+	else {
+		string_list_append(keys_uniq, str);
+		return;
+	}
+
+	strbuf_add(&sb, str, cut - str);
+	string_list_append(keys_uniq, sb.buf);
+	strbuf_release(&sb);
+}
+
+static void set_config_sections(struct string_list *keys_uniq, struct string_list_item *var)
+{
+	struct strbuf sb = STRBUF_INIT;
+	const char *str = var->string;
+	const char *dot = strchr(str, '.');
+	const char *cut;
+
+	if (dot)
+		cut = dot;
+	else {
+		set_config_vars(keys_uniq, var);
+		return;
+	}
+
+	strbuf_add(&sb, str, cut - str);
+	string_list_append(keys_uniq, sb.buf);
+	strbuf_release(&sb);
+}
+
 static void list_config_help(enum show_config_type type)
 {
 	struct slot_expansion slot_expansions[] = {
@@ -131,13 +174,12 @@ static void list_config_help(enum show_config_type type)
 	struct string_list keys = STRING_LIST_INIT_DUP;
 	struct string_list keys_uniq = STRING_LIST_INIT_DUP;
 	struct string_list_item *item;
+	struct strbuf sb = STRBUF_INIT;
 
 	for (p = config_name_list; *p; p++) {
 		const char *var = *p;
-		struct strbuf sb = STRBUF_INIT;
 
 		for (e = slot_expansions; e->prefix; e++) {
-
 			strbuf_reset(&sb);
 			strbuf_addf(&sb, "%s.%s", e->prefix, e->placeholder);
 			if (!strcasecmp(var, sb.buf)) {
@@ -146,60 +188,39 @@ static void list_config_help(enum show_config_type type)
 				break;
 			}
 		}
-		strbuf_release(&sb);
+
 		if (!e->prefix)
 			string_list_append(&keys, var);
 	}
 
+	strbuf_release(&sb);
+
 	for (e = slot_expansions; e->prefix; e++)
 		if (!e->found)
 			BUG("slot_expansion %s.%s is not used",
 			    e->prefix, e->placeholder);
 
-	string_list_sort(&keys);
 	for (size_t i = 0; i < keys.nr; i++) {
-		const char *var = keys.items[i].string;
-		const char *wildcard, *tag, *cut;
-		const char *dot = NULL;
-		struct strbuf sb = STRBUF_INIT;
-
 		switch (type) {
 		case SHOW_CONFIG_HUMAN:
-			puts(var);
-			continue;
+			string_list_append(&keys_uniq, keys.items[i].string);
+			break;
 		case SHOW_CONFIG_SECTIONS:
-			dot = strchr(var, '.');
+			set_config_sections(&keys_uniq, &keys.items[i]);
 			break;
 		case SHOW_CONFIG_VARS:
+			set_config_vars(&keys_uniq, &keys.items[i]);
 			break;
+		default:
+			BUG("%d: unexpected type", type);
 		}
-		wildcard = strchr(var, '*');
-		tag = strchr(var, '<');
-
-		if (!dot && !wildcard && !tag) {
-			string_list_append(&keys_uniq, var);
-			continue;
-		}
-
-		if (dot)
-			cut = dot;
-		else if (wildcard && !tag)
-			cut = wildcard;
-		else if (!wildcard && tag)
-			cut = tag;
-		else
-			cut = wildcard < tag ? wildcard : tag;
-
-		strbuf_add(&sb, var, cut - var);
-		string_list_append(&keys_uniq, sb.buf);
-		strbuf_release(&sb);
-
 	}
-	string_list_clear(&keys, 0);
-	string_list_remove_duplicates(&keys_uniq, 0);
+
+	string_list_sort_u(&keys_uniq, 0);
 	for_each_string_list_item(item, &keys_uniq)
 		puts(item->string);
 	string_list_clear(&keys_uniq, 0);
+	string_list_clear(&keys, 0);
 }
 
 static enum help_format parse_help_format(const char *format)
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index d3a0967e9d..40b2d656a5 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -141,20 +141,23 @@ test_expect_success 'git help -c' '
 
 	'\''git help config'\'' for more information
 	EOF
-	grep -v -E \
-		-e "^[^.]+\.[^.]+$" \
-		-e "^[^.]+\.[^.]+\.[^.]+$" \
-		help.output >actual &&
+	sed -E -e "
+		/^[^.]+\.[^.]+$/d
+		/^[^.]+\.[^.]+\.[^.]+$/d
+	" help.output >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'git help --config-for-completion' '
 	git help -c >human &&
-	grep -E \
-	     -e "^[^.]+\.[^.]+$" \
-	     -e "^[^.]+\.[^.]+\.[^.]+$" human |
-	     sed -e "s/\*.*//" -e "s/<.*//" |
-	     sort -u >human.munged &&
+	sed -E -e "
+		/^[^.]+\.[^.]+$/b out
+		/^[^.]+\.[^.]+\.[^.]+$/b out
+		d
+		: out
+		s/\*.*//
+		s/<.*//
+	" human | sort -u >human.munged &&
 
 	git help --config-for-completion >vars &&
 	test_cmp human.munged vars
@@ -162,14 +165,16 @@ test_expect_success 'git help --config-for-completion' '
 
 test_expect_success 'git help --config-sections-for-completion' '
 	git help -c >human &&
-	grep -E \
-	     -e "^[^.]+\.[^.]+$" \
-	     -e "^[^.]+\.[^.]+\.[^.]+$" human |
-	     sed -e "s/\..*//" |
-	     sort -u >human.munged &&
-
-	git help --config-sections-for-completion >sections &&
-	test_cmp human.munged sections
+	sed -E -e "
+		/^[^.]+\.[^.]+$/b out
+		/^[^.]+\.[^.]+\.[^.]+$/b out
+		d
+		: out
+		s/\..*//
+	" human | sort -u >expect &&
+	
+	git help --config-sections-for-completion >actual &&
+	test_cmp expect actual
 '
 
 test_section_spacing () {
-- 
2.52.0

[PATCH v5] help: cleanup the contruction of keys_uniq

[Amisha Chhajed]

construction of keys_uniq depends on sort operation
executed on keys before processing, which does not
gurantee that keys_uniq will be sorted.

refactor the code to shift the sort operation after
the processing to remove dependency on key's sort operation
and strictly maintain the sorted order of keys_uniq.

move strbuf init and release out of loop to reuse same buffer.

dedent sort -u and sed in tests and replace grep with sed, to
avoid piping grep's output to sed.

Suggested-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
Signed-off-by: Amisha Chhajed <amishhhaaaa@gmail.com>
---
 builtin/help.c  | 91 ++++++++++++++++++++++++++++++-------------------
 t/t0012-help.sh | 39 ++++++++++++---------
 2 files changed, 78 insertions(+), 52 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index c09cbc8912..daadc61c70 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -111,6 +111,49 @@ struct slot_expansion {
 	int found;
 };
 
+static void set_config_vars(struct string_list *keys_uniq, struct string_list_item *var)
+{
+	struct strbuf sb = STRBUF_INIT;
+	const char *str = var->string;
+	const char *wildcard = strchr(str, '*');
+	const char *tag = strchr(str, '<');
+	const char *cut;
+
+	if (wildcard && tag)
+		cut = wildcard < tag ? wildcard : tag;
+	else if (wildcard)
+		cut = wildcard;
+	else if (tag)
+		cut = tag;
+	else {
+		string_list_append(keys_uniq, str);
+		return;
+	}
+
+	strbuf_add(&sb, str, cut - str);
+	string_list_append(keys_uniq, sb.buf);
+	strbuf_release(&sb);
+}
+
+static void set_config_sections(struct string_list *keys_uniq, struct string_list_item *var)
+{
+	struct strbuf sb = STRBUF_INIT;
+	const char *str = var->string;
+	const char *dot = strchr(str, '.');
+	const char *cut;
+
+	if (dot)
+		cut = dot;
+	else {
+		set_config_vars(keys_uniq, var);
+		return;
+	}
+
+	strbuf_add(&sb, str, cut - str);
+	string_list_append(keys_uniq, sb.buf);
+	strbuf_release(&sb);
+}
+
 static void list_config_help(enum show_config_type type)
 {
 	struct slot_expansion slot_expansions[] = {
@@ -131,13 +174,12 @@ static void list_config_help(enum show_config_type type)
 	struct string_list keys = STRING_LIST_INIT_DUP;
 	struct string_list keys_uniq = STRING_LIST_INIT_DUP;
 	struct string_list_item *item;
+	struct strbuf sb = STRBUF_INIT;
 
 	for (p = config_name_list; *p; p++) {
 		const char *var = *p;
-		struct strbuf sb = STRBUF_INIT;
 
 		for (e = slot_expansions; e->prefix; e++) {
-
 			strbuf_reset(&sb);
 			strbuf_addf(&sb, "%s.%s", e->prefix, e->placeholder);
 			if (!strcasecmp(var, sb.buf)) {
@@ -146,60 +188,39 @@ static void list_config_help(enum show_config_type type)
 				break;
 			}
 		}
-		strbuf_release(&sb);
+
 		if (!e->prefix)
 			string_list_append(&keys, var);
 	}
 
+	strbuf_release(&sb);
+
 	for (e = slot_expansions; e->prefix; e++)
 		if (!e->found)
 			BUG("slot_expansion %s.%s is not used",
 			    e->prefix, e->placeholder);
 
-	string_list_sort(&keys);
 	for (size_t i = 0; i < keys.nr; i++) {
-		const char *var = keys.items[i].string;
-		const char *wildcard, *tag, *cut;
-		const char *dot = NULL;
-		struct strbuf sb = STRBUF_INIT;
-
 		switch (type) {
 		case SHOW_CONFIG_HUMAN:
-			puts(var);
-			continue;
+			string_list_append(&keys_uniq, keys.items[i].string);
+			break;
 		case SHOW_CONFIG_SECTIONS:
-			dot = strchr(var, '.');
+			set_config_sections(&keys_uniq, &keys.items[i]);
 			break;
 		case SHOW_CONFIG_VARS:
+			set_config_vars(&keys_uniq, &keys.items[i]);
 			break;
+		default:
+			BUG("%d: unexpected type", type);
 		}
-		wildcard = strchr(var, '*');
-		tag = strchr(var, '<');
-
-		if (!dot && !wildcard && !tag) {
-			string_list_append(&keys_uniq, var);
-			continue;
-		}
-
-		if (dot)
-			cut = dot;
-		else if (wildcard && !tag)
-			cut = wildcard;
-		else if (!wildcard && tag)
-			cut = tag;
-		else
-			cut = wildcard < tag ? wildcard : tag;
-
-		strbuf_add(&sb, var, cut - var);
-		string_list_append(&keys_uniq, sb.buf);
-		strbuf_release(&sb);
-
 	}
-	string_list_clear(&keys, 0);
-	string_list_remove_duplicates(&keys_uniq, 0);
+
+	string_list_sort_u(&keys_uniq, 0);
 	for_each_string_list_item(item, &keys_uniq)
 		puts(item->string);
 	string_list_clear(&keys_uniq, 0);
+	string_list_clear(&keys, 0);
 }
 
 static enum help_format parse_help_format(const char *format)
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index d3a0967e9d..40b2d656a5 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -141,20 +141,23 @@ test_expect_success 'git help -c' '
 
 	'\''git help config'\'' for more information
 	EOF
-	grep -v -E \
-		-e "^[^.]+\.[^.]+$" \
-		-e "^[^.]+\.[^.]+\.[^.]+$" \
-		help.output >actual &&
+	sed -E -e "
+		/^[^.]+\.[^.]+$/d
+		/^[^.]+\.[^.]+\.[^.]+$/d
+	" help.output >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'git help --config-for-completion' '
 	git help -c >human &&
-	grep -E \
-	     -e "^[^.]+\.[^.]+$" \
-	     -e "^[^.]+\.[^.]+\.[^.]+$" human |
-	     sed -e "s/\*.*//" -e "s/<.*//" |
-	     sort -u >human.munged &&
+	sed -E -e "
+		/^[^.]+\.[^.]+$/b out
+		/^[^.]+\.[^.]+\.[^.]+$/b out
+		d
+		: out
+		s/\*.*//
+		s/<.*//
+	" human | sort -u >human.munged &&
 
 	git help --config-for-completion >vars &&
 	test_cmp human.munged vars
@@ -162,14 +165,16 @@ test_expect_success 'git help --config-for-completion' '
 
 test_expect_success 'git help --config-sections-for-completion' '
 	git help -c >human &&
-	grep -E \
-	     -e "^[^.]+\.[^.]+$" \
-	     -e "^[^.]+\.[^.]+\.[^.]+$" human |
-	     sed -e "s/\..*//" |
-	     sort -u >human.munged &&
-
-	git help --config-sections-for-completion >sections &&
-	test_cmp human.munged sections
+	sed -E -e "
+		/^[^.]+\.[^.]+$/b out
+		/^[^.]+\.[^.]+\.[^.]+$/b out
+		d
+		: out
+		s/\..*//
+	" human | sort -u >expect &&
+	
+	git help --config-sections-for-completion >actual &&
+	test_cmp expect actual
 '
 
 test_section_spacing () {
-- 
2.52.0

Re: [PATCH v5] help: cleanup the contruction of keys_uniq

[Junio C Hamano]

Amisha Chhajed <amishhhaaaa@gmail.com> writes:

> @@ -162,14 +165,16 @@ test_expect_success 'git help --config-for-completion' '
> ...
> +	sed -E -e "
> +		/^[^.]+\.[^.]+$/b out
> +		/^[^.]+\.[^.]+\.[^.]+$/b out
> +		d
> +		: out
> +		s/\..*//
> +	" human | sort -u >expect &&
> +	
> +	git help --config-sections-for-completion >actual &&
> +	test_cmp expect actual
>  '

The blank has a HT, which is a trailing whitespace.  No need to
resend only to correct this, as "git am" on my end cleaned it up
already while queuing.

Re: [PATCH v4 1/1] help: cleanup the contruction of keys_uniq

[Amisha Chhajed]

> Perhaps something like this would replace the original "grep | sed"
> pipeline?
>
>         sed -E -e "
>                 /^[^.]+\.[^.]+$/b out
>                 /^[^.]+\.[^.]+\.[^.]+$/b out
>                 d
>                 : out
>                 s/\..*//
>         " human |
>         sort -u
>
>

Thank you for pointing me in the right direction!

-- 
Thanks,
Amisha

Re: [PATCH v4 1/1] help: cleanup the contruction of keys_uniq

[Junio C Hamano]

Amisha Chhajed <amishhhaaaa@gmail.com> writes:

>> Perhaps something like this would replace the original "grep | sed"
>> pipeline?
>>
>>         sed -E -e "
>>                 /^[^.]+\.[^.]+$/b out
>>                 /^[^.]+\.[^.]+\.[^.]+$/b out
>>                 d
>>                 : out
>>                 s/\..*//
>>         " human |
>>         sort -u
>>
>>
>
> Thank you for pointing me in the right direction!

This unfortunately runs afoul of t/check-non-portable-shell.pl aka
"make -C t test-lint".  The particular rule was introduced in early
2019 with e62e225f (test-lint: only use only sed [-n] [-e command]
[-f command_file], 2019-01-20).  See the attached patch at the end.

It does cite the then-current POSIX.1 (Issue 7, 2018 edition) but
the latest edition (Issue 8) documents "-E" as an option to use ERE

I wonder if the situation has improved in the past 7 years.

We seem to have started using "sed -E" without anybody complaining
in 2022, with 461fec41 (bisect run: keep some of the post-v2.30.0
output, 2022-11-10).  It was hidden because the 'E' was squished
with another single letter option.

t/t6030-bisect-porcelain.sh:	sed -En 's/.*(bisect.*code) (-?[0-9]+) (from.*)/\1 -1 \3/p' err >actual &&

So I think of no strong reason to reject another new use of "sed
-E".  I am tempted to revert e62e225f (test-lint: only use only sed
[-n] [-e command] [-f command_file], 2019-01-20), whose intention
was to reject anything other than "-[efn]", to its previous form
which rejected only "sed -i".

Alternatively, I would of course welcome volunteers to revamp the
check-non-portable-shell.pl script to make the pattern more robust,
and then add 'E' to the set of allowed options, but I somehow do not
think that is a good use of our engineering resources.

For example, in addition to the escape we see in t6030 above, the
current pattern would not catch use of -E if it is written this way:

	sed "-E" -e "
		...
	" human |
	sort -u

or

	sed \
		-E -e "
		...
	" human |
	sort -u

and million other ways to subvert the simple-minded pattern-match
based check.

Opinions?


commit e62e225ffb589e59c4f64d90b0a393aa6a0a5ace
Author: Torsten Bögershausen <tboegi@web.de>
Date:   Sun Jan 20 08:53:50 2019 +0100

    test-lint: only use only sed [-n] [-e command] [-f command_file]
    
    From `man sed` (on a Mac OS X box):
    The -E, -a and -i options are non-standard FreeBSD extensions and may not be available
    on other operating systems.
    
    From `man sed` on a Linux box:
    REGULAR EXPRESSIONS
           POSIX.2 BREs should be supported, but they aren't completely because of
           performance problems.  The \n sequence in a regular expression matches the newline
           character,  and  similarly  for \a, \t, and other sequences.
           The -E option switches to using extended regular expressions instead; the -E option
           has been supported for years by GNU sed, and is now included in POSIX.
    
    Well, there are still a lot of systems out there, which don't support it.
    Beside that, IEEE Std 1003.1TM-2017, see
    http://pubs.opengroup.org/onlinepubs/9699919799/
    does not mention -E either.
    
    To be on the safe side, don't allow -E (or -r, which is GNU).
    Change check-non-portable-shell.pl to only accept the portable options:
    sed [-n] [-e command] [-f command_file]
    
    Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
    Helped-by: Eric Sunshine <sunshine@sunshineco.com>
    Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    Signed-off-by: Torsten Bögershausen <tboegi@web.de>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index b45bdac688..f0edcf8eb0 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -35,7 +35,7 @@ sub err {
 		chomp;
 	}
 
-	/\bsed\s+-i/ and err 'sed -i is not portable';
+	/\bsed\s+-[^efn]\s+/ and err 'sed option not portable (use only -n, -e, -f)';
 	/\becho\s+-[neE]/ and err 'echo with option is not portable (use printf)';
 	/^\s*declare\s+/ and err 'arrays/declare not portable';
 	/^\s*[^#]\s*which\s/ and err 'which is not portable (use type)';

Re: [PATCH v4 1/1] help: cleanup the contruction of keys_uniq

[Eric Sunshine]

On Wed, Mar 11, 2026 at 5:11 PM Junio C Hamano <gitster@pobox.com> wrote:
> For example, in addition to the escape we see in t6030 above, the
> current pattern would not catch use of -E if it is written this way:
>
>         sed "-E" -e "
>                 ...
>         " human |
>         sort -u

Seems unlikely to arise in practice.

> or
>
>         sed \
>                 -E -e "
>                 ...
>         " human |
>         sort -u

For what it's worth, line folding capability was added to
check-non-portable-shell.pl by a0a630192d (t/check-non-portable-shell:
detect "FOO=bar shell_func", 2018-07-13), so it does correctly detect
the errant -E in this example.

> and million other ways to subvert the simple-minded pattern-match
> based check.

True, for sure.

Re: [PATCH v4 1/1] help: cleanup the contruction of keys_uniq

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>>         sed \
>>                 -E -e "
>>                 ...
>>         " human |
>>         sort -u
>
> For what it's worth, line folding capability was added to
> check-non-portable-shell.pl by a0a630192d (t/check-non-portable-shell:
> detect "FOO=bar shell_func", 2018-07-13), so it does correctly detect
> the errant -E in this example.

Ah, thanks for correcting me.

But "sed -n -i -e '/.../p'" would not catch "-i", and that is not
all that unlikely, I suspect.

Re: [PATCH v4 1/1] help: cleanup the contruction of keys_uniq

[Eric Sunshine]

On Wed, Mar 11, 2026 at 5:51 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> >>         sed \
> >>                 -E -e "
> >>                 ...
> >>         " human |
> >>         sort -u
> >
> > For what it's worth, line folding capability was added to
> > check-non-portable-shell.pl by a0a630192d (t/check-non-portable-shell:
> > detect "FOO=bar shell_func", 2018-07-13), so it does correctly detect
> > the errant -E in this example.
>
> Ah, thanks for correcting me.
>
> But "sed -n -i -e '/.../p'" would not catch "-i", and that is not
> all that unlikely, I suspect.

Correct. By only looking at the very first option following the
command name (`sed`), the checking performed by
check-non-portable-shell.pl is very weak indeed.