[PATCH] handle rename of case only, for windows

[PATCH] handle rename of case only, for windows

[Tim Abell]

Hi folks,

I've never contributed to git before so be gentle :-)

Would someone have the time to help me get this patch into mailine git?

I tripped over a failure to rename files on windows when only the case
has changed. I've created a patch which fixes it for me and doesn't seem
to break on linux or windows. I also created a test to demonstrate the
issue (part of this patch). This test passes on linux and fails on
windows before my patch for mv.c is applied, and passes on both windows
and linux for me after my patch is applied.

The problem with changing the case of a file happens because git mv
checks if the destination filename exists before performing a
move/rename, and on windows lstat reports that the destination file
*does* already exist because it ignores case for this check and
semi-erroneously finds the source file.

The way I've attempted to fix it in my patch is by checking if the inode
of the source and destination are the same before deciding to fail with
a "destination exists" error.

When using "git mv" it is possible to work around the error by using
--force.

I've also seen a problem with windows users pulling from remotes where a
case only rename has been performed which is more problematic as you
have to tell every user how to handle the issue. I'm not sure if I've
managed to fix that case or not.

The fault exists in both the current cygwin git and the current msysgit,
so I figured it would be good to get a patch to upstream (you) so that
it could work everywhere. 

I found an email from Linus relating to this issue here:
http://marc.info/?l=git&m=120612172706823 so it's a known problem.

Thanks

Tim Abell

---
 builtin/mv.c  |   33 ++++++++++++++++++++++-----------
 t/t7001-mv.sh |    9 +++++++++
 2 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 93e8995..6bb262e 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -63,6 +63,7 @@ int cmd_mv(int argc, const char **argv, const char
*prefix)
 	const char **source, **destination, **dest_path;
 	enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;
 	struct stat st;
+       struct stat src_st;
 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
 
 	git_config(git_default_config, NULL);
@@ -165,17 +166,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 files= system
(windows) http://is.gd/kyxgg
+                        * 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.
+                        */
+                       lstat(src, &src_st); //get file serial number
(inode) for source
+                       #warning("src inode: %s, dst inode: %s",
src_st.st_ino, st.st_ino);
+                       if (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 a845b15..95146bf 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -255,4 +255,13 @@ 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
+'
+
 test_done
-- 
1.5.6.5

Re: [PATCH] handle rename of case only, for windows

[Erik Faye-Lund]

On Fri, Jan 14, 2011 at 2:41 PM, Tim Abell <tim@timwise.co.uk> wrote:
> Hi folks,
>
> I've never contributed to git before so be gentle :-)
>
> Would someone have the time to help me get this patch into mailine git?
>

First of all, welcome!

There are some problems with your patch that aren't directly related
to the code:
- It's become white-space damaged, most likely from when you e-mailed
it. Perhaps you could try again with git-send-email?
- There's no real commit-message. This e-mail description isn't really
suited as a commit message as it is, IMO. It might just be a matter of
snipping away some stuff, though.
- The patch lacks a sign-off
- Since this is a Windows-issue, it would be nice if you CC'ed
msysgit@googlegroups.com as well. I've done that for now.

I suggest you read through Documentation/SubmittingPatches to get to
know the process.

> I tripped over a failure to rename files on windows when only the case
> has changed. I've created a patch which fixes it for me and doesn't seem
> to break on linux or windows. I also created a test to demonstrate the
> issue (part of this patch). This test passes on linux and fails on
> windows before my patch for mv.c is applied, and passes on both windows
> and linux for me after my patch is applied.
>
> The problem with changing the case of a file happens because git mv
> checks if the destination filename exists before performing a
> move/rename, and on windows lstat reports that the destination file
> *does* already exist because it ignores case for this check and
> semi-erroneously finds the source file.
>
<snip>
> When using "git mv" it is possible to work around the error by using
> --force.
>

Your reasoning seems to match what we've discussed on the msysGit
mailing list. Good work, and a clear description.

> The way I've attempted to fix it in my patch is by checking if the inode
> of the source and destination are the same before deciding to fail with
> a "destination exists" error.
>

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

> The fault exists in both the current cygwin git and the current msysgit,
> so I figured it would be good to get a patch to upstream (you) so that
> it could work everywhere.
>

It also affects MacOS X, AFAIK. So yes, it'd be good for upstream.

> ---
>  builtin/mv.c  |   33 ++++++++++++++++++++++-----------
>  t/t7001-mv.sh |    9 +++++++++
>  2 files changed, 31 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 93e8995..6bb262e 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -63,6 +63,7 @@ int cmd_mv(int argc, const char **argv, const char
> *prefix)
>        const char **source, **destination, **dest_path;
>        enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;
>        struct stat st;
> +       struct stat src_st;

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

And if not, perhaps you could piggy-back on the st-definition, like this:
-       struct stat st;
+       struct stat st, src_st;

>        struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
>
>        git_config(git_default_config, NULL);
> @@ -165,17 +166,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 files= system
> (windows) http://is.gd/kyxgg

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

> +                        * 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.
> +                        */
> +                       lstat(src, &src_st); //get file serial number
> (inode) for source
> +                       #warning("src inode: %s, dst inode: %s",
> src_st.st_ino, st.st_ino);

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

