On Tue, Dec 29, 2020 at 02:06:37AM +0000, Dan Moseley wrote:

First of all, thanks for submitting this to git.git.
I take the freedom to add some comments here.

Fix git mv to not assert when src is already in the index under a
different casing, core.caseInsensitive=true, and the file system
is case insensitive.

The config variable is named core.ignorecase

Does it make sense to illustrate the use case here, like this:

 git init
 echo foo >foo
 git add foo
 git mv foo FOO
 git mv foo bar

Since 9b906af657 the check that git mv does to ensure the src is in the
cache respects caseInsensitive. As a result git mv allows a move from a
file that has a different case in the index than it does on disk.
After the rename on disk, git mv fails to find the file in the cache
in order to rename it in the index, and asserts.
Assertion failed: pos >= 0, file builtin/mv.c, line 295

This is the simplest possible fix, suggested by @tboegi. It does leave
the file renamed on disk, but that is easy to reverse after the error.

We can expand the short-ish "@tboegi" into a "Helped-by" line, please see below.
And refrase the paragraf like this:

This is the simplest possible fix, it avoids to leaving a .git/index.lock
behind.  It does leave the file renamed on disk,
but that is easy to reverse after the error.

Another option would be to change the aforementioned check to always
be case sensitive, but I am not sure whether there is a scenario where
it is useful to be insensitive.

The intention of 9b906af657
Author: Chris Torek [off-list ref]
Date:   Mon Jul 20 06:17:52 2020 +0000
    git-mv: improve error message for conflicted file

was to give the user better and more helpful error messages.

Some background:
A case-insensitive file system does the same for
lstat("foo") or lstat("FOO"), but Git records only one case,
which is "FOO" after the `git mv foo FOO`

In that sense, replacing
cache_file_exists(src, length, ignore_case)
with
cache_file_exists(src, length, 0)
would be the correct solution (and an even simpler patch).

Doing so would give the error message
"not under version control"
when doing `git mv foo bar` after `git mv foo FOO`
I think, that this is technically correct, the user has just asked
to track "FOO", and "foo" is not in the repo any more.

A (may be) more helpful  message could be achieved by something like this
(white space dmaged) diff. Any thoughts, if this is really helpful ?

 diff --git a/builtin/mv.c b/builtin/mv.c
 index 7dac714af9..8572a5dae0 100644
 --- a/builtin/mv.c
 +++ b/builtin/mv.c
 @@ -221,8 +221,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
                                 }
                                 argc += last - first;
                         }
 -               } else if (!(ce = cache_file_exists(src, length, ignore_case))) {
 -                       bad = _("not under version control");
 +               } else if (!(ce = cache_file_exists(src, length, 0))) {
 +                       if (cache_file_exists(src, length, ignore_case))
 +                               bad = _("not under version control (upper/lower mixup)");
 +                       else
 +                               bad = _("not under version control");
                 } else if (ce_stage(ce)) {
 	                        bad = _("conflicted");
                 } else if (lstat(dst, &st) == 0 &&

Signed-off-by: Dan Moseley <redacted>

If you want, add a
Helped-by Torsten Bögershausen [off-list ref]

As discussed on Github: Is this the right code to test that the
code does not assert(). I dont know.

+
 test_expect_success "Michael Cassar's test case" '
        rm -fr .git papers partA &&
        git init &&
--
2.25.1