Re: [PATCH] Allow git mv FileA fILEa when core.ignorecase = true
- From
Torsten Bögershausen <tboegi@web.de>
- Date
- Mar 19, 2011, 14:28 UTC
- Message-ID
- <4D84BD8C.7020906@web.de>
- In-Reply-To
- <AANLkTikvkzUKBCqygGJoUG3qXNDArXpEX0RM5_5dbnBD@mail.gmail.com>
On 16.03.11 14:18, Erik Faye-Lund wrote:
Show 80 quoted lines
> 2011/3/16 Erik Faye-Lund <kusmabite@gmail.com>:
>> 2011/3/4 Torsten Bögershausen <tboegi@web.de>:
>>> The typical use case is when a file "FileA" should be renamed into fILEa
>>> and we are on a case insenstive file system (system core.ignorecase = true).
>>> Source and destination are the same file, it can be accessed under both names.
>>> This makes git think that the destination file exists.
>>> Unless used with --forced, git will refuse the "git mv FileA fILEa".
>>> This change will allow "git mv FileA fILEa", when core.ignorecase = true
>>> and source and destination filenames only differ in case and the file length
>>> is identical.
>>> On Linux/Unix/Mac OS X the mv is allowed when the inode of the source and
>>> destination are equal.
>>> On this allows renames of MÄRCHEN into Märchen on Mac OS X.
>>> (As a side effect, a file can be renamed to a name which is already
>>> hard-linked to the same inode)
>>>
>>> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
>>> ---
>>> builtin/mv.c | 20 +++++++++++++++-----
>>> t/t7001-mv.sh | 29 +++++++++++++++++++++++++++++
>>> 2 files changed, 44 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/builtin/mv.c b/builtin/mv.c
>>> index 93e8995..e0aad62 100644
>>> --- a/builtin/mv.c
>>> +++ b/builtin/mv.c
>>> @@ -62,7 +62,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 st, st_dst;
>>> struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
>>>
>>> git_config(git_default_config, NULL);
>>> @@ -164,15 +164,25 @@ 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) {
>>> + else if (lstat(dst, &st_dst) == 0) {
>>> + int allow_force = force;
>>> bad = "destination exists";
>>> - if (force) {
>>> + /* Allow when src and dst have the same inode (Mac OS X) */
>>> + /* Allow when ignore_case and same file length (Windows) */
>>
>> Wait, what? Same file length is sufficient to trigger overwriting
>> without -f? I find this to be a very dubious heuristic...
>>
>> Shouldn't you be checking something like nFileIndexLow/High from
>> BY_HANDLE_FILE_INFORMATION instead? (ref:
>> http://msdn.microsoft.com/en-us/library/aa363788(v=VS.85).aspx)
>>
>> Sure, we'd need some API to check that, but if we assume that this
>> code path is rare-ish we could do something like this (note,
>> untested):
>>
>> diff --git a/compat/mingw.c b/compat/mingw.c
>> index 6750e67..fee4113 100644
>> --- a/compat/mingw.c
>> +++ b/compat/mingw.c
>> @@ -299,6 +299,21 @@ void mingw_mark_as_git_dir(const char *dir)
>> "dotGitOnly" : "true"));
>> }
>>
>> +int is_same_file(const char *a, const char *b)
>> +{
>> + BY_HANDLE_FILE_INFORMATION hia, hib;
>> + HANDLE ha = OpenFileA(a, NULL, OF_READ),
>> + hb = OpenFileA(b, NULL, OF_READ);
>> + if (!ha || !hb ||
>> + !GetFileInformationByHandle(ha) ||
>> + !GetFileInformationByHandle(hb))
>> + return 0;
>> +
>
> And if couse:
> CloseHandle(ha);
> CloseHandle(hb);Good point. I will send a new patch, including your suggestion, but 50% different ;-) /Torsten