Bug: Case-insensitive filesystems can cause merge and checkout problems

> 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 <dturner@twitter.com>
> ---
>  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;
>  	}

> diff --git a/t/t7063-merge-ignorecase.sh b/t/t7063-merge-ignorecase.sh
> new file mode 100755
> index 0000000..6d4959f
> --- /dev/null
> +++ b/t/t7063-merge-ignorecase.sh

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

> David Turner <dturner@twopensource.com> 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 <dturner@twitter.com>
> > ---
> >  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 0000000..6d4959f
> > --- /dev/null
> > +++ b/t/t7063-merge-ignorecase.sh
> 
> Hmmm, did you really have to add a file dedicated for this single
> test?

> From: David Turner <dturner@twitter.com>
> 
> Make it possible to change the case of a filename on a
> case-insensitive filesystem using git mv.  Change git mv to allow
> moves where the destination file exists if either the destination file
> has the same inode as the source file (for Mac) or the same name
> ignoring case (for Win).
> 
> Signed-off-by: David Turner <dturner@twitter.com>
> ---
>  builtin/mv.c                | 18 ++++++++++--------
>  t/t6039-merge-ignorecase.sh |  2 +-
>  2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 45e57f3..8cead13 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -74,7 +74,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  	};
>  	const char **source, **destination, **dest_path, **submodule_gitfile;
>  	enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;
> -	struct stat st;
> +	struct stat src_st,dst_st;
>  	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
>  
>  	gitmodules_config();
> @@ -102,8 +102,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  	if (dest_path[0][0] == '\0')
>  		/* special case: "." was normalized to "" */
>  		destination = internal_copy_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
> -	else if (!lstat(dest_path[0], &st) &&
> -			S_ISDIR(st.st_mode)) {
> +	else if (!lstat(dest_path[0], &dst_st) &&
> +			S_ISDIR(dst_st.st_mode)) {
>  		dest_path[0] = add_slash(dest_path[0]);
>  		destination = internal_copy_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
>  	} else {
> @@ -122,13 +122,13 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  			printf(_("Checking rename of '%s' to '%s'\n"), src, dst);
>  
>  		length = strlen(src);
> -		if (lstat(src, &st) < 0)
> +		if (lstat(src, &src_st) < 0)
>  			bad = _("bad source");
>  		else if (!strncmp(src, dst, length) &&
>  				(dst[length] == 0 || dst[length] == '/')) {
>  			bad = _("can not move directory into itself");
> -		} else if ((src_is_dir = S_ISDIR(st.st_mode))
> -				&& lstat(dst, &st) == 0)
> +		} else if ((src_is_dir = S_ISDIR(src_st.st_mode))
> +				&& lstat(dst, &dst_st) == 0)
>  			bad = _("cannot move directory over file");
>  		else if (src_is_dir) {
>  			int first = cache_name_pos(src, length);
> @@ -202,14 +202,16 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  			}
>  		} else if (cache_name_pos(src, length) < 0)
>  			bad = _("not under version control");
> -		else if (lstat(dst, &st) == 0) {
> +		else if (lstat(dst, &dst_st) == 0 &&
> +			 (src_st.st_ino != dst_st.st_ino ||
> +			  (src_st.st_ino == 0 && strcasecmp(src, dst)))) {

>  			bad = _("destination exists");
>  			if (force) {
>  				/*
>  				 * only files can overwrite each other:
>  				 * check both source and destination
>  				 */
> -				if (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode)) {
> +				if (S_ISREG(dst_st.st_mode) || S_ISLNK(dst_st.st_mode)) {
>  					if (verbose)
>  						warning(_("overwriting '%s'"), dst);
>  					bad = NULL;
> diff --git a/t/t6039-merge-ignorecase.sh b/t/t6039-merge-ignorecase.sh
> index ad06752..28cfb49 100755
> --- a/t/t6039-merge-ignorecase.sh
> +++ b/t/t6039-merge-ignorecase.sh
> @@ -35,7 +35,7 @@ test_expect_success 'merge with case-changing rename on both sides' '
>  	git reset --hard baseline &&
>  	git branch -D with-camel &&
>  	git checkout -b with-camel &&
> -	git mv --force TestCase testcase &&
> +	git mv TestCase testcase &&
>  	git commit -m "recase on branch" &&
>  	> foo &&
>  	git add foo &&

> >  		} else if (cache_name_pos(src, length) < 0)
> >  			bad = _("not under version control");
> > -		else if (lstat(dst, &st) == 0) {
> > +		else if (lstat(dst, &dst_st) == 0 &&
> > +			 (src_st.st_ino != dst_st.st_ino ||
> > +			  (src_st.st_ino == 0 && strcasecmp(src, dst)))) {
> 
> Don't do that. st_ino is zero on Windows only because we do not spend time
> to fill in the field. Don't use it as an indicator for a case-insensitive
> file system; zero may be a valid inode number on other systems.

> On Wed, 2014-05-07 at 08:17 +0200, Johannes Sixt wrote:
>> >  		} else if (cache_name_pos(src, length) < 0)
>> >  			bad = _("not under version control");
>> > -		else if (lstat(dst, &st) == 0) {
>> > +		else if (lstat(dst, &dst_st) == 0 &&
>> > +			 (src_st.st_ino != dst_st.st_ino ||
>> > +			  (src_st.st_ino == 0 && strcasecmp(src, dst)))) {
>> 
>> Don't do that. st_ino is zero on Windows only because we do not spend time
>> to fill in the field. Don't use it as an indicator for a case-insensitive
>> file system; zero may be a valid inode number on other systems.
>
> I don't think it is a problem if zero is a valid inode.  The only thing
> that happens when there is a zero inode, is that we have to compare
> filenames.  The inode check is just an optimization to avoid doing a
> bunch of strcasecmp on systems that don't have to.

> David Turner <dturner@twopensource.com> writes:
> 
> > On Wed, 2014-05-07 at 08:17 +0200, Johannes Sixt wrote:
> >> >  		} else if (cache_name_pos(src, length) < 0)
> >> >  			bad = _("not under version control");
> >> > -		else if (lstat(dst, &st) == 0) {
> >> > +		else if (lstat(dst, &dst_st) == 0 &&
> >> > +			 (src_st.st_ino != dst_st.st_ino ||
> >> > +			  (src_st.st_ino == 0 && strcasecmp(src, dst)))) {
> >> 
> >> Don't do that. st_ino is zero on Windows only because we do not spend time
> >> to fill in the field. Don't use it as an indicator for a case-insensitive
> >> file system; zero may be a valid inode number on other systems.
> >
> > I don't think it is a problem if zero is a valid inode.  The only thing
> > that happens when there is a zero inode, is that we have to compare
> > filenames.  The inode check is just an optimization to avoid doing a
> > bunch of strcasecmp on systems that don't have to.
> 
> Am I correct to rephrase you that the code assumes that any
> filesystem that cannot give unique inum to different files will use
> 0 as the placeholder inum, so if src/dst share the same non-zero
> inum, it is guaranteed that is not a placeholder and we know they
> are different files without comparing the two paths?

> David Turner <dturner@twopensource.com> writes:
> 
>> On Wed, 2014-05-07 at 08:17 +0200, Johannes Sixt wrote:
>>>>  		} else if (cache_name_pos(src, length) < 0)
>>>>  			bad = _("not under version control");
>>>> -		else if (lstat(dst, &st) == 0) {
>>>> +		else if (lstat(dst, &dst_st) == 0 &&
>>>> +			 (src_st.st_ino != dst_st.st_ino ||
>>>> +			  (src_st.st_ino == 0 && strcasecmp(src, dst)))) {
>>>
>>> Don't do that. st_ino is zero on Windows only because we do not spend time
>>> to fill in the field. Don't use it as an indicator for a case-insensitive
>>> file system; zero may be a valid inode number on other systems.
>>
>> I don't think it is a problem if zero is a valid inode.  The only thing
>> that happens when there is a zero inode, is that we have to compare
>> filenames.  The inode check is just an optimization to avoid doing a
>> bunch of strcasecmp on systems that don't have to.
> 
> Am I correct to rephrase you that the code assumes that any
> filesystem that cannot give unique inum to different files will use
> 0 as the placeholder inum, so if src/dst share the same non-zero
> inum, it is guaranteed that is not a placeholder and we know they
> are different files without comparing the two paths?

> Am 5/7/2014 19:46, schrieb Junio C Hamano:
>> David Turner <dturner@twopensource.com> writes:
>>
>>> On Wed, 2014-05-07 at 08:17 +0200, Johannes Sixt wrote:
>>>>>   		} else if (cache_name_pos(src, length) < 0)
>>>>>   			bad = _("not under version control");
>>>>> -		else if (lstat(dst, &st) == 0) {
>>>>> +		else if (lstat(dst, &dst_st) == 0 &&
>>>>> +			 (src_st.st_ino != dst_st.st_ino ||
>>>>> +			  (src_st.st_ino == 0 && strcasecmp(src, dst)))) {
>>>> Don't do that. st_ino is zero on Windows only because we do not spend time
>>>> to fill in the field. Don't use it as an indicator for a case-insensitive
>>>> file system; zero may be a valid inode number on other systems.
>>> I don't think it is a problem if zero is a valid inode.  The only thing
>>> that happens when there is a zero inode, is that we have to compare
>>> filenames.  The inode check is just an optimization to avoid doing a
>>> bunch of strcasecmp on systems that don't have to.
>> Am I correct to rephrase you that the code assumes that any
>> filesystem that cannot give unique inum to different files will use
>> 0 as the placeholder inum, so if src/dst share the same non-zero
>> inum, it is guaranteed that is not a placeholder and we know they
>> are different files without comparing the two paths?
> "If src and dst share the same non-zero inum, it is guaranteed that it is
> not a placeholder and we know they are the same file" is more correct.
>
> What if both inums are zero? Can this happen on any sane POSIX system? I
> don't know, but my gut feeling is that inode zero is too special to be
> allocated for files or directories.
>
> In that case, it is safe to assume that the st_ino field is just a
> placeholder when it is zero, and we have to compare the file name. Then we
> can either assume that this happens only for our emulation layer on MinGW
> (and the comparison can be case-insensitive) or choose the comparison mode
> based on core.ignorecase. This patch does the former, but I think we
> should do the latter.

> On 05/08/2014 08:37 AM, Johannes Sixt wrote:
> > What if both inums are zero? Can this happen on any sane POSIX system? I
> > don't know, but my gut feeling is that inode zero is too special to be
> > allocated for files or directories.
> >
> > In that case, it is safe to assume that the st_ino field is just a
> > placeholder when it is zero, and we have to compare the file name. Then we
> > can either assume that this happens only for our emulation layer on MinGW
> > (and the comparison can be case-insensitive) or choose the comparison mode
> > based on core.ignorecase. This patch does the former, but I think we
> > should do the latter.
> Whatever we do, we should "protect" the strcasecmp() with ignore_case:
> 
> !ignore_case || strcasecmp(src, dst)
> 
> (And once that is done, you don't need to look at st_ino at all)

> From: David Turner <dturner@twitter.com>
>
> Make it possible to change the case of a filename on a
> case-insensitive filesystem using git mv.  Change git mv to allow
> moves where the destination file exists if the destination file has
> the same name as the source file ignoring case.
>
> Signed-off-by: David Turner <dturner@twitter.com>
> ---
>  builtin/mv.c                | 3 ++-
>  t/t6039-merge-ignorecase.sh | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 45e57f3..f4d89d0 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -202,7 +202,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  			}
>  		} else if (cache_name_pos(src, length) < 0)
>  			bad = _("not under version control");
> -		else if (lstat(dst, &st) == 0) {
> +		else if (lstat(dst, &st) == 0 &&
> +			 (!ignore_case || strcasecmp(src, dst))) {

>  			bad = _("destination exists");
>  			if (force) {
>  				/*
> diff --git a/t/t6039-merge-ignorecase.sh b/t/t6039-merge-ignorecase.sh
> index dfc9f17..a977653 100755
> --- a/t/t6039-merge-ignorecase.sh
> +++ b/t/t6039-merge-ignorecase.sh
> @@ -35,7 +35,7 @@ test_expect_success 'merge with case-changing rename on both sides' '
>  	git reset --hard baseline &&
>  	git branch -D with-camel &&
>  	git checkout -b with-camel &&
> -	git mv --force TestCase testcase &&
> +	git mv TestCase testcase &&
>  	git commit -m "recase on branch" &&
>  	>foo &&
>  	git add foo &&

> dturner@twopensource.com writes:
> 
> > From: David Turner <dturner@twitter.com>
> >
> > Make it possible to change the case of a filename on a
> > case-insensitive filesystem using git mv.  Change git mv to allow
> > moves where the destination file exists if the destination file has
> > the same name as the source file ignoring case.
> >
> > Signed-off-by: David Turner <dturner@twitter.com>
> > ---
> >  builtin/mv.c                | 3 ++-
> >  t/t6039-merge-ignorecase.sh | 2 +-
> >  2 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/builtin/mv.c b/builtin/mv.c
> > index 45e57f3..f4d89d0 100644
> > --- a/builtin/mv.c
> > +++ b/builtin/mv.c
> > @@ -202,7 +202,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
> >  			}
> >  		} else if (cache_name_pos(src, length) < 0)
> >  			bad = _("not under version control");
> > -		else if (lstat(dst, &st) == 0) {
> > +		else if (lstat(dst, &st) == 0 &&
> > +			 (!ignore_case || strcasecmp(src, dst))) {
> 
> Hmm, I would find it easier to read if it were:
> 
> 		... if (lstat(dst, &st) == 0 &&
>                 	!(ignore_case && !strcasecmp(src, dst))) {
> 
> That is, "it is an error for dst to exist, unless we are on a case
> insensitive filesystem and src and dst refer to the same file.", but
> maybe it is just me.

> More importantly, what is the end-user visible effect of this
> change?  Is it fair to summarize it like this?
> 
>     On a case-insensitive filesystem, "mv hello.txt Hello.txt"
>     always trigger the "dst already exists" error, because both
>     names refer to the same file to MS-DOS, requiring the user to

> Overwriting an existing file with "mv hello.txt Hello.txt" on a case
> sensitive filesystem *is* an unusual operation, and that is the
> reason why we require "--force" to make sure that the user means it.
> I have a slight suspicion that the same "mv hello.txt Hello.txt" on
> a case insensitive filesystem, where two names are known (to the end
> user of such a filesystem) to refer to the same path would equally
> be a very unusual thing to do, and such an operation may deserve a
> similar safety precaution to make sure that the user really meant to
> do so by requiring "--force".
> 
> So, I dunno.

>> Hmm, I would find it easier to read if it were:
>> 
>> 		... if (lstat(dst, &st) == 0 &&
>>                 	!(ignore_case && !strcasecmp(src, dst))) {
>> 
>> That is, "it is an error for dst to exist, unless we are on a case
>> insensitive filesystem and src and dst refer to the same file.", but
>> maybe it is just me.
>
> I personally dislike the double negative. I also considered breaking it
> out into a little function with a self-documenting name -- does that
> sound better?

>> More importantly, what is the end-user visible effect of this
>> change?  Is it fair to summarize it like this?
>> 
>>     On a case-insensitive filesystem, "mv hello.txt Hello.txt"
>>     always trigger the "dst already exists" error, because both
>>     names refer to the same file to MS-DOS, requiring the user to
>                                      ^^^^^^
> (I have not actually tested on Windows; I tested on the Mac since that's
> what I have handy)
>
>>     pass the "--force" option.  Allow it without "--force".
>
> Yes.
>
>> Overwriting an existing file with "mv hello.txt Hello.txt" on a case
>> sensitive filesystem *is* an unusual operation, and that is the
>> reason why we require "--force" to make sure that the user means it.
>> I have a slight suspicion that the same "mv hello.txt Hello.txt" on
>> a case insensitive filesystem, where two names are known (to the end
>> user of such a filesystem) to refer to the same path would equally
>> be a very unusual thing to do, and such an operation may deserve a
>> similar safety precaution to make sure that the user really meant to
>> do so by requiring "--force".
>> 
>> So, I dunno.
>
> The argument against --force is that git's behavior should not
> significantly differ between sensitive and insensitive filesystems
> (where possible).  I do not see a case-changing rename as unusual on a
> case-insensitive filesystem; these filesystems typically preserve case,
> and a user might reasonably care about the case of a filename either for
> aesthetic reasons or for functionality on sensible filesystems (e.g.
> developers who work on Macs but deploy on GNU/Linux, as is quite
> common).
>
> The Mac's interface itself provides conflicting evidence: on one hand,
> we might expect git mv to work like plain mv: nothing special is needed
> to do a case-changing mv). On the other hand, in the Finder, attempting
> a case-changing rename causes an error message (which there is no way to
> get around other than the two-rename dance).  I read this as "ordinary
> users never intentionally change the case of files, but developers
> sometimes do", but that's not the only possible reading.
>
> I myself am not actually a Mac user; I simply support a bunch of Mac
> users (which is where the merge bug came from).  So I don't know what
> Mac users would prefer.  Maybe there are some on the git mailing list?
>
> I also have not tried on Windows.  I put in an email to the one
> Windows-using friend I can think of to ask her to give Windows Explorer
> (or whatever it's called these days) a try.  My guess (based on a quick
> Google search) would be is that it works without error, but I will send
> an update if I hear otherwise.

> @@ -74,7 +74,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  	};
>  	const char **source, **destination, **dest_path, **submodule_gitfile;
>  	enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;
> -	struct stat st;
> +	struct stat src_st,dst_st;

> +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' '
> +	test $(git config core.ignorecase) = true &&

>> +	test -e testcase
>
> Please make it a habit to use "test -f" when you expect "the path
> exists as a file", not merely "something exists there I do not care
> if it is a file or a directory", for which "test -e" is perfectly
> appropriate.

>> Please make it a habit to use "test -f" when you expect "the path
>> exists as a file", not merely "something exists there I do not care
>> if it is a file or a directory", for which "test -e" is perfectly
>> appropriate.
>
> Or, better in tests,
>
> 	test_path_is_file testcase
>
> which prints an error instead of just silently failing when
> the path is not a file.