git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [GSOC][PATCH 0/1] Avoid suppression of git's exit code in tests
@ 2023-03-26 17:31 Edwin Fernando
  2023-03-26 17:31 ` [GSOC][PATCH 1/1] t3701: Avoid suppression of exit status of git Edwin Fernando
  2023-04-01 19:47 ` [GSOC][PATCH v2] t3701: don't lose "git" exit codes in test scripts Edwin Fernando
  0 siblings, 2 replies; 13+ messages in thread
From: Edwin Fernando @ 2023-03-26 17:31 UTC (permalink / raw)
  To: git; +Cc: vdye, Edwin Fernando

This is a GSOC microproject. Address the problem of errors resulting
from calling git in the test scripts being suppressed. Suppressions
due to pipes and command substitution were encountered in this 
file (t/t3701). These regexs where used to search for such occurences
.*\<git\>.*|.* and .*$(.*git.*).* respectively (in vim).
Both types have been resolved and the tests pass as before commit.

Edwin Fernando (1):
  t3701: Avoid suppression of exit status of git

 t/t3701-add-interactive.sh | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)


base-commit: d15644fe0226af7ffc874572d968598564a230dd
-- 
2.40.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [GSOC][PATCH 1/1] t3701: Avoid suppression of exit status of git
  2023-03-26 17:31 [GSOC][PATCH 0/1] Avoid suppression of git's exit code in tests Edwin Fernando
@ 2023-03-26 17:31 ` Edwin Fernando
  2023-03-26 20:14   ` Andrei Rybak
  2023-04-01 19:47 ` [GSOC][PATCH v2] t3701: don't lose "git" exit codes in test scripts Edwin Fernando
  1 sibling, 1 reply; 13+ messages in thread
From: Edwin Fernando @ 2023-03-26 17:31 UTC (permalink / raw)
  To: git; +Cc: vdye, Edwin Fernando

Pipes and command substitution in bash suppress the exit codes of the
first executed command. In the test scripts errors when running git
should be exposed. Remove all such suppressions of git by writing
output to files, chaining commands with &&, or other means.

Signed-off-by: Edwin Fernando <edwinfernando734@gmail.com>
---
 t/t3701-add-interactive.sh | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 3a99837d9b..80446b311d 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -292,8 +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 | grep content &&
-	git diff file | grep "new mode"
+	git show :file > show_file &&
+	grep content show_file &&
+	git diff file > diff_file &&
+	grep "new mode" diff_file
 '
 
 test_expect_success FILEMODE 'stage mode but not hunk' '
@@ -301,8 +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 | grep "new mode" &&
-	git diff          file | grep "+content"
+	git diff --cached file > diff_file &&
+	grep "new mode" diff_file &&
+	git diff file > diff_file &&
+	grep "+content" diff_file
 '
 
 
@@ -311,9 +315,12 @@ 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 | grep "new mode" &&
-	git diff --cached file | grep "+content" &&
-	test -z "$(git diff file)"
+	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)
 '
 
 # end of tests disabled when filemode is not usable
@@ -970,8 +977,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 &&
-	rev="$(git -C for-submodules/dirty-head rev-parse HEAD)" &&
-	grep "$rev" actual
+	git -C for-submodules/dirty-head rev-parse HEAD > rev &&
+	grep -f rev actual
 '
 
 test_expect_success 'set up pathological context' '
-- 
2.40.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [GSOC][PATCH 1/1] t3701: Avoid suppression of exit status of git
  2023-03-26 17:31 ` [GSOC][PATCH 1/1] t3701: Avoid suppression of exit status of git Edwin Fernando
@ 2023-03-26 20:14   ` Andrei Rybak
  2023-03-27 10:11     ` Eric Sunshine
  0 siblings, 1 reply; 13+ messages in thread
From: Andrei Rybak @ 2023-03-26 20:14 UTC (permalink / raw)
  To: Edwin Fernando; +Cc: vdye, git

Hello, Edwin.

In subject line "t3701: Avoid suppression of exit status of git" -- the first
word after a colon shouldn't be capitalized.  By the way, similar existing
commits call it:

   don't lose "git" exit codes

and

   preserve "git" exit codes

On 26/03/2023 19:31, Edwin Fernando wrote:
> Pipes and command substitution in bash suppress the exit codes of the

