Re: [PATCH v4 02/23] read-cache.c: remove 'const' from index_has_changes()

Duy Nguyen [off-list ref] writes:

On Mon, Sep 17, 2018 at 6:25 PM Junio C Hamano [off-list ref] wrote:

quoted

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

quoted

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.