On 02.08.2012 23:00, Junio C Hamano wrote:
Adam Butcher <dev.li...@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

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.


+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




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


To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to