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

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

[Brian Gernhardt]

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
-- 
1.6.1.2.418.gd79e6.dirty

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

[Johannes Schindelin]

Hi,

On Thu, 29 Jan 2009, Brian Gernhardt wrote:

> 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

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

[Junio C Hamano]

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}
 	echo "${3-$1}" > "$file" &&
 	git add "$file" &&
 	test_tick &&

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

[Johannes Schindelin]

Hi,

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

> 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

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

[Brian Gernhardt]

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)

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

[Brian Gernhardt]

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:
>
>> 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)

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

[Johannes Schindelin]

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

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

[Brian Gernhardt]

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.

> 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

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

[Johannes Schindelin]

Hi,

On Tue, 3 Feb 2009, Brian Gernhardt wrote:

> 
> 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.

> >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

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

[Brian Gernhardt]

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

> 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:

> 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 &&

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

[Johannes Schindelin]

Hi,

On Tue, 3 Feb 2009, Brian Gernhardt wrote:

> 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

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

[Brian Gernhardt]

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

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

[Junio C Hamano]

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?

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 ;-).

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

[Brian Gernhardt]

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.

~~ Brian

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

[Johannes Schindelin]

Hi,

On Tue, 3 Feb 2009, Brian Gernhardt wrote:

> 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

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

[Johannes Schindelin]

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

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> 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.