[PATCH 1/2] Teach '--cached' option to check-attr

[PATCH 1/2] Teach '--cached' option to check-attr

[Jay Soffian]

This option causes check-attr to consider .gitattributes from the index
only, ignoring .gitattributes from the working tree. This allows it to
be used in situations where a working tree does not exist.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
This doesn't seem too controversial to me, and allows server-side
reading of .gitattributes, albeit with the need to setup an index.
Still that's better than having to setup an entire working tree.

 Documentation/git-check-attr.txt |    3 +++
 builtin/check-attr.c             |    5 +++++
 t/t0003-attributes.sh            |   28 ++++++++++++++++++++++++----
 3 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-check-attr.txt b/Documentation/git-check-attr.txt
index 1f7312a189..22537fea23 100644
--- a/Documentation/git-check-attr.txt
+++ b/Documentation/git-check-attr.txt
@@ -24,6 +24,9 @@ OPTIONS
 	paths.  If this option is used, then 'unspecified' attributes
 	will not be included in the output.
 
+--cached::
+	Consider .gitattributes in the index only, ignoring the working tree.
+
 --stdin::
 	Read file names from stdin instead of from the command-line.
 
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 708988a0e1..5682f6d2c7 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -5,6 +5,7 @@
 #include "parse-options.h"
 
 static int all_attrs;
+static int cached_attrs;
 static int stdin_paths;
 static const char * const check_attr_usage[] = {
 "git check-attr [-a | --all | attr...] [--] pathname...",
@@ -16,6 +17,7 @@ static int null_term_line;
 
 static const struct option check_attr_options[] = {
 	OPT_BOOLEAN('a', "all", &all_attrs, "report all attributes set on file"),
+	OPT_BOOLEAN(0,  "cached", &cached_attrs, "use cached .gitattributes"),
 	OPT_BOOLEAN(0 , "stdin", &stdin_paths, "read file names from stdin"),
 	OPT_BOOLEAN('z', NULL, &null_term_line,
 		"input paths are terminated by a null character"),
@@ -99,6 +101,9 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
 		die("invalid cache");
 	}
 
+	if (cached_attrs)
+		git_attr_set_direction(GIT_ATTR_INDEX, NULL);
+
 	doubledash = -1;
 	for (i = 0; doubledash < 0 && i < argc; i++) {
 		if (!strcmp(argv[i], "--"))
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index ae2f1da28f..2e1b4a7f75 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -134,10 +134,20 @@ test_expect_success 'attribute test: read paths from stdin' '
 
 test_expect_success 'attribute test: --all option' '
 
-	grep -v unspecified < expect-all | sort > expect &&
-	sed -e "s/:.*//" < expect-all | uniq |
-		git check-attr --stdin --all | sort > actual &&
-	test_cmp expect actual
+	grep -v unspecified < expect-all | sort > specified-all &&
+	sed -e "s/:.*//" < expect-all | uniq > stdin-all &&
+	git check-attr --stdin --all < stdin-all | sort > actual &&
+	test_cmp specified-all actual
+'
+
+test_expect_success 'attribute test: --cached option' '
+
+	:> empty &&
+	git check-attr --cached --stdin --all < stdin-all | sort > actual &&
+	test_cmp empty actual &&
+	git add .gitattributes a/.gitattributes a/b/.gitattributes &&
+	git check-attr --cached --stdin --all < stdin-all | sort > actual &&
+	test_cmp specified-all actual
 '
 
 test_expect_success 'root subdir attribute test' '
@@ -168,6 +178,16 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '
 
 '
 
+test_expect_success 'bare repository: check that --cached honors index' '
+
+	export GIT_INDEX_FILE=../.git/index &&
+	git check-attr --cached --stdin --all < ../stdin-all |
+		sort > actual &&
+	test_cmp ../specified-all actual
+
+'
+
+
 test_expect_success 'bare repository: test info/attributes' '
 
 	(
-- 
1.7.7.rc2.5.g12a2f

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

[Jay Soffian]

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.

Further, with an index, but no working tree, the in-index
.gitattributes were ignored entirely.

Calling git_attr_set_direction(GIT_ATTR_INDEX) before generating
the diff fixes both of these behaviors.

---
This is a weather balloon patch I guess.

Obviously there is a behavior change here as evidenced by the change to
t4020-diff-external.sh. I think the old behavior was wrong and this is a
bug fix. But the old behavior has been that way a long time, so maybe
we should use '--cached-attributes' instead for the "correct" behavior.

Since I'm not really sure what we should do with --cached -R, I'm
punting on that for now.

Jeff's message regarding diff-tree made my head hurt, so tackling that
will have to wait...

 diff-lib.c               |    3 +++
 t/t4020-diff-external.sh |    4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index f8454dd291..fe218931e6 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -11,6 +11,7 @@
 #include "unpack-trees.h"
 #include "refs.h"
 #include "submodule.h"
+#include "attr.h"
 
 /*
  * diff-files
@@ -476,6 +477,8 @@ static int diff_cache(struct rev_info *revs,
 int run_diff_index(struct rev_info *revs, int cached)
 {
 	struct object_array_entry *ent;
+	if (cached)
+		git_attr_set_direction(GIT_ATTR_INDEX, NULL);
 
 	ent = revs->pending.objects;
 	if (diff_cache(revs, ent->item->sha1, ent->name, cached))
diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
index 083f62d1d6..c6fdab3e87 100755
--- a/t/t4020-diff-external.sh
+++ b/t/t4020-diff-external.sh
@@ -160,10 +160,10 @@ test_expect_success 'external diff with autocrlf = true' '
 '
 
 test_expect_success 'diff --cached' '
-	git add file &&
+	git add .gitattributes file &&
 	git update-index --assume-unchanged file &&
 	echo second >file &&
-	git diff --cached >actual &&
+	git diff --cached file >actual &&
 	test_cmp "$TEST_DIRECTORY"/t4020/diff.NUL actual
 '
 
-- 
1.7.7.rc2.5.g12a2f

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

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> 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.

Re: [PATCH 1/2] Teach '--cached' option to check-attr

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> This doesn't seem too controversial to me, and allows server-side
> reading of .gitattributes, albeit with the need to setup an index.

Thanks; will queue with a few trivial tweaks.

> +test_expect_success 'bare repository: check that --cached honors index' '
> +
> +	export GIT_INDEX_FILE=../.git/index &&
> +	git check-attr --cached --stdin --all < ../stdin-all |
> +		sort > actual &&
> +	test_cmp ../specified-all actual
> +
> +'

This is unfriendly to others who need to add more tests after this piece
by contaminating their environment. A single-shot export would be more
appropriate here:

	GIT_INDEX_FILE=../.git/index git check-attr --cached ...

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

[Jay Soffian]

On Thu, Sep 22, 2011 at 6:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jay Soffian <jaysoffian@gmail.com> 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.

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.

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

[Jay Soffian]

On Thu, Sep 22, 2011 at 8:38 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
> On Thu, Sep 22, 2011 at 6:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> 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.

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

[Michael Haggerty]

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/

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

[Jay Soffian]

On Fri, Sep 23, 2011 at 6:21 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> 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.

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.

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

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

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

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

[Jay Soffian]

On Fri, Sep 23, 2011 at 12:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

Hmm, I looked at that, but then it would work for git-diff, but not
git-diff-tree. I don't think there's any other diff options that work
only at the porcelain layer, are there?

j.

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

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> Hmm, I looked at that, but then it would work for git-diff, but not
> git-diff-tree.

Why not?

The same helper function that you would write for calling from
builtin_diff_tree() would certainly be usable in git-diff-tree, no?