Re: [PATCH] t: add clone test for files differing only in case

[brian m. carlson]

On Sun, Jan 21, 2018 at 03:07:28AM -0500, Eric Sunshine wrote:
> On Sun, Jan 21, 2018 at 02:46:15AM -0500, Eric Sunshine wrote:
> > Yep. In pretty much any other test script, this would work (it was
> > developed in a stand-alone script), but t5601 (which nukes .git as its
> > first action) isn't the most friendly place.
> 
> Here's a re-roll which fixes that problem (and has a slightly
> re-written commit message.

Yes, this looks good.  I did indeed intend to forge your From line on
the message and actually test that it was fixed, but only tested it on
the vfat partition, not my ext4 partition, where, of course, I got
distracted by the various other failures.  Sigh.

Thanks for fixing this up.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH] t: add clone test for files differing only in case

[Eric Sunshine]

On Sun, Jan 21, 2018 at 6:50 AM, Duy Nguyen  wrote:
> On Sun, Jan 21, 2018 at 3:33 AM, brian m. carlson
>> +test_expect_success 'clone on case-insensitive fs' '
>
> We have CASE_INSENSITIVE_FS prereq. Should we use it here? I know it
> does not harm running this test on case-sensitive filesystem, but the
> prereq could be useful for grepping.

I'd rather not hide it behind the CASE_INSENSITIVE_FS[1] prerequisite
since the test potentially could catch some sort of future regression
even on case-sensitive filesystems.

[1]: Todd Zullinger suggested the same:
https://public-inbox.org/git/CAPig+cSRN1zHc=zso1y_aq_eo+sbsd0cq5iz9hyz3ruk_e-...@mail.gmail.com/

Re: [PATCH] t: add clone test for files differing only in case

[Duy Nguyen]

On Sun, Jan 21, 2018 at 3:33 AM, brian m. carlson
 wrote:
> We recently introduced a regression in cloning repositories onto
> case-insensitive file systems where the repository contains multiple
> files differing only in case.  In such a case, we would segfault.  This
> segfault has already been fixed (repository: pre-initialize hash algo
> pointer), but it's not the first time similar problems have arisen.
> Introduce a test to catch this case and ensure the behavior does not
> regress.
>
> Signed-off-by: Eric Sunshine 
> Signed-off-by: brian m. carlson 
> ---
>  t/t5601-clone.sh | 13 +
>  1 file changed, 13 insertions(+)
>
> I've verified that the test does fail without the patch on a vfat file
> system.  However, many other tests also fail on a vfat file system on
> Linux, so unfortunately that doesn't look like a viable testing strategy
> going forward.

vfat is not very UNIXy. I suspect we need a bunch of tweaks that
cygwin/mingw uses to make the test suite pass. There are other
case-insensitive filesystems on linux though. JFS (with "mkfs.jfs -O"
to make it case-insensitive) seems to pass the test suite and crash
git without your fix so it's a good candidate if someone wants to test
this behavior on linux in future. HFS+ fails some unicode test in
t0050 and I didn't check further.

> +test_expect_success 'clone on case-insensitive fs' '

We have CASE_INSENSITIVE_FS prereq. Should we use it here? I know it
does not harm running this test on case-sensitive filesystem, but the
prereq could be useful for grepping.

