[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH] git-mv: fix git mv bug with case insensitive fs
From:       Torsten =?iso-8859-1?Q?B=F6gershausen?= <tboegi () web ! de>
Date:       2020-12-31 7:13:58
Message-ID: 20201231071357.mtcxmoxbg6jrq3gn () tb-raspi4
[Download RAW message or body]

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 <chris.torek@gmail.com>
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 <danmose@microsoft.com>
If you want, add a
Helped-by Torsten Bögershausen <tboegi@web.de>

> ---
> Originally reported in https://github.com/git-for-windows/git/issues/2920
> but this is not specific to Windows.
>
>  builtin/mv.c  | 6 ++++--
>  t/t7001-mv.sh | 8 ++++++++
>  2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/mv.c b/builtin/mv.c index 7dac714af9..e1fd8a5e00 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -292,8 +292,10 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>                         continue;
>
>                 pos = cache_name_pos(src, strlen(src));
> -               assert(pos >= 0);
> -               rename_cache_entry_at(pos, dst);
> +               if (pos >= 0)
> +                       rename_cache_entry_at(pos, dst);
> +               else if (!ignore_errors)
> +                       die(_("bad source: source=%s, destination=%s"),
> + src, dst);
>         }
>
>         if (gitmodules_modified)
> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh index 63d5f41a12..5c7fee9bd8 100755
> --- a/t/t7001-mv.sh
> +++ b/t/t7001-mv.sh
> @@ -152,6 +152,14 @@ test_expect_success \
>      'move into "."' \
>      'git mv path1/path2/ .'
>
> +test_expect_success \
> +    'fail to move file already in index under different cased name' \
> +    'echo 1 > foo &&
> +     git add foo &&
> +     git commit -m add_file -- foo &&
> +     git mv foo FOO &&
> +     test_expect_code 128 git mv foo BAR'
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
>
[prev in list] [next in list] [prev in thread] [next in thread]