From: Torsten Bögershausen <tboegi@web.de>
Date: Thu, 08 May 2014 08:55:28 GMT
Subject: Re: [PATCH 2/2] ignorecase: Fix git mv on insensitive filesystems
Message-ID: <536B4680.1010806@web.de>
In-Reply-To: <536B2637.2060808@viscovery.net>

On 05/08/2014 08:37 AM, Johannes Sixt wrote:
> Am 5/7/2014 19:46, schrieb Junio C Hamano:
>> David Turner <dturner@twopensource.com> writes:
>>
>>> On Wed, 2014-05-07 at 08:17 +0200, Johannes Sixt wrote:
>>>>>   		} else if (cache_name_pos(src, length) < 0)
>>>>>   			bad = _("not under version control");
>>>>> -		else if (lstat(dst, &st) == 0) {
>>>>> +		else if (lstat(dst, &dst_st) == 0 &&
>>>>> +			 (src_st.st_ino != dst_st.st_ino ||
>>>>> +			  (src_st.st_ino == 0 && strcasecmp(src, dst)))) {
>>>> Don't do that. st_ino is zero on Windows only because we do not spend time
>>>> to fill in the field. Don't use it as an indicator for a case-insensitive
>>>> file system; zero may be a valid inode number on other systems.
>>> I don't think it is a problem if zero is a valid inode.  The only thing
>>> that happens when there is a zero inode, is that we have to compare
>>> filenames.  The inode check is just an optimization to avoid doing a
>>> bunch of strcasecmp on systems that don't have to.
>> Am I correct to rephrase you that the code assumes that any
>> filesystem that cannot give unique inum to different files will use
>> 0 as the placeholder inum, so if src/dst share the same non-zero
>> inum, it is guaranteed that is not a placeholder and we know they
>> are different files without comparing the two paths?
> "If src and dst share the same non-zero inum, it is guaranteed that it is
> not a placeholder and we know they are the same file" is more correct.
>
> What if both inums are zero? Can this happen on any sane POSIX system? I
> don't know, but my gut feeling is that inode zero is too special to be
> allocated for files or directories.
>
> In that case, it is safe to assume that the st_ino field is just a
> placeholder when it is zero, and we have to compare the file name. Then we
> can either assume that this happens only for our emulation layer on MinGW
> (and the comparison can be case-insensitive) or choose the comparison mode
> based on core.ignorecase. This patch does the former, but I think we
> should do the latter.
Whatever we do, we should "protect" the strcasecmp() with ignore_case:

!ignore_case || strcasecmp(src, dst)

(And once that is done, you don't need to look at st_ino at all)