Re: [PATCH 1/3] read-cache: use shared perms when writing shared index
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(&temporary_sharedindex, > > git_path("sharedindex_XX")); > > + fd = create_tempfile(&temporary_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
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(&temporary_sharedindex, >> git_path("sharedindex_XX")); >> + fd = create_tempfile(&temporary_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
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(&temporary_sharedindex, > git_path("sharedindex_XX")); > + fd = create_tempfile(&temporary_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);