Re: [PATCH] merge-recursive.c: Fix case-changing merge bug
David Turner writes: > Would you prefer that I add it to t6022-merge-rename.sh? Or I could > add it to t7062-wtstatus-ignorecase.sh and rename that file to > t7062-ignorecase.sh. If I had only these two choices, t6022 would be it, as 6xxx series is where we have other tests for merge-recursive. I actually do not have a problem with adding a new file in t6xxx series as you did in this patch, if a longer term direction is to add more cases to it to make sure various paths that are only different in their cases (not just combination where one side renames, but things like combination where both sides rename, etc.) are handled correctly during a merge. Thanks. By the way, I see "touch" used to create a new file in the test, like this: + touch foo && + git add foo && Please don't. Instead, do it perhaps like this: >foo && git add foo && The primary purpose to use "touch" is to update a file's timestamp, and using it to create a file is misleading to readers. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] merge-recursive.c: Fix case-changing merge bug
On Tue, 2014-05-06 at 10:07 -0700, Junio C Hamano wrote:
> David Turner writes:
>
> > On a case-insensitive filesystem, when merging, a file would be
> > wrongly deleted from the working tree if an incoming commit had
> > renamed it changing only its case. When merging a rename, the file
> > with the old name would be deleted -- but since the filesystem
> > considers the old name to be the same as the new name, the new
> > file would in fact be deleted.
> >
> > We avoid this by not deleting files that have a case-clone in the
> > index at stage 0.
> >
> > Signed-off-by: David Turner
> > ---
> > merge-recursive.c | 6 ++
> > t/t7063-merge-ignorecase.sh | 32
> > 2 files changed, 38 insertions(+)
> > create mode 100755 t/t7063-merge-ignorecase.sh
> >
> > diff --git a/merge-recursive.c b/merge-recursive.c
> > index 4177092..cab16fa 100644
> > --- a/merge-recursive.c
> > +++ b/merge-recursive.c
> > @@ -589,6 +589,12 @@ static int remove_file(struct merge_options *o, int
> > clean,
> > return -1;
> > }
> > if (update_working_directory) {
> > + if (ignore_case) {
> > + struct cache_entry *ce;
> > + ce = cache_file_exists(path, strlen(path), ignore_case);
> > + if (ce && ce_stage(ce) == 0)
> > + return 0;
> > + }
> > if (remove_path(path))
> > return -1;
> > }
>
> Thanks.
>
> I wonder what happens if you did the same merge using the resolve
> strategy, though. If you see a similar issue, and the true reason
> of the breakage turns out to be because three-way merge performed by
> unpack_trees() (with opts.update set to 1) considers that these
> paths that only differ in case in "our" tree, in the index and in
> the working tree are different paths (I am not saying that is the
> case, but that was one of my first hunches after seeing the initial
> report) then it may suggest that the above might not be the best
> change to fix the issue.
I changed the test to -s resolve, and it changed from failing to
passing. So while I still don't know whether this is the right change,
it's at least not wrong for that reason.
> > diff --git a/t/t7063-merge-ignorecase.sh b/t/t7063-merge-ignorecase.sh
> > new file mode 100755
> > index 000..6d4959f
> > --- /dev/null
> > +++ b/t/t7063-merge-ignorecase.sh
>
> Hmmm, did you really have to add a file dedicated for this single
> test?
Would you prefer that I add it to t6022-merge-rename.sh? Or I could
add it to t7062-wtstatus-ignorecase.sh and rename that file to
t7062-ignorecase.sh.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] merge-recursive.c: Fix case-changing merge bug
David Turner writes:
> On a case-insensitive filesystem, when merging, a file would be
> wrongly deleted from the working tree if an incoming commit had
> renamed it changing only its case. When merging a rename, the file
> with the old name would be deleted -- but since the filesystem
> considers the old name to be the same as the new name, the new
> file would in fact be deleted.
>
> We avoid this by not deleting files that have a case-clone in the
> index at stage 0.
>
> Signed-off-by: David Turner
> ---
> merge-recursive.c | 6 ++
> t/t7063-merge-ignorecase.sh | 32
> 2 files changed, 38 insertions(+)
> create mode 100755 t/t7063-merge-ignorecase.sh
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 4177092..cab16fa 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -589,6 +589,12 @@ static int remove_file(struct merge_options *o, int
> clean,
> return -1;
> }
> if (update_working_directory) {
> + if (ignore_case) {
> + struct cache_entry *ce;
> + ce = cache_file_exists(path, strlen(path), ignore_case);
> + if (ce && ce_stage(ce) == 0)
> + return 0;
> + }
> if (remove_path(path))
> return -1;
> }
Thanks.
I wonder what happens if you did the same merge using the resolve
strategy, though. If you see a similar issue, and the true reason
of the breakage turns out to be because three-way merge performed by
unpack_trees() (with opts.update set to 1) considers that these
paths that only differ in case in "our" tree, in the index and in
the working tree are different paths (I am not saying that is the
case, but that was one of my first hunches after seeing the initial
report) then it may suggest that the above might not be the best
change to fix the issue.
> diff --git a/t/t7063-merge-ignorecase.sh b/t/t7063-merge-ignorecase.sh
> new file mode 100755
> index 000..6d4959f
> --- /dev/null
> +++ b/t/t7063-merge-ignorecase.sh
Hmmm, did you really have to add a file dedicated for this single
test?
> @@ -0,0 +1,32 @@
> +#!/bin/sh
> +
> +test_description='git-merge with case-changing rename on case-insensitive
> file system'
> +
> +. ./test-lib.sh
> +
> +if ! test_have_prereq CASE_INSENSITIVE_FS
> +then
> + skip_all='skipping case insensitive tests - case sensitive file system'
> + test_done
> +fi
> +
> +test_expect_success 'merge with case-changing rename with ignorecase=true' '
> + test $(git config core.ignorecase) = true &&
> + touch TestCase &&
> + git add TestCase &&
> + git commit -m "add TestCase" &&
> + git checkout -b with-camel &&
> + touch foo &&
> + git add foo &&
> + git commit -m "intervening commit" &&
> + git checkout master &&
> + git rm TestCase &&
> + touch testcase &&
> + git add testcase &&
> + git commit -m "rename to testcase" &&
> + git checkout with-camel &&
> + git merge master -m "merge" &&
> + test -e testcase
> +'
> +
> +test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
