From: Adam Butcher <dev.lists@jessamine.co.uk>
Date: Thu, 02 Aug 2012 22:58:56 GMT
Subject: Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.
Message-ID: <551f7f77570c84017ae93988f9202854@imap.force9.net>
In-Reply-To: <7vehnpc5ti.fsf@alter.siamese.dyndns.org>

On 02.08.2012 23:00, Junio C Hamano wrote:
> Adam Butcher <dev.lists@jessamine.co.uk> writes:
>
>> +# create a file containing numbers with no newline at
>> +# the end and modify it such that the starting 10 lines
>> +# are unchanged, the next 101 are rewritten and the last
>> +# line differs only in that in is terminated by a newline.
>> +seq 1 10 > seq
>> +seq 100 +1 200 >> seq
>> +printf 201 >> seq
>> +(git add seq; git commit seq -m seq) >/dev/null
>> +seq 1 10 > seq
>> +seq 300 -1 200 >> seq
>
> We would prefer to have these set-up steps in test_expect_success.
> That way, we will have more chance to catch potential and unintended
> breakage to "git add" and "git commit" when people attempt to update
> them.
>
Cool, no probs.  I had originally put them at the start of the first 
test that I added but decided to pull them out as prep.  I think that 
msysGit or something about my Windows shell session may have played a 
part in my not chaining them with && also (see below).  I'll clean them 
up and wrap them in a test_expect_success.

> Also, the redirect target sticks to redirect operator in our
> scripts, i.e. "cmd >seq" not "cmd > seq".
>
Okay, will change.

>> +test_expect_success 'no newline at eof is on its own line without 
>> -B'
>> +
>> +	(git diff seq; true) > res &&
>
> What is this subshell and true about?  A git diff does not exit with
> non zero to signal differences,
>
Hmm, (?confused?) yeah actually I didn't think it did -- I was 
surprised when git returned 1 for this line.  I think it must have been 
an issue with the version msysGit I was using or something sticking 
errorlevel in my Windows shell.  Git seemed to return 1 ALWAYS!  I 
usually use gnu/linux but on this occasion I wrote the fix and tests 
blind on a Windows machine testing the logic manually with msysGit.  I 
ran the tests on a linux machine at work and they did what I expected so 
I left them as was without rechecking this.

I'm glad that this can be simplified.  It felt wrong -- similar lines 
elsewhere in the script didn't do it so I wasn't really happy with it.  
Turns out it looks to be a Windows/environment issue.  I cannot 
reproduce it now.

> and even if it did, the right way to
> write it would be
>
> 	test_might_fail git cmd >res &&
>
Fair enough.  Good to know that's available.

> to allow us to make sure that the git command that may or may not
> exit with zero still does not die an uncontrolled death (e.g. segv).
>
>> +	grep "^\\\\ No newline at end of file$" res &&
>> +	grep -v "^.\\+\\\\ No newline at end of file" res &&
>> +	grep -v "\\\\ No newline at end of file.\\+$" res
>> +'
>
> It is preferrable not to spell "No newline at ..." part out, so that
> we won't have to worry about future rewords and i18n.
>
Okay no probs.  I was originally going to spell it out only once and 
use parameter expansion.  However I understand the point of not spelling 
it out at all.  The only reason I did so was to catch other potential 
errors where text may have been 'attached' to either side of the 
annotation string by some future (or other) bug.

> There are older
> tests that predate i18n and they do spell these out, but that is not
> a good reason to make things worse than they already are.
>
Agreed.  Should I just test for this prefix case (i.e. the bug at hand) 
only and not preempt future potential issues?  Or should I just stick 
the current string in a variable and keep the logic as is; at least then 
there would only be one place requiring a fix in the reword case (but 
still additional rework in the i18n case -- though I assume the test 
could force a particular locale to evade this).

> "git apply" only looks at the backslash-space at the beginning of
> line anyway.
>
Okay.

>> +test_expect_success 'no newline at eof is on its own line with -B' 
>> '
>> +
>> +	(git diff -B seq; true) > res &&
>> +	grep "^\\\\ No newline at end of file$" res &&
>> +	grep -v "^.\\+\\\\ No newline at end of file" res &&
>> +	grep -v "\\\\ No newline at end of file.\\+$" res
>> +'
>
> Likewise.
>
>>  test_done
>
> Thanks.

No probs.  I will address both your and Jeff's comments sometime 
tomorrow and hopefully send a well formatted patch in next time.

Cheers,
Adam