[PATCH 2/2] t5556: replace test_i18ngrep with a simple grep

[PATCH 2/2] t5556: replace test_i18ngrep with a simple grep

[Ramsay Jones]

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 <ramsay@ramsayjones.plus.com>
---
 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):' <error | grep -v 'hung up' >actual | sort &&
+	grep -E '^(fatal|warning):' <error | grep -v 'hung up' >actual &&
 	test_i18ncmp expected actual
 }
 
-- 
2.16.0

Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> 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 <ramsay@ramsayjones.plus.com>
> ---
>  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):' <error | grep -v 'hung up' >actual | sort &&
> +	grep -E '^(fatal|warning):' <error | grep -v 'hung up' >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.

Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep

[Junio C Hamano]

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

>> -	test_i18ngrep -E '^(fatal|warning):' <error | grep -v 'hung up' >actual | sort &&
>> +	grep -E '^(fatal|warning):' <error | grep -v 'hung up' >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]

On 12/02/18 20:18, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> 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 <ramsay@ramsayjones.plus.com>
>> ---
>>  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):' <error | grep -v 'hung up' >actual | sort &&
>> +	grep -E '^(fatal|warning):' <error | grep -v 'hung up' >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]

Ramsay Jones <ramsay@ramsayjones.plus.com> 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

[SZEDER Gábor]

> > 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):' <error | grep -v 'hung up' >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

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> 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):' <error | grep -v 'hung up' >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

[Jeff King]

On Tue, Feb 13, 2018 at 09:08:44AM -0800, Junio C Hamano wrote:

> SZEDER Gábor <szeder.dev@gmail.com> 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

[Junio C Hamano]

Jeff King <peff@peff.net> 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

[Junio C Hamano]

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

> Jeff King <peff@peff.net> 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 <szeder.dev@gmail.com>
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 <gitster@pobox.com>
---
 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):' <error | grep -v 'hung up' >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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> 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-3a4e61ea13f4@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

[Ramsay Jones]

On 27/02/18 22:05, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> 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-3a4e61ea13f4@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

[SZEDER Gábor]

On Wed, Feb 28, 2018 at 12:47 AM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
>
> On 27/02/18 22:05, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> 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-3a4e61ea13f4@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 <szeder.dev@gmail.com>

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

[Ramsay Jones]

On 28/02/18 00:42, SZEDER Gábor wrote:
> On Wed, Feb 28, 2018 at 12:47 AM, Ramsay Jones
> <ramsay@ramsayjones.plus.com> wrote:
>>
>>
>> On 27/02/18 22:05, Junio C Hamano wrote:
>>> Junio C Hamano <gitster@pobox.com> 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-3a4e61ea13f4@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 <szeder.dev@gmail.com>

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

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> 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.