Hi Duy,

On Mon, Apr 30, 2018 at 7:45 AM, Duy Nguyen [off-list ref] wrote:

On Mon, Apr 30, 2018 at 4:42 PM, Duy Nguyen [off-list ref] wrote:

quoted

On Sun, Apr 29, 2018 at 10:53 PM, Johannes Schindelin
[off-list ref] wrote:

quoted

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.

GAAAHH.. it works _with_ Elijah's changes (since he made the change
from dst to src) not without (and no performance regression).

So...is that an Acked-by for the patch, or does the "two wrong make a
right, I guess" comment suggest that we should still drop the
move_index_extensions change (essentially reverting to v1 of the PATCH
as found at 20180421193736.12722-1-newren@gmail.com), and you'll fix
things up further in a separate series?

This file really messes my brain up.

I'm glad I'm not the only one.  :-)


Elijah