Re: RFA: untracked cache vs git reset --hard

From: Duy Nguyen <pclouds@gmail.com>
Date: May 3, 2017, 10:54 UTC
Message-ID: <CACsJy8BasKLSuMuoqT1MNWbp93qxuG1Z+auiM6SaN7fBYT8sFw@mail.gmail.com>
In-Reply-To: <alpine.DEB.2.20.1705031202470.3480@virtualbox>

On Wed, May 3, 2017 at 5:27 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 16 quoted lines

> Hi all,
>
> I have a problem and would like to solicit advice how to fix it.
>
> The untracked cache has made a real difference on rather large
> repositories with tons of directories, and it is really, really useful.
>
> But. One innocuous `git reset --hard` will just blow it away.
>
> How? reset_index() calls unpack_trees() which in turn tries to populate a
> new index and then discards the old one:
>
>         https://github.com/git/git/blob/v2.12.2/unpack-trees.c#L1293
>
> That discard_index() unfortunately also blows away each and every index
> extension that had been read carefully before.

This is a real problem when we introduce non-optional extensions (i.e. extension name in lower case). Dropping them is not an option because they may contain vital/original information. We haven't any so far, but I've been wanting to add one for years (narrow clone). So I'm all for tackling the problem now :)

Show 8 quoted lines

> All users of `git reset --hard` (including `git stash`) suffer this.
>
> In fact, it looks as if *any* caller of unpack_trees() would suffer the
> same problem: git-am, git-checkout, git-commit, git-merge, etc
>
> Now, I could imagine that maybe we could just "move"
> o->dst_index.untracked to o->result.untracked, and that the machinery then
> would do the right thing.

These extensions may have dependencies in the o->result.cache[] (do we allow an extension to depend on another?). If invalidation is not handled correctly then it's not safe to simply copy the extension over.

For untracked cache, I think we do invalidation right and just moving it over dst_index (and resetting NULL in o->result so it does not get accidentally deleted) is fine.

I'd rather we have a common way of dealing with this for any extension though. Split index needs special treatment too [1]. Maybe we can add

int migrate_index_extensions(struct index_state *dst, struct index_state *src);

in read-cache.c where it calls migrate_XXX() for each extension. In some cases (cache-tree) we could even do more, like repair cache-tree there to avoid hitting performance regressions.

[1] https://github.com/git/git/blob/v2.12.2/unpack-trees.c#L1165-L1167

-- 
Duy