Bash is not the only Shell used for running Git's test suite, so referring
to it like this in the commit message might not be a good idea.

See also similar commit 9fdc79ecba ("tests: don't lose misc "git" exit codes",
2023-02-06) for inspiration in writing the commit message.

> first executed command. In the test scripts errors when running git
> should be exposed. Remove all such suppressions of git by writing
> output to files, chaining commands with &&, or other means.
> 
> Signed-off-by: Edwin Fernando <edwinfernando734@gmail.com>
> ---
>   t/t3701-add-interactive.sh | 25 ++++++++++++++++---------
>   1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 3a99837d9b..80446b311d 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -292,8 +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 | grep content &&
> -	git diff file | grep "new mode"
> +	git show :file > show_file &&

Code style here and in all output redirections below: lose the space between
redirection and the path, like so:

   git show :file >show_file

Also, the suffix "_file" is unnecessary.  Just

   git show :file >show

or

   git show :file >out

or

   git show :file >actual

might be better.

> +	grep content show_file &&
> +	git diff file > diff_file &&
> +	grep "new mode" diff_file
>   '
>   
>   test_expect_success FILEMODE 'stage mode but not hunk' '
> @@ -301,8 +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 | grep "new mode" &&
> -	git diff          file | grep "+content"
> +	git diff --cached file > diff_file &&
> +	grep "new mode" diff_file &&
> +	git diff file > diff_file &&
> +	grep "+content" diff_file
>   '
>   
>   
> @@ -311,9 +315,12 @@ 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 | grep "new mode" &&
> -	git diff --cached file | grep "+content" &&
> -	test -z "$(git diff file)"
> +	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)

Function test_stdout_line_count can be used here instead of "test -z".
test_stdout_line_count would produce a more helpful error message in
case of test failure.

>   '
>   
>   # end of tests disabled when filemode is not usable
> @@ -970,8 +977,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 &&
> -	rev="$(git -C for-submodules/dirty-head rev-parse HEAD)" &&
> -	grep "$rev" actual
> +	git -C for-submodules/dirty-head rev-parse HEAD > rev &&
> +	grep -f rev actual
>   '
>   
>   test_expect_success 'set up pathological context' '


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [GSOC][PATCH 1/1] t3701: Avoid suppression of exit status of git
  2023-03-26 20:14   ` Andrei Rybak
@ 2023-03-27 10:11     ` Eric Sunshine
  0 siblings, 0 replies; 13+ messages in thread
From: Eric Sunshine @ 2023-03-27 10:11 UTC (permalink / raw)
  To: Andrei Rybak; +Cc: Edwin Fernando, vdye, git

On Sun, Mar 26, 2023 at 4:21 PM Andrei Rybak <rybak.a.v@gmail.com> wrote:
> On 26/03/2023 19:31, Edwin Fernando wrote:
> In subject line "t3701: Avoid suppression of exit status of git" -- the first
> word after a colon shouldn't be capitalized.  By the way, similar existing
> commits call it:
>    don't lose "git" exit codes
> and
>    preserve "git" exit codes

Thanks, your review comments covered almost everything I was going to
say, and I agree with all you said, so I'll just add a couple comments
not covered by your review.

> > +     git show :file > show_file &&
>
> Code style here and in all output redirections below: lose the space between
> redirection and the path, like so:
>    git show :file >show_file
>
> Also, the suffix "_file" is unnecessary.  Just
>    git show :file >show
> or
>    git show :file >out
> or
>    git show :file >actual
> might be better.

Fully agree with either of the latter two suggestions.

> > @@ -311,9 +315,12 @@ test_expect_success FILEMODE 'stage mode and hunk' '
> > -     git diff --cached file | grep "new mode" &&
> > -     git diff --cached file | grep "+content" &&
> > -     test -z "$(git diff file)"
> > +     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)
>
> Function test_stdout_line_count can be used here instead of "test -z".
> test_stdout_line_count would produce a more helpful error message in
> case of test failure.

The other idiomatic possibility would be to use test_must_be_empty()
to verify that the file is empty.

> > -     rev="$(git -C for-submodules/dirty-head rev-parse HEAD)" &&
> > -     grep "$rev" actual
> > +     git -C for-submodules/dirty-head rev-parse HEAD > rev &&
> > +     grep -f rev actual

