{"thread":{"id":"47811","subject":"[PATCH 2/2] t5556: replace test_i18ngrep with a simple grep","startedAt":"2018-02-12T00:18:29Z","lastAt":"2018-02-28T16:18:50Z","messageCount":15,"participants":["Ramsay Jones","Junio C Hamano","SZEDER Gábor","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"339026","messageId":"d0e6c6cf-7166-bef6-f179-c4e6acf7b0ac@ramsayjones.plus.com","threadId":"47811","inReplyTo":null,"subject":"[PATCH 2/2] t5556: replace test_i18ngrep with a simple grep","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-02-12T00:18:23Z","receivedAt":"2018-02-12T00:18:29Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nAttempting to grep the output of test_i18ngrep will not work under a\npoison build, since the output is (almost) guaranteed not to have the\nstring you are looking for. In this case, the output of test_i18ngrep\nis further filtered by a simple piplined grep to exclude an '... remote\nend hung up unexpectedly' warning message. Use a regular 'grep -E' to\nreplace the call to test_i18ngrep in the filter pipeline.\n\nAlso, remove a useless invocation of 'sort' as the final element of the\npipeline.\n\nSigned-off-by: Ramsay Jones \n---\n t/t5536-fetch-conflicts.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5536-fetch-conflicts.sh b/t/t5536-fetch-conflicts.sh\nindex 2e42cf331..38381df5e 100755\n--- a/t/t5536-fetch-conflicts.sh\n+++ b/t/t5536-fetch-conflicts.sh\n@@ -22,7 +22,7 @@ verify_stderr () {\n \tcat >expected &&\n \t# We're not interested in the error\n \t# \"fatal: The remote end hung up unexpectedly\":\n-\ttest_i18ngrep -E '^(fatal|warning):' actual | sort &&\n+\tgrep -E '^(fatal|warning):' actual &&\n \ttest_i18ncmp expected actual\n }\n \n-- \n2.16.0\n"},{"id":"339101","messageId":"xmqq3726t11d.fsf@gitster-ct.c.googlers.com","threadId":"47811","inReplyTo":"d0e6c6cf-7166-bef6-f179-c4e6acf7b0ac@ramsayjones.plus.com","subject":"Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-12T20:18:06Z","receivedAt":"2018-02-12T20:18:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> Attempting to grep the output of test_i18ngrep will not work under a\n> poison build, since the output is (almost) guaranteed not to have the\n> string you are looking for. In this case, the output of test_i18ngrep\n> is further filtered by a simple piplined grep to exclude an '... remote\n> end hung up unexpectedly' warning message. Use a regular 'grep -E' to\n> replace the call to test_i18ngrep in the filter pipeline.\n> Also, remove a useless invocation of 'sort' as the final element of the\n> pipeline.\n>\n> Signed-off-by: Ramsay Jones \n> ---\n> t/t5536-fetch-conflicts.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t5536-fetch-conflicts.sh b/t/t5536-fetch-conflicts.sh\n> index 2e42cf331..38381df5e 100755\n> --- a/t/t5536-fetch-conflicts.sh\n> +++ b/t/t5536-fetch-conflicts.sh\n> @@ -22,7 +22,7 @@ verify_stderr () {\n> \tcat >expected &&\n> \t# We're not interested in the error\n> \t# \"fatal: The remote end hung up unexpectedly\":\n> -\ttest_i18ngrep -E '^(fatal|warning):' actual | sort &&\n> +\tgrep -E '^(fatal|warning):' actual &&\n> \ttest_i18ncmp expected actual\n\nOK, but not quite OK.\n\nTwo grep invocations will not leave anything useful in 'actual'\nunder poison build, and is almost guaranteed that 'expected' would\nnot match, but that is perfectly OK because the final comparison is\ndone.\n\nHowever, it is brittle to rely on the latter \"grep -v\" to exit with\nstatus 0 for the &&-chain to work. The original was most likely\nmasked by the \"| sort\" exiting with 0 status all the time ;-)\n\nThere needs an explanation why this commit thinks the invocation of\n\"sort\" useless. I do not particularly think \"suppressing a\nnot-found non-zero exit from 'grep'\" is a useful thing, but are we\ncommitted to show the two warnings seen in the last test in this\nfile to always in the shown order (i.e. the same order as the\nrefspecs are given to us)? If not, then \"sort\" does serve a\npurpose. Note that I do not think we want to be able to reorder the\nwarning messages in future versions of Git for that last case, so\nmaking that explicit may be a good justification.\n\n The \"sort\" as the last step in the pipeline makes sure that we\n do not have to guarantee the order in which we give the two\n warning messages from the last test in this script, but\n processing the refspecs in the same order as they are given on\n the command line and warning against them as we discover\n problems is a property we would rather want to keep, so drop it\n to make sure that we would catch regression when we accidentally\n change the order in which these messages are given.\n\nor something like that, perhaps.\n\n"},{"id":"339110","messageId":"xmqqo9ktsyj2.fsf@gitster-ct.c.googlers.com","threadId":"47811","inReplyTo":"xmqq3726t11d.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-12T21:12:17Z","receivedAt":"2018-02-12T21:12:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> -\ttest_i18ngrep -E '^(fatal|warning):' actual | sort &&\n>> +\tgrep -E '^(fatal|warning):' actual &&\n>> \ttest_i18ncmp expected actual\n>\n> OK, but not quite OK.\n>\n> Two grep invocations will not leave anything useful in 'actual'\n> under poison build, and is almost guaranteed that 'expected' would\n> not match, but that is perfectly OK because the final comparison is\n> done.\n\nSorry. s/is done./is done with i18ncmp./ is what I wanted to say.\n"},{"id":"339163","messageId":"69c7dc21-fb52-5982-f7d8-04518d06db6c@ramsayjones.plus.com","threadId":"47811","inReplyTo":"xmqq3726t11d.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-02-13T01:58:19Z","receivedAt":"2018-02-13T01:58:27Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 12/02/18 20:18, Junio C Hamano wrote:\n> Ramsay Jones writes:\n> \n>> Attempting to grep the output of test_i18ngrep will not work under a\n>> poison build, since the output is (almost) guaranteed not to have the\n>> string you are looking for. In this case, the output of test_i18ngrep\n>> is further filtered by a simple piplined grep to exclude an '... remote\n>> end hung up unexpectedly' warning message. Use a regular 'grep -E' to\n>> replace the call to test_i18ngrep in the filter pipeline.\n>> Also, remove a useless invocation of 'sort' as the final element of the\n>> pipeline.\n>>\n>> Signed-off-by: Ramsay Jones \n>> ---\n>> t/t5536-fetch-conflicts.sh | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/t/t5536-fetch-conflicts.sh b/t/t5536-fetch-conflicts.sh\n>> index 2e42cf331..38381df5e 100755\n>> --- a/t/t5536-fetch-conflicts.sh\n>> +++ b/t/t5536-fetch-conflicts.sh\n>> @@ -22,7 +22,7 @@ verify_stderr () {\n>> \tcat >expected &&\n>> \t# We're not interested in the error\n>> \t# \"fatal: The remote end hung up unexpectedly\":\n>> -\ttest_i18ngrep -E '^(fatal|warning):' actual | sort &&\n>> +\tgrep -E '^(fatal|warning):' actual &&\n>> \ttest_i18ncmp expected actual\n> \n> OK, but not quite OK.\n\n:-D\n\n> Two grep invocations will not leave anything useful in 'actual'\n> under poison build, and is almost guaranteed that 'expected' would\n> not match, but that is perfectly OK because the final comparison is\n> done.\n\n\"...is done with i18ncmp.\", indeed.\n\nThe contents of 'actual' would look like:\n\n warning: # GETTEXT POISON #\n warning: # GETTEXT POISON #\n\nor\n\n fatal: # GETTEXT POISON #\n\n... depending on which test we are looking at.\n\n> However, it is brittle to rely on the latter \"grep -v\" to exit with\n> status 0 for the &&-chain to work. The original was most likely\n> masked by the \"| sort\" exiting with 0 status all the time ;-)\n\nI must admit that I didn't think about the effect of the useless\n\"| sort\" on the exit status! What I saw was: a process that\nreceived no input, sorted nothing and produced no output - pretty\nmuch the definition of useless! ;-)\n\nAlso, the \"| grep -v\" part does remove the \"... hung up ...\"\nmessage (when present in the input), since that message has not\nbeen i18n-ed. I thought this was deliberate - but maybe not.\n(also, so long a _some_ output gets passed on by that grep, the\nexit status will be 0).\n\n> There needs an explanation why this commit thinks the invocation of\n> \"sort\" useless.\n\nYou mean, other than the fact that it is? ;-)\n\n> I do not particularly think \"suppressing a\n> not-found non-zero exit from 'grep'\" is a useful thing, but are we\n> committed to show the two warnings seen in the last test in this\n> file to always in the shown order (i.e. the same order as the\n> refspecs are given to us)? If not, then \"sort\" does serve a\n> purpose. Note that I do not think we want to be able to reorder the\n> warning messages in future versions of Git for that last case, so\n> making that explicit may be a good justification.\n\nI did not look back at the history of this test, so I can't say\nif that was the original _intent_ of the \"| sort\" part of the\npipeline. However, it is not serving any purpose now.\n\n> The \"sort\" as the last step in the pipeline makes sure that we\n> do not have to guarantee the order in which we give the two\n> warning messages from the last test in this script, but\n> processing the refspecs in the same order as they are given on\n> the command line and warning against them as we discover\n> problems is a property we would rather want to keep, so drop it\n> to make sure that we would catch regression when we accidentally\n> change the order in which these messages are given.\n> \n> or something like that, perhaps.\n\nHmm, so do you want anything other than a commit message update?\n\nATB,\nRamsay Jones\n\n\n"},{"id":"339174","messageId":"xmqqvaf1qqcx.fsf@gitster-ct.c.googlers.com","threadId":"47811","inReplyTo":"69c7dc21-fb52-5982-f7d8-04518d06db6c@ramsayjones.plus.com","subject":"Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-13T07:51:42Z","receivedAt":"2018-02-13T07:51:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> I must admit that I didn't think about the effect of the useless\n> \"| sort\" on the exit status! What I saw was: a process that\n> received no input, sorted nothing and produced no output - pretty\n> much the definition of useless! ;-)\n\nI am not sure what you mean by \"receive no input, sort nothing and\nproduce no output\".\n\nAhh, OK, this is a funny one. I think the original meant to do\n\n\tgrep ... | grep -v ... | sort >actual\n\nbut it did\n\n\tgrep ... | grep -v ... >actual | sort\n\ninstead by mistake.\n\nAnd we have two possible \"fixes\" for that mistake. Either removing\n\"|sort\" (and replace its only effect, which is to hide brittleness\nof relying on exit status of the second grep, with something else)\nto declare that we do care about the order multiple warning messages\nare given by the last test in the script (by the way, the script is\nt5536, not t5556; the patch needs to be retitled), or keeping the \"|\nsort\" and move the redirection into \">actual\" to the correct place,\nwhich is to follow through the intention of having that \"sort\" on\nthe pipeline in the first place. I somewhat favor the former in\nthis particular case myself, but the preference is not a very strong\none.\n\nThanks.\n\n"},{"id":"339177","messageId":"20180213100437.15685-1-szeder.dev@gmail.com","threadId":"47811","inReplyTo":"xmqqvaf1qqcx.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-13T10:04:37Z","receivedAt":"2018-02-13T10:05:08Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"> > I must admit that I didn't think about the effect of the useless\n> > \"| sort\" on the exit status! What I saw was: a process that\n> > received no input, sorted nothing and produced no output - pretty\n> > much the definition of useless! ;-)\n> \n> I am not sure what you mean by \"receive no input, sort nothing and\n> produce no output\".\n> \n> Ahh, OK, this is a funny one. I think the original meant to do\n> \n> \tgrep ... | grep -v ... | sort >actual\n> \n> but it did\n> \n> \tgrep ... | grep -v ... >actual | sort\n> \n> instead by mistake.\n> \n> And we have two possible \"fixes\" for that mistake. Either removing\n> \"|sort\" (and replace its only effect, which is to hide brittleness\n> of relying on exit status of the second grep, with something else)\n> to declare that we do care about the order multiple warning messages\n> are given by the last test in the script (by the way, the script is\n> t5536, not t5556; the patch needs to be retitled), or keeping the \"|\n> sort\" and move the redirection into \">actual\" to the correct place,\n> which is to follow through the intention of having that \"sort\" on\n> the pipeline in the first place. I somewhat favor the former in\n> this particular case myself, but the preference is not a very strong\n> one.\n\nA third possible fix, which is also in the \"we don't care about the\norder of multiple warning messages\" camp and has a nice looking\ndiffstat, would be something like this:\n\n\ndiff --git a/t/t5536-fetch-conflicts.sh b/t/t5536-fetch-conflicts.sh\nindex 2e42cf3316..91f28c2f78 100755\n--- a/t/t5536-fetch-conflicts.sh\n+++ b/t/t5536-fetch-conflicts.sh\n@@ -18,14 +18,6 @@ setup_repository () {\n \t)\n }\n \n-verify_stderr () {\n-\tcat >expected &&\n-\t# We're not interested in the error\n-\t# \"fatal: The remote end hung up unexpectedly\":\n-\ttest_i18ngrep -E '^(fatal|warning):' actual | sort &&\n-\ttest_i18ncmp expected actual\n-}\n-\n test_expect_success 'setup' '\n \tgit commit --allow-empty -m \"Initial\" &&\n \tgit branch branch1 &&\n@@ -48,9 +40,7 @@ test_expect_success 'fetch conflict: config vs. config' '\n \t\t\"+refs/heads/branch2:refs/remotes/origin/branch1\" && (\n \t\tcd ccc &&\n \t\ttest_must_fail git fetch origin 2>error &&\n-\t\tverify_stderr <<-\\EOF\n-\t\tfatal: Cannot fetch both refs/heads/branch1 and refs/heads/branch2 to refs/remotes/origin/branch1\n-\t\tEOF\n+\t\ttest_i18ngrep \"fatal: Cannot fetch both refs/heads/branch1 and refs/heads/branch2 to refs/remotes/origin/branch1\" error\n \t)\n '\n \n@@ -77,9 +67,7 @@ test_expect_success 'fetch conflict: arg vs. arg' '\n \t\ttest_must_fail git fetch origin \\\n \t\t\trefs/heads/*:refs/remotes/origin/* \\\n \t\t\trefs/heads/branch2:refs/remotes/origin/branch1 2>error &&\n-\t\tverify_stderr <<-\\EOF\n-\t\tfatal: Cannot fetch both refs/heads/branch1 and refs/heads/branch2 to refs/remotes/origin/branch1\n-\t\tEOF\n+\t\ttest_i18ngrep \"fatal: Cannot fetch both refs/heads/branch1 and refs/heads/branch2 to refs/remotes/origin/branch1\" error\n \t)\n '\n \n@@ -90,10 +78,8 @@ test_expect_success 'fetch conflict: criss-cross args' '\n \t\tgit fetch origin \\\n \t\t\trefs/heads/branch1:refs/remotes/origin/branch2 \\\n \t\t\trefs/heads/branch2:refs/remotes/origin/branch1 2>error &&\n-\t\tverify_stderr <<-\\EOF\n-\t\twarning: refs/remotes/origin/branch1 usually tracks refs/heads/branch1, not refs/heads/branch2\n-\t\twarning: refs/remotes/origin/branch2 usually tracks refs/heads/branch2, not refs/heads/branch1\n-\t\tEOF\n+\t\ttest_i18ngrep \"warning: refs/remotes/origin/branch1 usually tracks refs/heads/branch1, not refs/heads/branch2\" error &&\n+\t\ttest_i18ngrep \"warning: refs/remotes/origin/branch2 usually tracks refs/heads/branch2, not refs/heads/branch1\" error\n \t)\n '\n \n"},{"id":"339204","messageId":"xmqqr2porf4z.fsf@gitster-ct.c.googlers.com","threadId":"47811","inReplyTo":"20180213100437.15685-1-szeder.dev@gmail.com","subject":"Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-13T17:08:44Z","receivedAt":"2018-02-13T17:08:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> A third possible fix, which is also in the \"we don't care about the\n> order of multiple warning messages\" camp and has a nice looking\n> diffstat, would be something like this:\n\nHmph, we are running a \"git fetch\" locally and observing the error\noutput from both \"fetch\" and its counterpart \"upload-pack\", aren't\nwe? The \"fetch\" instances that are run with test_must_fail are\nexpected to stop talking to \"upload-pack\" by detecting an error and\nsevere the connection abruptly---depending on the relative timing\nbetween the processes, the other side may try to read and diagnose\n\"the remote end hung up unexpectedly\", no? \n\nI think \"grep -v\" filtering is an attempt to protect the test from\ngetting confused by that output, but is it safe not to worry about\nit these days?\n\n> diff --git a/t/t5536-fetch-conflicts.sh b/t/t5536-fetch-conflicts.sh\n> index 2e42cf3316..91f28c2f78 100755\n> --- a/t/t5536-fetch-conflicts.sh\n> +++ b/t/t5536-fetch-conflicts.sh\n> @@ -18,14 +18,6 @@ setup_repository () {\n> \t)\n> }\n> \n> -verify_stderr () {\n> -\tcat >expected &&\n> -\t# We're not interested in the error\n> -\t# \"fatal: The remote end hung up unexpectedly\":\n> -\ttest_i18ngrep -E '^(fatal|warning):' actual | sort &&\n> -\ttest_i18ncmp expected actual\n> -}\n> -\n> test_expect_success 'setup' '\n> \tgit commit --allow-empty -m \"Initial\" &&\n> \tgit branch branch1 &&\n> @@ -48,9 +40,7 @@ test_expect_success 'fetch conflict: config vs. config' '\n> \t\t\"+refs/heads/branch2:refs/remotes/origin/branch1\" && (\n> \t\tcd ccc &&\n> \t\ttest_must_fail git fetch origin 2>error &&\n> -\t\tverify_stderr <<-\\EOF\n> -\t\tfatal: Cannot fetch both refs/heads/branch1 and refs/heads/branch2 to refs/remotes/origin/branch1\n> -\t\tEOF\n> +\t\ttest_i18ngrep \"fatal: Cannot fetch both refs/heads/branch1 and refs/heads/branch2 to refs/remotes/origin/branch1\" error\n> \t)\n> '\n"},{"id":"339206","messageId":"20180213172603.GA10062@sigill.intra.peff.net","threadId":"47811","inReplyTo":"xmqqr2porf4z.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-13T17:26:04Z","receivedAt":"2018-02-13T17:26:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 13, 2018 at 09:08:44AM -0800, Junio C Hamano wrote:\n\n> SZEDER Gábor writes:\n> \n> > A third possible fix, which is also in the \"we don't care about the\n> > order of multiple warning messages\" camp and has a nice looking\n> > diffstat, would be something like this:\n> \n> Hmph, we are running a \"git fetch\" locally and observing the error\n> output from both \"fetch\" and its counterpart \"upload-pack\", aren't\n> we? The \"fetch\" instances that are run with test_must_fail are\n> expected to stop talking to \"upload-pack\" by detecting an error and\n> severe the connection abruptly---depending on the relative timing\n> between the processes, the other side may try to read and diagnose\n> \"the remote end hung up unexpectedly\", no? \n\nIf I understand Gábor's patch correctly, it is using test_i18ngrep for\nthe specific lines we care about so that we don't have to worry about\nother cruft lines that may or may not appear (including the hangup one).\n\nThe downside is that we would not notice if a _new_ error message\n(beyond the ones we expect and the one we were explicitly ignoring)\nappeared. IMHO that's probably fine.\n\n-Peff\n"},{"id":"339210","messageId":"xmqqeflorc9m.fsf@gitster-ct.c.googlers.com","threadId":"47811","inReplyTo":"20180213172603.GA10062@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-13T18:10:45Z","receivedAt":"2018-02-13T18:10:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If I understand Gábor's patch correctly, it is using test_i18ngrep for\n> the specific lines we care about so that we don't have to worry about\n> other cruft lines that may or may not appear (including the hangup one).\n>\n> The downside is that we would not notice if a _new_ error message\n> (beyond the ones we expect and the one we were explicitly ignoring)\n> appeared. IMHO that's probably fine.\n\nAh, OK, I didn't notice how the multi-line one was handled. Unable\nto notice new error messages and undisturbed by possible \"hung up\"\nmessages are the sides of the same coin---I myself am unsure if it\nis a good trade-off, but I'm inclined to defer to judgment of two\npeople ;-)\n"},{"id":"340439","messageId":"xmqq371mqjce.fsf@gitster-ct.c.googlers.com","threadId":"47811","inReplyTo":"xmqqeflorc9m.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-27T20:16:17Z","receivedAt":"2018-02-27T20:16:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> If I understand Gábor's patch correctly, it is using test_i18ngrep for\n>> the specific lines we care about so that we don't have to worry about\n>> other cruft lines that may or may not appear (including the hangup one).\n>>\n>> The downside is that we would not notice if a _new_ error message\n>> (beyond the ones we expect and the one we were explicitly ignoring)\n>> appeared. IMHO that's probably fine.\n>\n> Ah, OK, I didn't notice how the multi-line one was handled. Unable\n> to notice new error messages and undisturbed by possible \"hung up\"\n> messages are the sides of the same coin---I myself am unsure if it\n> is a good trade-off, but I'm inclined to defer to judgment of two\n> people ;-)\n\nOK, somehow I had the version from Ramsay on a topic branch that was\nnot merged to 'pu'. Here is the replacement for 2/2 I'd be queuing.\n\nWe'd need SZEDER to sign it off (optionally correcting mistakes in\nthe log message) if we are going with this solution.\n\nThanks.\n\n-- >8 --\nFrom: SZEDER Gábor \nDate: Tue, 13 Feb 2018 11:04:37 +0100\nSubject: [PATCH] t5536: simplify checks for fetch error verification\n\nThe verify_stderr helper had this construct\n\n\ttest_i18ngrep ... error | grep -v ... >actual | sort &&\n\t...\n\nin which 'sort' was clearly doing nothing (other than hiding the\nexit status of the \"grep -v\" from &&-chain). It obviously is a\nbotched attempt to make sure \"actual\" can be compared with expected\noutput without having to worry about the order of errors and\nwarnings in the input file, i.e.\n\n\ttest_i18ngrep ... error | grep -v ... | sort >actual &&\n\t...\n\nInstead of grabbing all errors and warnings from the command and\nseeing if they match what is expected after sorted, look for\nspecific errors and warnings each test cares about and eliminate\nthis buggy helper.\n\nSigned-off-by: Junio C Hamano \n---\n t/t5536-fetch-conflicts.sh | 22 ++++------------------\n 1 file changed, 4 insertions(+), 18 deletions(-)\n\ndiff --git a/t/t5536-fetch-conflicts.sh b/t/t5536-fetch-conflicts.sh\nindex 2e42cf3316..91f28c2f78 100755\n--- a/t/t5536-fetch-conflicts.sh\n+++ b/t/t5536-fetch-conflicts.sh\n@@ -18,14 +18,6 @@ setup_repository () {\n \t)\n }\n \n-verify_stderr () {\n-\tcat >expected &&\n-\t# We're not interested in the error\n-\t# \"fatal: The remote end hung up unexpectedly\":\n-\ttest_i18ngrep -E '^(fatal|warning):' actual | sort &&\n-\ttest_i18ncmp expected actual\n-}\n-\n test_expect_success 'setup' '\n \tgit commit --allow-empty -m \"Initial\" &&\n \tgit branch branch1 &&\n@@ -48,9 +40,7 @@ test_expect_success 'fetch conflict: config vs. config' '\n \t\t\"+refs/heads/branch2:refs/remotes/origin/branch1\" && (\n \t\tcd ccc &&\n \t\ttest_must_fail git fetch origin 2>error &&\n-\t\tverify_stderr <<-\\EOF\n-\t\tfatal: Cannot fetch both refs/heads/branch1 and refs/heads/branch2 to refs/remotes/origin/branch1\n-\t\tEOF\n+\t\ttest_i18ngrep \"fatal: Cannot fetch both refs/heads/branch1 and refs/heads/branch2 to refs/remotes/origin/branch1\" error\n \t)\n '\n \n@@ -77,9 +67,7 @@ test_expect_success 'fetch conflict: arg vs. arg' '\n \t\ttest_must_fail git fetch origin \\\n \t\t\trefs/heads/*:refs/remotes/origin/* \\\n \t\t\trefs/heads/branch2:refs/remotes/origin/branch1 2>error &&\n-\t\tverify_stderr <<-\\EOF\n-\t\tfatal: Cannot fetch both refs/heads/branch1 and refs/heads/branch2 to refs/remotes/origin/branch1\n-\t\tEOF\n+\t\ttest_i18ngrep \"fatal: Cannot fetch both refs/heads/branch1 and refs/heads/branch2 to refs/remotes/origin/branch1\" error\n \t)\n '\n \n@@ -90,10 +78,8 @@ test_expect_success 'fetch conflict: criss-cross args' '\n \t\tgit fetch origin \\\n \t\t\trefs/heads/branch1:refs/remotes/origin/branch2 \\\n \t\t\trefs/heads/branch2:refs/remotes/origin/branch1 2>error &&\n-\t\tverify_stderr <<-\\EOF\n-\t\twarning: refs/remotes/origin/branch1 usually tracks refs/heads/branch1, not refs/heads/branch2\n-\t\twarning: refs/remotes/origin/branch2 usually tracks refs/heads/branch2, not refs/heads/branch1\n-\t\tEOF\n+\t\ttest_i18ngrep \"warning: refs/remotes/origin/branch1 usually tracks refs/heads/branch1, not refs/heads/branch2\" error &&\n+\t\ttest_i18ngrep \"warning: refs/remotes/origin/branch2 usually tracks refs/heads/branch2, not refs/heads/branch1\" error\n \t)\n '\n \n-- \n2.16.2-264-ge3a80781f5\n\n"},{"id":"340467","messageId":"xmqq4lm2ozq3.fsf@gitster-ct.c.googlers.com","threadId":"47811","inReplyTo":"xmqq371mqjce.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-27T22:05:24Z","receivedAt":"2018-02-27T22:05:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> OK, somehow I had the version from Ramsay on a topic branch that was\n> not merged to 'pu'. Here is the replacement for 2/2 I'd be queuing.\n>\n> We'd need SZEDER to sign it off (optionally correcting mistakes in\n> the log message) if we are going with this solution.\n>\n> Thanks.\n\nI guess I missed Ramsay's v2 which already did this\n\n<550fb3f4-8d25-c5c4-0ecd-3a4e61ea13f4@ramsayjones.plus.com>\n\nso I'll use that version. We still want sign-off from Szeder,\nthough.\n\n"},{"id":"340491","messageId":"192d4ee4-dbdd-3e47-b45c-8d1f7b69b9af@ramsayjones.plus.com","threadId":"47811","inReplyTo":"xmqq4lm2ozq3.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-02-27T23:47:03Z","receivedAt":"2018-02-27T23:47:12Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 27/02/18 22:05, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> OK, somehow I had the version from Ramsay on a topic branch that was\n>> not merged to 'pu'. Here is the replacement for 2/2 I'd be queuing.\n>>\n>> We'd need SZEDER to sign it off (optionally correcting mistakes in\n>> the log message) if we are going with this solution.\n>>\n>> Thanks.\n> \n> I guess I missed Ramsay's v2 which already did this\n> \n> <550fb3f4-8d25-c5c4-0ecd-3a4e61ea13f4@ramsayjones.plus.com>\n\nYes, and as I said in the cover letter, I wasn't too sure that\nI had passed that patch along correctly. ;-)\n\n> so I'll use that version. We still want sign-off from Szeder,\n> though.\n\nI would be happy with either version, or maybe Szeder would like\nto tweak the commit message. In any event, it would be good to\nget sign-off from Szeder.\n\nThanks!\n\nATB,\nRamsay Jones\n\n\n"},{"id":"340499","messageId":"CAM0VKj=hbT_m21ssF+nedVDrfNiYHu8wd9mUD55mAdNgmnZSgA@mail.gmail.com","threadId":"47811","inReplyTo":"192d4ee4-dbdd-3e47-b45c-8d1f7b69b9af@ramsayjones.plus.com","subject":"Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-28T00:42:06Z","receivedAt":"2018-02-28T00:42:12Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Feb 28, 2018 at 12:47 AM, Ramsay Jones\n wrote:\n>\n>\n> On 27/02/18 22:05, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>>\n>>> OK, somehow I had the version from Ramsay on a topic branch that was\n>>> not merged to 'pu'. Here is the replacement for 2/2 I'd be queuing.\n>>>\n>>> We'd need SZEDER to sign it off (optionally correcting mistakes in\n>>> the log message) if we are going with this solution.\n>>>\n>>> Thanks.\n>>\n>> I guess I missed Ramsay's v2 which already did this\n>>\n>> <550fb3f4-8d25-c5c4-0ecd-3a4e61ea13f4@ramsayjones.plus.com>\n>\n> Yes, and as I said in the cover letter, I wasn't too sure that\n> I had passed that patch along correctly. ;-)\n>\n>> so I'll use that version. We still want sign-off from Szeder,\n>> though.\n>\n> I would be happy with either version, or maybe Szeder would like\n> to tweak the commit message. In any event, it would be good to\n> get sign-off from Szeder.\n\nCertainly, here you go:\n\nSigned-off-by: SZEDER Gábor \n\nHowever, I'm not sure about the authorship and taking credit for the\npatch. We ended up taking my patch, sure, but I think Ramsay did all\nthe real hard work, i.e. writing the commit message and, most\nimportantly, realizing that something is wrong with that '...| sort' at\nthe end of the line.\n"},{"id":"340568","messageId":"ffc3d3dc-6d21-4273-5362-cbf32e7e4e59@ramsayjones.plus.com","threadId":"47811","inReplyTo":"CAM0VKj=hbT_m21ssF+nedVDrfNiYHu8wd9mUD55mAdNgmnZSgA@mail.gmail.com","subject":"Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-02-28T15:33:51Z","receivedAt":"2018-02-28T15:34:05Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 28/02/18 00:42, SZEDER Gábor wrote:\n> On Wed, Feb 28, 2018 at 12:47 AM, Ramsay Jones\n> wrote:\n>>\n>>\n>> On 27/02/18 22:05, Junio C Hamano wrote:\n>>> Junio C Hamano writes:\n>>>\n>>>> OK, somehow I had the version from Ramsay on a topic branch that was\n>>>> not merged to 'pu'. Here is the replacement for 2/2 I'd be queuing.\n>>>>\n>>>> We'd need SZEDER to sign it off (optionally correcting mistakes in\n>>>> the log message) if we are going with this solution.\n>>>>\n>>>> Thanks.\n>>>\n>>> I guess I missed Ramsay's v2 which already did this\n>>>\n>>> <550fb3f4-8d25-c5c4-0ecd-3a4e61ea13f4@ramsayjones.plus.com>\n>>\n>> Yes, and as I said in the cover letter, I wasn't too sure that\n>> I had passed that patch along correctly. ;-)\n>>\n>>> so I'll use that version. We still want sign-off from Szeder,\n>>> though.\n>>\n>> I would be happy with either version, or maybe Szeder would like\n>> to tweak the commit message. In any event, it would be good to\n>> get sign-off from Szeder.\n> \n> Certainly, here you go:\n> \n> Signed-off-by: SZEDER Gábor \n\nThanks!\n\n> However, I'm not sure about the authorship and taking credit for the\n> patch. We ended up taking my patch, sure, but I think Ramsay did all\n> the real hard work, i.e. writing the commit message and, most\n> importantly, realizing that something is wrong with that '...| sort' at\n> the end of the line.\n\nNo, the patch and the credit are yours, I was just trying to\nhelp out and get the patches moving forward. At most, I would\nthink a 'Helped-by:' would be sufficient to note my input.\n\n[BTW, my 'Signed-off-by:' on that patch was in the spirit of\nthe dco section b. - again I wasn't quite sure ...]\n\nATB,\nRamsay Jones\n\n\n"},{"id":"340570","messageId":"xmqqo9k9m6jm.fsf@gitster-ct.c.googlers.com","threadId":"47811","inReplyTo":"ffc3d3dc-6d21-4273-5362-cbf32e7e4e59@ramsayjones.plus.com","subject":"Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-28T16:18:37Z","receivedAt":"2018-02-28T16:18:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n>> However, I'm not sure about the authorship and taking credit for the\n>> patch. We ended up taking my patch, sure, but I think Ramsay did all\n>> the real hard work, i.e. writing the commit message and, most\n>> importantly, realizing that something is wrong with that '...| sort' at\n>> the end of the line.\n>\n> No, the patch and the credit are yours, I was just trying to\n> help out and get the patches moving forward. At most, I would\n> think a 'Helped-by:' would be sufficient to note my input.\n\nOK.\n\n\n> [BTW, my 'Signed-off-by:' on that patch was in the spirit of\n> the dco section b. - again I wasn't quite sure ...]\n\nI take your \"wasn't\" to imply that by now you are a bit more sure?\nIt was a perfectly fine \"I am passing it along\" sign-off.\n\nThanks, both.\n"}]}