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

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

[David Turner]

By default, git sets core.ignorecase=true when git init or git clone
is run on a machine with a case-insensitive filesystem.  Here's a
test-case for some problems that this causes:

git checkout master
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'

One would expect a clean working copy at this point, but in fact, the
file 'testcase' will be deleted.

With core.ignorecase=false, we get a different failure.  The
penultimate command fails with:
$ git checkout with-camel
error: The following untracked working tree files would be overwritten
by checkout:
        TestCase
Please move or remove them before you can switch branches.
Aborting

Of course, there is no untracked working tree file by that name; there
is a tracked working tree file named testcase (all-lowercase).

[PATCH] merge-recursive.c: Fix case-changing merge bug

[David Turner]

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

Re: [PATCH] merge-recursive.c: Fix case-changing merge bug

[Junio C Hamano]

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?

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

Re: [PATCH] merge-recursive.c: Fix case-changing merge bug

[David Turner]

On Tue, 2014-05-06 at 10:07 -0700, Junio C Hamano wrote:
> 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.

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

Re: [PATCH] merge-recursive.c: Fix case-changing merge bug

[Junio C Hamano]

David Turner <dturner@twopensource.com> 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 <TC, TC, tc> combination where
one side renames, but things like <tc, TC TC> 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.

[PATCH 1/2] merge-recursive.c: Fix case-changing merge.

[dturner]

From: David Turner <dturner@twitter.com>

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/t6039-merge-ignorecase.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)
 create mode 100755 t/t6039-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/t6039-merge-ignorecase.sh b/t/t6039-merge-ignorecase.sh
new file mode 100755
index 0000000..ad06752
--- /dev/null
+++ b/t/t6039-merge-ignorecase.sh
@@ -0,0 +1,53 @@
+#!/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' '
+	test $(git config core.ignorecase) = true &&
+	> TestCase &&
+	git add TestCase &&
+	git commit -m "add TestCase" &&
+	git tag baseline
+	git checkout -b with-camel &&
+	> foo &&
+	git add foo &&
+	git commit -m "intervening commit" &&
+	git checkout master &&
+	git rm TestCase &&
+	> testcase &&
+	git add testcase &&
+	git commit -m "rename to testcase" &&
+	git checkout with-camel &&
+	git merge master -m "merge" &&
+	test -e testcase
+'
+
+test_expect_success 'merge with case-changing rename on both sides' '
+	git checkout master &&
+	git reset --hard baseline &&
+	git branch -D with-camel &&
+	git checkout -b with-camel &&
+	git mv --force TestCase testcase &&
+	git commit -m "recase on branch" &&
+	> foo &&
+	git add foo &&
+	git commit -m "intervening commit" &&
+	git checkout master &&
+	git rm TestCase &&
+	> testcase &&
+	git add testcase &&
+	git commit -m "rename to testcase" &&
+	git checkout with-camel &&
+	git merge master -m "merge" &&
+	test -e testcase
+'
+
+test_done
-- 
2.0.0.rc0.33.g27630aa

[PATCH 2/2] ignorecase: Fix git mv on insensitive filesystems

[dturner]

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 &&
-- 
2.0.0.rc0.33.g27630aa

Re: [PATCH 2/2] ignorecase: Fix git mv on insensitive filesystems

[Johannes Sixt]

Am 5/7/2014 0:59, schrieb dturner@twopensource.com:
> 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)))) {

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.

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

Re: [PATCH 2/2] ignorecase: Fix git mv on insensitive filesystems

[David Turner]

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.

Re: [PATCH 2/2] ignorecase: Fix git mv on insensitive filesystems

[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?

Re: [PATCH 2/2] ignorecase: Fix git mv on insensitive filesystems

[David Turner]

On Wed, 2014-05-07 at 10:46 -0700, Junio C Hamano wrote:
> 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?

Yes, this is indeed a fair rephrasing.  In fact, the entire zero-check
should not be necessary, as POSIX requires that the st_ino field has a
"meaningful" value.  So in the case that this ever runs into a problem,
we ought to wrap the lstat call with a compatibility layer anyway. 

But maybe there is an OS I'm not thinking of which fills in st_ino with
something else?

Re: [PATCH 2/2] ignorecase: Fix git mv on insensitive filesystems

[Johannes Sixt]

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.

-- Hannes

Re: [PATCH 2/2] ignorecase: Fix git mv on insensitive filesystems

[Torsten Bögershausen]

On 05/08/2014 08:37 AM, Johannes Sixt wrote:
> 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.
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)

[PATCH 0/2] Re: ignorecase: Fix git mv on insensitive filesystems

[dturner]

Torsten Bögershausen wrote:
> 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)

