Duy Nguyen <pclo...@gmail.com> writes: > On Mon, Sep 17, 2018 at 6:25 PM Junio C Hamano <gits...@pobox.com> wrote: >> >> Nguyễn Thái Ngọc Duy <pclo...@gmail.com> writes: >> >> > This function calls do_diff_cache() which eventually needs to set this >> > "istate" to unpack_options->src_index (*). This is an unfortunate fact >> > that unpack_trees() _will_ destroy src_index so we can't really pass a >> >> Wasn't the whole point of introducing src_index and dst_index to >> unpack-trees API so that we can keep the src_index read-only by >> writing the result of merge to a separate in-core dst_index? >> >> What does the above exactly mean by "will destroy src_index"? Is it >> now fundamental that src_index needs to lack constness, or is it >> something easy to fix? > > "destroy" is probably a strong word, but we do modify the src_index, e.g. > > mark_all_ce_unused(o->src_index); > mark_new_skip_worktree(.., o->src_index... > move_index_extensions(&o->result, o->src_index); > invalidate_ce_path(); > > all these update the source index. It is possible to fix, but I don't > think it's exactly easy and may even incur some performance cost (e.g. > if we stop modify ce_flags in the src_index, then we need to do one > extra lookup to wherever we store these flags).
Ah, OK. I thought that we'd be doing something, if write_tree() is called on src_index before and after these operation, to cause us to see two different trees, which made me worried. But what you mean is that transient bits in the istate or cache entries reachable from it are touched while we perform operations that are read-only in spirit, and we need to pass non const pointer around. The phrasing in the log message does need to be updated to avoid similar confusion by future readers, but I think I understand and agree with the direction/approach. Thanks.
