On 01/14, Duy Nguyen wrote:
> On Sun, Jan 14, 2018 at 5:37 AM, Thomas Gummerer <t.gummerer@xxxxxxxxx> wrote:
> > In a0a967568e ("update-index --split-index: do not split if $GIT_DIR is
> > read only", 2014-06-13), we tried to make sure we can still write an
> > index, even if the shared index can not be written.
> >
> > We did so by just calling 'do_write_locked_index()' from
> > 'write_shared_index()'. 'do_write_locked_index()' always at least
> > closes the tempfile nowadays, and used to close or commit the lockfile
> > if COMMIT_LOCK or CLOSE_LOCK were given at the time this feature was
> > introduced. COMMIT_LOCK or CLOSE_LOCK is passed in by most callers of
> > 'write_locked_index()'.
> >
> > After calling 'write_shared_index()', we call 'write_split_index()',
> > which calls 'do_write_locked_index()' again, which then tries to use the
> > closed lockfile again, but in fact fails to do so as it's already
> > closed.
> >
> > In the current version, git will in fact segfault if it can't create a
> > new file in $gitdir, and this feature seems to never have worked in the
> > first place.
> >
> > Ever since introducing the split index feature, nobody has complained
> > about this failing, and it really just papers over repositories that
> > will sooner or later need fixing anyway.
>
> Actually there's one valid case for this: you're accessing a read-only
> $GIT_DIR (.e.g maybe from a web server cgi script which may be run by
> user nobody or something) and creating a temporary index _outside_
> $GIT_DIR. I used to do this when I wanted to do "git grep" on some
> SHA-1 a couple times. Doing "git grep <SHA-1>" directly (a couple
> times) pays full cost for walking trees. If you prepare an index
> first, you pay it only once.
Makes sense, I didn't realize that usecase, thanks!
> > Therefore just make being unable to write the split index a proper
> > error, and have users fix their repositories instead of trying (but
> > failing) to paper over the error.
> >
> > Signed-off-by: Thomas Gummerer <t.gummerer@xxxxxxxxx>
> > ---
> > read-cache.c | 11 ++++-------
> > 1 file changed, 4 insertions(+), 7 deletions(-)
> >
> > diff --git a/read-cache.c b/read-cache.c
> > index d13ce83794..a9c8facdfd 100644
> > --- a/read-cache.c
> > +++ b/read-cache.c
> > @@ -2471,18 +2471,15 @@ static int clean_shared_index_files(const char *current_hex)
> > return 0;
> > }
> >
> > -static int write_shared_index(struct index_state *istate,
> > - struct lock_file *lock, unsigned flags)
> > +static int write_shared_index(struct index_state *istate)
> > {
> > struct tempfile *temp;
> > struct split_index *si = istate->split_index;
> > int ret;
> >
> > temp = mks_tempfile(git_path("sharedindex_XXXXXX"));
> > - if (!temp) {
> > - hashclr(si->base_sha1);
> > - return do_write_locked_index(istate, lock, flags);
>
> I think this code tries to do what's done near the beginning of
> write_locked_index() where we also bail out early:
>
> -- 8< --
> if (!si || alternate_index_output ||
> (istate->cache_changed & ~EXTMASK)) {
> if (si)
> hashclr(si->base_sha1);
> ret = do_write_locked_index(istate, lock, flags);
> goto out;
> }
> -- 8< --
>
> the only difference is it does not realize that it can't do "goto
> out;" like that code unless something goes wrong. I'll try to prepare
> a patch that move tempfile creation out of write_shared_index()
> instead. Patches coming in a bit..
Thanks for fixing this in a nicer way :)
> --
> Duy
