patchtests: modernize the test script t0010-racy-git.sh

From: aryangupta701 <garyan447@gmail.com>

Modernize the formatting of the test script to align with current standards and improve its overall readability.

Signed-off-by: Aryan Gupta <garyan447@gmail.com>
---
    [GSOC] [PATCH] Modernize a test script

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1675%2Faryangupta701%2Ftest-modernize-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1675/aryangupta701/test-modernize-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1675

 t/t0010-racy-git.sh | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

On Thu, Feb 29, 2024 at 7:24 AM Aryan Gupta via GitGitGadget <gitgitgadget@gmail.com> wrote:

The name in the "From:" header should match the name in the "Signed-off-by:" trailer.

The aim of this patch makes sense, but the implementation isn't quite correct.

The variable `trial` takes on values 0-4, and that value is meant to appear in the test title as the script runs:

    Racy GIT trial #0 part A
    Racy GIT trial #0 part B
    Racy GIT trial #1 part A
    Racy GIT trial #1 part B
    ...

However, by changing the title from a double-quote string to a single-quote string, you inhibit interpolation of the `trial` variable into the title, hence the test titles instead show up as:

    Racy GIT trial #$trial part A
    Racy GIT trial #$trial part B
    Racy GIT trial #$trial part A
    Racy GIT trial #$trial part B

which is undesirable.

So, the title should continue using double-quotes, and not be changed to single-quotes.

The other change, which fixes the style of the test's body, appears correct.

From: Aryan Gupta <garyan447@gmail.com>

Modernize the formatting of the test script to align with current standards and improve its overall readability.

Signed-off-by: Aryan Gupta <garyan447@gmail.com>
---
    [GSOC][PATCH] Modernize a test script

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1675%2Faryangupta701%2Ftest-modernize-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1675/aryangupta701/test-modernize-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1675

Range-diff vs v1:

 1:  06377119ea3 ! 1:  fa381d0b57a tests: modernize the test script t0010-racy-git.sh
     @@
       ## Metadata ##
     -Author: aryangupta701 <garyan447@gmail.com>
     +Author: Aryan Gupta <garyan447@gmail.com>
      
       ## Commit message ##
          tests: modernize the test script t0010-racy-git.sh
     @@ t/t0010-racy-git.sh: do
      -	test_expect_success \
      -	"Racy GIT trial #$trial part A" \
      -	'test "" != "$files"'
     -+	test_expect_success 'Racy git trial #$trial part A' '
     ++	test_expect_success "Racy git trial #$trial part A" '
      +		test "" != "$files"
      +	'
       
     @@ t/t0010-racy-git.sh: do
      -	"Racy GIT trial #$trial part B" \
      -	'test "" != "$files"'
      -
     -+	test_expect_success 'Racy git trial #$trial part B' '
     ++	test_expect_success "Racy git trial #$trial part B" '
      +		test "" != "$files"
      +	'
       done

 t/t0010-racy-git.sh | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

On Thu, Feb 29, 2024 at 4:57 PM Aryan Gupta via GitGitGadget <gitgitgadget@gmail.com> wrote:

This version (v2) addresses my review comments about v1. Thanks.

"Aryan Gupta via GitGitGadget" <gitgitgadget@gmail.com> writes:

OK.

This does not look like updating formatting to the current standard, or improving readability, though.

These are not wrong per-se, but if we are updating, I wonder if we want to get rid of statements outside the test_expect_success block, not just the formatting of individual test_expect_success tests.

Looking at an iteration of the loop, the executable statements from here ...

                rm -f .git/index
                echo frotz >infocom
                git update-index --add infocom
                echo xyzzy >infocom

                files=$(git diff-files -p)

... to here is what we would make part of one test, namely ...

                test_expect_success \
                "Racy GIT trial #$trial part A" \
                'test "" != "$files"'

... this one.  Then

                sleep 1

is a cricial delay to have before the next test, which we may want to have outside to make it clear what is going on to readers. But the following parts ...

                echo xyzzy >cornerstone
                git update-index --add cornerstone

                files=$(git diff-files -p)

... up to here, we would make part of the next test ...

                test_expect_success \
                "Racy GIT trial #$trial part B" \
                'test "" != "$files"'

