Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.
- From
- Adam Butcher <dev.lists@jessamine.co.uk>
- Date
- Aug 2, 2012, 22:58 UTC
- 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:
Show 18 quoted lines
> 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.
Show 8 quoted lines
>> +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.
Show 5 quoted lines
> 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.
Show 11 quoted lines
> 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.
Show 14 quoted lines
>> +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