Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep
Ramsay Jones writes: >> However, I'm not sure about the authorship and taking credit for the >> patch. We ended up taking my patch, sure, but I think Ramsay did all >> the real hard work, i.e. writing the commit message and, most >> importantly, realizing that something is wrong with that '...| sort' at >> the end of the line. > > No, the patch and the credit are yours, I was just trying to > help out and get the patches moving forward. At most, I would > think a 'Helped-by:' would be sufficient to note my input. OK. > [BTW, my 'Signed-off-by:' on that patch was in the spirit of > the dco section b. - again I wasn't quite sure ...] I take your "wasn't" to imply that by now you are a bit more sure? It was a perfectly fine "I am passing it along" sign-off. Thanks, both.
Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep
On 28/02/18 00:42, SZEDER Gábor wrote: > On Wed, Feb 28, 2018 at 12:47 AM, Ramsay Jones > wrote: >> >> >> On 27/02/18 22:05, Junio C Hamano wrote: >>> Junio C Hamano writes: >>> OK, somehow I had the version from Ramsay on a topic branch that was not merged to 'pu'. Here is the replacement for 2/2 I'd be queuing. We'd need SZEDER to sign it off (optionally correcting mistakes in the log message) if we are going with this solution. Thanks. >>> >>> I guess I missed Ramsay's v2 which already did this >>> >>> <550fb3f4-8d25-c5c4-0ecd-3a4e61ea1...@ramsayjones.plus.com> >> >> Yes, and as I said in the cover letter, I wasn't too sure that >> I had passed that patch along correctly. ;-) >> >>> so I'll use that version. We still want sign-off from Szeder, >>> though. >> >> I would be happy with either version, or maybe Szeder would like >> to tweak the commit message. In any event, it would be good to >> get sign-off from Szeder. > > Certainly, here you go: > > Signed-off-by: SZEDER Gábor Thanks! > However, I'm not sure about the authorship and taking credit for the > patch. We ended up taking my patch, sure, but I think Ramsay did all > the real hard work, i.e. writing the commit message and, most > importantly, realizing that something is wrong with that '...| sort' at > the end of the line. No, the patch and the credit are yours, I was just trying to help out and get the patches moving forward. At most, I would think a 'Helped-by:' would be sufficient to note my input. [BTW, my 'Signed-off-by:' on that patch was in the spirit of the dco section b. - again I wasn't quite sure ...] ATB, Ramsay Jones
Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep
On Wed, Feb 28, 2018 at 12:47 AM, Ramsay Jones wrote: > > > On 27/02/18 22:05, Junio C Hamano wrote: >> Junio C Hamano writes: >> >>> OK, somehow I had the version from Ramsay on a topic branch that was >>> not merged to 'pu'. Here is the replacement for 2/2 I'd be queuing. >>> >>> We'd need SZEDER to sign it off (optionally correcting mistakes in >>> the log message) if we are going with this solution. >>> >>> Thanks. >> >> I guess I missed Ramsay's v2 which already did this >> >> <550fb3f4-8d25-c5c4-0ecd-3a4e61ea1...@ramsayjones.plus.com> > > Yes, and as I said in the cover letter, I wasn't too sure that > I had passed that patch along correctly. ;-) > >> so I'll use that version. We still want sign-off from Szeder, >> though. > > I would be happy with either version, or maybe Szeder would like > to tweak the commit message. In any event, it would be good to > get sign-off from Szeder. Certainly, here you go: Signed-off-by: SZEDER Gábor However, I'm not sure about the authorship and taking credit for the patch. We ended up taking my patch, sure, but I think Ramsay did all the real hard work, i.e. writing the commit message and, most importantly, realizing that something is wrong with that '...| sort' at the end of the line.
Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep
On 27/02/18 22:05, Junio C Hamano wrote: > Junio C Hamano writes: > >> OK, somehow I had the version from Ramsay on a topic branch that was >> not merged to 'pu'. Here is the replacement for 2/2 I'd be queuing. >> >> We'd need SZEDER to sign it off (optionally correcting mistakes in >> the log message) if we are going with this solution. >> >> Thanks. > > I guess I missed Ramsay's v2 which already did this > > <550fb3f4-8d25-c5c4-0ecd-3a4e61ea1...@ramsayjones.plus.com> Yes, and as I said in the cover letter, I wasn't too sure that I had passed that patch along correctly. ;-) > so I'll use that version. We still want sign-off from Szeder, > though. I would be happy with either version, or maybe Szeder would like to tweak the commit message. In any event, it would be good to get sign-off from Szeder. Thanks! ATB, Ramsay Jones
Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep
Junio C Hamano writes: > OK, somehow I had the version from Ramsay on a topic branch that was > not merged to 'pu'. Here is the replacement for 2/2 I'd be queuing. > > We'd need SZEDER to sign it off (optionally correcting mistakes in > the log message) if we are going with this solution. > > Thanks. I guess I missed Ramsay's v2 which already did this <550fb3f4-8d25-c5c4-0ecd-3a4e61ea1...@ramsayjones.plus.com> so I'll use that version. We still want sign-off from Szeder, though.
Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep
Junio C Hamano writes: > Jeff King writes: > >> If I understand Gábor's patch correctly, it is using test_i18ngrep for >> the specific lines we care about so that we don't have to worry about >> other cruft lines that may or may not appear (including the hangup one). >> >> The downside is that we would not notice if a _new_ error message >> (beyond the ones we expect and the one we were explicitly ignoring) >> appeared. IMHO that's probably fine. > > Ah, OK, I didn't notice how the multi-line one was handled. Unable > to notice new error messages and undisturbed by possible "hung up" > messages are the sides of the same coin---I myself am unsure if it > is a good trade-off, but I'm inclined to defer to judgment of two > people ;-) OK, somehow I had the version from Ramsay on a topic branch that was not merged to 'pu'. Here is the replacement for 2/2 I'd be queuing. We'd need SZEDER to sign it off (optionally correcting mistakes in the log message) if we are going with this solution. Thanks. -- >8 -- From: SZEDER Gábor Date: Tue, 13 Feb 2018 11:04:37 +0100 Subject: [PATCH] t5536: simplify checks for fetch error verification The verify_stderr helper had this construct test_i18ngrep ... error | grep -v ... >actual | sort && ... in which 'sort' was clearly doing nothing (other than hiding the exit status of the "grep -v" from &&-chain). It obviously is a botched attempt to make sure "actual" can be compared with expected output without having to worry about the order of errors and warnings in the input file, i.e. test_i18ngrep ... error | grep -v ... | sort >actual && ... Instead of grabbing all errors and warnings from the command and seeing if they match what is expected after sorted, look for specific errors and warnings each test cares about and eliminate this buggy helper. Signed-off-by: Junio C Hamano --- t/t5536-fetch-conflicts.sh | 22 -- 1 file changed, 4 insertions(+), 18 deletions(-) diff --git a/t/t5536-fetch-conflicts.sh b/t/t5536-fetch-conflicts.sh index 2e42cf3316..91f28c2f78 100755 --- a/t/t5536-fetch-conflicts.sh +++ b/t/t5536-fetch-conflicts.sh @@ -18,14 +18,6 @@ setup_repository () { ) } -verify_stderr () { - cat >expected && - # We're not interested in the error - # "fatal: The remote end hung up unexpectedly": - test_i18ngrep -E '^(fatal|warning):' actual | sort && - test_i18ncmp expected actual -} - test_expect_success 'setup' ' git commit --allow-empty -m "Initial" && git branch branch1 && @@ -48,9 +40,7 @@ test_expect_success 'fetch conflict: config vs. config' ' "+refs/heads/branch2:refs/remotes/origin/branch1" && ( cd ccc && test_must_fail git fetch origin 2>error && - verify_stderr <<-\EOF - fatal: Cannot fetch both refs/heads/branch1 and refs/heads/branch2 to refs/remotes/origin/branch1 - EOF + test_i18ngrep "fatal: Cannot fetch both refs/heads/branch1 and refs/heads/branch2 to refs/remotes/origin/branch1" error ) ' @@ -77,9 +67,7 @@ test_expect_success 'fetch conflict: arg vs. arg' ' test_must_fail git fetch origin \ refs/heads/*:refs/remotes/origin/* \ refs/heads/branch2:refs/remotes/origin/branch1 2>error && - verify_stderr <<-\EOF - fatal: Cannot fetch both refs/heads/branch1 and refs/heads/branch2 to refs/remotes/origin/branch1 - EOF + test_i18ngrep "fatal: Cannot fetch both refs/heads/branch1 and refs/heads/branch2 to refs/remotes/origin/branch1" error ) ' @@ -90,10 +78,8 @@ test_expect_success 'fetch conflict: criss-cross args' ' git fetch origin \ refs/heads/branch1:refs/remotes/origin/branch2 \ refs/heads/branch2:refs/remotes/origin/branch1 2>error && - verify_stderr <<-\EOF - warning: refs/remotes/origin/branch1 usually tracks refs/heads/branch1, not refs/heads/branch2 - warning: refs/remotes/origin/branch2 usually tracks refs/heads/branch2, not refs/heads/branch1 - EOF + test_i18ngrep "warning: refs/remotes/origin/branch1 usually tracks refs/heads/branch1, not refs/heads/branch2" error && + test_i18ngrep "warning: refs/remotes/origin/branch2 usually tracks refs/heads/branch2, not refs/heads/branch1" error ) ' -- 2.16.2-264-ge3a80781f5
Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep
Jeff King writes: > If I understand Gábor's patch correctly, it is using test_i18ngrep for > the specific lines we care about so that we don't have to worry about > other cruft lines that may or may not appear (including the hangup one). > > The downside is that we would not notice if a _new_ error message > (beyond the ones we expect and the one we were explicitly ignoring) > appeared. IMHO that's probably fine. Ah, OK, I didn't notice how the multi-line one was handled. Unable to notice new error messages and undisturbed by possible "hung up" messages are the sides of the same coin---I myself am unsure if it is a good trade-off, but I'm inclined to defer to judgment of two people ;-)
Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep
On Tue, Feb 13, 2018 at 09:08:44AM -0800, Junio C Hamano wrote: > SZEDER Gábor writes: > > > A third possible fix, which is also in the "we don't care about the > > order of multiple warning messages" camp and has a nice looking > > diffstat, would be something like this: > > Hmph, we are running a "git fetch" locally and observing the error > output from both "fetch" and its counterpart "upload-pack", aren't > we? The "fetch" instances that are run with test_must_fail are > expected to stop talking to "upload-pack" by detecting an error and > severe the connection abruptly---depending on the relative timing > between the processes, the other side may try to read and diagnose > "the remote end hung up unexpectedly", no? If I understand Gábor's patch correctly, it is using test_i18ngrep for the specific lines we care about so that we don't have to worry about other cruft lines that may or may not appear (including the hangup one). The downside is that we would not notice if a _new_ error message (beyond the ones we expect and the one we were explicitly ignoring) appeared. IMHO that's probably fine. -Peff
Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep
SZEDER Gábor writes: > A third possible fix, which is also in the "we don't care about the > order of multiple warning messages" camp and has a nice looking > diffstat, would be something like this: Hmph, we are running a "git fetch" locally and observing the error output from both "fetch" and its counterpart "upload-pack", aren't we? The "fetch" instances that are run with test_must_fail are expected to stop talking to "upload-pack" by detecting an error and severe the connection abruptly---depending on the relative timing between the processes, the other side may try to read and diagnose "the remote end hung up unexpectedly", no? I think "grep -v" filtering is an attempt to protect the test from getting confused by that output, but is it safe not to worry about it these days? > diff --git a/t/t5536-fetch-conflicts.sh b/t/t5536-fetch-conflicts.sh > index 2e42cf3316..91f28c2f78 100755 > --- a/t/t5536-fetch-conflicts.sh > +++ b/t/t5536-fetch-conflicts.sh > @@ -18,14 +18,6 @@ setup_repository () { > ) > } > > -verify_stderr () { > - cat >expected && > - # We're not interested in the error > - # "fatal: The remote end hung up unexpectedly": > - test_i18ngrep -E '^(fatal|warning):' actual > | sort && > - test_i18ncmp expected actual > -} > - > test_expect_success 'setup' ' > git commit --allow-empty -m "Initial" && > git branch branch1 && > @@ -48,9 +40,7 @@ test_expect_success 'fetch conflict: config vs. config' ' > "+refs/heads/branch2:refs/remotes/origin/branch1" && ( > cd ccc && > test_must_fail git fetch origin 2>error && > - verify_stderr <<-\EOF > - fatal: Cannot fetch both refs/heads/branch1 and > refs/heads/branch2 to refs/remotes/origin/branch1 > - EOF > + test_i18ngrep "fatal: Cannot fetch both refs/heads/branch1 and > refs/heads/branch2 to refs/remotes/origin/branch1" error > ) > '
Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep
> > I must admit that I didn't think about the effect of the useless > > "| sort" on the exit status! What I saw was: a process that > > received no input, sorted nothing and produced no output - pretty > > much the definition of useless! ;-) > > I am not sure what you mean by "receive no input, sort nothing and > produce no output". > > Ahh, OK, this is a funny one. I think the original meant to do > > grep ... | grep -v ... | sort >actual > > but it did > > grep ... | grep -v ... >actual | sort > > instead by mistake. > > And we have two possible "fixes" for that mistake. Either removing > "|sort" (and replace its only effect, which is to hide brittleness > of relying on exit status of the second grep, with something else) > to declare that we do care about the order multiple warning messages > are given by the last test in the script (by the way, the script is > t5536, not t5556; the patch needs to be retitled), or keeping the "| > sort" and move the redirection into ">actual" to the correct place, > which is to follow through the intention of having that "sort" on > the pipeline in the first place. I somewhat favor the former in > this particular case myself, but the preference is not a very strong > one. A third possible fix, which is also in the "we don't care about the order of multiple warning messages" camp and has a nice looking diffstat, would be something like this: diff --git a/t/t5536-fetch-conflicts.sh b/t/t5536-fetch-conflicts.sh index 2e42cf3316..91f28c2f78 100755 --- a/t/t5536-fetch-conflicts.sh +++ b/t/t5536-fetch-conflicts.sh @@ -18,14 +18,6 @@ setup_repository () { ) } -verify_stderr () { - cat >expected && - # We're not interested in the error - # "fatal: The remote end hung up unexpectedly": - test_i18ngrep -E '^(fatal|warning):' actual | sort && - test_i18ncmp expected actual -} - test_expect_success 'setup' ' git commit --allow-empty -m "Initial" && git branch branch1 && @@ -48,9 +40,7 @@ test_expect_success 'fetch conflict: config vs. config' ' "+refs/heads/branch2:refs/remotes/origin/branch1" && ( cd ccc && test_must_fail git fetch origin 2>error && - verify_stderr <<-\EOF - fatal: Cannot fetch both refs/heads/branch1 and refs/heads/branch2 to refs/remotes/origin/branch1 - EOF + test_i18ngrep "fatal: Cannot fetch both refs/heads/branch1 and refs/heads/branch2 to refs/remotes/origin/branch1" error ) ' @@ -77,9 +67,7 @@ test_expect_success 'fetch conflict: arg vs. arg' ' test_must_fail git fetch origin \ refs/heads/*:refs/remotes/origin/* \ refs/heads/branch2:refs/remotes/origin/branch1 2>error && - verify_stderr <<-\EOF - fatal: Cannot fetch both refs/heads/branch1 and refs/heads/branch2 to refs/remotes/origin/branch1 - EOF + test_i18ngrep "fatal: Cannot fetch both refs/heads/branch1 and refs/heads/branch2 to refs/remotes/origin/branch1" error ) ' @@ -90,10 +78,8 @@ test_expect_success 'fetch conflict: criss-cross args' ' git fetch origin \ refs/heads/branch1:refs/remotes/origin/branch2 \ refs/heads/branch2:refs/remotes/origin/branch1 2>error && - verify_stderr <<-\EOF - warning: refs/remotes/origin/branch1 usually tracks refs/heads/branch1, not refs/heads/branch2 - warning: refs/remotes/origin/branch2 usually tracks refs/heads/branch2, not refs/heads/branch1 - EOF + test_i18ngrep "warning: refs/remotes/origin/branch1 usually tracks refs/heads/branch1, not refs/heads/branch2" error && + test_i18ngrep "warning: refs/remotes/origin/branch2 usually tracks refs/heads/branch2, not refs/heads/branch1" error ) '
Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep
Ramsay Jones writes: > I must admit that I didn't think about the effect of the useless > "| sort" on the exit status! What I saw was: a process that > received no input, sorted nothing and produced no output - pretty > much the definition of useless! ;-) I am not sure what you mean by "receive no input, sort nothing and produce no output". Ahh, OK, this is a funny one. I think the original meant to do grep ... | grep -v ... | sort >actual but it did grep ... | grep -v ... >actual | sort instead by mistake. And we have two possible "fixes" for that mistake. Either removing "|sort" (and replace its only effect, which is to hide brittleness of relying on exit status of the second grep, with something else) to declare that we do care about the order multiple warning messages are given by the last test in the script (by the way, the script is t5536, not t5556; the patch needs to be retitled), or keeping the "| sort" and move the redirection into ">actual" to the correct place, which is to follow through the intention of having that "sort" on the pipeline in the first place. I somewhat favor the former in this particular case myself, but the preference is not a very strong one. Thanks.
Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep
On 12/02/18 20:18, Junio C Hamano wrote: > Ramsay Jones writes: > >> Attempting to grep the output of test_i18ngrep will not work under a >> poison build, since the output is (almost) guaranteed not to have the >> string you are looking for. In this case, the output of test_i18ngrep >> is further filtered by a simple piplined grep to exclude an '... remote >> end hung up unexpectedly' warning message. Use a regular 'grep -E' to >> replace the call to test_i18ngrep in the filter pipeline. >> Also, remove a useless invocation of 'sort' as the final element of the >> pipeline. >> >> Signed-off-by: Ramsay Jones >> --- >> t/t5536-fetch-conflicts.sh | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/t/t5536-fetch-conflicts.sh b/t/t5536-fetch-conflicts.sh >> index 2e42cf331..38381df5e 100755 >> --- a/t/t5536-fetch-conflicts.sh >> +++ b/t/t5536-fetch-conflicts.sh >> @@ -22,7 +22,7 @@ verify_stderr () { >> cat >expected && >> # We're not interested in the error >> # "fatal: The remote end hung up unexpectedly": >> -test_i18ngrep -E '^(fatal|warning):' actual >> | sort && >> +grep -E '^(fatal|warning):' actual && >> test_i18ncmp expected actual > > OK, but not quite OK. :-D > Two grep invocations will not leave anything useful in 'actual' > under poison build, and is almost guaranteed that 'expected' would > not match, but that is perfectly OK because the final comparison is > done. "...is done with i18ncmp.", indeed. The contents of 'actual' would look like: warning: # GETTEXT POISON # warning: # GETTEXT POISON # or fatal: # GETTEXT POISON # ... depending on which test we are looking at. > However, it is brittle to rely on the latter "grep -v" to exit with > status 0 for the &&-chain to work. The original was most likely > masked by the "| sort" exiting with 0 status all the time ;-) I must admit that I didn't think about the effect of the useless "| sort" on the exit status! What I saw was: a process that received no input, sorted nothing and produced no output - pretty much the definition of useless! ;-) Also, the "| grep -v" part does remove the "... hung up ..." message (when present in the input), since that message has not been i18n-ed. I thought this was deliberate - but maybe not. (also, so long a _some_ output gets passed on by that grep, the exit status will be 0). > There needs an explanation why this commit thinks the invocation of > "sort" useless. You mean, other than the fact that it is? ;-) > I do not particularly think "suppressing a > not-found non-zero exit from 'grep'" is a useful thing, but are we > committed to show the two warnings seen in the last test in this > file to always in the shown order (i.e. the same order as the > refspecs are given to us)? If not, then "sort" does serve a > purpose. Note that I do not think we want to be able to reorder the > warning messages in future versions of Git for that last case, so > making that explicit may be a good justification. I did not look back at the history of this test, so I can't say if that was the original _intent_ of the "| sort" part of the pipeline. However, it is not serving any purpose now. > The "sort" as the last step in the pipeline makes sure that we > do not have to guarantee the order in which we give the two > warning messages from the last test in this script, but > processing the refspecs in the same order as they are given on > the command line and warning against them as we discover > problems is a property we would rather want to keep, so drop it > to make sure that we would catch regression when we accidentally > change the order in which these messages are given. > > or something like that, perhaps. Hmm, so do you want anything other than a commit message update? ATB, Ramsay Jones
Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep
Junio C Hamano writes: >> -test_i18ngrep -E '^(fatal|warning):' actual >> | sort && >> +grep -E '^(fatal|warning):' actual && >> test_i18ncmp expected actual > > OK, but not quite OK. > > Two grep invocations will not leave anything useful in 'actual' > under poison build, and is almost guaranteed that 'expected' would > not match, but that is perfectly OK because the final comparison is > done. Sorry. s/is done./is done with i18ncmp./ is what I wanted to say.
Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep
Ramsay Jones writes: > Attempting to grep the output of test_i18ngrep will not work under a > poison build, since the output is (almost) guaranteed not to have the > string you are looking for. In this case, the output of test_i18ngrep > is further filtered by a simple piplined grep to exclude an '... remote > end hung up unexpectedly' warning message. Use a regular 'grep -E' to > replace the call to test_i18ngrep in the filter pipeline. > Also, remove a useless invocation of 'sort' as the final element of the > pipeline. > > Signed-off-by: Ramsay Jones > --- > t/t5536-fetch-conflicts.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/t5536-fetch-conflicts.sh b/t/t5536-fetch-conflicts.sh > index 2e42cf331..38381df5e 100755 > --- a/t/t5536-fetch-conflicts.sh > +++ b/t/t5536-fetch-conflicts.sh > @@ -22,7 +22,7 @@ verify_stderr () { > cat >expected && > # We're not interested in the error > # "fatal: The remote end hung up unexpectedly": > - test_i18ngrep -E '^(fatal|warning):' actual > | sort && > + grep -E '^(fatal|warning):' actual && > test_i18ncmp expected actual OK, but not quite OK. Two grep invocations will not leave anything useful in 'actual' under poison build, and is almost guaranteed that 'expected' would not match, but that is perfectly OK because the final comparison is done. However, it is brittle to rely on the latter "grep -v" to exit with status 0 for the &&-chain to work. The original was most likely masked by the "| sort" exiting with 0 status all the time ;-) There needs an explanation why this commit thinks the invocation of "sort" useless. I do not particularly think "suppressing a not-found non-zero exit from 'grep'" is a useful thing, but are we committed to show the two warnings seen in the last test in this file to always in the shown order (i.e. the same order as the refspecs are given to us)? If not, then "sort" does serve a purpose. Note that I do not think we want to be able to reorder the warning messages in future versions of Git for that last case, so making that explicit may be a good justification. The "sort" as the last step in the pipeline makes sure that we do not have to guarantee the order in which we give the two warning messages from the last test in this script, but processing the refspecs in the same order as they are given on the command line and warning against them as we discover problems is a property we would rather want to keep, so drop it to make sure that we would catch regression when we accidentally change the order in which these messages are given. or something like that, perhaps.