threads / patch / 17439

patcht3411: Fix test 1 for case-insensitive file systems

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

## tl;dr

17 messages between Jan 29, 2009 and Feb 4, 2009. Diffs are folded; open one to read it.

replies: 16people: 3as markdown or json

Brian Gernhardt· Jan 29, 2009, 16:00 UTC · lore

The call to "git reset --hard B1" failed on case-insensitive file systems (such as the default settings for HFS+) because there was both a tag "B1" and a file "b1". Adding "--" to the command makes it clear that we mean commit B1.

Signed-off-by: Brian Gernhardt <benji@silverinsanity.com>
---
 t/t3411-rebase-preserve-around-merges.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to t/t3411-rebase-preserve-around-merges.sh +1 −1
diff --git a/t/t3411-rebase-preserve-around-merges.sh b/t/t3411-rebase-preserve-around-merges.sh
index 6533505..e544451 100755
--- a/t/t3411-rebase-preserve-around-merges.sh
+++ b/t/t3411-rebase-preserve-around-merges.sh
@@ -24,7 +24,7 @@ test_expect_success 'setup' '
 	test_commit A1 &&
 	test_commit B1 &&
 	test_commit C1 &&
-	git reset --hard B1 &&
+	git reset --hard B1 -- &&
 	test_commit D1 &&
 	test_merge E1 C1 &&
 	test_commit F1
-- 
1.6.1.2.418.gd79e6.dirty
Johannes Schindelin· Jan 29, 2009, 16:13 UTC · re: Brian Gernhardt · lore

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

Hi,
On Thu, 29 Jan 2009, Brian Gernhardt wrote:
Show 6 quoted lines
> The call to "git reset --hard B1" failed on case-insensitive file
> systems (such as the default settings for HFS+) because there was both
> a tag "B1" and a file "b1".  Adding "--" to the command makes it
> clear that we mean commit B1.
> 
> Signed-off-by: Brian Gernhardt <benji@silverinsanity.com>
Sigh.

ACK and thanks, Dscho

Junio C Hamano· Jan 29, 2009, 17:19 UTC · re: Brian Gernhardt · lore

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

Brian Gernhardt <benji@silverinsanity.com> writes:
Show 23 quoted lines
> The call to "git reset --hard B1" failed on case-insensitive file
> systems (such as the default settings for HFS+) because there was both
> a tag "B1" and a file "b1".  Adding "--" to the command makes it
> clear that we mean commit B1.
>
> Signed-off-by: Brian Gernhardt <benji@silverinsanity.com>
> ---
>  t/t3411-rebase-preserve-around-merges.sh |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/t/t3411-rebase-preserve-around-merges.sh b/t/t3411-rebase-preserve-around-merges.sh
> index 6533505..e544451 100755
> --- a/t/t3411-rebase-preserve-around-merges.sh
> +++ b/t/t3411-rebase-preserve-around-merges.sh
> @@ -24,7 +24,7 @@ test_expect_success 'setup' '
>  	test_commit A1 &&
>  	test_commit B1 &&
>  	test_commit C1 &&
> -	git reset --hard B1 &&
> +	git reset --hard B1 -- &&
>  	test_commit D1 &&
>  	test_merge E1 C1 &&
>  	test_commit F1

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(-)
Show changes to diff +1 −1
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 &&
Johannes Schindelin· Jan 29, 2009, 17:33 UTC · re: Junio C Hamano · lore

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

Hi,
On Thu, 29 Jan 2009, Junio C Hamano wrote:
Show 46 quoted lines
> Brian Gernhardt <benji@silverinsanity.com> writes:
> 
> > The call to "git reset --hard B1" failed on case-insensitive file
> > systems (such as the default settings for HFS+) because there was both
> > a tag "B1" and a file "b1".  Adding "--" to the command makes it
> > clear that we mean commit B1.
> >
> > Signed-off-by: Brian Gernhardt <benji@silverinsanity.com>
> > ---
> >  t/t3411-rebase-preserve-around-merges.sh |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/t/t3411-rebase-preserve-around-merges.sh b/t/t3411-rebase-preserve-around-merges.sh
> > index 6533505..e544451 100755
> > --- a/t/t3411-rebase-preserve-around-merges.sh
> > +++ b/t/t3411-rebase-preserve-around-merges.sh
> > @@ -24,7 +24,7 @@ test_expect_success 'setup' '
> >  	test_commit A1 &&
> >  	test_commit B1 &&
> >  	test_commit C1 &&
> > -	git reset --hard B1 &&
> > +	git reset --hard B1 -- &&
> >  	test_commit D1 &&
> >  	test_merge E1 C1 &&
> >  	test_commit F1
> 
> 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}
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

Brian Gernhardt· Jan 29, 2009, 19:10 UTC · re: Junio C Hamano · lore

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

On Jan 29, 2009, at 12:19 PM, Junio C Hamano wrote:
Show 5 quoted lines
> 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.
Show 6 quoted lines
> 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 <benji@silverinsanity.com> (HFS+ on Mac OS  
10.5.6)
Brian Gernhardt· Feb 3, 2009, 15:07 UTC · re: Brian Gernhardt · lore

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

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:
Show 22 quoted lines
>
> 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 <benji@silverinsanity.com> (HFS+ on Mac  
> OS 10.5.6)
Johannes Schindelin· Feb 3, 2009, 16:53 UTC · re: Brian Gernhardt · lore

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

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

Brian Gernhardt· Feb 3, 2009, 17:11 UTC · re: Johannes Schindelin · lore

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