This change is unnecessary since this is a cases in which the exit
code does not get lost; the exit code of the command substitution
becomes the exit code of the assignment:

  % x="$(true)"
  % echo $?
  0
  % x="$(false)"
  % echo $?
  1

So let's drop this change from the patch.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [GSOC][PATCH v2] t3701: don't lose "git" exit codes in test scripts
  2023-03-26 17:31 [GSOC][PATCH 0/1] Avoid suppression of git's exit code in tests Edwin Fernando
  2023-03-26 17:31 ` [GSOC][PATCH 1/1] t3701: Avoid suppression of exit status of git Edwin Fernando
@ 2023-04-01 19:47 ` Edwin Fernando
  2023-04-02 10:41   ` Andrei Rybak
  2023-04-02 11:36   ` [GSOC][PATCH v3] " Edwin Fernando
  1 sibling, 2 replies; 13+ messages in thread
From: Edwin Fernando @ 2023-04-01 19:47 UTC (permalink / raw)
  To: git; +Cc: Edwin Fernando, Eric Sunshine, Andrei Rybak

Exit codes were lost due to piping and command substitution:

- "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' '
-- 
2.40.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [GSOC][PATCH v2] t3701: don't lose "git" exit codes in test scripts
  2023-04-01 19:47 ` [GSOC][PATCH v2] t3701: don't lose "git" exit codes in test scripts Edwin Fernando
@ 2023-04-02 10:41   ` Andrei Rybak
  2023-04-02 11:36   ` [GSOC][PATCH v3] " Edwin Fernando
  1 sibling, 0 replies; 13+ messages in thread
From: Andrei Rybak @ 2023-04-02 10:41 UTC (permalink / raw)
  To: Edwin Fernando, git; +Cc: Eric Sunshine

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.

> 
> - "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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [GSOC][PATCH v3] t3701: don't lose "git" exit codes in test scripts
  2023-04-01 19:47 ` [GSOC][PATCH v2] t3701: don't lose "git" exit codes in test scripts Edwin Fernando
  2023-04-02 10:41   ` Andrei Rybak
@ 2023-04-02 11:36   ` Edwin Fernando
  2023-04-02 17:17     ` Eric Sunshine
  2023-04-02 19:17     ` [GSOC][PATCH v4] " Edwin Fernando
  1 sibling, 2 replies; 13+ messages in thread
From: Edwin Fernando @ 2023-04-02 11:36 UTC (permalink / raw)
  To: git; +Cc: Edwin Fernando, Andrei Rybak, Eric Sunshine

Exit codes are lost due to piping and command substitution:

- "git ... | <command>"
- "<command> $(git ... )"

Fix these issues using the intermediate step of writing output to file.

Signed-off-by: Edwin Fernando <edwinfernando734@gmail.com>
---
Changes from v2:
- use git rebase to squash commit history,
  and diff with upstream commit for review
- use present tense for code before the commit.

 t/t3701-add-interactive.sh | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 3a99837d9b..77aad9032a 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -292,8 +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 | grep content &&
-	git diff file | grep "new mode"
+	git show :file >show &&
+	grep content show &&
+	git diff file >diff &&
+	grep "new mode" diff
 '
 
 test_expect_success FILEMODE 'stage mode but not hunk' '
@@ -301,8 +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 | grep "new mode" &&
-	git diff          file | grep "+content"
+	git diff --cached file >diff &&
+	grep "new mode" diff &&
+	git diff file >diff &&
+	grep "+content" diff
 '
 
 
