Re: [GSOC][PATCH v2] t3701: don't lose "git" exit codes in test scripts

From: Andrei Rybak <rybak.a.v@gmail.com>
Date: Apr 2, 2023, 10:41 UTC
Message-ID: <96430edd-a4f8-1f3c-a1f2-860e0af5e4e7@gmail.com>
In-Reply-To: <20230401194756.5954-1-edwinfernando734@gmail.com>

On 01/04/2023 21:47, Edwin Fernando wrote:

> Exit codes were lost due to piping and command substitution:

s/were/are/

See section "[[present-tense]]" in Documentation/SubmittingPatches:

    The problem statement that describes the status quo is written in the
    present tense.  Write "The code does X when it is given input Y",
    instead of "The code used to do Y when given input X".  You do not
    have to say "Currently"---the status quo in the problem statement is
    about the code _without_ your change, by project convention.

Show 78 quoted lines

> 
> - "git ... | <command>"
> - "<command> $(git ... )"
> 
> Fix these issues using the intermediate step of writing output to file.
> ---
> Changes in response to review:
> - addressed code style issues: ">diff" not  "> diff_file"
> - a more direct alternative to "test -z $(cat ...)"
> - commit message similar to previous commits accomplishing same goals
> - revert unnecessary change. keep "<var> = $(git ...)"
> 
>   t/t3701-add-interactive.sh | 31 +++++++++++++++----------------
>   1 file changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 80446b311d..77aad9032a 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -292,10 +292,10 @@ test_expect_success FILEMODE 'patch does not affect mode' '
>   	echo content >>file &&
>   	chmod +x file &&
>   	printf "n\\ny\\n" | git add -p &&
> -	git show :file > show_file &&
> -	grep content show_file &&
> -	git diff file > diff_file &&
> -	grep "new mode" diff_file
> +	git show :file >show &&
> +	grep content show &&
> +	git diff file >diff &&
> +	grep "new mode" diff
>   '
>   
>   test_expect_success FILEMODE 'stage mode but not hunk' '
> @@ -303,10 +303,10 @@ test_expect_success FILEMODE 'stage mode but not hunk' '
>   	echo content >>file &&
>   	chmod +x file &&
>   	printf "y\\nn\\n" | git add -p &&
> -	git diff --cached file > diff_file &&
> -	grep "new mode" diff_file &&
> -	git diff file > diff_file &&
> -	grep "+content" diff_file
> +	git diff --cached file >diff &&
> +	grep "new mode" diff &&
> +	git diff file >diff &&
> +	grep "+content" diff
>   '
>   
>   
> @@ -315,12 +315,11 @@ test_expect_success FILEMODE 'stage mode and hunk' '
>   	echo content >>file &&
>   	chmod +x file &&
>   	printf "y\\ny\\n" | git add -p &&
> -	git diff --cached file > diff_file &&
> -	grep "new mode" diff_file &&
> -	git diff --cached file diff_file &&
> -	grep "+content" diff_file &&
> -	git diff file > diff_file &&
> -	test -z $(cat diff_file)
> +	git diff --cached file >diff &&
> +	grep "new mode" diff &&
> +	grep "+content" diff &&
> +	git diff file >diff &&
> +	test_must_be_empty diff
>   '
>   
>   # end of tests disabled when filemode is not usable
> @@ -977,8 +976,8 @@ test_expect_success 'handle submodules' '
>   
>   	force_color git -C for-submodules add -p dirty-head >output 2>&1 <y &&
>   	git -C for-submodules ls-files --stage dirty-head >actual &&
> -	git -C for-submodules/dirty-head rev-parse HEAD > rev &&
> -	grep -f rev actual
> +	rev="$(git -C for-submodules/dirty-head rev-parse HEAD)" &&
> +	grep "$rev" actual
>   '
>   
>   test_expect_success 'set up pathological context' '

It seems that you've created a second commit on top of the patch you've originally sent and have only sent the second commit as "v2". That is not what you want to show your reviewers, and more importantly, that is not what we want to record in our history.

Instead, you should squash these two commits together and send it as a single patch, as though the commit from v1 doesn't exist.