Jeff King <peff@xxxxxxxx> writes: > 4. At the end of unpack_trees, we forget about src_index, and copy > o->result into *o->dst_index byte for byte. I.e., we overwrite > the_index.cache_tree, which has been properly updated the whole > time, I strongly suspect that "properly updated" part needs to be thoroughly audited. I wouldn't be surprised that this behaviour is what we did when we split src_index vs dst_index when he rewrote unpack_trees() in order to emulate the original "unpack-trees is beyond salvation because it does not maintain cache tree correctly, just nuke it" behaviour. > But it does not actually insert the _destination_ tree into the cache > tree. Which we can do in certain situations, but only if there were no > paths in the tree that were left unchanged (e.g., you modify "foo", then > "git checkout HEAD^", which updates "bar". Your tree does not match > HEAD^, and must be invalidated). While it would be cool to be able to > handle those complex cases,... It may look cool but it may not be a good change. You are spending extra cycles to optimize for the next write-tree that may not happen before the index is further updated. > I think this implementation matches the intent of the original calls to > cache_tree_invalidate_path sprinkled throughout unpack-trees.c. Yes, and as long as we invalidate all the directories that need to be invalidated during the unpack-tree operation, I think it is a correct thing to do. > But I > have to say that it seems a little odd for us to be modifying the > o->src_index throughout the whole thing. Yes, that part is logically *wrong*. I think it is a remnant from the days when there was no distinction between src_index and dst_index. > I would think the right thing > would be to make a deep copy of o->src_index->cache_tree into > o->result.cache_tree as the very first thing, and then update > o->result.cache_tree throughout the tree traversal. Yes. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html