From: Karthik Nayak <karthik.188@gmail.com>
Date: Sat, 19 Sep 2026 18:00:58 GMT
Subject: Re: [PATCH v4 1/3] t4001: modernize
Message-ID: <CAOLa=ZSB05yNzFRzya1R=yGaphUaTtuZUprKgZqwsDKnP09JiA@mail.gmail.com>
In-Reply-To: <20260918171847.2670739-2-markchucarroll@fastmail.com>

"Mark C. Chu-Carroll" <markchucarroll@fastmail.com> writes:

> Old tests were written in a different style than modern
> ones; for better readability and test error messages,
> update t4001 to the modern style.
>
> * run everything inside of a test_expect_success block.
> * write title line on the same line as test_expect_success,
>   end that line with a single quote that opens the body of the test,
>   and end the test with a single quote that closes the body.
> * write expected output of a test to a file named "expect",
>   and actual output to a file named "actual".
> * write here-docs using "<<-" syntax, so that they're indented
>   uniformly with the rest of the test.
> * make test names more clearly reflect the functionality that
>   they test.
>
> Signed-off-by: Mark C. Chu-Carroll <markchucarroll@fastmail.com>
> ---
>  t/t4001-diff-rename.sh   | 94 ++++++++++++++++++----------------------
>  t/t4009-diff-rename-4.sh | 54 +++++++++++------------
>  2 files changed, 69 insertions(+), 79 deletions(-)
>
> diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
> index ad474100af..15567b52a0 100755
> --- a/t/t4001-diff-rename.sh
> +++ b/t/t4001-diff-rename.sh
> @@ -8,6 +8,7 @@ test_description='Test rename detection in diff engine.'
>  . ./test-lib.sh
>  . "$TEST_DIRECTORY"/lib-diff.sh
>
> +

Nit: looks like this was unintentional?

>  test_expect_success 'setup' '
>  	cat >path0 <<-\EOF &&
>  	Line 1
> @@ -26,7 +27,7 @@ test_expect_success 'setup' '
>  	Line 14
>  	Line 15
>  	EOF
> -	cat >expected <<-\EOF &&
> +	cat >expect <<-\EOF &&
>  	diff --git a/path0 b/path1
>  	rename from path0
>  	rename to path1
> @@ -42,7 +43,7 @@ test_expect_success 'setup' '
>  	 Line 13
>  	 Line 14
>  	EOF
> -	cat >no-rename <<-\EOF
> +	cat >expect-no-rename <<-\EOF &&
>  	diff --git a/path0 b/path0
>  	deleted file mode 100644
>  	index fdbec44..0000000
> @@ -86,47 +87,36 @@ test_expect_success 'setup' '
>  	+Line 14
>  	+Line 15
>  	EOF
> +    update-index --add a file. &&

Shouldn't this be `git update-index`?

  ❯ meson test t4001-diff-rename
  ninja: Entering directory `/home/karthik/code/git/build'
  [21/21] Linking target git-http-fetch
  1/1 git:t4001-diff-rename        ERROR           0.74s   exit status 1
  ...

  Ok:                0
  Fail:              1

Also what are these files?

> +    git update-index --add path0
>  '
>
> -test_expect_success \
> -    'update-index --add a file.' \
> -    'git update-index --add path0'
> -

Ah! So we remove this test and add it to the setup, isn't 'update-index
--add a file.' the name of the test?

> -test_expect_success \
> -    'write that tree.' \
> -    'tree=$(git write-tree) && echo $tree'
> -
> -sed -e 's/line/Line/' <path0 >path1
> -rm -f path0
> -test_expect_success \
> -    'renamed and edited the file.' \
> -    'git update-index --add --remove path0 path1'
> -
> -test_expect_success \
> -    'git diff-index -p -M after rename and editing.' \
> -    'git diff-index -p -M $tree >current'
> -
> -
> -test_expect_success \
> -    'validate the output.' \
> -    'compare_diff_patch current expected'
> +test_expect_success 'diff shows path0 renamed to path1 with edit.' '

Generally we don't end the test descriptions with a fullstop.

> +	initial_setup &&

What is `initial_setup` here? This fails running the test.

> +	tree=$(git write-tree) &&
> +	sed -e "s/line/Line/" <path0 >path1 &&
> +	rm -f path0 &&
> +	git update-index --add --remove path0 path1 &&
> +	git diff-index -p -M $tree >actual &&
> +	compare_diff_patch actual expect
> +'
>

[snip]