Blah, it's too tiresome to review this white-space broken version, and
I seen now that you have re-posted a non-broken version. Thanks!

[PATCH] handle rename of case only, for windows

[Tim Abell]

Hi folks,

I've never contributed to git before so be gentle :-)

Would someone have the time to help me get this patch into mailine git?

I tripped over a failure to rename files on windows when only the case
has changed. I've created a patch which fixes it for me and doesn't seem
to break on linux or windows. I also created a test to demonstrate the
issue (part of this patch). This test passes on linux and fails on
windows before my patch for mv.c is applied, and passes on both windows
and linux for me after my patch is applied.

The problem with changing the case of a file happens because git mv
checks if the destination filename exists before performing a
move/rename, and on windows lstat reports that the destination file
*does* already exist because it ignores case for this check and
semi-erroneously finds the source file.

The way I've attempted to fix it in my patch is by checking if the inode
of the source and destination are the same before deciding to fail with
a "destination exists" error.

When using "git mv" it is possible to work around the error by using
--force.

I've also seen a problem with windows users pulling from remotes where a
case only rename has been performed which is more problematic as you
have to tell every user how to handle the issue. I'm not sure if I've
managed to fix that case or not.

The fault exists in both the current cygwin git and the current msysgit,
so I figured it would be good to get a patch to upstream (you) so that
it could work everywhere. 

I found an email from Linus relating to this issue here:
http://marc.info/?l=git&m=120612172706823 so it's a known problem.

Thanks

Tim Abell

---
 builtin/mv.c  |   33 ++++++++++++++++++++++-----------
 t/t7001-mv.sh |    9 +++++++++
 2 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 93e8995..6bb262e 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -63,6 +63,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	const char **source, **destination, **dest_path;
 	enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;
 	struct stat st;
+	struct stat src_st;
 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
 
 	git_config(git_default_config, NULL);