@@ -311,9 +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 | grep "new mode" &&
-	git diff --cached file | grep "+content" &&
-	test -z "$(git 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
-- 
2.40.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [GSOC][PATCH v3] t3701: don't lose "git" exit codes in test scripts
  2023-04-02 11:36   ` [GSOC][PATCH v3] " Edwin Fernando
@ 2023-04-02 17:17     ` Eric Sunshine
  2023-04-02 19:17     ` [GSOC][PATCH v4] " Edwin Fernando
  1 sibling, 0 replies; 13+ messages in thread
From: Eric Sunshine @ 2023-04-02 17:17 UTC (permalink / raw)
  To: Edwin Fernando; +Cc: git, Andrei Rybak

On Sun, Apr 2, 2023 at 7:36 AM Edwin Fernando
<edwinfernando734@gmail.com> wrote:
> Exit codes are lost due to piping and command substitution:
>
> - "git ... | <command>"
> - "<command> $(git ... )"
>
> Fix these issues using the intermediate step of writing output to file.
>
> Signed-off-by: Edwin Fernando <edwinfernando734@gmail.com>
> ---
> Changes from v2:
> - use git rebase to squash commit history,
>   and diff with upstream commit for review
> - use present tense for code before the commit.

Thanks. The patch is looking a lot better. One comment, though...

> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> @@ -311,9 +315,11 @@ test_expect_success FILEMODE 'stage mode and hunk' '
>         printf "y\\ny\\n" | git add -p &&
> -       git diff --cached file | grep "new mode" &&
> -       git diff --cached file | grep "+content" &&
> -       test -z "$(git diff file)"
> +       git diff --cached file >diff &&
> +       grep "new mode" diff &&
> +       grep "+content" diff &&
> +       git diff file >diff &&
> +       test_must_be_empty diff
>  '

The other changes in your patch are worthwhile, but it seems that you
have based this patch on an outdated copy of the repository. This
particular change had already been made by 9fdc79ecba (tests: don't
lose misc "git" exit codes, 2023-02-06) a couple months ago[1].

So, you'll want to update your repository to the latest available from
Junio, re-roll using "master" as a base for the patch, and drop this
change while keeping the others.

[1]: https://github.com/git/git/commit/9fdc79ecba0f4a3ef885f1409d2db5a1dbabd649

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [GSOC][PATCH v4] t3701: don't lose "git" exit codes in test scripts
  2023-04-02 11:36   ` [GSOC][PATCH v3] " Edwin Fernando
  2023-04-02 17:17     ` Eric Sunshine
@ 2023-04-02 19:17     ` Edwin Fernando
  2023-04-03  6:24       ` Eric Sunshine
  1 sibling, 1 reply; 13+ messages in thread
From: Edwin Fernando @ 2023-04-02 19:17 UTC (permalink / raw)
  To: git; +Cc: Edwin Fernando, Eric Sunshine, Andrei Rybak

Exit codes are lost due to piping: "git ... | <command>"
Fix these issues using the intermediate step of writing output to file.

Signed-off-by: Edwin Fernando <edwinfernando734@gmail.com>
---
Apply my changes on top of the latest commit in upstream history.
Test 76 in t3701 fails. This commit breaks the test:
a9f4a01760 (Merge branch 'jk/add-p-unmerged-fix', 2023-03-19)
Identified by checking out commits in:
"git log --oneline -- t/t3701-add-interactive.sh"
I don't know if it is normal to have broken tests in the main repository.

 t/t3701-add-interactive.sh | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 3982b6b49d..b8291206a0 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -286,8 +286,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 | grep content &&
-	git diff file | grep "new mode"
+	git show :file >show &&
+	grep content show &&
+	git diff file >diff &&
+	grep "new mode" diff
 '
 
 test_expect_success FILEMODE 'stage mode but not hunk' '
@@ -295,8 +297,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 | grep "new mode" &&
-	git diff          file | grep "+content"
+	git diff --cached file >diff &&
+	grep "new mode" diff &&
+	git diff file >diff &&
+	grep "+content" diff
 '
 
 
-- 
2.40.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [GSOC][PATCH v4] t3701: don't lose "git" exit codes in test scripts
  2023-04-02 19:17     ` [GSOC][PATCH v4] " Edwin Fernando
@ 2023-04-03  6:24       ` Eric Sunshine
  2023-04-03  9:21         ` Edwin Fernando
  2023-04-03  9:34         ` Edwin Fernando
  0 siblings, 2 replies; 13+ messages in thread
From: Eric Sunshine @ 2023-04-03  6:24 UTC (permalink / raw)
  To: Edwin Fernando; +Cc: git, Andrei Rybak

On Sun, Apr 2, 2023 at 3:18 PM Edwin Fernando
<edwinfernando734@gmail.com> wrote:
> Exit codes are lost due to piping: "git ... | <command>"
> Fix these issues using the intermediate step of writing output to file.
>
> Signed-off-by: Edwin Fernando <edwinfernando734@gmail.com>
> ---
> Apply my changes on top of the latest commit in upstream history.
> Test 76 in t3701 fails. This commit breaks the test:
> a9f4a01760 (Merge branch 'jk/add-p-unmerged-fix', 2023-03-19)
> Identified by checking out commits in:
> "git log --oneline -- t/t3701-add-interactive.sh"
> I don't know if it is normal to have broken tests in the main repository.

It's not normal to have broken tests in the repository; they should not exist.

However, it's not clear what breakage you are seeing. In my testing, I
don't see any failures, either in a9f4a01760 or after applying your
patch to "master". Can you provide more information about the failure
you're experiencing?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [GSOC][PATCH v4] t3701: don't lose "git" exit codes in test scripts
  2023-04-03  6:24       ` Eric Sunshine
