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

[Erik Faye-Lund <kusmabite@gmail.com>]

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!