{"thread":{"id":"26282","subject":"[PATCH] handle rename of case only, for windows","startedAt":"2011-01-14T13:41:01Z","lastAt":"2011-01-14T14:22:43Z","messageCount":2,"participants":["Tim Abell","Erik Faye-Lund"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"159484","messageId":"1295012461.7403.1415291765@webmail.messagingengine.com","threadId":"26282","inReplyTo":null,"subject":"[PATCH] handle rename of case only, for windows","fromName":"Tim Abell","fromEmail":"tim@timwise.co.uk","sentAt":"2011-01-14T13:41:01Z","receivedAt":"2011-01-14T13:41:01Z","isPatch":true,"sender":{"key":"tim@timwise.co.uk","avatar":"https://gravatar.com/avatar/0d3f826b8cc4813b0a373fbfaf402be87a89e01328c16065e5762406b8066fee?d=mp&s=160"},"body":"Hi folks,\n\nI've never contributed to git before so be gentle :-)\n\nWould someone have the time to help me get this patch into mailine git?\n\nI tripped over a failure to rename files on windows when only the case\nhas changed. I've created a patch which fixes it for me and doesn't seem\nto break on linux or windows. I also created a test to demonstrate the\nissue (part of this patch). This test passes on linux and fails on\nwindows before my patch for mv.c is applied, and passes on both windows\nand linux for me after my patch is applied.\n\nThe problem with changing the case of a file happens because git mv\nchecks if the destination filename exists before performing a\nmove/rename, and on windows lstat reports that the destination file\n*does* already exist because it ignores case for this check and\nsemi-erroneously finds the source file.\n\nThe way I've attempted to fix it in my patch is by checking if the inode\nof the source and destination are the same before deciding to fail with\na \"destination exists\" error.\n\nWhen using \"git mv\" it is possible to work around the error by using\n--force.\n\nI've also seen a problem with windows users pulling from remotes where a\ncase only rename has been performed which is more problematic as you\nhave to tell every user how to handle the issue. I'm not sure if I've\nmanaged to fix that case or not.\n\nThe fault exists in both the current cygwin git and the current msysgit,\nso I figured it would be good to get a patch to upstream (you) so that\nit could work everywhere. \n\nI found an email from Linus relating to this issue here:\nhttp://marc.info/?l=git&m=120612172706823 so it's a known problem.\n\nThanks\n\nTim Abell\n\n---\n builtin/mv.c  |   33 ++++++++++++++++++++++-----------\n t/t7001-mv.sh |    9 +++++++++\n 2 files changed, 31 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 93e8995..6bb262e 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -63,6 +63,7 @@ int cmd_mv(int argc, const char **argv, const char\n*prefix)\n \tconst char **source, **destination, **dest_path;\n \tenum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;\n \tstruct stat st;\n+       struct stat src_st;\n \tstruct string_list src_for_dst = STRING_LIST_INIT_NODUP;\n \n \tgit_config(git_default_config, NULL);\n@@ -165,17 +166,27 @@ int cmd_mv(int argc, const char **argv, const char\n*prefix)\n \t\t} else if (cache_name_pos(src, length) < 0)\n \t\t\tbad = \"not under version control\";\n \t\telse if (lstat(dst, &st) == 0) {\n-                       bad = \"destination exists\";\n-                       if (force) {\n-                               /*\n-                                * only files can overwrite each other:\n-                                * check both source and destination\n-                                */\n-                               if (S_ISREG(st.st_mode) ||\nS_ISLNK(st.st_mode)) {\n-                                       warning(\"%s; will overwrite!\",\nbad);\n-                                       bad = NULL;\n-                               } else\n-                                       bad = \"Cannot overwrite\";\n+                       /* If we are on a case insensitive files= system\n(windows) http://is.gd/kyxgg\n+                        * and we are only changing the case of the file\nthen lstat for the\n+                        * destination will return != 0 because it sees\nthe source file.\n+                        * To prevent this causing failure, lstat is\nused to get the inode of the src\n+                        * and see if it's actually the same file.\n+                        */\n+                       lstat(src, &src_st); //get file serial number\n(inode) for source\n+                       #warning(\"src inode: %s, dst inode: %s\",\nsrc_st.st_ino, st.st_ino);\n+                       if (src_st.st_ino != st.st_ino) {\n+                               bad = \"destination exists\";\n+                               if (force) {\n+                                       /*\n+                                        * only files can overwrite each\nother:\n+                                        * check both source and\ndestination\n+                                        */\n+                                       if (S_ISREG(st.st_mode) ||\nS_ISLNK(st.st_mode)) {\n+                                               warning(\"%s; will\noverwrite!\", bad);\n+                                               bad = NULL;\n+                                       } else\n+                                               bad = \"Cannot\noverwrite\";\n+                               }\n \t\t\t}\n \t\t} else if (string_list_has_string(&src_for_dst, dst))\n \t\t\tbad = \"multiple sources for the same target\";\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex a845b15..95146bf 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -255,4 +255,13 @@ test_expect_success SYMLINKS 'git mv should\noverwrite file with a symlink' '\n \n rm -f moved symlink\n \n+test_expect_success 'git mv should not fail when only changing case' '\n+\n+       rm -fr .git &&\n+       git init &&\n+       >foo.txt &&\n+       git add foo.txt &&\n+       git mv foo.txt Foo.txt\n+'\n+\n test_done\n-- \n1.5.6.5\n"},{"id":"159488","messageId":"AANLkTi=R0bkZgUjn01A0ZLZBad2ERGOfQneqBMEPTa4j@mail.gmail.com","threadId":"26282","inReplyTo":"1295012461.7403.1415291765@webmail.messagingengine.com","subject":"Re: [PATCH] handle rename of case only, for windows","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-01-14T14:22:43Z","receivedAt":"2011-01-14T14:22:43Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Jan 14, 2011 at 2:41 PM, Tim Abell <tim@timwise.co.uk> wrote:\n> Hi folks,\n>\n> I've never contributed to git before so be gentle :-)\n>\n> Would someone have the time to help me get this patch into mailine git?\n>\n\nFirst of all, welcome!\n\nThere are some problems with your patch that aren't directly related\nto the code:\n- It's become white-space damaged, most likely from when you e-mailed\nit. Perhaps you could try again with git-send-email?\n- There's no real commit-message. This e-mail description isn't really\nsuited as a commit message as it is, IMO. It might just be a matter of\nsnipping away some stuff, though.\n- The patch lacks a sign-off\n- Since this is a Windows-issue, it would be nice if you CC'ed\nmsysgit@googlegroups.com as well. I've done that for now.\n\nI suggest you read through Documentation/SubmittingPatches to get to\nknow the process.\n\n> I tripped over a failure to rename files on windows when only the case\n> has changed. I've created a patch which fixes it for me and doesn't seem\n> to break on linux or windows. I also created a test to demonstrate the\n> issue (part of this patch). This test passes on linux and fails on\n> windows before my patch for mv.c is applied, and passes on both windows\n> and linux for me after my patch is applied.\n>\n> The problem with changing the case of a file happens because git mv\n> checks if the destination filename exists before performing a\n> move/rename, and on windows lstat reports that the destination file\n> *does* already exist because it ignores case for this check and\n> semi-erroneously finds the source file.\n>\n<snip>\n> When using \"git mv\" it is possible to work around the error by using\n> --force.\n>\n\nYour reasoning seems to match what we've discussed on the msysGit\nmailing list. Good work, and a clear description.\n\n> The way I've attempted to fix it in my patch is by checking if the inode\n> of the source and destination are the same before deciding to fail with\n> a \"destination exists\" error.\n>\n\nHmm, not so good. st_ino is always 0 on Windows, so this would make\nfalse positives, no?\n\n> The fault exists in both the current cygwin git and the current msysgit,\n> so I figured it would be good to get a patch to upstream (you) so that\n> it could work everywhere.\n>\n\nIt also affects MacOS X, AFAIK. So yes, it'd be good for upstream.\n\n> ---\n>  builtin/mv.c  |   33 ++++++++++++++++++++++-----------\n>  t/t7001-mv.sh |    9 +++++++++\n>  2 files changed, 31 insertions(+), 11 deletions(-)\n>\n> diff --git a/builtin/mv.c b/builtin/mv.c\n> index 93e8995..6bb262e 100644\n> --- a/builtin/mv.c\n> +++ b/builtin/mv.c\n> @@ -63,6 +63,7 @@ int cmd_mv(int argc, const char **argv, const char\n> *prefix)\n>        const char **source, **destination, **dest_path;\n>        enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;\n>        struct stat st;\n> +       struct stat src_st;\n\nCouldn't this be moved inside the scope around \"cache_name_pos\"?\nThat's the only scope it is valid inside anyway...\n\nAnd if not, perhaps you could piggy-back on the st-definition, like this:\n-       struct stat st;\n+       struct stat st, src_st;\n\n>        struct string_list src_for_dst = STRING_LIST_INIT_NODUP;\n>\n>        git_config(git_default_config, NULL);\n> @@ -165,17 +166,27 @@ int cmd_mv(int argc, const char **argv, const char\n> *prefix)\n>                } else if (cache_name_pos(src, length) < 0)\n>                        bad = \"not under version control\";\n>                else if (lstat(dst, &st) == 0) {\n> -                       bad = \"destination exists\";\n> -                       if (force) {\n> -                               /*\n> -                                * only files can overwrite each other:\n> -                                * check both source and destination\n> -                                */\n> -                               if (S_ISREG(st.st_mode) ||\n> S_ISLNK(st.st_mode)) {\n> -                                       warning(\"%s; will overwrite!\",\n> bad);\n> -                                       bad = NULL;\n> -                               } else\n> -                                       bad = \"Cannot overwrite\";\n> +                       /* If we are on a case insensitive files= system\n> (windows) http://is.gd/kyxgg\n\nPerhaps you could use the full URL (and maybe put it in the commit\nmessage insted)? It'd be nice if we could reach this information even\nif is.gd disappears...\n\n> +                        * and we are only changing the case of the file\n> then lstat for the\n> +                        * destination will return != 0 because it sees\n> the source file.\n> +                        * To prevent this causing failure, lstat is\n> used to get the inode of the src\n> +                        * and see if it's actually the same file.\n> +                        */\n> +                       lstat(src, &src_st); //get file serial number\n> (inode) for source\n> +                       #warning(\"src inode: %s, dst inode: %s\",\n> src_st.st_ino, st.st_ino);\n\nUhm, is this debug-leftovers? #warning is a preprocessor-construct,\nand it can't understand varaibles in c. Especially not formatted as\nstrings. Can #warning even do varags? :P\n\nBlah, it's too tiresome to review this white-space broken version, and\nI seen now that you have re-posted a non-broken version. Thanks!\n"}]}