Re: [PATCH 2/2] diff_index: honor in-index, not working-tree, .gitattributes
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:52:06
Jay Soffian [off-list ref] writes:
quoted hunk
This area of git is still black magic to me. My best guess is something like this:diff --git a/tree-diff.c b/tree-diff.c index b3cc2e4753..6fd84eb2bb 100644 --- a/tree-diff.c +++ b/tree-diff.c@@ -280,6 +282,19 @@ int diff_tree_sha1(const unsigned char *old,const unsigned char *new, const cha die("unable to read destination tree (%s)", sha1_to_hex(new)); init_tree_desc(&t1, tree1, size1); init_tree_desc(&t2, tree2, size2); + + if (is_bare_repository()) { + struct unpack_trees_options unpack_opts; + memset(&unpack_opts, 0, sizeof(unpack_opts)); + unpack_opts.index_only = 1; + unpack_opts.head_idx = -1; + unpack_opts.src_index = &the_index; + unpack_opts.dst_index = &the_index; + unpack_opts.fn = oneway_merge; + if (unpack_trees(1, DIFF_OPT_TST(opt, REVERSE_DIFF) ? &t1 : &t2, &unpack_opts) == 0) + git_attr_set_direction(GIT_ATTR_INDEX, &the_index); + }
This is hooking at too low a level in the callchain. diff_tree_sha1() is
meant to be a general purpose "I have two tree-ish objects and I want the
comparison machinery to work on them" library function [*1*].
- One of the more important uses is the history simplification done
during revision traversal by checking if the subtrees and the blobs
have the same SHA-1, and we should not pay penalty of reading the index
for each and every tree here.
- The caller may be using the index for its own purposes, and your use of
"the_index" here will break them.
If you want to allow use of in-tree attributes in _all_ callers of
diff_tree_sha1(), then the right approach is to add an instance of "struct
index_state" to "struct diff_options", have the caller _explicitly_ ask
for use of in-tree attributes by setting a bit somewhere in "struct
diff_options", and read the tree into that separate index_state using
tree.c::read_tree(). I however doubt it is worth it.
I would think it makes more sense to add a codeblock like that at the
beginning of builtin/diff.c::builtin_diff_tree() when a new command option
asks for it. In that codepath, you _know_ that we are not using the index
at all, and reading the index there will not interfere with other uses of
the index in the program.
[Footnote]
*1* which means that it is not a good justification to say "no current
caller is broken by this change". We need to make the library usable
for future callers.