On Feb 3, 2009, at 11:53 AM, Johannes Schindelin wrote:
Show 11 quoted lines
> 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!

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
Johannes Schindelin· Feb 3, 2009, 17:18 UTC · re: Brian Gernhardt · lore

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

Hi,
On Tue, 3 Feb 2009, Brian Gernhardt wrote:
Show 14 quoted lines
> 
> On Feb 3, 2009, at 11:53 AM, Johannes Schindelin wrote:
> 
> >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!
> 
> 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.

Show 8 quoted lines
> >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

Brian Gernhardt· Feb 3, 2009, 17:43 UTC · re: Johannes Schindelin · lore

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

On Feb 3, 2009, at 12:18 PM, Johannes Schindelin wrote:
Show 19 quoted lines
> On Tue, 3 Feb 2009, Brian Gernhardt wrote:
>
>> On Feb 3, 2009, at 11:53 AM, Johannes Schindelin wrote:
>>
>>> 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:
Show 13 quoted lines
> 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 &&
Johannes Schindelin· Feb 3, 2009, 18:19 UTC · re: Brian Gernhardt · lore

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

Hi,
On Tue, 3 Feb 2009, Brian Gernhardt wrote:
Show 6 quoted lines
> On Feb 3, 2009, at 12:18 PM, Johannes Schindelin wrote:
> 
> >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

Brian Gernhardt· Feb 3, 2009, 18:42 UTC · re: Johannes Schindelin · lore

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

On Feb 3, 2009, at 1:19 PM, Johannes Schindelin wrote:
Show 7 quoted lines
> $ 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
Junio C Hamano· Feb 3, 2009, 19:02 UTC · re: Brian Gernhardt · lore

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

Brian Gernhardt <benji@silverinsanity.com> writes:
Show 7 quoted lines
> 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.
Thanks, both, and let's not be unnecessarily un-nice to each other.

I agree that Dscho's "far enough" comment was not the best way to say "I really want to make sure you did verify the way I would myself", but please just let it pass, pretend that he just expressed frustration on his lack of time to do the verification himself with that comment. Ok?

The thing is, nobody active in git land is paid to work on git. You don't, Dscho doesn't and I neither.

I'll try to find time myself to triple check if you two want to, even though it is known that I often get tripped by the same kind of brain-slippage as Dscho when Dscho does get tripped, so my auditing might not add much to the collective confidence level ;-).

Brian Gernhardt· Feb 3, 2009, 19:12 UTC · re: Junio C Hamano · lore

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

On Feb 3, 2009, at 2:02 PM, Junio C Hamano wrote:
Show 19 quoted lines
> Brian Gernhardt <benji@silverinsanity.com> writes:
>
>> 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.
>
> Thanks, both, and let's not be unnecessarily un-nice to each other.
>
> I agree that Dscho's "far enough" comment was not the best way to  
> say "I
> really want to make sure you did verify the way I would myself", but
> please just let it pass, pretend that he just expressed frustration  
> on his
> lack of time to do the verification himself with that comment.  Ok?

Fair enough. My own word choice wasn't exactly wise, either. This has been an especially frustrating day for me in many other respects and it's affected my commentary here.

Dscho, my apologies.
~~ Brian
Johannes Schindelin· Feb 3, 2009, 20:16 UTC · re: Brian Gernhardt · lore

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

Hi,
On Tue, 3 Feb 2009, Brian Gernhardt wrote:
Show 24 quoted lines
> On Feb 3, 2009, at 2:02 PM, Junio C Hamano wrote:
> 
> >Brian Gernhardt <benji@silverinsanity.com> writes:
> >
> > >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.
> >
> >Thanks, both, and let's not be unnecessarily un-nice to each other.
> >
> >I agree that Dscho's "far enough" comment was not the best way to say "I
> >really want to make sure you did verify the way I would myself", but
> >please just let it pass, pretend that he just expressed frustration on his
> >lack of time to do the verification himself with that comment.  Ok?
> 
> Fair enough.  My own word choice wasn't exactly wise, either.  This has been
> an especially frustrating day for me in many other respects and it's affected
> my commentary here.
> 
> Dscho, my apologies.
Accepted, and mine to you.

Ciao, Dscho

Johannes Schindelin· Feb 3, 2009, 20:38 UTC · re: Junio C Hamano · lore

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

Hi,
On Tue, 3 Feb 2009, Junio C Hamano wrote:
> I'll try to find time myself to triple check if you two want to, even 
> though it is known that I often get tripped by the same kind of 
> brain-slippage as Dscho when Dscho does get tripped, so my auditing 
> might not add much to the collective confidence level ;-).
Thanks ;-)

I inspected all files that git grep -p showed me, and I see that t3412 uses the file names, but passes explicit parameters to test_commit for them. So that is fine, too.

Just as a matter of preference, I'd not downcase, but instead append a ".file" in test_commit, but that's not a strong preference. (I'd not downcase, because that was done only to prevent the tag to have the same name as the file, but that did not work...)

Ciao, Dscho

Junio C Hamano· Feb 4, 2009, 02:09 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 8 quoted lines
> I inspected all files that git grep -p showed me, and I see that t3412 
> uses the file names, but passes explicit parameters to test_commit for 
> them.  So that is fine, too.
>
> Just as a matter of preference, I'd not downcase, but instead append a 
> ".file" in test_commit, but that's not a strong preference.  (I'd not 
> downcase, because that was done only to prevent the tag to have the same 
> name as the file, but that did not work...)
Agreed, there is no point to downcase the name.

← back to recent threads