Re: [PATCH 2/2] diff_index: honor in-index, not working-tree, .gitattributes

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 23, 2011, 16:44 UTC
Message-ID: <7vobybw6mv.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CAG+J_Dyh=t2VAZ6rAqcF2meEgBCN5c+J_m_YvVQbKfvXeJ8WGA@mail.gmail.com>

Jay Soffian <jaysoffian@gmail.com> writes:

Show 25 quoted lines

> 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.