From: "Torsten Bögershausen" <tboegi@web.de>
To: Dan Moseley <Dan.Moseley@microsoft.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: [PATCH] git-mv: fix git mv bug with case insensitive fs
Date: Thu, 31 Dec 2020 08:13:58 +0100 [thread overview]
Message-ID: <20201231071357.mtcxmoxbg6jrq3gn@tb-raspi4> (raw)
In-Reply-To: <BYAPR21MB1158D9E88C96D51259A0CC04E0D89@BYAPR21MB1158.namprd21.prod.outlook.com>
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
>
next prev parent reply other threads:[~2020-12-31 7:24 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <BYAPR21MB1158F64E1141453F7D00B46CE0D89@BYAPR21MB1158.namprd21.prod.outlook.com>
[not found] ` <BYAPR21MB11585FFD46DEE7AD4EEEFEABE0D89@BYAPR21MB1158.namprd21.prod.outlook.com>
2020-12-29 2:06 ` [PATCH] git-mv: fix git mv bug with case insensitive fs Dan Moseley
2020-12-31 7:13 ` Torsten Bögershausen [this message]
2021-01-06 4:05 ` Junio C Hamano
2021-01-06 10:53 ` [PATCH/RFC v1 1/1] git mv foo FOO ; git mv foo bar gave an assert tboegi
2021-01-06 23:51 ` Junio C Hamano
2021-03-01 17:05 ` [PATCH v2 " tboegi
2021-03-01 19:21 ` Junio C Hamano
2021-03-01 19:36 ` Chris Torek
2021-03-01 21:53 ` Junio C Hamano
[not found] ` <20210301170425.12154-1-tboegi@web.de>
2021-03-01 20:23 ` [EXTERNAL] " Dan Moseley
2021-03-01 21:47 ` Junio C Hamano
2021-03-01 21:41 ` [PATCH v3 " tboegi
2021-03-01 23:59 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201231071357.mtcxmoxbg6jrq3gn@tb-raspi4 \
--to=tboegi@web.de \
--cc=Dan.Moseley@microsoft.com \
--cc=git@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).