@@ -165,17 +166,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 files= system (windows) http://is.gd/kyxgg
+			 * 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.
+			 */
+			lstat(src, &src_st); //get file serial number (inode) for source
+			#warning("src inode: %s, dst inode: %s", src_st.st_ino, st.st_ino);
+			if (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 a845b15..95146bf 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -255,4 +255,13 @@ 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
+'
+
 test_done
-- 
1.5.6.5

Re: [PATCH] handle rename of case only, for windows

[Nguyen Thai Ngoc Duy]

On Fri, Jan 14, 2011 at 8:44 PM, Tim Abell <tim@timwise.co.uk> wrote:
>                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 files= system (windows) http://is.gd/kyxgg
> +                        * 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.
> +                        */
> +                       lstat(src, &src_st); //get file serial number (inode) for source
> +                       #warning("src inode: %s, dst inode: %s", src_st.st_ino, st.st_ino);
> +                       if (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";

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

[PATCH] Handle rename of case only, for Windows

[Tim Abell]

>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

Added test to show rename failing in windows.
Added test to show that this patch doesn't break protection against overwriting
an existing file, and another to show that "mv --force" still works.

Altered mv.c to check whether the source and destination file are actually
the same file based on inode number before failing.
If the inode is zero then the data is no use so old behaviour is maintained.

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

Hi folks, this is my second attempt at providing a useful patch for this issue.
Thanks for all the great feedback from my first attempt. I've attempted to address the problems raised.

Hopefully my commit message is more like what is needed this time.
I hadn't realised before that you can split the commit message from this bit with the "---".

>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

Re: [PATCH] Handle rename of case only, for Windows

[René Scharfe]

Am 30.01.2011 00:45, schrieb Tim Abell:
>> 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.

On MinGW on Vista st_ino is zero, git mv refuses to overwrite and the
added case change test fails as a consequence.

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

Hmm, if the patch only works for Cygwin and maybe OS X it's a step
forward, I guess.  A more complete solution would be better, of course.

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

Can you make the lines 80 characters long at most?  This subtly helps
avoiding excessive levels of indentation by encouraging to factor out
nice and small functions.

> +			struct stat src_st;
> +			lstat(src, &src_st);

Shouldn't you check the return value of this call?  OK, the source
probably always exists, but still.  Oh, we actually know that because
that's the first lstat() call in this for loop.  You can reuse its
result instead of calling the function again.

> +			if (src_st.st_ino == 0 || src_st.st_ino != st.st_ino) {

It may be nice to avoid adding another level of indentation by combining
this if with the preceding one.

> +				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";

René

Re: [PATCH] Handle rename of case only, for Windows

[Tim Abell]

> +			struct stat src_st;
> +			lstat(src, &src_st);

Shouldn't you check the return value of this call?  OK, the source
probably always exists, but still.  Oh, we actually know that because
that's the first lstat() call in this for loop.  You can reuse its
result instead of calling the function again.

Thanks for your feedback, you are undoubtedly right about checking the return value, I hadn't though of that. Just out of interest I tried moving a file with git mv that i'd removed from the filesystem. Here's the result, so at least it's not dying horribly:

  $ rm foo.txt

  $ git status
  # On branch master
  # Changed but not updated:
  #   (use "git add/rm <file>..." to update what will be committed)
  #   (use "git checkout -- <file>..." to discard changes in working directory)
  #
  #       deleted:    foo.txt
  #
  no changes added to commit (use "git add" and/or "git commit -a")

  $ git mv foo.txt bar.txt
  fatal: bad source, source=foo.txt, destination=bar.txt

--

What do I need to do from here to make my patch acceptable/useful to git's maintainers?

Thanks

Tim Abell

Re: [PATCH] Handle rename of case only, for Windows

[Jonathan Nieder]

Hi Tim,

Tim Abell wrote:

> Thanks for your feedback, you are undoubtedly right about checking
> the return value
[...]
> What do I need to do from here to make my patch acceptable/useful to
> git's maintainers?

Generic answer: see Documentation/SubmittingPatches, section labelled
"An ideal patch flow".  Basically you (or anyone else interested it
getting it accepted) are the maintainer of the patch, so you can make
improvements until you and others are happy with it.

Hope that helps,
Jonathan

Re: [PATCH] Handle rename of case only, for Windows

[Ramsay Jones]

[Sorry for the late reply to this... I'm somewhat backed-up!]

Tim Abell wrote:
>>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

This should not work with the MinGW or msvc build, since they *always* set
the st_ino to zero (see do_lstat() and mingw_fstat() in compat/mingw.c).
Also, *with default configuration*, it will also not work on cygwin... so
saying that this works on Windows may be overstating things a bit ... ;-)

[Hmm, I can't check, but it would probably work on Mac OS X ...]

> Added test to show rename failing in windows.

This fails for me. (er... just to be clear, test #30 fails with your patch
applied).

> Added test to show that this patch doesn't break protection against overwriting
> an existing file, and another to show that "mv --force" still works.
> 
> Altered mv.c to check whether the source and destination file are actually
> the same file based on inode number before failing.
> If the inode is zero then the data is no use so old behaviour is maintained.

With default configuration, the st_ino will always be zero on cygwin. (see for
example the "schizophrenic l/stat() functions" commits adbc0b6, 7faee6b and
7974843, along with the "force core.filemode" commit c869753).

So, you must have core.filemode or core.ignorecygwinfstricks set somewhere in
your environment (in the system or global (user) config files?).

[I *always* set core.filemode after git-init or git-clone, so this patch may
actually work for me *in practice* (I haven't tested it), but I may well not
be a typical user...]

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

Again, you must have non-default configuration set...

ATB,
Ramsay Jones