Re: [PATCH] t3411: Fix test 1 for case-insensitive file systems

Brian Gernhardt [off-list ref] writes:

It is not just B1 that is ambiguous, even though that is the only
ambiguous one this particular test uses.

If we really wanted to care about case-folding file systems, shouldn't we
make test_commit shell function a bit more than the downcasing?  How about
this patch instead?

 t/test-lib.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git c/t/test-lib.sh w/t/test-lib.sh
index c1839f7..8066c25 100644
--- c/t/test-lib.sh
+++ w/t/test-lib.sh

@@ -201,7 +201,7 @@ test_tick () {
 # Both <file> and <contents> default to <message>.
 
 test_commit () {
-	file=${2:-$(echo "$1" | tr 'A-Z' 'a-z')}
+	file=${2:-$(echo "$1" | tr 'A-Z' 'a-z').t}
 	echo "${3-$1}" > "$file" &&
 	git add "$file" &&
 	test_tick &&

Hi,

On Thu, 29 Jan 2009, Junio C Hamano wrote:

Or

	file=${2:-$1.file}

but I was not quite sure about the impact; I might have checked for 
specific filenames.

Brian, if you would have the time to go through the tests if an automatic 
filename was used, that would be smashing!  (We cannot just run the tests 
and look at the failures, as absence of files could be tested; I don't 
remember, debugging zlib/valgrind currently turns my brain into tatties.)

Ciao,
Dscho

On Jan 29, 2009, at 12:19 PM, Junio C Hamano wrote:

If we really wanted to care about case-folding file systems,  
shouldn't we
make test_commit shell function a bit more than the downcasing?  How  
about
this patch instead?

That's a good point.  Always good to prevent future issues.

test_commit () {
-	file=${2:-$(echo "$1" | tr 'A-Z' 'a-z')}
+	file=${2:-$(echo "$1" | tr 'A-Z' 'a-z').t}
	echo "${3-$1}" > "$file" &&
	git add "$file" &&
	test_tick &&

Added this and ran through the tests.  Works for me. :-D

Tested-by: Brian Gernhardt <redacted> (HFS+ on Mac OS  
10.5.6)

This change appears to have been forgotten, but does fix the problems  
I was having.  Junio, can this make it into the official repo instead  
of floating around in my local?  I'd send in a patch, but it was your  
code and I don't want to take credit for it.

~~ Brian

On Jan 29, 2009, at 2:10 PM, Brian Gernhardt wrote:

On Jan 29, 2009, at 12:19 PM, Junio C Hamano wrote:

quoted

If we really wanted to care about case-folding file systems,  
shouldn't we
make test_commit shell function a bit more than the downcasing?   
How about
this patch instead?

That's a good point.  Always good to prevent future issues.

quoted

test_commit () {
-	file=${2:-$(echo "$1" | tr 'A-Z' 'a-z')}
+	file=${2:-$(echo "$1" | tr 'A-Z' 'a-z').t}
	echo "${3-$1}" > "$file" &&
	git add "$file" &&
	test_tick &&

Added this and ran through the tests.  Works for me. :-D

Tested-by: Brian Gernhardt <redacted> (HFS+ on Mac  
OS 10.5.6)

Hi,

On Tue, 3 Feb 2009, Brian Gernhardt wrote:

This change appears to have been forgotten, but does fix the problems I 
was having.  Junio, can this make it into the official repo instead of 
floating around in my local?  I'd send in a patch, but it was your code 
and I don't want to take credit for it.

Top-poster!


Besides, I think that my latest comment still stands there: testing is not 
good enough, code inspection is required if something expects the file 
names as they used to be.

Ciao,
Dscho

On Feb 3, 2009, at 11:53 AM, Johannes Schindelin wrote:

On Tue, 3 Feb 2009, Brian Gernhardt wrote:

quoted

This change appears to have been forgotten, but does fix the  
problems I
was having.  Junio, can this make it into the official repo instead  
of
floating around in my local?  I'd send in a patch, but it was your  
code
and I don't want to take credit for it.

Top-poster!

Well, yes.  I wasn't replying to anything in the e-mail, I just wanted  
to bring it back to attention.

Besides, I think that my latest comment still stands there: testing  
is not
good enough, code inspection is required if something expects the file
names as they used to be.

As far as I can tell, no test relies on the auto-generated name of the  
test file.  In fact, only t3411 uses that feature at all and it only  
performs operations on commits.  All other uses of test_commit give a  
filename (even though many of them don't appear to use the file).

~~ Brian

Hi,

On Tue, 3 Feb 2009, Brian Gernhardt wrote:

On Feb 3, 2009, at 11:53 AM, Johannes Schindelin wrote:

quoted

On Tue, 3 Feb 2009, Brian Gernhardt wrote:

quoted

This change appears to have been forgotten, but does fix the problems 
I was having.  Junio, can this make it into the official repo instead 
of floating around in my local?  I'd send in a patch, but it was your 
code and I don't want to take credit for it.

Top-poster!

Well, yes.  I wasn't replying to anything in the e-mail, I just wanted to
bring it back to attention.

You did, in fact, refer a little bit to the content of the thread you were 
replying to.

quoted

Besides, I think that my latest comment still stands there: testing is not
good enough, code inspection is required if something expects the file
names as they used to be.

As far as I can tell, no test relies on the auto-generated name of the test
file.  In fact, only t3411 uses that feature at all and it only performs
operations on commits.  All other uses of test_commit give a filename (even
though many of them don't appear to use the file).

You did not look far.

Ciao,
Dscho

On Feb 3, 2009, at 12:18 PM, Johannes Schindelin wrote:

On Tue, 3 Feb 2009, Brian Gernhardt wrote:

quoted

On Feb 3, 2009, at 11:53 AM, Johannes Schindelin wrote:

quoted

Besides, I think that my latest comment still stands there:  
testing is not
good enough, code inspection is required if something expects the  
file
names as they used to be.

As far as I can tell, no test relies on the auto-generated name of  
the test
file.  In fact, only t3411 uses that feature at all and it only  
performs
operations on commits.  All other uses of test_commit give a  
filename (even
though many of them don't appear to use the file).

You did not look far.

If there's something I missed, could you perhaps say what it is  
instead of being cryptic?  I have a day job and am attempting to  
squeeze in searching for this between tasks.  This fix (or similar)  
needs to make it into master so that the tests can run on case- 
insensitive file systems.

I used git grep to find uses of test_commit in next, and the only uses  
that did not provide a file name argument were in t3411.  I read  
through every test in the file, and the only operations I saw were  
test_commit, test_merge, checkout -b, reset without filenames, rebase,  
and rev-parse.  All operations on commits, not files.

The tests all run properly, and I've run t3411 individually (since  
it's apparently the only one using this feature) using -v to ensure  
that it was actually performing work and it seems to be.

So, based on the above, every usage of test_commit either provides a  
file name or does not care about the names of the files.  Unless you  
have something that says it isn't, I'd like to see the fix Junio gave  
applied (repeated here as a reminder):

On Jan 29, 2009, at 12:19 PM, Junio C Hamano wrote:

Hi,

On Tue, 3 Feb 2009, Brian Gernhardt wrote:

On Feb 3, 2009, at 12:18 PM, Johannes Schindelin wrote:

quoted

You did not look far.

If there's something I missed, could you perhaps say what it is instead 
of being cryptic?

$ git grep -l test_commit junio/next -- t/
junio/next:t/README
junio/next:t/t1450-fsck.sh
junio/next:t/t3410-rebase-preserve-dropped-merges.sh
junio/next:t/t3411-rebase-preserve-around-merges.sh
junio/next:t/t3412-rebase-root.sh
junio/next:t/test-lib.sh

But I understood.  I will audit the code myself later this evening.

Ciao,
Dscho

On Feb 3, 2009, at 1:19 PM, Johannes Schindelin wrote:

$ git grep -l test_commit junio/next -- t/
junio/next:t/README
junio/next:t/t1450-fsck.sh
junio/next:t/t3410-rebase-preserve-dropped-merges.sh
junio/next:t/t3411-rebase-preserve-around-merges.sh
junio/next:t/t3412-rebase-root.sh
junio/next:t/test-lib.sh

I examined all of those files.  I looked at every line that calls  
test_commit.  All of those files except for t34110-rebase-preserve- 
around-merges.sh provide a filename of their own and are therefore  
unaffected by the change

On Jan 29, 2009, at 12:19 PM, Junio C Hamano wrote:

-	file=${2:-$(echo "$1" | tr 'A-Z' 'a-z')}
+	file=${2:-$(echo "$1" | tr 'A-Z' 'a-z').t}

Use "git grep" without the -l and you will see that all uses outside  
of t3411 are of the form "test_commit <commit> <filename>".  Only  
t3411 uses the form "test_commit <commit>" (without filename), which  
is when this change would matter.  And t3411 does not use the  
generated filename, only the resultant commits.

I was irritated by your statement that I "did not look far" because _I  
read all those files_.  I gave you the result of reading them which  
you seem to have completely ignored, as it was the part of my last e- 
mail that you did not quote.

If you want to double-check me, fine.  But do not claim I did not do  
the work.

~~ Brian