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

Junio C Hamano [off-list ref] writes:

Johannes Schindelin [off-list ref] writes:

quoted

On Fri, 8 Jan 2016, Junio C Hamano wrote:

quoted

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.