git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH] t4205: don't exit test script on failure
@ 2022-12-01 21:48 René Scharfe
  2022-12-01 23:05 ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 6+ messages in thread
From: René Scharfe @ 2022-12-01 21:48 UTC (permalink / raw)
  To: Git List
  Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
	Eric Sunshine

Only abort the individual check instead of exiting the whole test script
if git show fails.  Noticed with GIT_TEST_PASSING_SANITIZE_LEAK=check.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
Formatted with --function-context for easier review.
ac52d9410e (t4205: cover `git log --reflog -z` blindspot,
2019-11-19) added the exit call.

 t/t4205-log-pretty-formats.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index e448ef2928..0404491d6e 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -154,12 +154,12 @@ done
 test_expect_success 'NUL termination with --reflog --pretty=oneline' '
 	revs="$(git rev-list --reflog)" &&
 	for r in $revs
 	do
 		git show -s --pretty=oneline "$r" >raw &&
-		cat raw | lf_to_nul || exit 1
+		cat raw | lf_to_nul || return 1
 	done >expect &&
 	# the trailing NUL is already produced so we do not need to
 	# output another one
 	git log -z --pretty=oneline --reflog >actual &&
 	test_cmp expect actual
 '
--
2.30.2

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

* Re: [PATCH] t4205: don't exit test script on failure
  2022-12-01 21:48 [PATCH] t4205: don't exit test script on failure René Scharfe
@ 2022-12-01 23:05 ` Ævar Arnfjörð Bjarmason
  2022-12-01 23:24   ` Junio C Hamano
  0 siblings, 1 reply; 6+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-12-01 23:05 UTC (permalink / raw)
  To: René Scharfe; +Cc: Git List, Junio C Hamano, Eric Sunshine


On Thu, Dec 01 2022, René Scharfe wrote:

> Only abort the individual check instead of exiting the whole test script
> if git show fails.  Noticed with GIT_TEST_PASSING_SANITIZE_LEAK=check.
>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> Formatted with --function-context for easier review.
> ac52d9410e (t4205: cover `git log --reflog -z` blindspot,
> 2019-11-19) added the exit call.
>
>  t/t4205-log-pretty-formats.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> index e448ef2928..0404491d6e 100755
> --- a/t/t4205-log-pretty-formats.sh
> +++ b/t/t4205-log-pretty-formats.sh
> @@ -154,12 +154,12 @@ done
>  test_expect_success 'NUL termination with --reflog --pretty=oneline' '
>  	revs="$(git rev-list --reflog)" &&
>  	for r in $revs
>  	do
>  		git show -s --pretty=oneline "$r" >raw &&
> -		cat raw | lf_to_nul || exit 1
> +		cat raw | lf_to_nul || return 1
>  	done >expect &&
>  	# the trailing NUL is already produced so we do not need to
>  	# output another one
>  	git log -z --pretty=oneline --reflog >actual &&
>  	test_cmp expect actual
>  '

This is also 6/6 in this series to submit a bunch of these sorts of
issues, which I submitted back in July:
https://lore.kernel.org/git/cover-0.6-00000000000-20220721T064349Z-avarab@gmail.com/

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

* Re: [PATCH] t4205: don't exit test script on failure
  2022-12-01 23:05 ` Ævar Arnfjörð Bjarmason
@ 2022-12-01 23:24   ` Junio C Hamano
  2022-12-02  0:07     ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2022-12-01 23:24 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: René Scharfe, Git List, Eric Sunshine

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> This is also 6/6 in this series to submit a bunch of these sorts of
> issues, which I submitted back in July:
> https://lore.kernel.org/git/cover-0.6-00000000000-20220721T064349Z-avarab@gmail.com/

Such a comment is the least productive, I am afraid.

When the series originally did not get any traction would have been
a good time to ask others for reviews to push it forward, but not in
a thread of other people.

Thanks.

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

