{"thread":{"id":"28463","subject":"[PATCH 1/2] Teach '--cached' option to check-attr","startedAt":"2011-09-22T21:44:20Z","lastAt":"2011-09-23T21:48:50Z","messageCount":11,"participants":["Jay Soffian","Junio C Hamano","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"176019","messageId":"1316727861-90460-1-git-send-email-jaysoffian@gmail.com","threadId":"28463","inReplyTo":null,"subject":"[PATCH 1/2] Teach '--cached' option to check-attr","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-09-22T21:44:20Z","receivedAt":"2011-09-22T21:44:20Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"This option causes check-attr to consider .gitattributes from the index\nonly, ignoring .gitattributes from the working tree. This allows it to\nbe used in situations where a working tree does not exist.\n\nSigned-off-by: Jay Soffian \n---\nThis doesn't seem too controversial to me, and allows server-side\nreading of .gitattributes, albeit with the need to setup an index.\nStill that's better than having to setup an entire working tree.\n\n Documentation/git-check-attr.txt | 3 +++\n builtin/check-attr.c | 5 +++++\n t/t0003-attributes.sh | 28 ++++++++++++++++++++++++----\n 3 files changed, 32 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-check-attr.txt b/Documentation/git-check-attr.txt\nindex 1f7312a189..22537fea23 100644\n--- a/Documentation/git-check-attr.txt\n+++ b/Documentation/git-check-attr.txt\n@@ -24,6 +24,9 @@ OPTIONS\n \tpaths. If this option is used, then 'unspecified' attributes\n \twill not be included in the output.\n \n+--cached::\n+\tConsider .gitattributes in the index only, ignoring the working tree.\n+\n --stdin::\n \tRead file names from stdin instead of from the command-line.\n \ndiff --git a/builtin/check-attr.c b/builtin/check-attr.c\nindex 708988a0e1..5682f6d2c7 100644\n--- a/builtin/check-attr.c\n+++ b/builtin/check-attr.c\n@@ -5,6 +5,7 @@\n #include \"parse-options.h\"\n \n static int all_attrs;\n+static int cached_attrs;\n static int stdin_paths;\n static const char * const check_attr_usage[] = {\n \"git check-attr [-a | --all | attr...] [--] pathname...\",\n@@ -16,6 +17,7 @@ static int null_term_line;\n \n static const struct option check_attr_options[] = {\n \tOPT_BOOLEAN('a', \"all\", &all_attrs, \"report all attributes set on file\"),\n+\tOPT_BOOLEAN(0, \"cached\", &cached_attrs, \"use cached .gitattributes\"),\n \tOPT_BOOLEAN(0 , \"stdin\", &stdin_paths, \"read file names from stdin\"),\n \tOPT_BOOLEAN('z', NULL, &null_term_line,\n \t\t\"input paths are terminated by a null character\"),\n@@ -99,6 +101,9 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)\n \t\tdie(\"invalid cache\");\n \t}\n \n+\tif (cached_attrs)\n+\t\tgit_attr_set_direction(GIT_ATTR_INDEX, NULL);\n+\n \tdoubledash = -1;\n \tfor (i = 0; doubledash < 0 && i < argc; i++) {\n \t\tif (!strcmp(argv[i], \"--\"))\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex ae2f1da28f..2e1b4a7f75 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -134,10 +134,20 @@ test_expect_success 'attribute test: read paths from stdin' '\n \n test_expect_success 'attribute test: --all option' '\n \n-\tgrep -v unspecified < expect-all | sort > expect &&\n-\tsed -e \"s/:.*//\" < expect-all | uniq |\n-\t\tgit check-attr --stdin --all | sort > actual &&\n-\ttest_cmp expect actual\n+\tgrep -v unspecified < expect-all | sort > specified-all &&\n+\tsed -e \"s/:.*//\" < expect-all | uniq > stdin-all &&\n+\tgit check-attr --stdin --all < stdin-all | sort > actual &&\n+\ttest_cmp specified-all actual\n+'\n+\n+test_expect_success 'attribute test: --cached option' '\n+\n+\t:> empty &&\n+\tgit check-attr --cached --stdin --all < stdin-all | sort > actual &&\n+\ttest_cmp empty actual &&\n+\tgit add .gitattributes a/.gitattributes a/b/.gitattributes &&\n+\tgit check-attr --cached --stdin --all < stdin-all | sort > actual &&\n+\ttest_cmp specified-all actual\n '\n \n test_expect_success 'root subdir attribute test' '\n@@ -168,6 +178,16 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '\n \n '\n \n+test_expect_success 'bare repository: check that --cached honors index' '\n+\n+\texport GIT_INDEX_FILE=../.git/index &&\n+\tgit check-attr --cached --stdin --all < ../stdin-all |\n+\t\tsort > actual &&\n+\ttest_cmp ../specified-all actual\n+\n+'\n+\n+\n test_expect_success 'bare repository: test info/attributes' '\n \n \t(\n-- \n1.7.7.rc2.5.g12a2f\n"},{"id":"176020","messageId":"1316727861-90460-2-git-send-email-jaysoffian@gmail.com","threadId":"28463","inReplyTo":"1316727861-90460-1-git-send-email-jaysoffian@gmail.com","subject":"[PATCH 2/2] diff_index: honor in-index, not working-tree, .gitattributes","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-09-22T21:44:21Z","receivedAt":"2011-09-22T21:44:21Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"When diff'ing the index against a tree (using either diff-index\nor diff --cached), git previously looked at .gitattributes in the\nworking tree before considering .gitattributes in the index, even\nthough the diff itself otherwise ignores the working tree.\n\nFurther, with an index, but no working tree, the in-index\n.gitattributes were ignored entirely.\n\nCalling git_attr_set_direction(GIT_ATTR_INDEX) before generating\nthe diff fixes both of these behaviors.\n\n---\nThis is a weather balloon patch I guess.\n\nObviously there is a behavior change here as evidenced by the change to\nt4020-diff-external.sh. I think the old behavior was wrong and this is a\nbug fix. But the old behavior has been that way a long time, so maybe\nwe should use '--cached-attributes' instead for the \"correct\" behavior.\n\nSince I'm not really sure what we should do with --cached -R, I'm\npunting on that for now.\n\nJeff's message regarding diff-tree made my head hurt, so tackling that\nwill have to wait...\n\n diff-lib.c | 3 +++\n t/t4020-diff-external.sh | 4 ++--\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex f8454dd291..fe218931e6 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -11,6 +11,7 @@\n #include \"unpack-trees.h\"\n #include \"refs.h\"\n #include \"submodule.h\"\n+#include \"attr.h\"\n \n /*\n * diff-files\n@@ -476,6 +477,8 @@ static int diff_cache(struct rev_info *revs,\n int run_diff_index(struct rev_info *revs, int cached)\n {\n \tstruct object_array_entry *ent;\n+\tif (cached)\n+\t\tgit_attr_set_direction(GIT_ATTR_INDEX, NULL);\n \n \tent = revs->pending.objects;\n \tif (diff_cache(revs, ent->item->sha1, ent->name, cached))\ndiff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh\nindex 083f62d1d6..c6fdab3e87 100755\n--- a/t/t4020-diff-external.sh\n+++ b/t/t4020-diff-external.sh\n@@ -160,10 +160,10 @@ test_expect_success 'external diff with autocrlf = true' '\n '\n \n test_expect_success 'diff --cached' '\n-\tgit add file &&\n+\tgit add .gitattributes file &&\n \tgit update-index --assume-unchanged file &&\n \techo second >file &&\n-\tgit diff --cached >actual &&\n+\tgit diff --cached file >actual &&\n \ttest_cmp \"$TEST_DIRECTORY\"/t4020/diff.NUL actual\n '\n \n-- \n1.7.7.rc2.5.g12a2f\n"},{"id":"176030","messageId":"7v8vpgxkvb.fsf@alter.siamese.dyndns.org","threadId":"28463","inReplyTo":"1316727861-90460-2-git-send-email-jaysoffian@gmail.com","subject":"Re: [PATCH 2/2] diff_index: honor in-index, not working-tree, .gitattributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-22T22:39:20Z","receivedAt":"2011-09-22T22:39:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jay Soffian writes:\n\n> When diff'ing the index against a tree (using either diff-index\n> or diff --cached), git previously looked at .gitattributes in the\n> working tree before considering .gitattributes in the index, even\n> though the diff itself otherwise ignores the working tree.\n\nWe can take attributes only from one place (so far from the working tree\nand perhaps from the index), people had to live within the limitation that\ncomes from the \"single source only\" semantics. It also happens to be\neasier to understand (recall the complexity of the examples Jeff gave\nabout \"textconv\" during \"diff\" which ideally should apply from its own\nside and \"funcname\", which does not even have a right answer).\n\nIn practice, because development progresses by making everything\n(including the .gitattributes file) better, I think \"use the newer one\"\nwould be a good compromise when we have two possible sources to grab\nattributes from but we can only use one source.\n\nIn that sense, I am somewhat skeptical about what this patch tries to\ndo. The working tree is where people make the progress to update the\nindex.\n\nA related tangent.\n\nI think the logical conclusion of assuming that we will keep the \"single\nsource only\" semantics (which I think we will, by the way, unless I hear a\nconcrete proposal to how we apply attributes from more than one sources in\nwhat way to which side of the diff) is that a patch might be an\nimprovement over the current behaviour if it teaches \"diff-tree\" to read\nfrom the tree and populate the in-core index (never writing it out to\n$GIT_DIR/index) from the postimage tree (i.e. \"diff preimage postimage\" or\n\"diff -R postimage preimage\") when it is run in a bare repository. It\nwould be a regression if the attributes mechanism is used for auditing\npurposes (as we start reading from a tree that is being audited using the\nvery attributes it brings in), though.\n"},{"id":"176035","messageId":"7vzkhww3n6.fsf@alter.siamese.dyndns.org","threadId":"28463","inReplyTo":"1316727861-90460-1-git-send-email-jaysoffian@gmail.com","subject":"Re: [PATCH 1/2] Teach '--cached' option to check-attr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-22T23:36:45Z","receivedAt":"2011-09-22T23:36:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jay Soffian writes:\n\n> This doesn't seem too controversial to me, and allows server-side\n> reading of .gitattributes, albeit with the need to setup an index.\n\nThanks; will queue with a few trivial tweaks.\n\n> +test_expect_success 'bare repository: check that --cached honors index' '\n> +\n> +\texport GIT_INDEX_FILE=../.git/index &&\n> +\tgit check-attr --cached --stdin --all < ../stdin-all |\n> +\t\tsort > actual &&\n> +\ttest_cmp ../specified-all actual\n> +\n> +'\n\nThis is unfriendly to others who need to add more tests after this piece\nby contaminating their environment. A single-shot export would be more\nappropriate here:\n\n\tGIT_INDEX_FILE=../.git/index git check-attr --cached ...\n"},{"id":"176038","messageId":"CAG+J_DzUQ3OGfiX=vHVGC7SHvwToVjD7uwFyDa8Tq6t7YwX12Q@mail.gmail.com","threadId":"28463","inReplyTo":"7v8vpgxkvb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] diff_index: honor in-index, not working-tree, .gitattributes","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-09-23T00:38:14Z","receivedAt":"2011-09-23T00:38:14Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Thu, Sep 22, 2011 at 6:39 PM, Junio C Hamano wrote:\n> Jay Soffian writes:\n>\n>> When diff'ing the index against a tree (using either diff-index\n>> or diff --cached), git previously looked at .gitattributes in the\n>> working tree before considering .gitattributes in the index, even\n>> though the diff itself otherwise ignores the working tree.\n>\n> We can take attributes only from one place (so far from the working tree\n> and perhaps from the index), people had to live within the limitation that\n> comes from the \"single source only\" semantics. It also happens to be\n> easier to understand (recall the complexity of the examples Jeff gave\n> about \"textconv\" during \"diff\" which ideally should apply from its own\n> side and \"funcname\", which does not even have a right answer).\n>\n> In practice, because development progresses by making everything\n> (including the .gitattributes file) better, I think \"use the newer one\"\n> would be a good compromise when we have two possible sources to grab\n> attributes from but we can only use one source.\n\nI agree with that...\n\n> In that sense, I am somewhat skeptical about what this patch tries to\n> do. The working tree is where people make the progress to update the\n> index.\n\n... but it still seems inconsistent that --cached ignores the working\ntree except for .gitattributes.\n\nThis also happens to be the only way to get diff-index to work with a\nbare repo and temporary (on-disk) index. But that's less important if\nwe implement what's suggested in the next paragraph.\n\n> A related tangent.\n>\n> I think the logical conclusion of assuming that we will keep the \"single\n> source only\" semantics (which I think we will, by the way, unless I hear a\n> concrete proposal to how we apply attributes from more than one sources in\n> what way to which side of the diff) is that a patch might be an\n> improvement over the current behaviour if it teaches \"diff-tree\" to read\n> from the tree and populate the in-core index (never writing it out to\n> $GIT_DIR/index) from the postimage tree (i.e. \"diff preimage postimage\" or\n> \"diff -R postimage preimage\") when it is run in a bare repository.\n\nOkay, I can give that a try.\n\n> It\n> would be a regression if the attributes mechanism is used for auditing\n> purposes (as we start reading from a tree that is being audited using the\n> very attributes it brings in), though.\n\n--[no-]tree-attributes?\n\nj.\n"},{"id":"176043","messageId":"CAG+J_Dyh=t2VAZ6rAqcF2meEgBCN5c+J_m_YvVQbKfvXeJ8WGA@mail.gmail.com","threadId":"28463","inReplyTo":"CAG+J_DzUQ3OGfiX=vHVGC7SHvwToVjD7uwFyDa8Tq6t7YwX12Q@mail.gmail.com","subject":"Re: [PATCH 2/2] diff_index: honor in-index, not working-tree, .gitattributes","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-09-23T05:37:13Z","receivedAt":"2011-09-23T05:37:13Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Thu, Sep 22, 2011 at 8:38 PM, Jay Soffian wrote:\n> On Thu, Sep 22, 2011 at 6:39 PM, Junio C Hamano wrote:\n>\n>> I think the logical conclusion of assuming that we will keep the \"single\n>> source only\" semantics (which I think we will, by the way, unless I hear a\n>> concrete proposal to how we apply attributes from more than one sources in\n>> what way to which side of the diff) is that a patch might be an\n>> improvement over the current behaviour if it teaches \"diff-tree\" to read\n>> from the tree and populate the in-core index (never writing it out to\n>> $GIT_DIR/index) from the postimage tree (i.e. \"diff preimage postimage\" or\n>> \"diff -R postimage preimage\") when it is run in a bare repository.\n>\n> Okay, I can give that a try.\n\nThis area of git is still black magic to me. My best guess is\nsomething like this:\n\ndiff --git a/tree-diff.c b/tree-diff.c\nindex b3cc2e4753..6fd84eb2bb 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -5,6 +5,8 @@\n #include \"diff.h\"\n #include \"diffcore.h\"\n #include \"tree.h\"\n+#include \"attr.h\"\n+#include \"unpack-trees.h\"\n\n static void show_entry(struct diff_options *opt, const char *prefix,\n \t\t struct tree_desc *desc, struct strbuf *base);\n@@ -280,6 +282,19 @@ int diff_tree_sha1(const unsigned char *old,\nconst unsigned char *new, const cha\n \t\tdie(\"unable to read destination tree (%s)\", sha1_to_hex(new));\n \tinit_tree_desc(&t1, tree1, size1);\n \tinit_tree_desc(&t2, tree2, size2);\n+\n+\tif (is_bare_repository()) {\n+\t\tstruct unpack_trees_options unpack_opts;\n+\t\tmemset(&unpack_opts, 0, sizeof(unpack_opts));\n+\t\tunpack_opts.index_only = 1;\n+\t\tunpack_opts.head_idx = -1;\n+\t\tunpack_opts.src_index = &the_index;\n+\t\tunpack_opts.dst_index = &the_index;\n+\t\tunpack_opts.fn = oneway_merge;\n+\t\tif (unpack_trees(1, DIFF_OPT_TST(opt, REVERSE_DIFF) ? &t1 : &t2,\n&unpack_opts) == 0)\n+\t\t\tgit_attr_set_direction(GIT_ATTR_INDEX, &the_index);\n+\t}\n+\n \tretval = diff_tree(&t1, &t2, base, opt);\n \tif (!*base && DIFF_OPT_TST(opt, FOLLOW_RENAMES) && diff_might_be_rename()) {\n \t\tinit_tree_desc(&t1, tree1, size1);\n\n(And in case gmail line wraps that -- https://gist.github.com/1236806)\n\nAm I barking up the right tree? (Obviously still needs tests, and\nmaybe an --[no]-tree-attributes option.)\n\nj.\n"},{"id":"176055","messageId":"4E7C5DC3.8030409@alum.mit.edu","threadId":"28463","inReplyTo":"7v8vpgxkvb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] diff_index: honor in-index, not working-tree, .gitattributes","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-23T10:21:55Z","receivedAt":"2011-09-23T10:21:55Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/23/2011 12:39 AM, Junio C Hamano wrote:\n> [...] It\n> would be a regression if the attributes mechanism is used for auditing\n> purposes (as we start reading from a tree that is being audited using the\n> very attributes it brings in), though.\n\nI'm confused by this comment.\n\nIf an auditing system can be subverted by altering .gitattributes, then\nI can do just as much harm by changing the .gitattributes in one commit\nand making the \"nasty\" change in a second. So any rigorous auditing\nsystem based on .gitattributes would have to prevent me from committing\nmodifications to .gitattributes, in which case my commit will be\nrejected anyway.\n\nIf by \"auditing\" you mean other less rigorous checks to which exceptions\nare *allowed*, then it is preferable to add the exception in the same\ncommit as the otherwise-offending content, and therefore it is\n*required* that the .gitattributes of the new tree be used when checking\nthe contents of that tree.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"176071","messageId":"CAG+J_Dz7uQcYjwEZrAg-h2GAwJ0gCjW2kw2Erz3UWGYcCeTAVQ@mail.gmail.com","threadId":"28463","inReplyTo":"4E7C5DC3.8030409@alum.mit.edu","subject":"Re: [PATCH 2/2] diff_index: honor in-index, not working-tree, .gitattributes","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-09-23T15:50:37Z","receivedAt":"2011-09-23T15:50:37Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Fri, Sep 23, 2011 at 6:21 AM, Michael Haggerty wrote:\n> On 09/23/2011 12:39 AM, Junio C Hamano wrote:\n>> [...] It\n>> would be a regression if the attributes mechanism is used for auditing\n>> purposes (as we start reading from a tree that is being audited using the\n>> very attributes it brings in), though.\n>\n> I'm confused by this comment.\n>\n> If an auditing system can be subverted by altering .gitattributes, then\n> I can do just as much harm by changing the .gitattributes in one commit\n> and making the \"nasty\" change in a second.  So any rigorous auditing\n> system based on .gitattributes would have to prevent me from committing\n> modifications to .gitattributes, in which case my commit will be\n> rejected anyway.\n>\n> If by \"auditing\" you mean other less rigorous checks to which exceptions\n> are *allowed*, then it is preferable to add the exception in the same\n> commit as the otherwise-offending content, and therefore it is\n> *required* that the .gitattributes of the new tree be used when checking\n> the contents of that tree.\n\nCurrently, an auditing hook that cares about attributes, and which\nruns in a bare repo, ignores the in-repo .gitattributes, considering\nonly the attributes set outside of the repo.\n\nSo by making git care about .gitattributes in a bare repo, such a hook\ncan suddenly be bypassed.\n\nj.\n"},{"id":"176075","messageId":"7vobybw6mv.fsf@alter.siamese.dyndns.org","threadId":"28463","inReplyTo":"CAG+J_Dyh=t2VAZ6rAqcF2meEgBCN5c+J_m_YvVQbKfvXeJ8WGA@mail.gmail.com","subject":"Re: [PATCH 2/2] diff_index: honor in-index, not working-tree, .gitattributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-23T16:44:24Z","receivedAt":"2011-09-23T16:44:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jay Soffian writes:\n\n> This area of git is still black magic to me. My best guess is\n> something like this:\n>\n> diff --git a/tree-diff.c b/tree-diff.c\n> index b3cc2e4753..6fd84eb2bb 100644\n> --- a/tree-diff.c\n> +++ b/tree-diff.c\n> @@ -280,6 +282,19 @@ int diff_tree_sha1(const unsigned char *old,\n> const unsigned char *new, const cha\n> \t\tdie(\"unable to read destination tree (%s)\", sha1_to_hex(new));\n> \tinit_tree_desc(&t1, tree1, size1);\n> \tinit_tree_desc(&t2, tree2, size2);\n> +\n> +\tif (is_bare_repository()) {\n> +\t\tstruct unpack_trees_options unpack_opts;\n> +\t\tmemset(&unpack_opts, 0, sizeof(unpack_opts));\n> +\t\tunpack_opts.index_only = 1;\n> +\t\tunpack_opts.head_idx = -1;\n> +\t\tunpack_opts.src_index = &the_index;\n> +\t\tunpack_opts.dst_index = &the_index;\n> +\t\tunpack_opts.fn = oneway_merge;\n> +\t\tif (unpack_trees(1, DIFF_OPT_TST(opt, REVERSE_DIFF) ? &t1 : &t2,\n> &unpack_opts) == 0)\n> +\t\t\tgit_attr_set_direction(GIT_ATTR_INDEX, &the_index);\n> +\t}\n\nThis is hooking at too low a level in the callchain. diff_tree_sha1() is\nmeant to be a general purpose \"I have two tree-ish objects and I want the\ncomparison machinery to work on them\" library function [*1*].\n\n - One of the more important uses is the history simplification done\n during revision traversal by checking if the subtrees and the blobs\n have the same SHA-1, and we should not pay penalty of reading the index\n for each and every tree here.\n\n - The caller may be using the index for its own purposes, and your use of\n \"the_index\" here will break them.\n\nIf you want to allow use of in-tree attributes in _all_ callers of\ndiff_tree_sha1(), then the right approach is to add an instance of \"struct\nindex_state\" to \"struct diff_options\", have the caller _explicitly_ ask\nfor use of in-tree attributes by setting a bit somewhere in \"struct\ndiff_options\", and read the tree into that separate index_state using\ntree.c::read_tree(). I however doubt it is worth it.\n\nI would think it makes more sense to add a codeblock like that at the\nbeginning of builtin/diff.c::builtin_diff_tree() when a new command option\nasks for it. In that codepath, you _know_ that we are not using the index\nat all, and reading the index there will not interfere with other uses of\nthe index in the program.\n\n\n[Footnote]\n\n*1* which means that it is not a good justification to say \"no current\n caller is broken by this change\". We need to make the library usable\n for future callers.\n"},{"id":"176101","messageId":"CAG+J_DwGuR6bqE4iG26-xwc0_TuRRKUDmmJZ44Qr+y5EEzTnNA@mail.gmail.com","threadId":"28463","inReplyTo":"7vobybw6mv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] diff_index: honor in-index, not working-tree, .gitattributes","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-09-23T21:32:20Z","receivedAt":"2011-09-23T21:32:20Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Fri, Sep 23, 2011 at 12:44 PM, Junio C Hamano wrote:\n> If you want to allow use of in-tree attributes in _all_ callers of\n> diff_tree_sha1(), then the right approach is to add an instance of \"struct\n> index_state\" to \"struct diff_options\", have the caller _explicitly_ ask\n> for use of in-tree attributes by setting a bit somewhere in \"struct\n> diff_options\", and read the tree into that separate index_state using\n> tree.c::read_tree(). I however doubt it is worth it.\n>\n> I would think it makes more sense to add a codeblock like that at the\n> beginning of builtin/diff.c::builtin_diff_tree() when a new command option\n> asks for it. In that codepath, you _know_ that we are not using the index\n> at all, and reading the index there will not interfere with other uses of\n> the index in the program.\n\nHmm, I looked at that, but then it would work for git-diff, but not\ngit-diff-tree. I don't think there's any other diff options that work\nonly at the porcelain layer, are there?\n\nj.\n"},{"id":"176102","messageId":"7vfwjnszel.fsf@alter.siamese.dyndns.org","threadId":"28463","inReplyTo":"CAG+J_DwGuR6bqE4iG26-xwc0_TuRRKUDmmJZ44Qr+y5EEzTnNA@mail.gmail.com","subject":"Re: [PATCH 2/2] diff_index: honor in-index, not working-tree, .gitattributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-23T21:48:50Z","receivedAt":"2011-09-23T21:48:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jay Soffian writes:\n\n> Hmm, I looked at that, but then it would work for git-diff, but not\n> git-diff-tree.\n\nWhy not?\n\nThe same helper function that you would write for calling from\nbuiltin_diff_tree() would certainly be usable in git-diff-tree, no?\n"}]}