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

From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Jan 14, 2011, 14:22 UTC
Message-ID: <AANLkTi=R0bkZgUjn01A0ZLZBad2ERGOfQneqBMEPTa4j@mail.gmail.com>
In-Reply-To: <1295012461.7403.1415291765@webmail.messagingengine.com>

On Fri, Jan 14, 2011 at 2:41 PM, Tim Abell <tim@timwise.co.uk> wrote:

Show 6 quoted lines

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

Show 13 quoted lines

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

Show 15 quoted lines

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

Show 23 quoted lines

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

Show 12 quoted lines

> +                        * 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!