On Fri, Jan 14, 2011 at 2:41 PM, Tim Abell <tim@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx 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!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html