* Re: [PATCH] t4205: don't exit test script on failure
  2022-12-01 23:24   ` Junio C Hamano
@ 2022-12-02  0:07     ` Ævar Arnfjörð Bjarmason
  2022-12-02  1:45       ` Junio C Hamano
  0 siblings, 1 reply; 6+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-12-02  0:07 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: René Scharfe, Git List, Eric Sunshine


On Fri, Dec 02 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> This is also 6/6 in this series to submit a bunch of these sorts of
>> issues, which I submitted back in July:
>> https://lore.kernel.org/git/cover-0.6-00000000000-20220721T064349Z-avarab@gmail.com/
>
> Such a comment is the least productive, I am afraid.
>
> When the series originally did not get any traction would have been
> a good time to ask others for reviews to push it forward, but not in
> a thread of other people.

I think it's a productive comment on a patch to point out that it's the
exact same change someone else has submitted independently before.

In my mind that's better than a "LGTM" or "Reviewed-by". Those are both
versions of "I looked over your work", but if you independently come up
with the same thing that's usually a stronger sign that the proposed
solution is a good one.

If you read it as complaining that my series didn't get picked up that
wasn't the intent. I don't care either if René's commit ends up on
master, or mine. Just as long as the issue gets fixed.

I did just re-submit that series just now, re-rolled to get past a minor
conflict, and with a few other changes.

It's still just the tip of the iceberg in terms of these issues in the
test suite, but hopefully it can get picked up this time around, and
perhaps get some reviewer interest...

1. https://lore.kernel.org/git/cover-v2-0.8-00000000000-20221202T000227Z-avarab@gmail.com/

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

* Re: [PATCH] t4205: don't exit test script on failure
  2022-12-02  0:07     ` Ævar Arnfjörð Bjarmason
@ 2022-12-02  1:45       ` Junio C Hamano
  2022-12-03  0:46         ` Junio C Hamano
  0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2022-12-02  1:45 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: René Scharfe, Git List, Eric Sunshine

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> I think it's a productive comment on a patch to point out that it's the
> exact same change someone else has submitted independently before.
>
> In my mind that's better than a "LGTM" or "Reviewed-by". Those are both
> versions of "I looked over your work", but if you independently come up
> with the same thing that's usually a stronger sign that the proposed
> solution is a good one.

Not necessarily.

Past effort that did not fare well needs to be re-examined to make
sure it was not picked up because it was crappy, because two people
independently coming up with the same crappiness does not help us
build more confidence.  Instead of forcing other reviewers waste
their time looking at older threads, it would help to explain what
you find good in the patch you are reviewing.


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

* Re: [PATCH] t4205: don't exit test script on failure
  2022-12-02  1:45       ` Junio C Hamano
@ 2022-12-03  0:46         ` Junio C Hamano
  0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2022-12-03  0:46 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: René Scharfe, Git List, Eric Sunshine

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

>> In my mind that's better than a "LGTM" or "Reviewed-by". Those are both
>> versions of "I looked over your work", but if you independently come up
>> with the same thing that's usually a stronger sign that the proposed
>> solution is a good one.
>
> Not necessarily.
>
> Past effort that did not fare well needs to be re-examined to make
> sure it was not picked up because it was crappy, because two people
> independently coming up with the same crappiness does not help us
> build more confidence.  Instead of forcing other reviewers waste
> their time looking at older threads, it would help to explain what
> you find good in the patch you are reviewing.

Related to this, another thing you often do is very helpful: to say
that the patch being proposed solves the same problem another patch
that is already in our tree solved in a different part of the code
base.  If it was good for another part of the system, it is likely
that the same solution may be a good fit for the part being touched
as well.

Compared to that, referring to an earlier patch that failed to hit
our code base is not all that helpful.

Thanks.


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

end of thread, other threads:[~2022-12-03  0:54 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-01 21:48 [PATCH] t4205: don't exit test script on failure René Scharfe
2022-12-01 23:05 ` Ævar Arnfjörð Bjarmason
2022-12-01 23:24   ` Junio C Hamano
2022-12-02  0:07     ` Ævar Arnfjörð Bjarmason
2022-12-02  1:45       ` Junio C Hamano
2022-12-03  0:46         ` Junio C Hamano

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