If both st_inos are zero on POSIX systems, there are two
possibilities: the two files are case-clones, or the two files are
hard-links of one another.  Checking st_ino was just an optimization
to avoid the slow string comparison.  I hadn't considered the
hard-link case important, but I guess it's better to be correct.  So
here's a patch without (as well as a cleaned-up version of the first
patch, to keep the series together).

[PATCH 1/2] merge-recursive.c: Fix case-changing merge.

[dturner]

From: David Turner <dturner@twitter.com>

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/t6039-merge-ignorecase.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)
 create mode 100755 t/t6039-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/t6039-merge-ignorecase.sh b/t/t6039-merge-ignorecase.sh
new file mode 100755
index 0000000..dfc9f17
--- /dev/null
+++ b/t/t6039-merge-ignorecase.sh
@@ -0,0 +1,53 @@
+#!/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' '
+	test $(git config core.ignorecase) = true &&
+	>TestCase &&
+	git add TestCase &&
+	git commit -m "add TestCase" &&
+	git tag baseline
+	git checkout -b with-camel &&
+	>foo &&
+	git add foo &&
+	git commit -m "intervening commit" &&
+	git checkout master &&
+	git rm TestCase &&
+	>testcase &&
+	git add testcase &&
+	git commit -m "rename to testcase" &&
+	git checkout with-camel &&
+	git merge master -m "merge" &&
+	test_path_is_file testcase
+'
+
+test_expect_success 'merge with case-changing rename on both sides' '
+	git checkout master &&
+	git reset --hard baseline &&
+	git branch -D with-camel &&
+	git checkout -b with-camel &&
+	git mv --force TestCase testcase &&
+	git commit -m "recase on branch" &&
+	>foo &&
+	git add foo &&
+	git commit -m "intervening commit" &&
+	git checkout master &&
+	git rm TestCase &&
+	>testcase &&
+	git add testcase &&
+	git commit -m "rename to testcase" &&
+	git checkout with-camel &&
+	git merge master -m "merge" &&
+	test_path_is_file testcase
+'
+
+test_done
-- 
2.0.0.rc0.33.g27630aa

Re: [PATCH 1/2] merge-recursive.c: Fix case-changing merge.

[Junio C Hamano]

Thanks; I think this is identical to what we already have on the
dt/merge-recursive-case-insensitive topic.

[PATCH 2/2] ignorecase: Fix git mv on insensitive filesystems

[dturner]

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 &&
-- 
2.0.0.rc0.33.g27630aa

Re: [PATCH 2/2] ignorecase: Fix git mv on insensitive filesystems

[Junio C Hamano]

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
    pass the "--force" option.  Allow it without "--force".

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.

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

Re: [PATCH 2/2] ignorecase: Fix git mv on insensitive filesystems

[David Turner]

On Thu, 2014-05-08 at 12:54 -0700, Junio C Hamano wrote:
> 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.

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.

Re: [PATCH 2/2] ignorecase: Fix git mv on insensitive filesystems

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

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

No, it shows that it is just me.  I did not say that the original is
unreadable.

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

Alright.  Thanks for sanity checking.

I've already queued your patch as-is when I was composing the
message you responded to and today's integration cycle is already
going, so unless other people have ideas to convince us both that
this is a bad idea, all is well without any further action.

Thanks.

Re: [PATCH 2/2] ignorecase: Fix git mv on insensitive filesystems

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 641 bytes --]

On Tue, May 06, 2014 at 03:59:04PM -0700, dturner@twopensource.com wrote:
> @@ -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;

Extremely minor nit: we generally put a space after the comma.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] merge-recursive.c: Fix case-changing merge.

[Junio C Hamano]

dturner@twopensource.com writes:

> +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 &&

This check seems a bit strange.  You already know you are on a case
insensitive filesystem, so I would understand that if you assume it
is set, or if you make sure it is set (if you are really paranoid).

But I'll let it pass, as it is not wrong per-se.  Just looked strange.

> +	> TestCase &&

Please have no SP between redirection operator and its target.

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

I'll fix up locally before queuing; no need to resend.

Thanks.

Re: [PATCH 1/2] merge-recursive.c: Fix case-changing merge.

[Jonathan Nieder]

Junio C Hamano wrote:
> dturner@twopensource.com writes:

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

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.

Thanks,
Jonathan

Re: [PATCH 1/2] merge-recursive.c: Fix case-changing merge.

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

Thanks, will tweak.