[PATCH] git-mv: fix git mv bug with case insensitive fs

[PATCH] git-mv: fix git mv bug with case insensitive fs

[Dan Moseley]

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.

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.

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.

Signed-off-by: Dan Moseley <danmose@microsoft.com>
---
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'
+
 test_expect_success "Michael Cassar's test case" '
        rm -fr .git papers partA &&
        git init &&
--
2.25.1

Re: [PATCH] git-mv: fix git mv bug with case insensitive fs

[Torsten Bögershausen]

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
>

Re: [PATCH] git-mv: fix git mv bug with case insensitive fs

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> 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.

Sorry but I feel lost here.  So this is not a fix with which the end
user does not have to do anything after kicking in?  The only thing
it "fixes" is to avoid hitting a BUG() or something that can leave
the .lock file behind (so that the user does not have to run "rm
.git/index.lock" after the operation)?

[PATCH/RFC v1 1/1] git mv foo FOO ; git mv foo bar gave an assert

[tboegi]

From: Torsten Bögershausen <tboegi@web.de>

The following sequence, on a case-insensitive file system,
(strictly speeking with core.ignorecase=true)
leads to an assertion, and leaves .git/index.lock behind.

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

This regression was introduced in Commit 9b906af657,
"git-mv: improve error message for conflicted file"

Don't check if the case-insensitive version is in the index.
In the sense of 9b906af657 supply the user with a more helpful message.

This fixes
https://github.com/git-for-windows/git/issues/2920

Reported-By: Dan Moseley <Dan.Moseley@microsoft.com>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---

Note:
There is an ongoing effort to replace cache_file_exists() with
index_file_exists().
So this patch may need to be re-done.

Note2: @Dan Moseley: Do you want to continue with this work ?

builtin/mv.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

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 &&
--
2.28.0.97.gdc04167d37

Re: [PATCH/RFC v1 1/1] git mv foo FOO ; git mv foo bar gave an assert

[Junio C Hamano]

tboegi@web.de writes:

> From: Torsten Bögershausen <tboegi@web.de>
>
> The following sequence, on a case-insensitive file system,
> (strictly speeking with core.ignorecase=true)
> leads to an assertion, and leaves .git/index.lock behind.

"an assertion failure" you mean.

>
> git init
> echo foo >foo
> git add foo
> git mv foo FOO
> git mv foo bar
>
> This regression was introduced in Commit 9b906af657,
> "git-mv: improve error message for conflicted file"
>
> Don't check if the case-insensitive version is in the index.
> In the sense of 9b906af657 supply the user with a more helpful message.
>
> This fixes
> https://github.com/git-for-windows/git/issues/2920
>
> Reported-By: Dan Moseley <Dan.Moseley@microsoft.com>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
>
> Note:
> There is an ongoing effort to replace cache_file_exists() with
> index_file_exists().  So this patch may need to be re-done.

Thanks for a heads-up.

>
> Note2: @Dan Moseley: Do you want to continue with this work ?
>
> builtin/mv.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> 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 &&
> --
> 2.28.0.97.gdc04167d37

[PATCH v2 1/1] git mv foo FOO ; git mv foo bar gave an assert

[tboegi]

From: Torsten Bögershausen <tboegi@web.de>

The following sequence, on a case-insensitive file system,
(strictly speeking with core.ignorecase=true)
leads to an assertion, and leaves .git/index.lock behind.

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

This regression was introduced in Commit 9b906af657,
"git-mv: improve error message for conflicted file"

The bugfix is to change the "file exist case-insensitive in the index"
into the correct "file exist (case-sensitive) in the index".
This avoids the "assert".

Reported-By: Dan Moseley <Dan.Moseley@microsoft.com>

This fixes
https://github.com/git-for-windows/git/issues/2920

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 builtin/mv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 7dac714af9..3fccdcb645 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -221,7 +221,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				}
 				argc += last - first;
 			}
