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

From: Kousik Sanagavarapu <five231003@gmail.com>
Date: Dec 3, 2023, 08:22 UTC
Message-ID: <ZWw6r2EDGkpgyYEM@five231003>
In-Reply-To: <CAPYXD64yCuMta_iGE+ZwgxrJn0U5shcwcB9jaiNkFhvff=R7MQ@mail.gmail.com>

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"

Show 23 quoted lines

> 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