Re: [PATCH v2 2/2] Handle more file writes correctly in shared repos

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 11, 2016, 17:06 UTC
Message-ID: <xmqqtwmkxcz3.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqio30yur4.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 52 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> On Fri, 8 Jan 2016, Junio C Hamano wrote:
>>
>>> I think it is correct not to touch this codepath in this patch,
>>> because of the above two reasons, but more simply and generally, it
>>> is correct not to touch this codepath because core.sharedRepository
>>> is not about working tree files, and .rej is a file you use in your
>>> working tree.
>>
>> I am happy to adjust the log message, but I am pretty certain that the
>> `core.sharedRepository` setting actually also affects the working tree. At
>> least in my hands, calling
>>
>> 	git clone -c core.sharedRepository=group . test-shared
>>
>> results in all of the working tree files being group-writable.
>
> Interesting.  I have a suspicion that "clone" does not honor the
> configuration given that way, though.
>
>  $ umask 077
>  $ git clone -c core.sharedRepository=group ~/w/git.git sharedtest
>  $ cd sharedtest
>  $ ls -l COPYING .git/index
>  -rw------- 1 jch eng 18765 Jan 11 07:43 COPYING
>  -rw------- 1 jch eng 272037 Jan 11 07:43 .git/index
>
> Notice that the permission bits in the working tree is correct, but
> in the resulting .git/ they are bogus, so from this we cannot
> clearly see the reason why COPYING is not group-readable is because
> the checkout codepath (write_entry(), I think) is correctly omitting
> the call to adjust_perm(), or simply the configuration is ignored
> during the clone.
>
> With a workaround to ensure that checkout happens definitely after
> the configuration gets in effect by doing config and pull/checkout
> as two separate steps:
>
>  $ rm -fr sharedtest
>  $ umask 077
>  $ git init sharedtest && cd sharedtest
>  $ git config core.sharedRepository group
>  $ git pull ~/w/git.git/ master
>  $ ls -l COPYING .git/index
>  -rw------- 1 jch eng 18765 Jan 11 07:48 COPYING
>  -rw-rw---- 1 jch eng 272037 Jan 11 07:48 .git/index
>
> we can see that the configuration affects only the $GIT_DIR/ files
> and not working tree.
>
> So you found a bug in clone, I think ;-)

Having said all that, the above does not mean that I'll refuse to consider changing the semantics of core.sharedRepository in a future major version bump by doing adjust_perm() for working tree files, which we have deliberately chosen not to do in the current code.

But that is not within the scope of the patch we are discussing, and I am not convinced it is a good idea (I haven't heard either sides of arguments), so based on the current design, I think "we don't do fopen_for_writing() for working tree files" is a valid justification that is short-and-sweet for this patch.