-		} else if (!(ce = cache_file_exists(src, length, ignore_case))) {
+		} else if (!(ce = cache_file_exists(src, length, 0))) {
 			bad = _("not under version control");
 		} else if (ce_stage(ce)) {
 			bad = _("conflicted");
--
2.30.0.155.g66e871b664

Re: [PATCH v2 1/1] git mv foo FOO ; git mv foo bar gave an assert

[Junio C Hamano]

tboegi@web.de writes:

> From: Torsten Bögershausen <tboegi@web.de>
>
> The following sequence, on a case-insensitive file system,
> (strictly speeking with core.ignorecase=true)
> leads to an assertion, and leaves .git/index.lock behind.

"an assertion failure", I presume?

>
> git init
> echo foo >foo
> git add foo
> git mv foo FOO
> git mv foo bar
>
> This regression was introduced in Commit 9b906af657,
> "git-mv: improve error message for conflicted file"
>
> The bugfix is to change the "file exist case-insensitive in the index"
> into the correct "file exist (case-sensitive) in the index".
> This avoids the "assert".
>
> Reported-By: Dan Moseley <Dan.Moseley@microsoft.com>
>
> This fixes
> https://github.com/git-for-windows/git/issues/2920
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>

By breaking the list of trailers with an unrelated comment in the
middle, the "Reported-by" won't be recognized as a trailer.  We'd
want to credit non-authorship contribution in the release notes
starting in the upcoming release, and this will start mattering.
Please fix it.

> @@ -221,7 +221,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  				}
>  				argc += last - first;
>  			}
> -		} else if (!(ce = cache_file_exists(src, length, ignore_case))) {
> +		} else if (!(ce = cache_file_exists(src, length, 0))) {

Good finding.

Before the problematic patch, this used to be

               } else if (cache_name_pos(src, length) < 0)

I wonder if we should revert the change to use cache_file_exists()
in the first place (and rewrite the subsequent use of ce to match),
though.

Thanks.

Re: [PATCH v2 1/1] git mv foo FOO ; git mv foo bar gave an assert

[Chris Torek]

On Mon, Mar 1, 2021 at 11:26 AM Junio C Hamano <gitster@pobox.com> wrote:
> Before the problematic patch, this used to be
>
>                } else if (cache_name_pos(src, length) < 0)
>
> I wonder if we should revert the change to use cache_file_exists()
> in the first place (and rewrite the subsequent use of ce to match),
> though.

For what it's worth, that was what I did originally; the change
to look up the ce "up front" was because someone objected to the
double search implied by calling cache_name_pos once, then
cache_file_exists to determine the correct error message...

(I've never been 100% on how the ignore-case stuff works
internally.)

Chris

Re: [PATCH v2 1/1] git mv foo FOO ; git mv foo bar gave an assert

[Junio C Hamano]

Chris Torek <chris.torek@gmail.com> writes:

> On Mon, Mar 1, 2021 at 11:26 AM Junio C Hamano <gitster@pobox.com> wrote:
>> Before the problematic patch, this used to be
>>
>>                } else if (cache_name_pos(src, length) < 0)
>>
>> I wonder if we should revert the change to use cache_file_exists()
>> in the first place (and rewrite the subsequent use of ce to match),
>> though.
>
> For what it's worth, that was what I did originally; the change
> to look up the ce "up front" was because someone objected to the
> double search implied by calling cache_name_pos once, then
> cache_file_exists to determine the correct error message...

cache_name_pos() bypasses the name-hash altogether because it won't
need case insensitive search at all, so the comparison is apples and
oranges.  The use of cache_file_exists() made sense if ignore_case
is wanted, but since with the "fix", we always match case
sensitively, I would suspect that it would start making sense to use
cache_name_pos() to grab exactly the ce we want.

Thanks.

RE: [EXTERNAL] [PATCH v2 1/1] git mv foo FOO ; git mv foo bar gave an assert

[Dan Moseley]