... in the modern style.

So, we may want to do it more like this, perhaps?

	test_expect_success "Racy GIT trial #$trial part A" '
		rm -f .git/index &&
		echo frotz >infocom &&
		git update-index --add infocom &&
		echo xyzzy >infocom &&

		files=$(git diff-files -p) &&
                test "" != "$files"
	'

	sleep 1

	test_expect_success "Racy GIT trial #$trial part B" '
		echo xyzzy >cornerstone &&
		git update-index --add cornerstone &&

		files=$(git diff-files -p) &&
		test "" != "$files"
	'

An added benefit of the more modern style to place as much as possible to &&-chain in test_expect_success block is that we would catch breakage in "git update-index" and other things used to set-up the test as well. With the original loop, breakages in things outside the test_expect_success blocks will go unchecked.

On Thu, Feb 29, 2024 at 6:14 PM Junio C Hamano <gitster@pobox.com> wrote:

If taking it to this extent, then the modernized version of the last couple lines would be:

    git diff-files -p >out &&
    test_file_not_empty out

Eric Sunshine <sunshine@sunshineco.com> writes:

Yes. The modern style seems to prefer temporary files over variables; the reason probably is because it tends to be easier to remotely post-mortem?

On Thu, Feb 29, 2024 at 6:36 PM Junio C Hamano <gitster@pobox.com> wrote:

Yes, that seems likely. Functions such as test_file_not_empty() also provide an immediate indication of what went wrong without having to spelunk the test detritus:

    'foo' is not a non-empty file. [*]

whereas `test "" != "$foo"` provides no output at all, so it's not immediately clear which part of the test failed. Peff's `verbose` function was intended to mitigate that problem by making the expression verbose upon failure:

    verbose test "" != "$foo" &&

but never really caught on and was eventually retired by 8ddfce7144 (t: drop "verbose" helper function, 2023-05-08).

[*]: Admittedly, the double-negative in "'foo' is not a non-empty file." is more than a little confusing. It probably would have been better phrased as "'foo' should be empty but is not".

On Thu, Feb 29, 2024 at 6:52 PM Eric Sunshine <sunshine@sunshineco.com> wrote:

The double-negative confused me even when suggesting a replacement. What I meant was that a better phrasing would perhaps have been:

    'foo' is empty but should not be

Eric Sunshine <sunshine@sunshineco.com> writes:

+1.  Thanks for caring.

From: Aryan Gupta <garyan447@gmail.com>

Modernize the formatting of the test script to align with current standards and improve its overall readability.

Signed-off-by: Aryan Gupta <garyan447@gmail.com>
---
    [GSOC][PATCH] Modernize a test script

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1675%2Faryangupta701%2Ftest-modernize-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1675/aryangupta701/test-modernize-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1675

Range-diff vs v2:

 1:  fa381d0b57a ! 1:  05ee9e8a458 tests: modernize the test script t0010-racy-git.sh
     @@ Commit message
          Signed-off-by: Aryan Gupta <garyan447@gmail.com>
      
       ## t/t0010-racy-git.sh ##
     -@@
     - #!/bin/sh
     +@@ t/t0010-racy-git.sh: TEST_PASSES_SANITIZE_LEAK=true
       
     --test_description='racy GIT'
     -+test_description='racy git'
     - 
     - TEST_PASSES_SANITIZE_LEAK=true
     - . ./test-lib.sh
     -@@ t/t0010-racy-git.sh: do
     - 	echo xyzzy >infocom
     - 
     - 	files=$(git diff-files -p)
     + for trial in 0 1 2 3 4
     + do
     +-	rm -f .git/index
     +-	echo frotz >infocom
     +-	git update-index --add infocom
     +-	echo xyzzy >infocom
     +-
     +-	files=$(git diff-files -p)
      -	test_expect_success \
      -	"Racy GIT trial #$trial part A" \
      -	'test "" != "$files"'
     +-
      +	test_expect_success "Racy git trial #$trial part A" '
     -+		test "" != "$files"
     ++		rm -f .git/index &&
     ++		echo frotz >infocom &&
     ++		git update-index --add infocom &&
     ++		echo xyzzy >infocom &&
     ++
     ++		git diff-files -p >out &&
     ++		test_file_not_empty out
      +	'
     - 
       	sleep 1
     - 	echo xyzzy >cornerstone
     - 	git update-index --add cornerstone
     +-	echo xyzzy >cornerstone
     +-	git update-index --add cornerstone
       
     - 	files=$(git diff-files -p)
     +-	files=$(git diff-files -p)
      -	test_expect_success \
      -	"Racy GIT trial #$trial part B" \
      -	'test "" != "$files"'
     --
      +	test_expect_success "Racy git trial #$trial part B" '
     -+		test "" != "$files"
     ++		echo xyzzy >cornerstone &&
     ++		git update-index --add cornerstone &&
     + 
     ++		git diff-files -p >out &&
     ++		test_file_not_empty out
      +	'
       done
       

 t/t0010-racy-git.sh | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

