[Patch] test-lib-functions.sh : change test_i18ngrep to test_grep

[Patch] test-lib-functions.sh : change test_i18ngrep to test_grep

[Shreyansh Paliwal]

Recently the test_i18ngrep was deprecated from the source code and
test_grep was implemented but in the test-lib-functions.sh file , in
the test_grep() function definition,
it is written BUG "too few parameters to test_i18ngrep".
So the following patch solves the minor problem.

Signed-off-by: Shreyansh Paliwal <Shreyanshpaliwalcmsmn@gmail.com>
---
 t/test-lib-functions.sh | 2 +-
 1 file changed, 1 insertions(+), 1 deletions(-)

 t/test-lib-functions.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)diff --git
a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 9c3cf12b26..8737c95e0c 100644
--- a/t/test-lib-functions.sh
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1277,7 +1277,7 @@ test_grep () {
        if test $# -lt 2 ||
           { test "x!" = "x$1" && test $# -lt 3 ; }
        then
-               BUG "too few parameters to test_i18ngrep"
+               BUG "too few parameters to test_grep"
        fi

        if test "x!" = "x$1"
--
2.43

Re: [Patch] test-lib-functions.sh : change test_i18ngrep to test_grep

[Kousik Sanagavarapu]

Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> wrote:

> Subject: [Patch] test-lib-functions.sh : change test_i18ngrep to test_grep

For anyone reading the subject, I think reading

	change test_i18ngrep to test_grep

would be confusing, as from the looks of it, the patch does remove
test_i18ngrep() and replace it with test_grep (I mean the plan is to
remove test_i18ngrep only after we are sure that it doesn't exist in the
code anywhere, anymore) but only making a change in the wording of an
error message within test_grep().

Also I think we can drop the SP after "related topic" part of the patch
and the colon (but have the SP after the colon), that is

	"test-lib-functions.sh: ..."

Also, nit, but I think we should have [PATCH] instead of [Patch]. I'm not
really sure if Junio's setup treats [PATCH] and [Patch] to be same :)

> Recently the test_i18ngrep was deprecated from the source code and
> test_grep was implemented but in the test-lib-functions.sh file , in
> the test_grep() function definition,

This recent deprecation was made in the commit,
2e87fca189 (test framework: further deprecate test_i18ngrep, 2023-10-31)
and it makes sense to include it in the commit message as the following
change is essentially something that the previous commit seems to have
forgotten to do.

> it is written BUG "too few parameters to test_i18ngrep".

I think it is not necessary to mention what is the current code
in _this case_ as it can be read in the change itself :)

> So the following patch solves the minor problem.

What exactly is the problem? I think it should be mentioned in the commit
message that the wording of the error message causes confusion ;) as when
test_grep() is used in a test and this test fails. That the change is - it
would be clear to see

	"too few parameters to test_grep"

instead of

	"too few parameters to test_i18ngrep"

> Signed-off-by: Shreyansh Paliwal <Shreyanshpaliwalcmsmn@gmail.com>
> ---
>  t/test-lib-functions.sh | 2 +-
>  1 file changed, 1 insertions(+), 1 deletions(-)
> 
>  t/test-lib-functions.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)diff --git
> a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 9c3cf12b26..8737c95e0c 100644
> --- a/t/test-lib-functions.sh
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1277,7 +1277,7 @@ test_grep () {
>         if test $# -lt 2 ||
>            { test "x!" = "x$1" && test $# -lt 3 ; }
>         then
> -               BUG "too few parameters to test_i18ngrep"
> +               BUG "too few parameters to test_grep"
>         fi
> 
>         if test "x!" = "x$1"
> --
> 2.43

The diff format doesn't seem proper (some repeated lines and no newlines
at the required places).

If you have no go-to tool to send patches through email then git-send-email
is a really good tool to do it. It handles most of the work for you.
"MyFirstContribution" has a guide to do so

	https://git-send-email.io/ (also has setup with GMail)
	https://git-scm.com/docs/MyFirstContribution#howto-git-send-email

Another good resource which is not linked often is

	https://flusp.ime.usp.br/git/sending-patches-by-email-with-git/

by Matheus Tavares, also a Git Contributor. It also has other useful links
which are worth a read.

Thanks

Re: [Patch] test-lib-functions.sh : change test_i18ngrep to test_grep

[Junio C Hamano]

Kousik Sanagavarapu <five231003@gmail.com> writes:

> Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> wrote:
>
>> Subject: [Patch] test-lib-functions.sh : change test_i18ngrep to test_grep
>
> For anyone reading the subject, I think reading
>
> 	change test_i18ngrep to test_grep
>
> would be confusing, as from the looks of it, the patch does remove
> test_i18ngrep() and replace it with test_grep (I mean the plan is to
> remove test_i18ngrep only after we are sure that it doesn't exist in the
> code anywhere, anymore) but only making a change in the wording of an
> error message within test_grep().

;-)  