> From: Torsten Bögershausen <tboegi@web.de>
> 
> The following sequence, on a case-insensitive file system,
> (strictly speeking with core.ignorecase=true)
> leads to an assertion, and leaves .git/index.lock behind.
> 
> git init
> echo foo >foo
> git add foo
> git mv foo FOO
> git mv foo bar
> 
> This regression was introduced in Commit 9b906af657,
> "git-mv: improve error message for conflicted file"
> 
> The bugfix is to change the "file exist case-insensitive in the index"
> into the correct "file exist (case-sensitive) in the index".
> This avoids the "assert".
> 
> Reported-By: Dan Moseley <Dan.Moseley@microsoft.com>
> 
> This fixes
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fgit-for-windows%2Fgit%2Fissues%2F2920&amp;data=04%7C01%7CDan.Moseley%40microsoft.com%7Cb659ba5925c543bcc3e708d8dcd41268%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637502150683596627%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=%2BLjQf5NooTUgOKbsyn10XCFIAgMw9v%2BKlhx8kDD6%2BIg%3D&amp;reserved=0
> 
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
>  builtin/mv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 7dac714af9..3fccdcb645 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -221,7 +221,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  				}
>  				argc += last - first;
>  			}
> -		} else if (!(ce = cache_file_exists(src, length, ignore_case))) {
> +		} else if (!(ce = cache_file_exists(src, length, 0))) {
>  			bad = _("not under version control");
>  		} else if (ce_stage(ce)) {
>  			bad = _("conflicted");
> --
> 2.30.0.155.g66e871b664
>

Thank you Thorsten. This makes sense to me. Do you want to add a test? 
I believe this is what I had in my original patch, that worked pretty well:

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'
+
 test_expect_success "Michael Cassar's test case" '
        rm -fr .git papers partA &&
        git init &&
--
2.25.1

Re: [EXTERNAL] [PATCH v2 1/1] git mv foo FOO ; git mv foo bar gave an assert

[Junio C Hamano]

Dan Moseley <Dan.Moseley@microsoft.com> writes:

> Thank you Thorsten. This makes sense to me. Do you want to add a test? 
> I believe this is what I had in my original patch, that worked pretty well:

Would this succeed unconditionally on all platforms, or only
relevant on case-insensitive filesystems?  Lack of any prerequisite
on a new test is a good thing (if it is correct), but the
description of the change talked about case insensitivity, so ...

> 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'
> +
>  test_expect_success "Michael Cassar's test case" '
>         rm -fr .git papers partA &&
>         git init &&

[PATCH v3 1/1] git mv foo FOO ; git mv foo bar gave an assert

[tboegi]

From: Torsten Bögershausen <tboegi@web.de>

The following sequence, on a case-insensitive file system,
(strictly speeking with core.ignorecase=true)
leads to an assertion failure and leaves .git/index.lock behind.

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

This regression was introduced in Commit 9b906af657,
"git-mv: improve error message for conflicted file"

The bugfix is to change the "file exist case-insensitive in the index"
into the correct "file exist (case-sensitive) in the index".

This avoids the "assert" later in the code and keeps setting up the
"ce" pointer for ce_stage(ce) done in the next else if.

This fixes
https://github.com/git-for-windows/git/issues/2920

Reported-By: Dan Moseley <Dan.Moseley@microsoft.com>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
Changes since V2:
  avoid the trailer mess-up by putting "this fixes" before the trailers
  Make clearer why cache_name_pos() (leaving a dangling ce) followed
  by an additional cache_file_exists() to set up ce can be optimized
  into a single cache_file_exists()

builtin/mv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 7dac714af9..3fccdcb645 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -221,7 +221,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				}
 				argc += last - first;
 			}
-		} else if (!(ce = cache_file_exists(src, length, ignore_case))) {
+		} else if (!(ce = cache_file_exists(src, length, 0))) {
 			bad = _("not under version control");
 		} else if (ce_stage(ce)) {
 			bad = _("conflicted");
--
2.30.0.155.g66e871b664

Re: [PATCH v3 1/1] git mv foo FOO ; git mv foo bar gave an assert

[Junio C Hamano]

tboegi@web.de writes:

> From: Torsten Bögershausen <tboegi@web.de>
>
> The following sequence, on a case-insensitive file system,
> (strictly speeking with core.ignorecase=true)
> leads to an assertion failure and leaves .git/index.lock behind.
>
> git init
> echo foo >foo
> git add foo
> git mv foo FOO
> git mv foo bar
>
> This regression was introduced in Commit 9b906af657,
> "git-mv: improve error message for conflicted file"
>
> The bugfix is to change the "file exist case-insensitive in the index"
> into the correct "file exist (case-sensitive) in the index".
>
> This avoids the "assert" later in the code and keeps setting up the
> "ce" pointer for ce_stage(ce) done in the next else if.
>
> This fixes
> https://github.com/git-for-windows/git/issues/2920
>
> Reported-By: Dan Moseley <Dan.Moseley@microsoft.com>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>

Thanks, that looks better.

> ---
> Changes since V2:
>   avoid the trailer mess-up by putting "this fixes" before the trailers
>   Make clearer why cache_name_pos() (leaving a dangling ce) followed
>   by an additional cache_file_exists() to set up ce can be optimized
>   into a single cache_file_exists()
>
> builtin/mv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 7dac714af9..3fccdcb645 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -221,7 +221,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  				}
>  				argc += last - first;
>  			}
> -		} else if (!(ce = cache_file_exists(src, length, ignore_case))) {
> +		} else if (!(ce = cache_file_exists(src, length, 0))) {
>  			bad = _("not under version control");
>  		} else if (ce_stage(ce)) {
>  			bad = _("conflicted");
> --
> 2.30.0.155.g66e871b664