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.
> 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..
--
Duy
