[PATCH] Handle rename of case only, for Windows

From

Tim Abell <tim@timwise.co.uk>

Date

Jan 29, 2011, 23:45 UTC

Message-ID

<1296344717.25999.1417928123@webmail.messagingengine.com>

>From ddab003ede9f25d93f4e7eea833523a0aa29d16d Mon Sep 17 00:00:00 2001

From: Tim Abell <tim@timwise.co.uk>
Date: Thu, 27 Jan 2011 22:53:31 +0000
Subject: [PATCH] Handle rename of case only, for Windows

A message from Linus on the subject:
 http://kerneltrap.org/mailarchive/git/2008/3/21/1220814
 (apparently he never got the energy :-)

Signed-off-by: Tim Abell <tim@timwise.co.uk>
---

>Hmm, not so good. st_ino is always 0 on Windows, so this would make
>false positives, no?

I tested this on windows 7 under cygwin (which is what I have available) and st_ino reports real numbers for me, I also tested that attempting to overwrite another file without --force still fails and added a new test case for this scenario. I have now made sure that if zero is returned then git won't accidentally overwrite other files as I hadn't thought of this before. So this patch shouldn't be regressive even if other versions of windows or other filesystems don't provide valid inode data.

Adding the following line after "lstat(src, &src_st);" shows the inode numbers when calling git mv:
 printf("src inode: %lu, dst inode: %lu", (unsigned long) src_st.st_ino, (unsigned long) st.st_ino);
And here is my ouput showing it in action:

$ git mv foo.txt bar.txt
  fatal: destination exists, source=foo.txt, destination=bar.txt
  src inode: 67064, dst inode: 229369
$ git mv foo.txt Foo.txt
  src inode: 67064, dst inode: 67064

>I wonder if we can make lstat_case() that would only return 0 if it
>matches exactly the filename, even on FAT. FindFirstFile/FindNextFile
>should return true file name, I think. If not, we can make
>lstat_case() take two paths (src and dst) and move all inode
>comparison code in there. Much cleaner.

I'm afraid this is a bit beyond me at the moment, but I'm fairly happy with the solution I have. Thanks for the feedback though.

>Couldn't this be moved inside the scope around "cache_name_pos"?
>That's the only scope it is valid inside anyway...

Done. I wasn't sure about this initially as I'm not very experienced with C programming. Thanks for the pointer.

>Perhaps you could use the full URL (and maybe put it in the commit
>message insted)? It'd be nice if we could reach this information even
>if is.gd disappears...

Good point. I've put the full url in the commit message instead.

>Uhm, is this debug-leftovers? #warning is a preprocessor-construct,
>and it can't understand varaibles in c. Especially not formatted as
>strings. Can #warning even do varags? :P

Yes, it was a debug line. Doh! Complete chance that it compiled, I've been doing too much bash scripting recently it seems. I've stripped it out. A better version is available above should anyone want to see the inode numbers.

Thanks all, I welcome any more feedback.

Does this now look like something that anyone on the git project would like to pick up as a contribution?

Yours

Tim Abell

 builtin/mv.c  |   32 +++++++++++++++++++++-----------
 t/t7001-mv.sh |   35 +++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 11 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index cdbb094..c2f726a 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -165,17 +165,27 @@ 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) {
-			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)) {
-					warning("%s; will overwrite!", bad);
-					bad = NULL;
-				} else
-					bad = "Cannot overwrite";
+			/* If we are on a case insensitive file system (windows) and we are only
+			 * changing the case of the file then lstat for the destination will
+			 * return != 0 because it sees the source file.
+			 * To prevent this causing failure, lstat is used to get the inode of the src
+			 * and see if it's actually the same file as dst. If the inode == 0 then
+			 * we can't tell whether it is the same file so we fail regardless. */
+			struct stat src_st;
+			lstat(src, &src_st);
+			if (src_st.st_ino == 0 || src_st.st_ino != st.st_ino) {
+				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)) {
+						warning("%s; will overwrite!", bad);
+						bad = NULL;
+					} else
+						bad = "Cannot overwrite";
+				}
 			}
 		} else if (string_list_has_string(&src_for_dst, dst))
 			bad = "multiple sources for the same target";
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 65a35d9..abaecb6 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -255,4 +255,39 @@ test_expect_success SYMLINKS 'git mv should overwrite file with a symlink' '
 
 rm -f moved symlink
 
+test_expect_success 'git mv should not fail when only changing case' '
+
+	rm -fr .git &&
+	git init &&
+	>foo.txt &&
+	git add foo.txt &&
+	git mv foo.txt Foo.txt
+'
+
+rm *.txt
+
+test_expect_success 'git mv should not overwrite existing file' '
+
+	rm -fr .git &&
+	git init &&
+	>foo.txt &&
+	>bar.txt &&
+	git add foo.txt &&
+	test_must_fail git mv foo.txt bar.txt
+'
+
+rm *.txt
+
+test_expect_success 'git mv -f should overwrite existing file' '
+
+	rm -fr .git &&
+	git init &&
+	>foo.txt &&
+	>bar.txt &&
+	git add foo.txt &&
+	git mv -f foo.txt bar.txt
+'
+
+rm *.txt
+
 test_done

-- 
1.7.3.5.3.g2b35a