| Message ID | 20180915161759.8272-3-pclouds@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,01/23] archive.c: remove implicit dependency the_repository | expand |
Nguyễn Thái Ngọc Duy <pclouds@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?
On Mon, Sep 17, 2018 at 6:25 PM Junio C Hamano <gitster@pobox.com> wrote: > > Nguyễn Thái Ngọc Duy <pclouds@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).
Duy Nguyen <pclouds@gmail.com> writes: > On Mon, Sep 17, 2018 at 6:25 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> Nguyễn Thái Ngọc Duy <pclouds@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.
diff --git a/cache.h b/cache.h index 4d014541ab..260e4ee44a 100644 --- a/cache.h +++ b/cache.h @@ -703,7 +703,7 @@ extern int unmerged_index(const struct index_state *); * provided, the space-separated list of files that differ will be appended * to it. */ -extern int index_has_changes(const struct index_state *istate, +extern int index_has_changes(struct index_state *istate, struct tree *tree, struct strbuf *sb); diff --git a/read-cache.c b/read-cache.c index 7b1354d759..86134e56a6 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2122,7 +2122,7 @@ int unmerged_index(const struct index_state *istate) return 0; } -int index_has_changes(const struct index_state *istate, +int index_has_changes(struct index_state *istate, struct tree *tree, struct strbuf *sb) {
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 const index_state there. Just remove 'const'. (*) Right now diff_cache() in diff-lib.c assigns the_index to src_index. But the plan is to get rid of the_index, so it should be 'istate' from here that gets assigned to src_index. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- cache.h | 2 +- read-cache.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)