Re: [PATCH] git-mv: fix git mv bug with case insensitive fs
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
Show 10 quoted lines
>
> 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 filewas 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>
Show 39 quoted lines
> ---
> 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.
Show 7 quoted lines
> +
> test_expect_success "Michael Cassar's test case" '
> rm -fr .git papers partA &&
> git init &&
> --
> 2.25.1
>