> +   o=$(git hash-object -w --stdin  +   t=$(printf "100644 X\0${o}100644 x\0${o}" |
> +   git hash-object -w -t tree --stdin) &&
> +   c=$(git commit-tree -m bogus $t) &&
> +   git update-ref refs/heads/bogus $c &&
> +   git clone -b bogus . bogus
> +'
> +
>  test_done
-- 
Duy

Re: [PATCH] t: add clone test for files differing only in case

[Eric Sunshine]

On Sun, Jan 21, 2018 at 02:46:15AM -0500, Eric Sunshine wrote:
> On Sun, Jan 21, 2018 at 2:33 AM, Junio C Hamano  wrote:
> > "brian m. carlson"  writes:
> >> +test_expect_success 'clone on case-insensitive fs' '
> >> + o=$(git hash-object -w --stdin  >> + t=$(printf "100644 X\0${o}100644 x\0${o}" |
> >> + git hash-object -w -t tree --stdin) &&
> >> + c=$(git commit-tree -m bogus $t) &&
> >> + git update-ref refs/heads/bogus $c &&
> >> + git clone -b bogus . bogus
> >> +'
> >
> > Hmm, I seem to be seeing a failure from this thing:
> > fatal: repository '.' does not exist
> > even on a case sensitive platform.
> 
> Yep. In pretty much any other test script, this would work (it was
> developed in a stand-alone script), but t5601 (which nukes .git as its
> first action) isn't the most friendly place.

Here's a re-roll which fixes that problem (and has a slightly
re-written commit message.

--- >8 ---
From: Eric Sunshine 
Subject: [PATCH] t5601-clone: test case-conflicting files on case-insensitive
 filesystem

A recently introduced regression caused a segfault at clone time on
case-insensitive filesystems when filenames differing only in case are
present. This bug has already been fixed (repository: pre-initialize
hash algo pointer, 2018-01-18), but it's not the first time similar
problems have arisen. Therefore, introduce a test to catch this case and
protect against future regressions.

Signed-off-by: Eric Sunshine 
Signed-off-by: brian m. carlson 
Signed-off-by: Eric Sunshine 
---
 t/t5601-clone.sh | 17 +
 1 file changed, 17 insertions(+)

diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 0f895478f0..2d1490f631 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -611,4 +611,21 @@ test_expect_success 'GIT_TRACE_PACKFILE produces a usable 
pack' '
git -C replay.git index-pack -v --stdin 

Re: [PATCH] t: add clone test for files differing only in case

[Eric Sunshine]

On Sun, Jan 21, 2018 at 2:33 AM, Junio C Hamano  wrote:
> "brian m. carlson"  writes:
>> +test_expect_success 'clone on case-insensitive fs' '
>> + o=$(git hash-object -w --stdin > + t=$(printf "100644 X\0${o}100644 x\0${o}" |
>> + git hash-object -w -t tree --stdin) &&
>> + c=$(git commit-tree -m bogus $t) &&
>> + git update-ref refs/heads/bogus $c &&
>> + git clone -b bogus . bogus
>> +'
>> +
>>  test_done
>
> Hmm, I seem to be seeing a failure from this thing:
>
> expecting success:
> o=$(git hash-object -w --stdin  t=$(printf "100644 X\0${o}100644 x\0${o}" |
> git hash-object -w -t tree --stdin) &&
> c=$(git commit-tree -m bogus $t) &&
> git update-ref refs/heads/bogus $c &&
> git clone -b bogus . bogus
>
> fatal: repository '.' does not exist
>
> even on a case sensitive platform.

Yep. In pretty much any other test script, this would work (it was
developed in a stand-alone script), but t5601 (which nukes .git as its
first action) isn't the most friendly place.

Re: [PATCH] t: add clone test for files differing only in case

[Junio C Hamano]

"brian m. carlson"  writes:

> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index 0f895478f0..53b2dda9d2 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -611,4 +611,17 @@ test_expect_success 'GIT_TRACE_PACKFILE produces a 
> usable pack' '
>   git -C replay.git index-pack -v --stdin   '
>  
> +hex2oct() {
> + perl -ne 'printf "\\%03o", hex for /../g'
> +}
> +
> +test_expect_success 'clone on case-insensitive fs' '
> + o=$(git hash-object -w --stdin  + t=$(printf "100644 X\0${o}100644 x\0${o}" |
> + git hash-object -w -t tree --stdin) &&
> + c=$(git commit-tree -m bogus $t) &&
> + git update-ref refs/heads/bogus $c &&
> + git clone -b bogus . bogus
> +'
> +
>  test_done

Hmm, I seem to be seeing a failure from this thing:

expecting success:
o=$(git hash-object -w --stdin 

Re: [PATCH] t: add clone test for files differing only in case

[Eric Sunshine]

On Sat, Jan 20, 2018 at 3:33 PM, brian m. carlson
 wrote:
> We recently introduced a regression in cloning repositories onto
> case-insensitive file systems where the repository contains multiple
> files differing only in case.  In such a case, we would segfault.  This
> segfault has already been fixed (repository: pre-initialize hash algo
> pointer), but it's not the first time similar problems have arisen.
> Introduce a test to catch this case and ensure the behavior does not
> regress.

I guess you'd probably want a "From: Eric Sunshine "
header before this paragraph.

The patch itself looks correct... ;-)

> Signed-off-by: Eric Sunshine 
> Signed-off-by: brian m. carlson 
> ---
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index 0f895478f0..53b2dda9d2 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -611,4 +611,17 @@ test_expect_success 'GIT_TRACE_PACKFILE produces a 
> usable pack' '
> git -C replay.git index-pack -v --stdin   '
>
> +hex2oct() {
> +   perl -ne 'printf "\\%03o", hex for /../g'
> +}
> +
> +test_expect_success 'clone on case-insensitive fs' '
> +   o=$(git hash-object -w --stdin  +   t=$(printf "100644 X\0${o}100644 x\0${o}" |
> +   git hash-object -w -t tree --stdin) &&
> +   c=$(git commit-tree -m bogus $t) &&
> +   git update-ref refs/heads/bogus $c &&
> +   git clone -b bogus . bogus
> +'
> +
>  test_done

[PATCH] t: add clone test for files differing only in case

[brian m. carlson]

We recently introduced a regression in cloning repositories onto
case-insensitive file systems where the repository contains multiple
files differing only in case.  In such a case, we would segfault.  This
segfault has already been fixed (repository: pre-initialize hash algo
pointer), but it's not the first time similar problems have arisen.
Introduce a test to catch this case and ensure the behavior does not
regress.

Signed-off-by: Eric Sunshine 
Signed-off-by: brian m. carlson 
---
 t/t5601-clone.sh | 13 +
 1 file changed, 13 insertions(+)

I've verified that the test does fail without the patch on a vfat file
system.  However, many other tests also fail on a vfat file system on
Linux, so unfortunately that doesn't look like a viable testing strategy
going forward.

I didn't include an object ID for the commit referenced simply because I
didn't see one yet and I didn't want to insert a local one that wouldn't
work for anyone else.

diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 0f895478f0..53b2dda9d2 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -611,4 +611,17 @@ test_expect_success 'GIT_TRACE_PACKFILE produces a usable 
pack' '
git -C replay.git index-pack -v --stdin