Re: [PATCH] apply: support case-only renames in case-insensitive filesystems

[Tao Klerks <tao@klerks.biz>]

On Mon, Jun 13, 2022 at 1:30 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Tao Klerks via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +if ! test_have_prereq CASE_INSENSITIVE_FS
> > +then
> > +     test_set_prereq CASE_SENSITIVE_FS
> > +     echo nuts
> > +fi
>
> You can easily say !CASE_INSENSITIVE_FS as the prerequiste, so I do
> not see the point of this.  I do not see the point of "nuts", either.

I was not aware of negated prerequisite support (I did not see it in
the README nor in any examples I scanned), but I agree this is much
cleaner of course!

"nuts" was a debugging leak, my apologies.

>
> But it probably is a moot point as I do not think you should do the
> prerequisite at all.
>
> Instead, you can explicitly set the core.ignorecase configuration,
> i.e. "git -c core.ignorecase=yes/no", and possibly "apply --cached"
> so that you do not have to worry about the case sensitivity of the
> filesystem at all.

Sure, I can see how we can test most of the case-sensitive logic, even
on a case-insensitive filesystem, with "--cached" and "-c
core.ignorecase=no". I'm not sure whether there's a need to test the
same things against the actual file system or not (certainly in the
case-insensitive path there is, as this is where the errors/conflicts
actually occur).

>
> > +test_expect_success setup '
> > +     echo "This is some content in the file." > file1 &&
>
> Style.  Redirection operator ">" sticks to its operand, i.e.
>
>         echo "This is some content in the file." >file1 &&
>

Thx.

> > +     echo "A completely different file." > file2 &&
> > +     git update-index --add file1 &&
> > +     git update-index --add file2 &&
> > +     cat >case_only_rename_patch <<-\EOF
> > +     diff --git a/file1 b/File1
> > +     similarity index 100%
> > +     rename from file1
> > +     rename to File1
> > +     EOF
>
> You are better off not writing the diff output manually.  Instead,
> you can let the test write it for you, e.g.
>
>         echo "This is some content in the file." >file1 &&
>         git update-index --add file1 &&
>         file1blob=$(git rev-parse :file1) &&
>         git commit -m "Initial - file1" &&
>         git update-index --add --cacheinfo 100644,$file1blob,File1 &&
>         git rm --cached file1 &&
>         git diff --cached -M HEAD >case-only-rename-patch
>

Makes sense, thx.

> If you want to be extra careful not to rely on your filesystem
> corrupting the pathnames you feed (e.g. the redireciton to "file1"
> might create file FILE1 on MS-DOS ;-), you could even do:
>
>         file1blob=$(echo "This is some content in the file." |
>                     git hash-object -w --stdin) &&
>         file2blob=$(echo "A completeloy different contents." |
>                     git hash-object -w --stdin) &&
>         git update-index --add --cacheinfo 100644,$file1blob,file1 &&
>
>         git commit -m "Initial - file1" &&
>         git update-index --add --cacheinfo 100644,$file1blob,File1 &&
>         git rm --cached file1 &&
>         git diff --cached -M HEAD >rename-file1-to-File2 &&
>
>         git reset --hard HEAD &&
>         git update-index --add --cacheinfo 100644,$file1blob,file2 &&
>         git rm --cached file1 &&
>         git diff --cached -M HEAD >rename-file1-to-file2 &&
>
>         # from here on, HEAD has file1 and file2
>         git reset --hard HEAD &&
>         git update-index --add --cacheinfo 100644,$file2blob,file2 &&
>         git commit -m 'file1 and file2'
>

Cool, but probably excessive? (do we support MS-DOS??)

> > +'
> > +
> > +test_expect_success 'refuse to apply rename patch with conflict' '
> > +     cat >conflict_patch <<-\EOF &&
> > +     diff --git a/file1 b/file2
> > +     similarity index 100%
> > +     rename from file1
> > +     rename to file2
> > +     EOF
> > +     test_must_fail git apply --index conflict_patch
>
> And then, you could use --cached (not --index) to bypass the working
> tree altogether, which is a good way to test the feature without
> getting affected by the underlying filesystem.  Check both case
> sensitive and case insensitive cases:
>
>         # Start from a known state
>         git reset --hard HEAD &&
>         test_must_fail git -c core.ignorecase=no apply --cached rename-file1-to-file2 &&
>
>         # Start from a known state
>         git reset --hard HEAD &&
>         test_must_fail git -c core.ignorecase=yes apply --cached rename-file1-to-file2 &&
>

Makes sense, understanding that this tests "happy paths" - it doesn't
fail even if talking to the (case-insensitive) filesystem actually
would (which here it wouldn't of course).

> > +'
> > +
> > +test_expect_success CASE_SENSITIVE_FS 'refuse to apply case-only rename patch with conflict, in case-sensitive FS' '
>
> Lose the prerequisite, replace --index with --cached, and force core.ignorecase
> to both case insensitive and sensitive to check the behaviour.
>

Sure, makes sense - you can test case-sensitive behaviors in git
without needing a case-sensitive FS.

> > +     test_when_finished "git mv File1 file2" &&
> > +     git mv file2 File1 &&
> > +     test_must_fail git apply --index case_only_rename_patch
> > +'
> > +
> > +test_expect_success 'apply case-only rename patch without conflict' '
>
> Likewise, try both sensitive and insensitive one.
>

This one will fail on a case-insensitive filesystem if you disable
core.ignorecase, so explicitly trying with both settings in a single
test, without prerequisites, presumably isn't the right thing. I
assume the right thing is to have 2 versions of the same test, one
which expects success in all cases on a case-sensitive filesystem, and
one which expects failure when case-insensitivity is disabled on a
case-insensitive filesystem?

> > +     git apply --index case_only_rename_patch
> > +'
> > +
> > +test_done
> >
> > base-commit: 1e59178e3f65880188caedb965e70db5ceeb2d64
>
> Thanks.
>

Thank you!