Re: [PATCH 2/2] config: use chmod() instead of fchmod()

From: Karsten Blees <karsten.blees@gmail.com>
Date: Jul 17, 2014, 08:57 UTC
Message-ID: <53C79016.2020303@gmail.com>
In-Reply-To: <xmqqlhrtq7a7.fsf@gitster.dls.corp.google.com>

Am 17.07.2014 00:16, schrieb Junio C Hamano:

Show 15 quoted lines

> Karsten Blees <karsten.blees@gmail.com> writes:
> 
>> There is no fchmod() on native Windows platforms (MinGW and MSVC), and the
>> equivalent Win32 API (SetFileInformationByHandle) requires Windows Vista.
>>
>> Use chmod() instead.
>>
>> Signed-off-by: Karsten Blees <blees@dcon.de>
>> ---
> 
> I am wondering if it is saner to just revert the fchmod() patch and
> replace it with something along the lines of
> 
> http://thread.gmane.org/gmane.comp.version-control.git/251682/focus=253219
>

I also think it makes a lot of sense to handle permissions centrally.

However, with this patch, the permissions of the target file will additionally be limited by umask (by passing them to open()), and then overridden completely if core.sharedRepository is set.

Perhaps the lockfile API should respect the location of the lock files (i.e. use core.sharedRepository in .git, 0666 in the work-tree, and copy permissions anywhere else).

Another thing I find strange is that, by doing copy/replace, git silently overwrites readonly files. If we grab the permissions from the source file anyway, we should perhaps add 'if (!(perms & 0222)) error("file is readonly");', or even 'access(filename, W_OK)'?

> Having said that, these are the only two callers of fchmod()
> currently in our code base, so I'll queue this patch to allow us to
> kick the problem-can down the road ;-)
>

Thanks.