"Aryan Gupta via GitGitGadget" <gitgitgadget@gmail.com> writes:

Both the above proposed commit log message and the proposed changes to the code look good.

This is a tangent but as can be seen at

  https://lore.kernel.org/git/pull.1675.v3.git.1709676557639.gitgitgadget@gmail.com/raw

your message has these headers that are not syntactically correct and may even cause responses to bounce unless editted.

    Cc: "Patrick Steinhardt [ ]" <ps@pks.im>,
        "Michal =?UTF-8?Q?Such=C3=A1nek?= [ ]" <msuchanek@suse.de>,
        "=?UTF-8?Q?Jean-No=C3=ABl?= AVILA [ ]" <jn.avila@free.fr>,
        Kristoffer Haugsbakk <[code@khaugsbakk.name]>,
        Eric Sunshine <sunshine@sunshineco.com>,
        Aryan Gupta <garyan447@gmail.com>,
        Aryan Gupta <garyan447@gmail.com>

I cannot tell if this is a bug in GGG, or a fault of a corrupt pull request thrown at GGG, but it is annoying that recipients have to edit the addresses in e-mail header. Can somebody take a look into it, please (or is it a known issue?).

Thanks.

From: Aryan Gupta <garyan447@gmail.com>

Modernize the formatting of the test script to align with current standards and improve its overall readability.

Signed-off-by: Aryan Gupta <garyan447@gmail.com>
---
    [GSOC][PATCH] Modernize a test script

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1675%2Faryangupta701%2Ftest-modernize-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1675/aryangupta701/test-modernize-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1675

Range-diff vs v3:

 1:  05ee9e8a458 = 1:  14c7137baea tests: modernize the test script t0010-racy-git.sh

 t/t0010-racy-git.sh | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

On Wed, Mar 6, 2024 at 10:46 AM Aryan Gupta via GitGitGadget <gitgitgadget@gmail.com> wrote:

This tells us that nothing changed in the patch since v3, so we can only wonder why you sent this v4.

Did you fix some headers? Please explain.

Otherwise, the patch looks good to me. Thanks.

On Thu, Mar 7, 2024 at 2:28 PM Christian Couder <christian.couder@gmail.com> wrote:

Hey. Sorry for making a lot of mistakes in my emails.

I thought that there were some bugs in GGG due to which it sent some headers which were not syntactically correct. So I tried sending it again. And that was the whole purpose of this.

Aryan Gupta <garyan447@gmail.com> writes:

I was wondering about the same thing. I still see an unwanted "[ ]" around Kristoffer's e-mail address that will break responding to the message in your [PATCH v4] e-mail that can be seen at

  https://lore.kernel.org/git/pull.1675.v4.git.1709716446874.gitgitgadget@gmail.com/raw

so, the experiment revealed that it did send some headers were broken.

Thanks for a clarification.

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

This does not necessarily mean GGG is broken. The majority of the patches I see here from GGG are without these funny [square-bracket] around addresses at all, and this was the only patch (or it is possible that your other patches may have had the same issue; I do not remember) with that problem. It might be caused by what you feed GGG (e.g., the messages you give it in your pull request) that caused GGG to hiccup, perhaps?

On Thu, Mar 7, 2024 at 7:30 PM Junio C Hamano <gitster@pobox.com> wrote:

Ohh yes. I think I got the problem. I have fixed it. Can I try sending the patch again now?