Re: [PATCH 1/3] read-cache: use shared perms when writing shared index

[Ævar Arnfjörð Bjarmason]

On Thu, 22 Jun 2017 12:51:54 -0700 Junio wrote:

> > Let's fix that by using create_tempfile() instead of mks_tempfile()
> > to create the shared index file.
> >
> > ...
> > - fd = mks_tempfile(_sharedindex, 
> > git_path("sharedindex_XX"));
> > + fd = create_tempfile(_sharedindex, 
> > git_path("sharedindex_XX"));
>
> So we used to create a temporary file that made sure its name is
> unique but now we create sharedindex_XX with 6 X's literally
> at the end?

I'm looking at some of this again. Yeah that was a bug in Christian's
code, but interestingly if you just create literal sharedindex_XX
files (don't replace the X's) the whole test suite passes under
GIT_TEST_SPLIT_INDEX=true

That seems like a major blindspot, i.e. we don't seem to have tests that
stress test the case of >1 split index.

Re: [PATCH 1/3] read-cache: use shared perms when writing shared index

[Christian Couder]

On Thu, Jun 22, 2017 at 9:51 PM, Junio C Hamano  wrote:
> Christian Couder  writes:

>> Let's fix that by using create_tempfile() instead of mks_tempfile()
>> to create the shared index file.
>>
>> ...
>> - fd = mks_tempfile(_sharedindex, 
>> git_path("sharedindex_XX"));
>> + fd = create_tempfile(_sharedindex, 
>> git_path("sharedindex_XX"));
>
> So we used to create a temporary file that made sure its name is
> unique but now we create sharedindex_XX with 6 X's literally
> at the end?
>
> Doesn't mks_tempfile() family include a variant where you can give
> custom mode?  Better yet, perhaps you can call adjust_shared_perm()
> on the path _after_ seeing that mks_tempfile() succeeds (you can ask
> get_tempfile_path() which path to adjust, I presume)?

Yeah, adjust_shared_perm() is called after mks_tempfile() succeeds, in
the next version.

Re: [PATCH 1/3] read-cache: use shared perms when writing shared index

[Junio C Hamano]

Christian Couder  writes:

> Since f6ecc62dbf (write_shared_index(): use tempfile module, 2015-08-10)
> write_shared_index() has been using mks_tempfile() to create the
> temporary file that will become the shared index.
>
> But even before that, it looks like the functions used to create this
> file didn't call adjust_shared_perm(), which means that the shared
> index file has always been created with 600 permissions regardless
> of the shared permission settings.
>
> This means that on repositories created with `git init --shared=all`
> and using the split index feature one gets an error like:
>
> fatal: .git/sharedindex.a52f910b489bc462f187ab572ba0086f7b5157de: index file 
> open failed: Permission denied
>
> when another user performs any operation that reads the shared index.

Assuming that a "shared" repository setting should allow uses by
different users, the above analysis makes sense to me.

But the conclusion does not.

> Let's fix that by using create_tempfile() instead of mks_tempfile()
> to create the shared index file.
>
> ...
> - fd = mks_tempfile(_sharedindex, 
> git_path("sharedindex_XX"));
> + fd = create_tempfile(_sharedindex, 
> git_path("sharedindex_XX"));

So we used to create a temporary file that made sure its name is
unique but now we create sharedindex_XX with 6 X's literally 
at the end?

Doesn't mks_tempfile() family include a variant where you can give
custom mode?  Better yet, perhaps you can call adjust_shared_perm()
on the path _after_ seeing that mks_tempfile() succeeds (you can ask
get_tempfile_path() which path to adjust, I presume)?

>   if (fd < 0) {
>   hashclr(si->base_sha1);
>   return do_write_locked_index(istate, lock, flags);