threads / patch / 28463

patch, 2 partsTeach '--cached' option to check-attr

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

## tl;dr

11 messages between Sep 22, 2011 and Sep 23, 2011. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Jay Soffian· Sep 22, 2011, 21:44 UTC · lore

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(-)
Show changes to 3 files +32 −4

Documentation/git-check-attr.txt, builtin/check-attr.c, t/t0003-attributes.sh

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
Jay Soffian· Sep 22, 2011, 21:44 UTC · re: Jay Soffian · lore

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

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(-)
Show changes to 2 files +5 −2

diff-lib.c, t/t4020-diff-external.sh

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
Junio C Hamano· Sep 22, 2011, 22:39 UTC · re: Jay Soffian · lore

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

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.

Jay Soffian· Sep 23, 2011, 00:38 UTC · re: Junio C Hamano · lore

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

On Thu, Sep 22, 2011 at 6:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
> 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.

Show 10 quoted lines
> 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.
Jay Soffian· Sep 23, 2011, 05:37 UTC · re: Jay Soffian · lore

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

On Thu, Sep 22, 2011 at 8:38 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
Show 12 quoted lines
> 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:

Show changes to tree-diff.c +15 −0
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.
Junio C Hamano· Sep 23, 2011, 16:44 UTC · re: Jay Soffian · lore

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

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.
Jay Soffian· Sep 23, 2011, 21:32 UTC · re: Junio C Hamano · lore

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

On Fri, Sep 23, 2011 at 12:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
> 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.
Junio C Hamano· Sep 23, 2011, 21:48 UTC · re: Jay Soffian · lore

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

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?

Michael Haggerty· Sep 23, 2011, 10:21 UTC · re: Junio C Hamano · lore

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

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/
Jay Soffian· Sep 23, 2011, 15:50 UTC · re: Michael Haggerty · lore

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

On Fri, Sep 23, 2011 at 6:21 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
Show 20 quoted lines
> 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.
Junio C Hamano· Sep 22, 2011, 23:36 UTC · re: Jay Soffian · lore

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

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.
Show 8 quoted lines
> +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 ...

← back to recent threads