{"thread":{"id":"58886","subject":"[PATCH] t4205: don't exit test script on failure","startedAt":"2022-12-01T21:48:37Z","lastAt":"2022-12-03T00:53:26Z","messageCount":6,"participants":["René Scharfe","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"468356","messageId":"c5b4d091-23c1-5a75-a255-99ec83973d8d@web.de","threadId":"58886","inReplyTo":null,"subject":"[PATCH] t4205: don't exit test script on failure","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-01T21:48:24Z","receivedAt":"2022-12-01T21:48:37Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Only abort the individual check instead of exiting the whole test script\nif git show fails. Noticed with GIT_TEST_PASSING_SANITIZE_LEAK=check.\n\nHelped-by: Eric Sunshine \nSigned-off-by: René Scharfe \n---\nFormatted with --function-context for easier review.\nac52d9410e (t4205: cover `git log --reflog -z` blindspot,\n2019-11-19) added the exit call.\n\n t/t4205-log-pretty-formats.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex e448ef2928..0404491d6e 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -154,12 +154,12 @@ done\n test_expect_success 'NUL termination with --reflog --pretty=oneline' '\n \trevs=\"$(git rev-list --reflog)\" &&\n \tfor r in $revs\n \tdo\n \t\tgit show -s --pretty=oneline \"$r\" >raw &&\n-\t\tcat raw | lf_to_nul || exit 1\n+\t\tcat raw | lf_to_nul || return 1\n \tdone >expect &&\n \t# the trailing NUL is already produced so we do not need to\n \t# output another one\n \tgit log -z --pretty=oneline --reflog >actual &&\n \ttest_cmp expect actual\n '\n--\n2.30.2\n"},{"id":"468368","messageId":"221202.86edtihgga.gmgdl@evledraar.gmail.com","threadId":"58886","inReplyTo":"c5b4d091-23c1-5a75-a255-99ec83973d8d@web.de","subject":"Re: [PATCH] t4205: don't exit test script on failure","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-01T23:05:45Z","receivedAt":"2022-12-01T23:06:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Dec 01 2022, René Scharfe wrote:\n\n> Only abort the individual check instead of exiting the whole test script\n> if git show fails. Noticed with GIT_TEST_PASSING_SANITIZE_LEAK=check.\n>\n> Helped-by: Eric Sunshine \n> Signed-off-by: René Scharfe \n> ---\n> Formatted with --function-context for easier review.\n> ac52d9410e (t4205: cover `git log --reflog -z` blindspot,\n> 2019-11-19) added the exit call.\n>\n> t/t4205-log-pretty-formats.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\n> index e448ef2928..0404491d6e 100755\n> --- a/t/t4205-log-pretty-formats.sh\n> +++ b/t/t4205-log-pretty-formats.sh\n> @@ -154,12 +154,12 @@ done\n> test_expect_success 'NUL termination with --reflog --pretty=oneline' '\n> \trevs=\"$(git rev-list --reflog)\" &&\n> \tfor r in $revs\n> \tdo\n> \t\tgit show -s --pretty=oneline \"$r\" >raw &&\n> -\t\tcat raw | lf_to_nul || exit 1\n> +\t\tcat raw | lf_to_nul || return 1\n> \tdone >expect &&\n> \t# the trailing NUL is already produced so we do not need to\n> \t# output another one\n> \tgit log -z --pretty=oneline --reflog >actual &&\n> \ttest_cmp expect actual\n> '\n\nThis is also 6/6 in this series to submit a bunch of these sorts of\nissues, which I submitted back in July:\nhttps://lore.kernel.org/git/cover-0.6-00000000000-20220721T064349Z-avarab@gmail.com/\n"},{"id":"468370","messageId":"xmqqr0xir9lk.fsf@gitster.g","threadId":"58886","inReplyTo":"221202.86edtihgga.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] t4205: don't exit test script on failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-01T23:24:23Z","receivedAt":"2022-12-01T23:24:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> This is also 6/6 in this series to submit a bunch of these sorts of\n> issues, which I submitted back in July:\n> https://lore.kernel.org/git/cover-0.6-00000000000-20220721T064349Z-avarab@gmail.com/\n\nSuch a comment is the least productive, I am afraid.\n\nWhen the series originally did not get any traction would have been\na good time to ask others for reviews to push it forward, but not in\na thread of other people.\n\nThanks.\n"},{"id":"468381","messageId":"221202.86a646hdb6.gmgdl@evledraar.gmail.com","threadId":"58886","inReplyTo":"xmqqr0xir9lk.fsf@gitster.g","subject":"Re: [PATCH] t4205: don't exit test script on failure","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-02T00:07:56Z","receivedAt":"2022-12-02T00:14:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Dec 02 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> This is also 6/6 in this series to submit a bunch of these sorts of\n>> issues, which I submitted back in July:\n>> https://lore.kernel.org/git/cover-0.6-00000000000-20220721T064349Z-avarab@gmail.com/\n>\n> Such a comment is the least productive, I am afraid.\n>\n> When the series originally did not get any traction would have been\n> a good time to ask others for reviews to push it forward, but not in\n> a thread of other people.\n\nI think it's a productive comment on a patch to point out that it's the\nexact same change someone else has submitted independently before.\n\nIn my mind that's better than a \"LGTM\" or \"Reviewed-by\". Those are both\nversions of \"I looked over your work\", but if you independently come up\nwith the same thing that's usually a stronger sign that the proposed\nsolution is a good one.\n\nIf you read it as complaining that my series didn't get picked up that\nwasn't the intent. I don't care either if René's commit ends up on\nmaster, or mine. Just as long as the issue gets fixed.\n\nI did just re-submit that series just now, re-rolled to get past a minor\nconflict, and with a few other changes.\n\nIt's still just the tip of the iceberg in terms of these issues in the\ntest suite, but hopefully it can get picked up this time around, and\nperhaps get some reviewer interest...\n\n1. https://lore.kernel.org/git/cover-v2-0.8-00000000000-20221202T000227Z-avarab@gmail.com/\n"},{"id":"468392","messageId":"xmqq5yeur32x.fsf@gitster.g","threadId":"58886","inReplyTo":"221202.86a646hdb6.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] t4205: don't exit test script on failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-02T01:45:10Z","receivedAt":"2022-12-02T01:45:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> I think it's a productive comment on a patch to point out that it's the\n> exact same change someone else has submitted independently before.\n>\n> In my mind that's better than a \"LGTM\" or \"Reviewed-by\". Those are both\n> versions of \"I looked over your work\", but if you independently come up\n> with the same thing that's usually a stronger sign that the proposed\n> solution is a good one.\n\nNot necessarily.\n\nPast effort that did not fare well needs to be re-examined to make\nsure it was not picked up because it was crappy, because two people\nindependently coming up with the same crappiness does not help us\nbuild more confidence. Instead of forcing other reviewers waste\ntheir time looking at older threads, it would help to explain what\nyou find good in the patch you are reviewing.\n\n"},{"id":"468476","messageId":"xmqq5yetuxek.fsf@gitster.g","threadId":"58886","inReplyTo":"xmqq5yeur32x.fsf@gitster.g","subject":"Re: [PATCH] t4205: don't exit test script on failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-03T00:46:27Z","receivedAt":"2022-12-03T00:53:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> In my mind that's better than a \"LGTM\" or \"Reviewed-by\". Those are both\n>> versions of \"I looked over your work\", but if you independently come up\n>> with the same thing that's usually a stronger sign that the proposed\n>> solution is a good one.\n>\n> Not necessarily.\n>\n> Past effort that did not fare well needs to be re-examined to make\n> sure it was not picked up because it was crappy, because two people\n> independently coming up with the same crappiness does not help us\n> build more confidence. Instead of forcing other reviewers waste\n> their time looking at older threads, it would help to explain what\n> you find good in the patch you are reviewing.\n\nRelated to this, another thing you often do is very helpful: to say\nthat the patch being proposed solves the same problem another patch\nthat is already in our tree solved in a different part of the code\nbase. If it was good for another part of the system, it is likely\nthat the same solution may be a good fit for the part being touched\nas well.\n\nCompared to that, referring to an earlier patch that failed to hit\nour code base is not all that helpful.\n\nThanks.\n\n"}]}