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

Jay Soffian [off-list ref] writes:

When diff'ing the index against a tree (using either diff-index
or diff --cached), git previously looked at .gitattributes in the
working tree before considering .gitattributes in the index, even
though the diff itself otherwise ignores the working tree.

We can take attributes only from one place (so far from the working tree
and perhaps from the index), people had to live within the limitation that
comes from the "single source only" semantics. It also happens to be
easier to understand (recall the complexity of the examples Jeff gave
about "textconv" during "diff" which ideally should apply from its own
side and "funcname", which does not even have a right answer).

In practice, because development progresses by making everything
(including the .gitattributes file) better, I think "use the newer one"
would be a good compromise when we have two possible sources to grab
attributes from but we can only use one source.

In that sense, I am somewhat skeptical about what this patch tries to
do. The working tree is where people make the progress to update the
index.

A related tangent.

I think the logical conclusion of assuming that we will keep the "single
source only" semantics (which I think we will, by the way, unless I hear a
concrete proposal to how we apply attributes from more than one sources in
what way to which side of the diff) is that a patch might be an
improvement over the current behaviour if it teaches "diff-tree" to read
from the tree and populate the in-core index (never writing it out to
$GIT_DIR/index) from the postimage tree (i.e. "diff preimage postimage" or
"diff -R postimage preimage") when it is run in a bare repository. It
would be a regression if the attributes mechanism is used for auditing
purposes (as we start reading from a tree that is being audited using the
very attributes it brings in), though.

On Thu, Sep 22, 2011 at 6:39 PM, Junio C Hamano [off-list ref] wrote:

Jay Soffian [off-list ref] writes:

quoted

When diff'ing the index against a tree (using either diff-index
or diff --cached), git previously looked at .gitattributes in the
working tree before considering .gitattributes in the index, even
though the diff itself otherwise ignores the working tree.

We can take attributes only from one place (so far from the working tree
and perhaps from the index), people had to live within the limitation that
comes from the "single source only" semantics. It also happens to be
easier to understand (recall the complexity of the examples Jeff gave
about "textconv" during "diff" which ideally should apply from its own
side and "funcname", which does not even have a right answer).

In practice, because development progresses by making everything
(including the .gitattributes file) better, I think "use the newer one"
would be a good compromise when we have two possible sources to grab
attributes from but we can only use one source.

I agree with that...

In that sense, I am somewhat skeptical about what this patch tries to
do. The working tree is where people make the progress to update the
index.

... but it still seems inconsistent that --cached ignores the working
tree except for .gitattributes.

This also happens to be the only way to get diff-index to work with a
bare repo and temporary (on-disk) index. But that's less important if
we implement what's suggested in the next paragraph.

A related tangent.

I think the logical conclusion of assuming that we will keep the "single
source only" semantics (which I think we will, by the way, unless I hear a
concrete proposal to how we apply attributes from more than one sources in
what way to which side of the diff) is that a patch might be an
improvement over the current behaviour if it teaches "diff-tree" to read
from the tree and populate the in-core index (never writing it out to
$GIT_DIR/index) from the postimage tree (i.e. "diff preimage postimage" or
"diff -R postimage preimage") when it is run in a bare repository.

Okay, I can give that a try.

It
would be a regression if the attributes mechanism is used for auditing
purposes (as we start reading from a tree that is being audited using the
very attributes it brings in), though.

--[no-]tree-attributes?

j.

On Thu, Sep 22, 2011 at 8:38 PM, Jay Soffian [off-list ref] wrote:

On Thu, Sep 22, 2011 at 6:39 PM, Junio C Hamano [off-list ref] wrote:

quoted

I think the logical conclusion of assuming that we will keep the "single
source only" semantics (which I think we will, by the way, unless I hear a
concrete proposal to how we apply attributes from more than one sources in
what way to which side of the diff) is that a patch might be an
improvement over the current behaviour if it teaches "diff-tree" to read
from the tree and populate the in-core index (never writing it out to
$GIT_DIR/index) from the postimage tree (i.e. "diff preimage postimage" or
"diff -R postimage preimage") when it is run in a bare repository.

Okay, I can give that a try.

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

@@ -5,6 +5,8 @@
 #include "diff.h"
 #include "diffcore.h"
 #include "tree.h"
+#include "attr.h"
+#include "unpack-trees.h"

 static void show_entry(struct diff_options *opt, const char *prefix,
 		       struct tree_desc *desc, struct strbuf *base);

@@ -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);
+	}
+
 	retval = diff_tree(&t1, &t2, base, opt);
 	if (!*base && DIFF_OPT_TST(opt, FOLLOW_RENAMES) && diff_might_be_rename()) {
 		init_tree_desc(&t1, tree1, size1);

(And in case gmail line wraps that -- https://gist.github.com/1236806)

Am I barking up the right tree? (Obviously still needs tests, and
maybe an --[no]-tree-attributes option.)

j.

On 09/23/2011 12:39 AM, Junio C Hamano wrote:

[...] It
would be a regression if the attributes mechanism is used for auditing
purposes (as we start reading from a tree that is being audited using the
very attributes it brings in), though.

I'm confused by this comment.

If an auditing system can be subverted by altering .gitattributes, then
I can do just as much harm by changing the .gitattributes in one commit
and making the "nasty" change in a second.  So any rigorous auditing
system based on .gitattributes would have to prevent me from committing
modifications to .gitattributes, in which case my commit will be
rejected anyway.

If by "auditing" you mean other less rigorous checks to which exceptions
are *allowed*, then it is preferable to add the exception in the same
commit as the otherwise-offending content, and therefore it is
*required* that the .gitattributes of the new tree be used when checking
the contents of that tree.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

On Fri, Sep 23, 2011 at 6:21 AM, Michael Haggerty [off-list ref] wrote:

On 09/23/2011 12:39 AM, Junio C Hamano wrote:

quoted

[...] It
would be a regression if the attributes mechanism is used for auditing
purposes (as we start reading from a tree that is being audited using the
very attributes it brings in), though.

I'm confused by this comment.

If an auditing system can be subverted by altering .gitattributes, then
I can do just as much harm by changing the .gitattributes in one commit
and making the "nasty" change in a second.  So any rigorous auditing
system based on .gitattributes would have to prevent me from committing
modifications to .gitattributes, in which case my commit will be
rejected anyway.

If by "auditing" you mean other less rigorous checks to which exceptions
are *allowed*, then it is preferable to add the exception in the same
commit as the otherwise-offending content, and therefore it is
*required* that the .gitattributes of the new tree be used when checking
the contents of that tree.

Currently, an auditing hook that cares about attributes, and which
runs in a bare repo, ignores the in-repo .gitattributes, considering
only the attributes set outside of the repo.

So by making git care about .gitattributes in a bare repo, such a hook
can suddenly be bypassed.

j.