@ 2023-04-03  9:21         ` Edwin Fernando
  2023-04-03  9:34         ` Edwin Fernando
  1 sibling, 0 replies; 13+ messages in thread
From: Edwin Fernando @ 2023-04-03  9:21 UTC (permalink / raw)
  To: sunshine; +Cc: edwinfernando734, git, rybak.a.v



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [GSOC][PATCH v4] t3701: don't lose "git" exit codes in test scripts
  2023-04-03  6:24       ` Eric Sunshine
  2023-04-03  9:21         ` Edwin Fernando
@ 2023-04-03  9:34         ` Edwin Fernando
  2023-04-03 17:03           ` Eric Sunshine
  1 sibling, 1 reply; 13+ messages in thread
From: Edwin Fernando @ 2023-04-03  9:34 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: git, Andrei Rybak

On Mon, 3 Apr 2023 at 07:25, Eric Sunshine <sunshine@sunshineco.com> wrote:
> It's not normal to have broken tests in the repository; they should not exist.

Sorry the test did not break after all.

> However, it's not clear what breakage you are seeing. In my testing, I
> don't see any failures, either in a9f4a01760 or after applying your
> patch to "master". Can you provide more information about the failure
> you're experiencing?

I cloned git again, ran "make" and all the tests in t3701 passed. The
local repository I have been working on so far still gives the same error
though. The error message is not very helpful so I haven't included it. I
ran "make" on the faulty repository and the tests passed this time. So I
think the problem was that whatever "make" does was not up to date.
Also please ignore some mails you may have received because of
misconfiguration.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [GSOC][PATCH v4] t3701: don't lose "git" exit codes in test scripts
  2023-04-03  9:34         ` Edwin Fernando
@ 2023-04-03 17:03           ` Eric Sunshine
  0 siblings, 0 replies; 13+ messages in thread
From: Eric Sunshine @ 2023-04-03 17:03 UTC (permalink / raw)
  To: Edwin Fernando; +Cc: git, Andrei Rybak

On Mon, Apr 3, 2023 at 5:35 AM Edwin Fernando
<edwinfernando734@gmail.com> wrote:
> On Mon, 3 Apr 2023 at 07:25, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > However, it's not clear what breakage you are seeing. In my testing, I
> > don't see any failures, either in a9f4a01760 or after applying your
> > patch to "master". Can you provide more information about the failure
> > you're experiencing?
>
> I cloned git again, ran "make" and all the tests in t3701 passed. The
> local repository I have been working on so far still gives the same error
> though. The error message is not very helpful so I haven't included it. I
> ran "make" on the faulty repository and the tests passed this time. So I
> think the problem was that whatever "make" does was not up to date.
> Also please ignore some mails you may have received because of
> misconfiguration.

That makes sense. An outdated build could indeed account for an
unexpected test failure.

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2023-04-03 17:04 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-26 17:31 [GSOC][PATCH 0/1] Avoid suppression of git's exit code in tests Edwin Fernando
2023-03-26 17:31 ` [GSOC][PATCH 1/1] t3701: Avoid suppression of exit status of git Edwin Fernando
2023-03-26 20:14   ` Andrei Rybak
2023-03-27 10:11     ` Eric Sunshine
2023-04-01 19:47 ` [GSOC][PATCH v2] t3701: don't lose "git" exit codes in test scripts Edwin Fernando
2023-04-02 10:41   ` Andrei Rybak
2023-04-02 11:36   ` [GSOC][PATCH v3] " Edwin Fernando
2023-04-02 17:17     ` Eric Sunshine
2023-04-02 19:17     ` [GSOC][PATCH v4] " Edwin Fernando
2023-04-03  6:24       ` Eric Sunshine
2023-04-03  9:21         ` Edwin Fernando
2023-04-03  9:34         ` Edwin Fernando
2023-04-03 17:03           ` Eric Sunshine

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).