| Message ID | 20210301170536.12265-1-tboegi@web.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/1] git mv foo FOO ; git mv foo bar gave an assert | expand |
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.
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
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.
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");