Re: [PATCH v3 4/3] read-cache: don't try to write index if we can't write shared index

[Thomas Gummerer <t.gummerer@gmail.com>]

On 01/14, Duy Nguyen wrote:
> On Sun, Jan 14, 2018 at 5:37 AM, Thomas Gummerer <t.gummerer@gmail.com> 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@gmail.com>
> > ---
> >  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