Re: [PATCH v3] unpack_trees: fix breakage when o->src_index != o->dst_index
From: Duy Nguyen <hidden>
Date: 2018-04-30 14:43:05
On Sun, Apr 29, 2018 at 10:53 PM, Johannes Schindelin [off-list ref] wrote:
quoted
quoted
@@ -1412,12 +1422,13 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options WRITE_TREE_SILENT | WRITE_TREE_REPAIR); } - move_index_extensions(&o->result, o->dst_index); + move_index_extensions(&o->result, o->src_index);While this looks like the right thing to do on paper, I believe it's actually broken for a specific case of untracked cache. In short, please do not touch this line. I will send a patch to revert edf3b90553 (unpack-trees: preserve index extensions - 2017-05-08), which essentially deletes this line, with proper explanation and perhaps a test if I could come up with one. When we update the index, we depend on the fact that all updates must invalidate the right untracked cache correctly. In this unpack operations, we start copying entries over from src to result. Since 'result' (at least from the beginning) does not have an untracked cache, it has nothing to invalidate when we copy entries over. By the time we have done preparing 'result', what's recorded in src's (or dst's for that matter) untracked cache may or may not apply to 'result' index anymore. This copying only leads to more problems when untracked cache is used.Is there really no way to invalidate just individual entries?
Grr.... the short answer is the current code (i.e. without Elijah's changes) works but in a twisted way. So you get to keep untracked cache in the end. I was right about the invalidation stuff. I knew about invalidate_ce_path() in this file. What I didn't remember was this function actually invalidates entries from the _source_ index, not the result one. What kind of logic is that? You copy/move entries from source to result than you go invalidate the source. Since the original move_index_extensions() call moves extensions from the source, these are already properly invalidated (both untracked cache and cache tree), it it looks like it does the right thing. Two wrongs make a right, I guess. Sorry for venting. I was not happy with what I found. And sorry for wasting your time making this move_index.. change then remove it.
I have a couple of worktrees which are *huge*. And edf3b90553 really helped relieve the pain a bit when running `git status`. Now you say that even a `git checkout -b new-branch` would blow the untracked cache away again? It would be *really* nice if we could prevent that performance regression somehow. Ciao, Dscho
-- Duy