Yes, that was exactly my reaction to the subject (I'm on
vacation so I only scanned the subject lines of incoming patches
without looking at anything else and thought "hmph, it is good
somebody else is cleaning up new uses of test_i18ngrep that have
been introduced by topics simultaneously in flight").

[PATCH v2] test-lib-functions.sh: fix test_grep fail message wording

[Shreyansh Paliwal]

From: shreyp135 <shreyanshpaliwalcmsmn@gmail.com>

In the recent commit
2e87fca189 (test framework: further deprecate test_i18ngrep, 2023-10-31),
the test_i18ngrep() function was deprecated.

So if a test employing this function fails,
the error messages may be confusing due to wording issues.

It's important to address these wording changes to ensure smooth transitions
for developers adapting to the deprecation of test_i18ngrep,
and to maintain the effectiveness of the testing process.

Signed-off-by: Shreyansh Paliwal <Shreyanshpaliwalcmsmn@gmail.com>
---
 t/test-lib-functions.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 9c3cf12b26..8737c95e0c 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1277,7 +1277,7 @@ test_grep () {
 	if test $# -lt 2 ||
 	   { test "x!" = "x$1" && test $# -lt 3 ; }
 	then
-		BUG "too few parameters to test_i18ngrep"
+		BUG "too few parameters to test_grep"
 	fi
 
 	if test "x!" = "x$1"
-- 
2.43.0.1

Re: [PATCH v2] test-lib-functions.sh: fix test_grep fail message wording

[Kousik Sanagavarapu]

On Sun, Dec 03, 2023 at 10:47:59PM +0530, Shreyansh Paliwal wrote:
> From: shreyp135 <shreyanshpaliwalcmsmn@gmail.com>
> 
> In the recent commit
> 2e87fca189 (test framework: further deprecate test_i18ngrep, 2023-10-31),
> the test_i18ngrep() function was deprecated.

s/In the/In a

is gramatically correct, but probably not worth a reroll.

> So if a test employing this function fails,
> the error messages may be confusing due to wording issues.

Isn't the confusion due to test_i18ngrep being displayed in place of
test_grep and not the other way around? Because the formation of the
sentence makes it look like the latter.

> It's important to address these wording changes to ensure smooth transitions
> for developers adapting to the deprecation of test_i18ngrep,
> and to maintain the effectiveness of the testing process.
> 
> Signed-off-by: Shreyansh Paliwal <Shreyanshpaliwalcmsmn@gmail.com>
> ---
>  t/test-lib-functions.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 9c3cf12b26..8737c95e0c 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1277,7 +1277,7 @@ test_grep () {
>  	if test $# -lt 2 ||
>  	   { test "x!" = "x$1" && test $# -lt 3 ; }
>  	then
> -		BUG "too few parameters to test_i18ngrep"
> +		BUG "too few parameters to test_grep"
>  	fi
>  
>  	if test "x!" = "x$1"
> -- 
> 2.43.0.1

Rest looks good.

Have a great time at the vacation Junio (and sorry for pinging in the
first place... although this email will indirectly ping too :P).

Thanks

Re: [PATCH v2] test-lib-functions.sh: fix test_grep fail message wording

[Shreyansh Paliwal]

From: shreyp135 <shreyanshpaliwalcmsmn@gmail.com>

ping.

Re: [PATCH v2] test-lib-functions.sh: fix test_grep fail message wording

[Eric Sunshine]

On Sun, Dec 17, 2023 at 10:32 AM Shreyansh Paliwal
<shreyanshpaliwalcmsmn@gmail.com> wrote:
> ping.

Junio was on vacation at the time[1] that this patch was submitted, so
it's quite possible that it simply got overlooked or he hasn't gotten
through the backlog of emails which accumulated while he was away. So,
pinging is indeed the correct thing to do, and the patch is obviously
an improvement, so hopefully it will be picked up soon.

[1]: https://lore.kernel.org/git/xmqq34wj4e55.fsf@gitster.g/

Re: [PATCH v2] test-lib-functions.sh: fix test_grep fail message wording

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Sun, Dec 17, 2023 at 10:32 AM Shreyansh Paliwal
> <shreyanshpaliwalcmsmn@gmail.com> wrote:
>> ping.
>
> Junio was on vacation at the time[1] that this patch was submitted, so
> it's quite possible that it simply got overlooked or he hasn't gotten
> through the backlog of emails which accumulated while he was away.

It was dropped due to automated filter that noticed that the address
on its in-body From: line does not appear on any of its Signed-off-by:
line ;-)

I'll see if that is the only glitch in the patch (in which case I'll
manually adjust the authorship and apply) or respond on list
(otherwise).

Thanks for pinging and ponging.

> So,
> pinging is indeed the correct thing to do, and the patch is obviously
> an improvement, so hopefully it will be picked up soon.
>
> [1]: https://lore.kernel.org/git/xmqq34wj4e55.fsf@gitster.g/

Re: [PATCH v2] test-lib-functions.sh: fix test_grep fail message wording

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I'll see if that is the only glitch in the patch (in which case I'll
> manually adjust the authorship and apply) or respond on list
> (otherwise).
>
> Thanks for pinging and ponging.

Here is the version I queued.
Thanks, both.

--- >8 ---
From: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>
Date: Sun, 3 Dec 2023 22:47:59 +0530
Subject: [PATCH] test-lib-functions.sh: fix test_grep fail message wording

In the recent commit 2e87fca189 (test framework: further deprecate
test_i18ngrep, 2023-10-31), the test_i18ngrep function was
deprecated, and all the callers were updated to call the test_grep
function instead.  But test_grep inherited an error message that
still refers to test_i18ngrep by mistake.  Correct it so that a
broken call to the test_grep will identify itself as such.

Signed-off-by: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/test-lib-functions.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index c50bc18861..502f892fad 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1222,7 +1222,7 @@ test_grep () {
 	if test $# -lt 2 ||
 	   { test "x!" = "x$1" && test $# -lt 3 ; }
 	then
-		BUG "too few parameters to test_i18ngrep"
+		BUG "too few parameters to test_grep"
 	fi
 
 	if test "x!" = "x$1"
-- 
2.43.0-76-g1a87c842ec

Re: [PATCH v2] test-lib-functions.sh: fix test_grep fail message wording

[Eric Sunshine]

On Mon, Dec 18, 2023 at 1:47 PM Junio C Hamano <gitster@pobox.com> wrote:
> Here is the version I queued.
>
> --- >8 ---
> From: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>
> Date: Sun, 3 Dec 2023 22:47:59 +0530
> Subject: [PATCH] test-lib-functions.sh: fix test_grep fail message wording
>
> In the recent commit 2e87fca189 (test framework: further deprecate
> test_i18ngrep, 2023-10-31), the test_i18ngrep function was
> deprecated, and all the callers were updated to call the test_grep
> function instead.  But test_grep inherited an error message that
> still refers to test_i18ngrep by mistake.  Correct it so that a
> broken call to the test_grep will identify itself as such.

This rewritten commit message gets directly to the point without
wasted words, making the purpose of the patch, and its justification,
easier to understand on first read. Nicely done.

> Signed-off-by: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>