{"thread":{"id":"59394","subject":"[PATCH 0/2] diff: support bare repositories when reading gitattributes for diff algorithm","startedAt":"2023-03-14T01:57:06Z","lastAt":"2023-03-17T14:11:53Z","messageCount":15,"participants":["John Cai via GitGitGadget","Junio C Hamano","Philippe Blain","John Cai","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"473487","messageId":"pull.1459.git.git.1678758818.gitgitgadget@gmail.com","threadId":"59394","inReplyTo":null,"subject":"[PATCH 0/2] diff: support bare repositories when reading gitattributes for diff algorithm","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-14T01:53:36Z","receivedAt":"2023-03-14T01:57:06Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"This patch series adds support for bare repositories for the feature added\nin [1]. When using a bare repository, by default we will look for\ngitattributes from HEAD. When the --attr-source option is passed, we will\ntry to read gitattributes from the commit.\n\nA side effect of this patch series is that custom drivers will now also work\nwith bare repositories.\n\n 1. (a4cf900ee7 diff: teach diff to read algorithm from diff driver,\n 2022-02-20)\n\nJohn Cai (2):\n diff: use HEAD for attributes when using bare repository\n diff: add --attr-source to read gitattributes from a commit\n\n Documentation/diff-options.txt | 4 ++++\n Documentation/gitattributes.txt | 8 +++++++\n diff.c | 37 ++++++++++++++++++++++++++++++---\n diff.h | 1 +\n t/lib-diff-alternative.sh | 33 ++++++++++++++++++++++++-----\n t/t4018-diff-funcname.sh | 29 ++++++++++++++++++++++++++\n userdiff.c | 9 +++++++-\n userdiff.h | 4 ++++\n 8 files changed, 116 insertions(+), 9 deletions(-)\n\n\nbase-commit: d15644fe0226af7ffc874572d968598564a230dd\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1459%2Fjohn-cai%2Fjc%2Fdiff-attr-bare-repo-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1459/john-cai/jc/diff-attr-bare-repo-v1\nPull-Request: https://github.com/git/git/pull/1459\n-- \ngitgitgadget\n"},{"id":"473488","messageId":"bc9c2b1463a625ddab58cb72d4281f01d1f53bdb.1678758818.git.gitgitgadget@gmail.com","threadId":"59394","inReplyTo":"pull.1459.git.git.1678758818.gitgitgadget@gmail.com","subject":"[PATCH 2/2] diff: add --attr-source to read gitattributes from a commit","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-14T01:53:38Z","receivedAt":"2023-03-14T01:57:06Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nThe previous commit allows the diff machinery to read gitattributes from\nHEAD when a bare repository is being used.\n\nSome users may want to specify which commit to read gitattributes from.\nTeach diff a new --attr-source that can be used to pass in a commit.\n\nSigned-off-by: John Cai \n---\n Documentation/diff-options.txt | 4 ++++\n Documentation/gitattributes.txt | 8 ++++++--\n diff.c | 16 ++++++++++-----\n diff.h | 1 +\n t/lib-diff-alternative.sh | 35 ++++++++++++++++++++++-----------\n t/t4018-diff-funcname.sh | 18 +++++++++++++++++\n 6 files changed, 64 insertions(+), 18 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 7d73e976d99..d3a04937dde 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -173,6 +173,10 @@ endif::git-log[]\n \n --anchored=::\n \tGenerate a diff using the \"anchored diff\" algorithm.\n+\n+--attr-source=::\n+\tSpecify a commit to read gitattributes from when using a bare\n+\trepository when defining external drivers, or internal algorithms.\n +\n This option may be specified more than once.\n +\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 15488bd92b2..7f9451b56fa 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -758,7 +758,9 @@ with the above configuration, i.e. `j-c-diff`, with 7\n parameters, just like `GIT_EXTERNAL_DIFF` program is called.\n See linkgit:git[1] for details.\n \n-When using a bare repository, the gitattributes from HEAD will be used.\n+When using a bare repository, the gitattributes from HEAD will be used, unless\n+the --attr-source option is used to specify a commit from which the\n+gitattributes will be read.\n \n Setting the internal diff algorithm\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n@@ -787,7 +789,9 @@ This diff algorithm applies to user facing diff output like git-diff(1),\n git-show(1) and is used for the `--stat` output as well. The merge machinery\n will not use the diff algorithm set through this method.\n \n-When using a bare repository, the gitattributes from HEAD will be used.\n+When using a bare repository, the gitattributes from HEAD will be used, unless\n+the --attr-source option is used to specify a commit from which the\n+gitattributes will be read.\n \n NOTE: If `diff..command` is defined for path with the\n `diff=` attribute, it is executed as an external diff driver\ndiff --git a/diff.c b/diff.c\nindex 51baf893bb0..e86cf33b7a9 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4448,10 +4448,13 @@ static void get_userdiff(struct diff_options *o,\n \t\t\t struct userdiff_driver **drv,\n \t\t\t const char *attr_path)\n {\n-\tconst char *commit = \"HEAD\";\n+\tconst char *commit = o->attr_source;\n \tstruct object_id *tree_oid = NULL;\n \n-\tif (is_bare_repository() && o->repo->gitdir) {\n+\tif (!commit)\n+\t\tcommit = \"HEAD\";\n+\n+\tif ((o->attr_source || is_bare_repository()) && o->repo->gitdir) {\n \t\tstruct object_id oid;\n \n \t\tif (!get_oid(commit, &oid)) {\n@@ -4459,6 +4462,8 @@ static void get_userdiff(struct diff_options *o,\n \n \t\t\tif (t)\n \t\t\t\ttree_oid = &t->object.oid;\n+\t\t} else if (o->attr_source) {\n+\t\t\tdie(_(\"%s is not a valid object\"), commit);\n \t\t}\n \t}\n \n@@ -4480,10 +4485,8 @@ static void run_diff_cmd(const char *pgm,\n \tint must_show_header = 0;\n \tstruct userdiff_driver *drv = NULL;\n \n-\tif (o->flags.allow_external || !o->ignore_driver_algorithm) {\n-\n+\tif (o->flags.allow_external || !o->ignore_driver_algorithm)\n \t\tget_userdiff(o, &drv, attr_path);\n-\t}\n \n \tif (o->flags.allow_external && drv && drv->external)\n \t\tpgm = drv->external;\n@@ -5699,6 +5702,9 @@ struct option *add_diff_options(const struct option *opts,\n \t\t\t N_(\"disable all output of the program\")),\n \t\tOPT_BOOL(0, \"ext-diff\", &options->flags.allow_external,\n \t\t\t N_(\"allow an external diff helper to be executed\")),\n+\t\tOPT_STRING(0, \"attr-source\", &options->attr_source,\n+\t\t\t N_(\"attributes-source\"),\n+\t\t\t N_(\"the commit to read attributes from\")),\n \t\tOPT_CALLBACK_F(0, \"textconv\", options, NULL,\n \t\t\t N_(\"run external text conversion filters when comparing binary files\"),\n \t\t\t PARSE_OPT_NOARG, diff_opt_textconv),\ndiff --git a/diff.h b/diff.h\nindex 8d770b1d579..59db40f66be 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -334,6 +334,7 @@ struct diff_options {\n \tconst char *stat_sep;\n \tint xdl_opts;\n \tint ignore_driver_algorithm;\n+\tconst char *attr_source;\n \n \t/* see Documentation/diff-options.txt */\n \tchar **anchors;\ndiff --git a/t/lib-diff-alternative.sh b/t/lib-diff-alternative.sh\nindex 0d99af83dd2..a7cd4966749 100644\n--- a/t/lib-diff-alternative.sh\n+++ b/t/lib-diff-alternative.sh\n@@ -112,25 +112,38 @@ EOF\n \n \tSTRATEGY=$1\n \n-\ttest_expect_success \"$STRATEGY diff from attributes\" '\n+\ttest_expect_success \"setup attributes files for tests with $STRATEGY\" '\n+\t\tgit checkout -b master &&\n \t\techo \"file* diff=driver\" >.gitattributes &&\n-\t\tgit config diff.driver.algorithm \"$STRATEGY\" &&\n-\t\ttest_must_fail git diff --no-index file1 file2 > output &&\n-\t\tcat expect &&\n-\t\tcat output &&\n+\t\tgit add file1 file2 .gitattributes &&\n+\t\tgit commit -m \"adding files\" &&\n+\t\tgit checkout -b branchA &&\n+\t\techo \"file* diff=driverA\" >.gitattributes &&\n+\t\tgit add .gitattributes &&\n+\t\tgit commit -m \"adding driverA as diff driver\" &&\n+\t\tgit checkout master &&\n+\t\tgit clone --bare --no-local . bare.git\n+\t'\n+\n+\ttest_expect_success \"$STRATEGY diff from attributes\" '\n+\t\ttest_must_fail git -c diff.driver.algorithm=$STRATEGY diff --no-index file1 file2 > output &&\n \t\ttest_cmp expect output\n \t'\n \n \ttest_expect_success \"$STRATEGY diff from attributes with bare repo\" '\n-\t\techo \"file* diff=driver\" >.gitattributes &&\n-\t\tgit add file1 file2 .gitattributes &&\n-\t\tgit commit -m \"adding files\" &&\n-\t\tgit clone --bare --no-local . bare.git &&\n-\t\tgit -C bare.git config diff.driver.algorithm \"$STRATEGY\" &&\n-\t\tgit -C bare.git diff HEAD:file1 HEAD:file2 > output &&\n+\t\tgit -C bare.git -c diff.driver.algorithm=$STRATEGY diff HEAD:file1 HEAD:file2 >output &&\n+\t\ttest_cmp expect output\n+\t'\n+\n+\ttest_expect_success \"diff from attributes with bare repo when branch different than HEAD\" '\n+\t\tgit -C bare.git -c diff.driver.algorithm=myers -c diff.driverA.algorithm=$STRATEGY diff --attr-source branchA HEAD:file1 HEAD:file2 >output &&\n \t\ttest_cmp expect output\n \t'\n \n+\ttest_expect_success \"diff from attributes with bare repo with invalid source\" '\n+\t\ttest_must_fail git -C bare.git diff --attr-source invalid-branch HEAD:file1 HEAD:file2\n+\t'\n+\n \ttest_expect_success \"$STRATEGY diff from attributes has valid diffstat\" '\n \t\techo \"file* diff=driver\" >.gitattributes &&\n \t\tgit config diff.driver.algorithm \"$STRATEGY\" &&\ndiff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh\nindex 451af08c611..30babcfae91 100755\n--- a/t/t4018-diff-funcname.sh\n+++ b/t/t4018-diff-funcname.sh\n@@ -63,6 +63,7 @@ do\n \t\ttest_i18ngrep ! fatal msg &&\n \t\ttest_i18ngrep ! error msg\n \t'\n+\n \ttest_expect_success \"builtin $p pattern compiles on bare repo\" '\n \t\ttest_when_finished \"rm -rf bare.git\" &&\n \t\techo \"*.java diff=$p\" >.gitattributes &&\n@@ -74,6 +75,23 @@ do\n \t\ttest_i18ngrep ! fatal msg &&\n \t\ttest_i18ngrep ! error msg\n \t'\n+\ttest_expect_success \"builtin $p pattern compiles on bare repo with --attr-source\" '\n+\t\ttest_when_finished \"rm -rf bare.git\" &&\n+\t\tgit checkout -B master &&\n+\t\techo \"*.java diff=notexist\" > .gitattributes &&\n+\t\tgit add .gitattributes &&\n+\t\tgit commit -am \"changing gitattributes\" &&\n+\t\tgit checkout -B branchA &&\n+\t\techo \"*.java diff=$p\" >.gitattributes &&\n+\t\tgit add .gitattributes &&\n+\t\tgit commit -am \"changing gitattributes\" &&\n+\t\tgit clone --bare --no-local . bare.git &&\n+\t\tgit -C bare.git symbolic-ref HEAD refs/heads/master &&\n+\t\ttest_expect_code 1 git -C bare.git diff --exit-code \\\n+\t\t\t--attr-source branchA HEAD:A.java HEAD:B.java 2>msg &&\n+\t\ttest_i18ngrep ! fatal msg &&\n+\t\ttest_i18ngrep ! error msg\n+\t'\n done\n \n test_expect_success 'last regexp must not be negated' '\n-- \ngitgitgadget\n"},{"id":"473489","messageId":"0fc704cf1c0724473a61086098d44c3a82938b03.1678758818.git.gitgitgadget@gmail.com","threadId":"59394","inReplyTo":"pull.1459.git.git.1678758818.gitgitgadget@gmail.com","subject":"[PATCH 1/2] diff: use HEAD for attributes when using bare repository","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-14T01:53:37Z","receivedAt":"2023-03-14T01:57:06Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\n(a4cf900e diff: teach diff to read algorithm from diff driver,\n2022-02-20) does not support bare repositories. Since running diff\non bare repositories is often done on Git servers, it would be useful to\nallow bare repositories to also take advantage of setting the algorithm\nvia the diff driver.\n\nTeach diff to check against the attributes from HEAD if a bare\nrepository is being operated on. This change also allows custom diff\ndrivers to work with bare repositories.\n\nSigned-off-by: John Cai \n---\n Documentation/gitattributes.txt | 4 ++++\n diff.c | 33 +++++++++++++++++++++++++++++----\n t/lib-diff-alternative.sh | 10 ++++++++++\n t/t4018-diff-funcname.sh | 11 +++++++++++\n userdiff.c | 9 ++++++++-\n userdiff.h | 4 ++++\n 6 files changed, 66 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 39bfbca1ffe..15488bd92b2 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -758,6 +758,8 @@ with the above configuration, i.e. `j-c-diff`, with 7\n parameters, just like `GIT_EXTERNAL_DIFF` program is called.\n See linkgit:git[1] for details.\n \n+When using a bare repository, the gitattributes from HEAD will be used.\n+\n Setting the internal diff algorithm\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n \n@@ -785,6 +787,8 @@ This diff algorithm applies to user facing diff output like git-diff(1),\n git-show(1) and is used for the `--stat` output as well. The merge machinery\n will not use the diff algorithm set through this method.\n \n+When using a bare repository, the gitattributes from HEAD will be used.\n+\n NOTE: If `diff..command` is defined for path with the\n `diff=` attribute, it is executed as an external diff driver\n (see above), and adding `diff..algorithm` has no effect, as the\ndiff --git a/diff.c b/diff.c\nindex 469e18aed20..51baf893bb0 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -29,6 +29,7 @@\n #include \"promisor-remote.h\"\n #include \"dir.h\"\n #include \"strmap.h\"\n+#include \"tree.h\"\n \n #ifdef NO_FAST_WORKING_DIRECTORY\n #define FAST_WORKING_DIRECTORY 0\n@@ -4443,6 +4444,27 @@ static void fill_metainfo(struct strbuf *msg,\n \t}\n }\n \n+static void get_userdiff(struct diff_options *o,\n+\t\t\t struct userdiff_driver **drv,\n+\t\t\t const char *attr_path)\n+{\n+\tconst char *commit = \"HEAD\";\n+\tstruct object_id *tree_oid = NULL;\n+\n+\tif (is_bare_repository() && o->repo->gitdir) {\n+\t\tstruct object_id oid;\n+\n+\t\tif (!get_oid(commit, &oid)) {\n+\t\t\tstruct tree *t = parse_tree_indirect(&oid);\n+\n+\t\t\tif (t)\n+\t\t\t\ttree_oid = &t->object.oid;\n+\t\t}\n+\t}\n+\n+\t*drv = userdiff_find_by_tree_and_path(o->repo->index, tree_oid, attr_path);\n+}\n+\n static void run_diff_cmd(const char *pgm,\n \t\t\t const char *name,\n \t\t\t const char *other,\n@@ -4458,8 +4480,10 @@ static void run_diff_cmd(const char *pgm,\n \tint must_show_header = 0;\n \tstruct userdiff_driver *drv = NULL;\n \n-\tif (o->flags.allow_external || !o->ignore_driver_algorithm)\n-\t\tdrv = userdiff_find_by_path(o->repo->index, attr_path);\n+\tif (o->flags.allow_external || !o->ignore_driver_algorithm) {\n+\n+\t\tget_userdiff(o, &drv, attr_path);\n+\t}\n \n \tif (o->flags.allow_external && drv && drv->external)\n \t\tpgm = drv->external;\n@@ -4586,8 +4610,9 @@ static void run_diffstat(struct diff_filepair *p, struct diff_options *o,\n \tconst char *other;\n \n \tif (!o->ignore_driver_algorithm) {\n-\t\tstruct userdiff_driver *drv = userdiff_find_by_path(o->repo->index,\n-\t\t\t\t\t\t\t\t p->one->path);\n+\t\tstruct userdiff_driver *drv = NULL;\n+\n+\t\tget_userdiff(o, &drv, p->one->path);\n \n \t\tif (drv && drv->algorithm)\n \t\t\tset_diff_algorithm(o, drv->algorithm);\ndiff --git a/t/lib-diff-alternative.sh b/t/lib-diff-alternative.sh\nindex a8f5d3274a5..0d99af83dd2 100644\n--- a/t/lib-diff-alternative.sh\n+++ b/t/lib-diff-alternative.sh\n@@ -121,6 +121,16 @@ EOF\n \t\ttest_cmp expect output\n \t'\n \n+\ttest_expect_success \"$STRATEGY diff from attributes with bare repo\" '\n+\t\techo \"file* diff=driver\" >.gitattributes &&\n+\t\tgit add file1 file2 .gitattributes &&\n+\t\tgit commit -m \"adding files\" &&\n+\t\tgit clone --bare --no-local . bare.git &&\n+\t\tgit -C bare.git config diff.driver.algorithm \"$STRATEGY\" &&\n+\t\tgit -C bare.git diff HEAD:file1 HEAD:file2 > output &&\n+\t\ttest_cmp expect output\n+\t'\n+\n \ttest_expect_success \"$STRATEGY diff from attributes has valid diffstat\" '\n \t\techo \"file* diff=driver\" >.gitattributes &&\n \t\tgit config diff.driver.algorithm \"$STRATEGY\" &&\ndiff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh\nindex 42a2b9a13b7..451af08c611 100755\n--- a/t/t4018-diff-funcname.sh\n+++ b/t/t4018-diff-funcname.sh\n@@ -63,6 +63,17 @@ do\n \t\ttest_i18ngrep ! fatal msg &&\n \t\ttest_i18ngrep ! error msg\n \t'\n+\ttest_expect_success \"builtin $p pattern compiles on bare repo\" '\n+\t\ttest_when_finished \"rm -rf bare.git\" &&\n+\t\techo \"*.java diff=$p\" >.gitattributes &&\n+\t\tgit add . &&\n+\t\tgit commit -am \"adding files\" &&\n+\t\tgit clone --bare --no-local . bare.git &&\n+\t\ttest_expect_code 1 git -C bare.git diff --exit-code \\\n+\t\t\tHEAD:A.java HEAD:B.java 2>msg &&\n+\t\ttest_i18ngrep ! fatal msg &&\n+\t\ttest_i18ngrep ! error msg\n+\t'\n done\n \n test_expect_success 'last regexp must not be negated' '\ndiff --git a/userdiff.c b/userdiff.c\nindex 58a3d59ef8f..2305d363244 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -408,6 +408,13 @@ struct userdiff_driver *userdiff_find_by_name(const char *name)\n \n struct userdiff_driver *userdiff_find_by_path(struct index_state *istate,\n \t\t\t\t\t const char *path)\n+{\n+\treturn userdiff_find_by_tree_and_path(istate, NULL, path);\n+}\n+\n+struct userdiff_driver *userdiff_find_by_tree_and_path(struct index_state *istate,\n+\t\t\t\t\t\t const struct object_id *tree_oid,\n+\t\t\t\t\t\t const char *path)\n {\n \tstatic struct attr_check *check;\n \n@@ -415,7 +422,7 @@ struct userdiff_driver *userdiff_find_by_path(struct index_state *istate,\n \t\tcheck = attr_check_initl(\"diff\", NULL);\n \tif (!path)\n \t\treturn NULL;\n-\tgit_check_attr(istate, NULL, path, check);\n+\tgit_check_attr(istate, tree_oid, path, check);\n \n \tif (ATTR_TRUE(check->items[0].value))\n \t\treturn &driver_true;\ndiff --git a/userdiff.h b/userdiff.h\nindex 24419db6973..f9cd7c238de 100644\n--- a/userdiff.h\n+++ b/userdiff.h\n@@ -33,6 +33,10 @@ int userdiff_config(const char *k, const char *v);\n struct userdiff_driver *userdiff_find_by_name(const char *name);\n struct userdiff_driver *userdiff_find_by_path(struct index_state *istate,\n \t\t\t\t\t const char *path);\n+struct userdiff_driver *userdiff_find_by_tree_and_path(struct index_state *istate,\n+\t\t\t\t\t\t const struct object_id *tree_oid,\n+\t\t\t\t\t\t const char *path);\n+\n \n /*\n * Initialize any textconv-related fields in the driver and return it, or NULL\n-- \ngitgitgadget\n\n"},{"id":"473501","messageId":"xmqqttynqnnj.fsf@gitster.g","threadId":"59394","inReplyTo":"0fc704cf1c0724473a61086098d44c3a82938b03.1678758818.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] diff: use HEAD for attributes when using bare repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-14T16:54:24Z","receivedAt":"2023-03-14T16:54:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> From: John Cai \n>\n> (a4cf900e diff: teach diff to read algorithm from diff driver,\n> 2022-02-20) does not support bare repositories.\n\nWrong placement of opening parenthesis, i.e. \"a4cf900e (diff: teach\ndiff to read algorithm from diff driver, 2023-02-20)\". The date is\nalso wrong. Use \"git show -s --format=reference a4cf900e\". Insert\nits output directly to your editor instead of transcribing manually.\n\nI do not think the commit is to blame in the first place for two\nreasons.\n\n * Even in a bare repository, the attributes defined in the\n $GIT_DIR/info/attributes file are in effect, and the new feature\n added by a4cf900e should work just fine with it.\n\n * By definition, there is no working tree in a bare repository, and\n it always has been naturally expected we won't read attributes\n from working tree files. This is in no way limited to the diff\n driver feature recently added.\n\nSo the above does not look like a good first sentence to explain\nthis change.\n\n> Since running diff\n> on bare repositories is often done on Git servers, it would be useful to\n> allow bare repositories to also take advantage of setting the algorithm\n> via the diff driver.\n\nSince referring to in-tree attributes is often useful with any\ncommand, not limited to \"diff\", I wonder if it is feasible to add\nsupport for the --attr-source= option globally, instead of\nimplementing such an option piecemeal. If your \"git diff\" in a bare\nrepository starts honoring attributes recorded in HEAD, don't your\nusers expect your \"git log\" and \"git show\" to also honor them?\n"},{"id":"473502","messageId":"ddd7cbb6-4f4d-ba0e-857e-5981779326d0@gmail.com","threadId":"59394","inReplyTo":"pull.1459.git.git.1678758818.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] diff: support bare repositories when reading gitattributes for diff algorithm","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2023-03-14T17:21:50Z","receivedAt":"2023-03-14T17:21:56Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi John,\n\nLe 2023-03-13 à 21:53, John Cai via GitGitGadget a écrit :\n> This patch series adds support for bare repositories for the feature added\n> in [1]. When using a bare repository, by default we will look for\n> gitattributes from HEAD. When the --attr-source option is passed, we will\n> try to read gitattributes from the commit.\n\nWhen I read that I immediately thought of the config settings 'mailmap.file', \n'mailmap.blob' which allow the same sort of thing, but for the mailmap.\n\nFor the sake of consistency of the whole system, maybe we would want to support\n'attr.file' and 'attr.blob' ? In that case, maybe we don't need a new command\nline option at all...\n\nCheers,\n\nPhilippe.\n"},{"id":"473503","messageId":"xmqqa60fqld6.fsf@gitster.g","threadId":"59394","inReplyTo":"xmqqttynqnnj.fsf@gitster.g","subject":"Re: [PATCH 1/2] diff: use HEAD for attributes when using bare repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-14T17:43:49Z","receivedAt":"2023-03-14T17:43:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Since referring to in-tree attributes is often useful with any\n> command, not limited to \"diff\", I wonder if it is feasible to add\n> support for the --attr-source= option globally, instead of\n> implementing such an option piecemeal. If your \"git diff\" in a bare\n> repository starts honoring attributes recorded in HEAD, don't your\n> users expect your \"git log\" and \"git show\" to also honor them?\n\nJust for illustration, here is one way to do so.\n\nThe implementation goes in the opposite direction from the more\nrecent trend, which is why I am not making it an official patch, but\nwith this you can do things like:\n\n $ git --attr-source=e83c5163 check-attr whitespace cache.h\n cache.h: whitespace: unspecified\n $ git --attr-source=e2f6331a142^ check-attr whitespace cache.h\n cache.h: whitespace: set\n $ git --attr-source=HEAD check-attr whitespace cache.h\n cache.h: whitespace: indent,trail,space\n\nwhere e83c5163 is the very first version of Git from 2005 where\n.gitattributes did not exist, e2f6331a142^ is the last version\nbefore we set whitespace to indent,trail,space, and HEAD is a more\nrecent version of our source tree.\n\nIn the following illustration patch, the attr-source tree object\nname is kept as a string as long as possible and at the very last\nminute when we call git_check_attr() for the first time we turn it\ninto an object id. This is because at the very early stage when we\nparse the global option we may not even know where our repository is\n(hence do not have enough information to parse HEAD). We also figure\nout is_bare_repository() late in the process for the same reason.\n\n\n\n attr.c | 29 +++++++++++++++++++++++++++++\n attr.h | 6 ++++++\n git.c | 3 +++\n 3 files changed, 38 insertions(+)\n\ndiff --git c/attr.c w/attr.c\nindex 1053dfcd4b..2fd6e0eab2 100644\n--- c/attr.c\n+++ w/attr.c\n@@ -1165,12 +1165,41 @@ static void collect_some_attrs(struct index_state *istate,\n \tfill(path, pathlen, basename_offset, check->stack, check->all_attrs, rem);\n }\n \n+static const char *default_attr_source_tree_object_name;\n+\n+void set_git_attr_source(const char *tree_object_name)\n+{\n+\tdefault_attr_source_tree_object_name = xstrdup(tree_object_name);\n+}\n+\n+\n+static struct object_id *default_attr_source(void)\n+{\n+\tstatic struct object_id attr_source;\n+\n+\tif (is_null_oid(&attr_source)) {\n+\t\tif (!default_attr_source_tree_object_name &&\n+\t\t is_bare_repository())\n+\t\t\tdefault_attr_source_tree_object_name = \"HEAD\";\n+\n+\t\tif (default_attr_source_tree_object_name &&\n+\t\t is_null_oid(&attr_source))\n+\t\t\tget_oid(default_attr_source_tree_object_name, &attr_source);\n+\t}\n+\tif (is_null_oid(&attr_source))\n+\t\treturn NULL;\n+\treturn &attr_source;\n+}\n+\n void git_check_attr(struct index_state *istate,\n \t\t const struct object_id *tree_oid, const char *path,\n \t\t struct attr_check *check)\n {\n \tint i;\n \n+\tif (!tree_oid)\n+\t\ttree_oid = default_attr_source();\n+\n \tcollect_some_attrs(istate, tree_oid, path, check);\n \n \tfor (i = 0; i < check->nr; i++) {\ndiff --git c/attr.h w/attr.h\nindex 9884ea2bc6..a77e3713b2 100644\n--- c/attr.h\n+++ w/attr.h\n@@ -135,6 +135,12 @@ struct git_attr;\n struct all_attrs_item;\n struct attr_stack;\n \n+/*\n+ * The textual object name for the tree-ish used by git_check_attr()\n+ * when NULL is given to tree_oid.\n+ */\n+void set_git_attr_source(const char *);\n+\n /*\n * Given a string, return the gitattribute object that\n * corresponds to it.\ndiff --git c/git.c w/git.c\nindex 6171fd6769..21bddc5718 100644\n--- c/git.c\n+++ w/git.c\n@@ -4,6 +4,7 @@\n #include \"help.h\"\n #include \"run-command.h\"\n #include \"alias.h\"\n+#include \"attr.h\"\n #include \"shallow.h\"\n \n #define RUN_SETUP\t\t(1<<0)\n@@ -307,6 +308,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t} else {\n \t\t\t\texit(list_cmds(cmd));\n \t\t\t}\n+\t\t} else if (skip_prefix(cmd, \"--attr-source=\", &cmd)) {\n+\t\t\tset_git_attr_source(cmd);\n \t\t} else {\n \t\t\tfprintf(stderr, _(\"unknown option: %s\\n\"), cmd);\n \t\t\tusage(git_usage_string);\n"},{"id":"473504","messageId":"20230314191833.qmiisyvsu2ppu4sh@pop-os","threadId":"59394","inReplyTo":"ddd7cbb6-4f4d-ba0e-857e-5981779326d0@gmail.com","subject":"Re: [PATCH 0/2] diff: support bare repositories when reading gitattributes for diff algorithm","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2023-03-14T19:18:33Z","receivedAt":"2023-03-14T19:18:40Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"On 23/03/14 01:21PM, Philippe Blain wrote:\n> Hi John,\n\nHi Philippe,\n\n> \n> Le 2023-03-13 à 21:53, John Cai via GitGitGadget a écrit :\n> > This patch series adds support for bare repositories for the feature added\n> > in [1]. When using a bare repository, by default we will look for\n> > gitattributes from HEAD. When the --attr-source option is passed, we will\n> > try to read gitattributes from the commit.\n> \n> When I read that I immediately thought of the config settings 'mailmap.file', \n> 'mailmap.blob' which allow the same sort of thing, but for the mailmap.\n> \n> For the sake of consistency of the whole system, maybe we would want to support\n> 'attr.file' and 'attr.blob' ? In that case, maybe we don't need a new command\n> line option at all...\n\nI wasn't aware of those config options. Indeed that's a good option! My only\nconcern with that is that there is already some precedence for passing a\n as a source for attributes in [1], so I thought adding a command line\noption would be somewhat consistent.\n\nBut I can also see the benefit of using a config value since there is also\nprecedence because of mailmap.file and mailmap.blob. Not sure what others think,\nbut this may be the less invasive way to go.\n\n1. https://git-scm.com/docs/git-check-attr#Documentation/git-check-attr.txt---sourcelttree-ishgt\n\n> \n> Cheers,\n> \n> Philippe.\n"},{"id":"473505","messageId":"20230314193839.wevenngf324pijhb@pop-os","threadId":"59394","inReplyTo":"xmqqa60fqld6.fsf@gitster.g","subject":"Re: [PATCH 1/2] diff: use HEAD for attributes when using bare repository","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2023-03-14T19:38:39Z","receivedAt":"2023-03-14T19:38:47Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"On 23/03/14 10:43AM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n\nHi Junio,\n\n> \n> > Since referring to in-tree attributes is often useful with any\n> > command, not limited to \"diff\", I wonder if it is feasible to add\n> > support for the --attr-source= option globally, instead of\n> > implementing such an option piecemeal. If your \"git diff\" in a bare\n> > repository starts honoring attributes recorded in HEAD, don't your\n> > users expect your \"git log\" and \"git show\" to also honor them?\n> \n> Just for illustration, here is one way to do so.\n> \n> The implementation goes in the opposite direction from the more\n> recent trend, which is why I am not making it an official patch, but\n\nCould you explain why this goes against the \"more recent trend\" for my\nunderstanding?\n\n> with this you can do things like:\n> \n> $ git --attr-source=e83c5163 check-attr whitespace cache.h\n> cache.h: whitespace: unspecified\n> $ git --attr-source=e2f6331a142^ check-attr whitespace cache.h\n> cache.h: whitespace: set\n> $ git --attr-source=HEAD check-attr whitespace cache.h\n> cache.h: whitespace: indent,trail,space\n\nI like the idea of an option that is global. For git-check-attr however, we\nalready have a --source option. Not sure how big of a deal that is. If we wanted\nto, we could put --source on a deprecation path by making it hidden for a\nwhile, etc.\n\n> \n> where e83c5163 is the very first version of Git from 2005 where\n> .gitattributes did not exist, e2f6331a142^ is the last version\n> before we set whitespace to indent,trail,space, and HEAD is a more\n> recent version of our source tree.\n> \n> In the following illustration patch, the attr-source tree object\n> name is kept as a string as long as possible and at the very last\n> minute when we call git_check_attr() for the first time we turn it\n> into an object id. This is because at the very early stage when we\n> parse the global option we may not even know where our repository is\n> (hence do not have enough information to parse HEAD). We also figure\n> out is_bare_repository() late in the process for the same reason.\n> \n> \n> \n> attr.c | 29 +++++++++++++++++++++++++++++\n> attr.h | 6 ++++++\n> git.c | 3 +++\n> 3 files changed, 38 insertions(+)\n> \n> diff --git c/attr.c w/attr.c\n> index 1053dfcd4b..2fd6e0eab2 100644\n> --- c/attr.c\n> +++ w/attr.c\n> @@ -1165,12 +1165,41 @@ static void collect_some_attrs(struct index_state *istate,\n> \tfill(path, pathlen, basename_offset, check->stack, check->all_attrs, rem);\n> }\n> \n> +static const char *default_attr_source_tree_object_name;\n> +\n> +void set_git_attr_source(const char *tree_object_name)\n> +{\n> +\tdefault_attr_source_tree_object_name = xstrdup(tree_object_name);\n> +}\n> +\n> +\n> +static struct object_id *default_attr_source(void)\n> +{\n> +\tstatic struct object_id attr_source;\n> +\n> +\tif (is_null_oid(&attr_source)) {\n> +\t\tif (!default_attr_source_tree_object_name &&\n> +\t\t is_bare_repository())\n> +\t\t\tdefault_attr_source_tree_object_name = \"HEAD\";\n> +\n> +\t\tif (default_attr_source_tree_object_name &&\n> +\t\t is_null_oid(&attr_source))\n> +\t\t\tget_oid(default_attr_source_tree_object_name, &attr_source);\n> +\t}\n> +\tif (is_null_oid(&attr_source))\n> +\t\treturn NULL;\n> +\treturn &attr_source;\n> +}\n> +\n> void git_check_attr(struct index_state *istate,\n> \t\t const struct object_id *tree_oid, const char *path,\n> \t\t struct attr_check *check)\n> {\n> \tint i;\n> \n> +\tif (!tree_oid)\n> +\t\ttree_oid = default_attr_source();\n> +\n> \tcollect_some_attrs(istate, tree_oid, path, check);\n> \n> \tfor (i = 0; i < check->nr; i++) {\n> diff --git c/attr.h w/attr.h\n> index 9884ea2bc6..a77e3713b2 100644\n> --- c/attr.h\n> +++ w/attr.h\n> @@ -135,6 +135,12 @@ struct git_attr;\n> struct all_attrs_item;\n> struct attr_stack;\n> \n> +/*\n> + * The textual object name for the tree-ish used by git_check_attr()\n> + * when NULL is given to tree_oid.\n> + */\n> +void set_git_attr_source(const char *);\n> +\n> /*\n> * Given a string, return the gitattribute object that\n> * corresponds to it.\n> diff --git c/git.c w/git.c\n> index 6171fd6769..21bddc5718 100644\n> --- c/git.c\n> +++ w/git.c\n> @@ -4,6 +4,7 @@\n> #include \"help.h\"\n> #include \"run-command.h\"\n> #include \"alias.h\"\n> +#include \"attr.h\"\n> #include \"shallow.h\"\n> \n> #define RUN_SETUP\t\t(1<<0)\n> @@ -307,6 +308,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n> \t\t\t} else {\n> \t\t\t\texit(list_cmds(cmd));\n> \t\t\t}\n> +\t\t} else if (skip_prefix(cmd, \"--attr-source=\", &cmd)) {\n> +\t\t\tset_git_attr_source(cmd);\n> \t\t} else {\n> \t\t\tfprintf(stderr, _(\"unknown option: %s\\n\"), cmd);\n> \t\t\tusage(git_usage_string);\n"},{"id":"473506","messageId":"xmqqy1nzoyqr.fsf@gitster.g","threadId":"59394","inReplyTo":"20230314193839.wevenngf324pijhb@pop-os","subject":"Re: [PATCH 1/2] diff: use HEAD for attributes when using bare repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-14T20:37:48Z","receivedAt":"2023-03-14T20:37:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Cai writes:\n\n>> Just for illustration, here is one way to do so.\n>> \n>> The implementation goes in the opposite direction from the more\n>> recent trend, which is why I am not making it an official patch, but\n>\n> Could you explain why this goes against the \"more recent trend\" for my\n> understanding?\n\nThe illustration uses a global state.\n\nThe recent trend is to reduce reliance on global states and use the\nrepository object and others that hold such state through the\ncallchain.\n\nBut a new global variable that holds the fallback tree-ish object name\nwas a so convenient way to illustrate the core of the idea, without\nhaving to change many callchains.\n\n>> with this you can do things like:\n>> \n>> $ git --attr-source=e83c5163 check-attr whitespace cache.h\n>> cache.h: whitespace: unspecified\n>> $ git --attr-source=e2f6331a142^ check-attr whitespace cache.h\n>> cache.h: whitespace: set\n>> $ git --attr-source=HEAD check-attr whitespace cache.h\n>> cache.h: whitespace: indent,trail,space\n>\n> I like the idea of an option that is global. For git-check-attr however, we\n\nI guess I shouldn't have used check-attr to avoid confusion.\n\nThe point is that the internal mechanisms introduced by 47cfc9bd\n(attr: add flag `--source` to work with tree-ish, 2023-01-14), which\ntaught check-attr the --source option and is reused by this\nillustration patch, was a good idea, but its UI was a mistake. We\ndo not need per-command --source option the commit adds if we did\nthe global option from day one. Yes, I think we can deprecate the\n\"--source\" option from there, if we all prefer the global option\navenue. I _think_ git_all_attrs() needs to be told about the\ndefault attr-source trick (which I didn't touch in my illustration\npatch) before it happens, though.\n\nIf you used the mechanism in the illustration patch I gave you, and\nadjusted the test part of your patch to match (i.e. \"diff\" does not\nlearn \"--attr-source\" option, but \"git --attr-source=... diff\" is\nhow you make it read attributes from a tree-ish), would the result\nwork well, or do we need more work to make it usable? How well do\nother commands (e.g. \"git show\") work in the same test repository\nyou created in your version?\n\nThanks.\n"},{"id":"473508","messageId":"xmqqttynoyg4.fsf@gitster.g","threadId":"59394","inReplyTo":"20230314191833.qmiisyvsu2ppu4sh@pop-os","subject":"Re: [PATCH 0/2] diff: support bare repositories when reading gitattributes for diff algorithm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-14T20:44:11Z","receivedAt":"2023-03-14T20:44:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Cai writes:\n\n> I wasn't aware of those config options. Indeed that's a good option! My only\n> concern with that is that there is already some precedence for passing a\n> as a source for attributes in [1], so I thought adding a command line\n> option would be somewhat consistent.\n>\n> But I can also see the benefit of using a config value since there is also\n> precedence because of mailmap.file and mailmap.blob. Not sure what others think,\n> but this may be the less invasive way to go.\n\nI actually hate these one-off variables, and I wish we didn't do\nmailmap.file or mailmap.blob at all. Instead we could have taught\nthe machinery to read from $GIT_DIR/info/mailmap just like the\nexclude and the attribute machinery already knows to read excludea\nand attributes files in the directory.\n\nAs your \"per filetype (determined by the attribute) diff algorithm\nin a bare repository\" feature already depends on being able to use\nconfig (as the look-up is \"attributes determines the diff filetype\nname, and then diff..algo is looked up in the config\") to\nconfigure, it does not sound too bad to add the attribute info in\nthe $GIT_DIR/info/attributes file in your bare repository (and you'd\nset the algorithm in $GIT_DIR/config file there), and it would just\nwork without any new \"read from HEAD\" feature, I would presume?\n\nThanks.\n"},{"id":"473587","messageId":"ZBMn5T6zfKK+PYUe@coredump.intra.peff.net","threadId":"59394","inReplyTo":"xmqqttynoyg4.fsf@gitster.g","subject":"Re: [PATCH 0/2] diff: support bare repositories when reading gitattributes for diff algorithm","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-16T14:29:57Z","receivedAt":"2023-03-16T14:30:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 14, 2023 at 01:44:11PM -0700, Junio C Hamano wrote:\n\n> John Cai writes:\n> \n> > I wasn't aware of those config options. Indeed that's a good option! My only\n> > concern with that is that there is already some precedence for passing a\n> > as a source for attributes in [1], so I thought adding a command line\n> > option would be somewhat consistent.\n> >\n> > But I can also see the benefit of using a config value since there is also\n> > precedence because of mailmap.file and mailmap.blob. Not sure what others think,\n> > but this may be the less invasive way to go.\n> \n> I actually hate these one-off variables, and I wish we didn't do\n> mailmap.file or mailmap.blob at all. Instead we could have taught\n> the machinery to read from $GIT_DIR/info/mailmap just like the\n> exclude and the attribute machinery already knows to read excludea\n> and attributes files in the directory.\n\nI don't think $GIT_DIR/info/mailmap is a good substitute for\nmailmap.blob. The point there is to pull it from the repository, and\nyou'd have to do:\n\n git cat-file HEAD:.mailmap >.git/info/mailmap\n\nbefore each command to get the equivalent. It is even worse for\nattributes, because the in-tree representation is not even a single file\nthat is compatible with $GIT_DIR/ (there can be many attributes spread\nthroughout the tree).\n\nIt's true that mailmap.file, when specified in the local config, is\nredundant with a repo-level info/mailmap file. I always assumed that\npeople put it in their ~/.gitconfig, though, to cover multiple projects\n(just like we have core.attributesFile in addition to the in-repo\nmeta-file).\n\n> As your \"per filetype (determined by the attribute) diff algorithm\n> in a bare repository\" feature already depends on being able to use\n> config (as the look-up is \"attributes determines the diff filetype\n> name, and then diff..algo is looked up in the config\") to\n> configure, it does not sound too bad to add the attribute info in\n> the $GIT_DIR/info/attributes file in your bare repository (and you'd\n> set the algorithm in $GIT_DIR/config file there), and it would just\n> work without any new \"read from HEAD\" feature, I would presume?\n\nI don't think that's quite a substitute.\n\nIf you have a static list of attributes, that is OK (though you are\npresumably better off with /etc/gitattributes or similar that covers all\nrepos). But if you want to respect in-repo ones, you need to look at the\nwhole tree (and can't even just dump them out).\n\nIt does seem at first glance that you are limited to a static list of\nattributes, because any diff \"type\" you mention has to have a\ncorresponding entry in the config to do anything useful. But these lists\nare only loosely coupled. For example, you could have static entries\nlike this:\n\n # in config\n [diff \"json\"]\n algorithm = patience\n\n # in attributes\n *.json diff=json\n\nbut still tell users that \"json\" is a well-known name, and if they want\nto get the same treatment for an arbitrary file \"foo\" that they would\nget for \"foo.json\", they have to add attributes mapping it, like:\n\n foo diff=json\n\ninside their repo.\n\n-Peff\n"},{"id":"473595","messageId":"xmqq1qlomysj.fsf@gitster.g","threadId":"59394","inReplyTo":"ZBMn5T6zfKK+PYUe@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] diff: support bare repositories when reading gitattributes for diff algorithm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-16T16:44:12Z","receivedAt":"2023-03-16T16:44:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If you have a static list of attributes, that is OK (though you are\n> presumably better off with /etc/gitattributes or similar that covers all\n> repos).\n> ...\n> but still tell users that \"json\" is a well-known name, and if they want\n> to get the same treatment for an arbitrary file \"foo\" that they would\n> get for \"foo.json\", they have to add attributes mapping it, like:\n>\n> foo diff=json\n>\n> inside their repo.\n\nYes, you described the reason very well why we didn't add \"global\"\nor \"system-wide\" attribute files.\n\nThanks for commenting on my somewhat tongue-in-cheek \"devil's\nadvocate\" remark.\n\n"},{"id":"473596","messageId":"9E479AA2-2D54-4CAF-899B-B25894B746AA@gmail.com","threadId":"59394","inReplyTo":"xmqqy1nzoyqr.fsf@gitster.g","subject":"Re: [PATCH 1/2] diff: use HEAD for attributes when using bare repository","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2023-03-16T16:46:51Z","receivedAt":"2023-03-16T16:46:59Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Junio,\n\nOn 14 Mar 2023, at 16:37, Junio C Hamano wrote:\n\n> John Cai writes:\n>\n>>> Just for illustration, here is one way to do so.\n>>>\n>>> The implementation goes in the opposite direction from the more\n>>> recent trend, which is why I am not making it an official patch, but\n>>\n>> Could you explain why this goes against the \"more recent trend\" for my\n>> understanding?\n>\n> The illustration uses a global state.\n>\n> The recent trend is to reduce reliance on global states and use the\n> repository object and others that hold such state through the\n> callchain.\n\nAh I see--in that case, what would be a good object to put this state into? Mabe\nrepo_settings?\n\n>\n> But a new global variable that holds the fallback tree-ish object name\n> was a so convenient way to illustrate the core of the idea, without\n> having to change many callchains.\n>\n>>> with this you can do things like:\n>>>\n>>> $ git --attr-source=e83c5163 check-attr whitespace cache.h\n>>> cache.h: whitespace: unspecified\n>>> $ git --attr-source=e2f6331a142^ check-attr whitespace cache.h\n>>> cache.h: whitespace: set\n>>> $ git --attr-source=HEAD check-attr whitespace cache.h\n>>> cache.h: whitespace: indent,trail,space\n>>\n>> I like the idea of an option that is global. For git-check-attr however, we\n>\n> I guess I shouldn't have used check-attr to avoid confusion.\n>\n> The point is that the internal mechanisms introduced by 47cfc9bd\n> (attr: add flag `--source` to work with tree-ish, 2023-01-14), which\n> taught check-attr the --source option and is reused by this\n> illustration patch, was a good idea, but its UI was a mistake. We\n> do not need per-command --source option the commit adds if we did\n> the global option from day one. Yes, I think we can deprecate the\n> \"--source\" option from there, if we all prefer the global option\n> avenue. I _think_ git_all_attrs() needs to be told about the\n> default attr-source trick (which I didn't touch in my illustration\n> patch) before it happens, though.\n\nGood point. I think to have a global flag would be a better user experience.\n\n>\n> If you used the mechanism in the illustration patch I gave you, and\n> adjusted the test part of your patch to match (i.e. \"diff\" does not\n> learn \"--attr-source\" option, but \"git --attr-source=... diff\" is\n> how you make it read attributes from a tree-ish), would the result\n> work well, or do we need more work to make it usable? How well do\n> other commands (e.g. \"git show\") work in the same test repository\n> you created in your version?\n\nI think it works pretty smoothly. I adjusted my tests to work with this git\nflag, and I also tested git show and it works well.\n\nIn fact, your proposal would serve the needs of my patch independently of any of\nthe diff code, which is nice.\n\n>\n> Thanks.\n\nThanks,\nJohn\n"},{"id":"473615","messageId":"xmqqcz58i9ud.fsf@gitster.g","threadId":"59394","inReplyTo":"9E479AA2-2D54-4CAF-899B-B25894B746AA@gmail.com","subject":"Re: [PATCH 1/2] diff: use HEAD for attributes when using bare repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-16T22:56:42Z","receivedAt":"2023-03-16T22:56:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Cai writes:\n\n> Ah I see--in that case, what would be a good object to put this state into? Mabe\n> repo_settings?\n\nI personally think a global is good enough for a case like this. It\nis not like the code in the attribute subsystem is structured to\nwork in multiple repositories at the same time in the first place, so\nin repo_settings, the_repository, or elsewhere is just as good as\nhaving it in a plain old file-scope static, and even after moving it\ninto the_repository or something, the access will initially be done\nby referring to the_repository global (which may not even exist by\nthe time \"git --attr-source=\" gets parsed), and not by plumbing\na \"struct repository *\" parameter through the callchain.\n\nAnother thing to note is that there needs a mechanism to convey the\ntree object used to read the attributes from down to subprocesses,\ne.g. exporting the GIT_ATTR_SOURCE environment variable and make Git\nbehave as if it saw the \"git --attr-source=\" option when such\nan environment variable exists, or something. Otherwise, the custom\nattribute source would only take effect inside built-in commands.\n\nIn any case, here is what I have now, stealing some tests from your\npatch. I do not think I'll be working on it further at least for\nnow, so feel free to run with it. I am not adding the \"in a bare\nrepository, always read from HEAD unless told otherwise\", as I do\nnot see a good way to countermand it from a command line.\n\n----- >8 ----- cut here ----- >8 ----- cut here ----- >8 -----\nSubject: [PATCH] attr: teach \"--attr-source=\" global option to \"git\"\n\nEarlier, 47cfc9bd (attr: add flag `--source` to work with tree-ish,\n2023-01-14) taught \"git check-attr\" the \"--source=\" option to\nallow it to read attribute files from a tree-ish, but did so only\nfor the command. Just like \"check-attr\" users wanted a way to use\nattributes from a tree-ish and not from the working tree files,\nusers of other commands (like \"git diff\") would benefit from the\nsame.\n\nUndo most of the UI change the commit made, while keeping the\ninternal logic to read attributes from a given tree-ish. Expose the\ninternal logic via a new \"--attr-source=\" command line option\ngiven to \"git\", so that it can be used with any git command that\nruns internally.\n\nThe tests were stolen and adjusted from John Cai's effort where only\n\"git diff\" learned the \"--attr-source\" option to read from a\ntree-ish.\n\nSigned-off-by: Junio C Hamano \n---\n archive.c | 2 +-\n attr.c | 34 ++++++++++++++++++++++++++++++++--\n attr.h | 13 +++++++++----\n builtin/check-attr.c | 17 ++++++++---------\n builtin/pack-objects.c | 2 +-\n convert.c | 2 +-\n git.c | 3 +++\n ll-merge.c | 4 ++--\n pathspec.c | 2 +-\n t/lib-diff-alternative.sh | 31 ++++++++++++++++++++++++++-----\n t/t0003-attributes.sh | 7 ++++++-\n t/t4018-diff-funcname.sh | 19 +++++++++++++++++++\n userdiff.c | 2 +-\n ws.c | 2 +-\n 14 files changed, 111 insertions(+), 29 deletions(-)\n\ndiff --git c/archive.c w/archive.c\nindex 9aeaf2bd87..385aa5f248 100644\n--- c/archive.c\n+++ w/archive.c\n@@ -120,7 +120,7 @@ static const struct attr_check *get_archive_attrs(struct index_state *istate,\n \tstatic struct attr_check *check;\n \tif (!check)\n \t\tcheck = attr_check_initl(\"export-ignore\", \"export-subst\", NULL);\n-\tgit_check_attr(istate, NULL, path, check);\n+\tgit_check_attr(istate, path, check);\n \treturn check;\n }\n \ndiff --git c/attr.c w/attr.c\nindex 1053dfcd4b..d34c7e9d54 100644\n--- c/attr.c\n+++ w/attr.c\n@@ -1165,11 +1165,40 @@ static void collect_some_attrs(struct index_state *istate,\n \tfill(path, pathlen, basename_offset, check->stack, check->all_attrs, rem);\n }\n \n+static const char *default_attr_source_tree_object_name;\n+\n+void set_git_attr_source(const char *tree_object_name)\n+{\n+\tdefault_attr_source_tree_object_name = xstrdup(tree_object_name);\n+}\n+\n+\n+static void compute_default_attr_source(struct object_id *attr_source)\n+{\n+\tif (!default_attr_source_tree_object_name || !is_null_oid(attr_source))\n+\t\treturn;\n+\n+\tif (get_oid_treeish(default_attr_source_tree_object_name, attr_source))\n+\t\tdie(_(\"bad --attr-source object\"));\n+}\n+\n+static struct object_id *default_attr_source(void)\n+{\n+\tstatic struct object_id attr_source;\n+\n+\tif (is_null_oid(&attr_source))\n+\t\tcompute_default_attr_source(&attr_source);\n+\tif (is_null_oid(&attr_source))\n+\t\treturn NULL;\n+\treturn &attr_source;\n+}\n+\n void git_check_attr(struct index_state *istate,\n-\t\t const struct object_id *tree_oid, const char *path,\n+\t\t const char *path,\n \t\t struct attr_check *check)\n {\n \tint i;\n+\tconst struct object_id *tree_oid = default_attr_source();\n \n \tcollect_some_attrs(istate, tree_oid, path, check);\n \n@@ -1182,10 +1211,11 @@ void git_check_attr(struct index_state *istate,\n \t}\n }\n \n-void git_all_attrs(struct index_state *istate, const struct object_id *tree_oid,\n+void git_all_attrs(struct index_state *istate,\n \t\t const char *path, struct attr_check *check)\n {\n \tint i;\n+\tconst struct object_id *tree_oid = default_attr_source();\n \n \tattr_check_reset(check);\n \tcollect_some_attrs(istate, tree_oid, path, check);\ndiff --git c/attr.h w/attr.h\nindex 9884ea2bc6..676bd17ce2 100644\n--- c/attr.h\n+++ w/attr.h\n@@ -45,7 +45,7 @@\n * const char *path;\n *\n * setup_check();\n- * git_check_attr(&the_index, tree_oid, path, check);\n+ * git_check_attr(&the_index, path, check);\n * ------------\n *\n * - Act on `.value` member of the result, left in `check->items[]`:\n@@ -120,7 +120,6 @@\n #define ATTR_MAX_FILE_SIZE (100 * 1024 * 1024)\n \n struct index_state;\n-struct object_id;\n \n /**\n * An attribute is an opaque object that is identified by its name. Pass the\n@@ -135,6 +134,12 @@ struct git_attr;\n struct all_attrs_item;\n struct attr_stack;\n \n+/*\n+ * The textual object name for the tree-ish used by git_check_attr()\n+ * to read attributes from (instead of from the working tree).\n+ */\n+void set_git_attr_source(const char *);\n+\n /*\n * Given a string, return the gitattribute object that\n * corresponds to it.\n@@ -203,14 +208,14 @@ void attr_check_free(struct attr_check *check);\n const char *git_attr_name(const struct git_attr *);\n \n void git_check_attr(struct index_state *istate,\n-\t\t const struct object_id *tree_oid, const char *path,\n+\t\t const char *path,\n \t\t struct attr_check *check);\n \n /*\n * Retrieve all attributes that apply to the specified path.\n * check holds the attributes and their values.\n */\n-void git_all_attrs(struct index_state *istate, const struct object_id *tree_oid,\n+void git_all_attrs(struct index_state *istate,\n \t\t const char *path, struct attr_check *check);\n \n enum git_attr_direction {\ndiff --git c/builtin/check-attr.c w/builtin/check-attr.c\nindex d7a40e674c..1a7929c980 100644\n--- c/builtin/check-attr.c\n+++ w/builtin/check-attr.c\n@@ -58,7 +58,7 @@ static void output_attr(struct attr_check *check, const char *file)\n }\n \n static void check_attr(const char *prefix, struct attr_check *check,\n-\t\t const struct object_id *tree_oid, int collect_all,\n+\t\t int collect_all,\n \t\t const char *file)\n \n {\n@@ -66,9 +66,9 @@ static void check_attr(const char *prefix, struct attr_check *check,\n \t\tprefix_path(prefix, prefix ? strlen(prefix) : 0, file);\n \n \tif (collect_all) {\n-\t\tgit_all_attrs(&the_index, tree_oid, full_path, check);\n+\t\tgit_all_attrs(&the_index, full_path, check);\n \t} else {\n-\t\tgit_check_attr(&the_index, tree_oid, full_path, check);\n+\t\tgit_check_attr(&the_index, full_path, check);\n \t}\n \toutput_attr(check, file);\n \n@@ -76,7 +76,7 @@ static void check_attr(const char *prefix, struct attr_check *check,\n }\n \n static void check_attr_stdin_paths(const char *prefix, struct attr_check *check,\n-\t\t\t\t const struct object_id *tree_oid, int collect_all)\n+\t\t\t\t int collect_all)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct strbuf unquoted = STRBUF_INIT;\n@@ -90,7 +90,7 @@ static void check_attr_stdin_paths(const char *prefix, struct attr_check *check,\n \t\t\t\tdie(\"line is badly quoted\");\n \t\t\tstrbuf_swap(&buf, &unquoted);\n \t\t}\n-\t\tcheck_attr(prefix, check, tree_oid, collect_all, buf.buf);\n+\t\tcheck_attr(prefix, check, collect_all, buf.buf);\n \t\tmaybe_flush_or_die(stdout, \"attribute to stdout\");\n \t}\n \tstrbuf_release(&buf);\n@@ -106,7 +106,6 @@ static NORETURN void error_with_usage(const char *msg)\n int cmd_check_attr(int argc, const char **argv, const char *prefix)\n {\n \tstruct attr_check *check;\n-\tstruct object_id *tree_oid = NULL;\n \tstruct object_id initialized_oid;\n \tint cnt, i, doubledash, filei;\n \n@@ -182,14 +181,14 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)\n \tif (source) {\n \t\tif (repo_get_oid_tree(the_repository, source, &initialized_oid))\n \t\t\tdie(\"%s: not a valid tree-ish source\", source);\n-\t\ttree_oid = &initialized_oid;\n+\t\tset_git_attr_source(source);\n \t}\n \n \tif (stdin_paths)\n-\t\tcheck_attr_stdin_paths(prefix, check, tree_oid, all_attrs);\n+\t\tcheck_attr_stdin_paths(prefix, check, all_attrs);\n \telse {\n \t\tfor (i = filei; i < argc; i++)\n-\t\t\tcheck_attr(prefix, check, tree_oid, all_attrs, argv[i]);\n+\t\t\tcheck_attr(prefix, check, all_attrs, argv[i]);\n \t\tmaybe_flush_or_die(stdout, \"attribute to stdout\");\n \t}\n \ndiff --git c/builtin/pack-objects.c w/builtin/pack-objects.c\nindex 74a167a180..d561541b8c 100644\n--- c/builtin/pack-objects.c\n+++ w/builtin/pack-objects.c\n@@ -1320,7 +1320,7 @@ static int no_try_delta(const char *path)\n \n \tif (!check)\n \t\tcheck = attr_check_initl(\"delta\", NULL);\n-\tgit_check_attr(the_repository->index, NULL, path, check);\n+\tgit_check_attr(the_repository->index, path, check);\n \tif (ATTR_FALSE(check->items[0].value))\n \t\treturn 1;\n \treturn 0;\ndiff --git c/convert.c w/convert.c\nindex a54d1690c0..9b67649032 100644\n--- c/convert.c\n+++ w/convert.c\n@@ -1308,7 +1308,7 @@ void convert_attrs(struct index_state *istate,\n \t\tgit_config(read_convert_config, NULL);\n \t}\n \n-\tgit_check_attr(istate, NULL, path, check);\n+\tgit_check_attr(istate, path, check);\n \tccheck = check->items;\n \tca->crlf_action = git_path_check_crlf(ccheck + 4);\n \tif (ca->crlf_action == CRLF_UNDEFINED)\ndiff --git c/git.c w/git.c\nindex 6171fd6769..21bddc5718 100644\n--- c/git.c\n+++ w/git.c\n@@ -4,6 +4,7 @@\n #include \"help.h\"\n #include \"run-command.h\"\n #include \"alias.h\"\n+#include \"attr.h\"\n #include \"shallow.h\"\n \n #define RUN_SETUP\t\t(1<<0)\n@@ -307,6 +308,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t} else {\n \t\t\t\texit(list_cmds(cmd));\n \t\t\t}\n+\t\t} else if (skip_prefix(cmd, \"--attr-source=\", &cmd)) {\n+\t\t\tset_git_attr_source(cmd);\n \t\t} else {\n \t\t\tfprintf(stderr, _(\"unknown option: %s\\n\"), cmd);\n \t\t\tusage(git_usage_string);\ndiff --git c/ll-merge.c w/ll-merge.c\nindex 130d26501c..22a603e8af 100644\n--- c/ll-merge.c\n+++ w/ll-merge.c\n@@ -391,7 +391,7 @@ enum ll_merge_result ll_merge(mmbuffer_t *result_buf,\n \t\tnormalize_file(theirs, path, istate);\n \t}\n \n-\tgit_check_attr(istate, NULL, path, check);\n+\tgit_check_attr(istate, path, check);\n \tll_driver_name = check->items[0].value;\n \tif (check->items[1].value) {\n \t\tmarker_size = atoi(check->items[1].value);\n@@ -419,7 +419,7 @@ int ll_merge_marker_size(struct index_state *istate, const char *path)\n \n \tif (!check)\n \t\tcheck = attr_check_initl(\"conflict-marker-size\", NULL);\n-\tgit_check_attr(istate, NULL, path, check);\n+\tgit_check_attr(istate, path, check);\n \tif (check->items[0].value) {\n \t\tmarker_size = atoi(check->items[0].value);\n \t\tif (marker_size <= 0)\ndiff --git c/pathspec.c w/pathspec.c\nindex ab70fcbe61..74e02c75fc 100644\n--- c/pathspec.c\n+++ w/pathspec.c\n@@ -730,7 +730,7 @@ int match_pathspec_attrs(struct index_state *istate,\n \tif (name[namelen])\n \t\tname = to_free = xmemdupz(name, namelen);\n \n-\tgit_check_attr(istate, NULL, name, item->attr_check);\n+\tgit_check_attr(istate, name, item->attr_check);\n \n \tfree(to_free);\n \ndiff --git c/t/lib-diff-alternative.sh w/t/lib-diff-alternative.sh\nindex a8f5d3274a..02381eb7f1 100644\n--- c/t/lib-diff-alternative.sh\n+++ w/t/lib-diff-alternative.sh\n@@ -112,15 +112,36 @@ EOF\n \n \tSTRATEGY=$1\n \n-\ttest_expect_success \"$STRATEGY diff from attributes\" '\n+\ttest_expect_success \"setup attributes files for tests with $STRATEGY\" '\n+\t\tgit checkout -b master &&\n \t\techo \"file* diff=driver\" >.gitattributes &&\n-\t\tgit config diff.driver.algorithm \"$STRATEGY\" &&\n-\t\ttest_must_fail git diff --no-index file1 file2 > output &&\n-\t\tcat expect &&\n-\t\tcat output &&\n+\t\tgit add file1 file2 .gitattributes &&\n+\t\tgit commit -m \"adding files\" &&\n+\t\tgit checkout -b branchA &&\n+\t\techo \"file* diff=driverA\" >.gitattributes &&\n+\t\tgit add .gitattributes &&\n+\t\tgit commit -m \"adding driverA as diff driver\" &&\n+\t\tgit checkout master &&\n+\t\tgit clone --bare --no-local . bare.git\n+\t'\n+\n+\ttest_expect_success \"$STRATEGY diff from attributes\" '\n+\t\ttest_must_fail git -c diff.driver.algorithm=$STRATEGY diff --no-index file1 file2 > output &&\n \t\ttest_cmp expect output\n \t'\n \n+\ttest_expect_success \"diff from attributes with bare repo when branch different than HEAD\" '\n+\t\tgit -C bare.git --attr-source=branchA -c diff.driver.algorithm=myers \\\n+\t\t\t-c diff.driverA.algorithm=$STRATEGY \\\n+\t\t\tdiff HEAD:file1 HEAD:file2 >output &&\n+\t\ttest_cmp expect output\n+\t'\n+\n+\ttest_expect_success \"diff from attributes with bare repo with invalid source\" '\n+\t\ttest_must_fail git -C bare.git --attr-source=invalid-branch diff \\\n+\t\t\tHEAD:file1 HEAD:file2\n+\t'\n+\n \ttest_expect_success \"$STRATEGY diff from attributes has valid diffstat\" '\n \t\techo \"file* diff=driver\" >.gitattributes &&\n \t\tgit config diff.driver.algorithm \"$STRATEGY\" &&\ndiff --git c/t/t0003-attributes.sh w/t/t0003-attributes.sh\nindex 89b306cb11..73db37a7f3 100755\n--- c/t/t0003-attributes.sh\n+++ w/t/t0003-attributes.sh\n@@ -30,8 +30,13 @@ attr_check_quote () {\n attr_check_source () {\n \tpath=\"$1\" expect=\"$2\" source=\"$3\" git_opts=\"$4\" &&\n \n-\tgit $git_opts check-attr --source $source test -- \"$path\" >actual 2>err &&\n \techo \"$path: test: $expect\" >expect &&\n+\n+\tgit $git_opts check-attr --source $source test -- \"$path\" >actual 2>err &&\n+\ttest_cmp expect actual &&\n+\ttest_must_be_empty err &&\n+\n+\tgit $git_opts --attr-source=\"$source\" check-attr test -- \"$path\" >actual 2>err &&\n \ttest_cmp expect actual &&\n \ttest_must_be_empty err\n }\ndiff --git c/t/t4018-diff-funcname.sh w/t/t4018-diff-funcname.sh\nindex 42a2b9a13b..c8d555771d 100755\n--- c/t/t4018-diff-funcname.sh\n+++ w/t/t4018-diff-funcname.sh\n@@ -63,6 +63,25 @@ do\n \t\ttest_i18ngrep ! fatal msg &&\n \t\ttest_i18ngrep ! error msg\n \t'\n+\n+\ttest_expect_success \"builtin $p pattern compiles on bare repo with --attr-source\" '\n+\t\ttest_when_finished \"rm -rf bare.git\" &&\n+\t\tgit checkout -B master &&\n+\t\tgit add . &&\n+\t\techo \"*.java diff=notexist\" >.gitattributes &&\n+\t\tgit add .gitattributes &&\n+\t\tgit commit -am \"changing gitattributes\" &&\n+\t\tgit checkout -B branchA &&\n+\t\techo \"*.java diff=$p\" >.gitattributes &&\n+\t\tgit add .gitattributes &&\n+\t\tgit commit -am \"changing gitattributes\" &&\n+\t\tgit clone --bare --no-local . bare.git &&\n+\t\tgit -C bare.git symbolic-ref HEAD refs/heads/master &&\n+\t\ttest_expect_code 1 git -C bare.git --attr-source=branchA \\\n+\t\t\tdiff --exit-code HEAD:A.java HEAD:B.java 2>msg &&\n+\t\ttest_i18ngrep ! fatal msg &&\n+\t\ttest_i18ngrep ! error msg\n+\t'\n done\n \n test_expect_success 'last regexp must not be negated' '\ndiff --git c/userdiff.c w/userdiff.c\nindex 58a3d59ef8..156660eaca 100644\n--- c/userdiff.c\n+++ w/userdiff.c\n@@ -415,7 +415,7 @@ struct userdiff_driver *userdiff_find_by_path(struct index_state *istate,\n \t\tcheck = attr_check_initl(\"diff\", NULL);\n \tif (!path)\n \t\treturn NULL;\n-\tgit_check_attr(istate, NULL, path, check);\n+\tgit_check_attr(istate, path, check);\n \n \tif (ATTR_TRUE(check->items[0].value))\n \t\treturn &driver_true;\ndiff --git c/ws.c w/ws.c\nindex da3d0e28cb..903bfcd53e 100644\n--- c/ws.c\n+++ w/ws.c\n@@ -79,7 +79,7 @@ unsigned whitespace_rule(struct index_state *istate, const char *pathname)\n \tif (!attr_whitespace_rule)\n \t\tattr_whitespace_rule = attr_check_initl(\"whitespace\", NULL);\n \n-\tgit_check_attr(istate, NULL, pathname, attr_whitespace_rule);\n+\tgit_check_attr(istate, pathname, attr_whitespace_rule);\n \tvalue = attr_whitespace_rule->items[0].value;\n \tif (ATTR_TRUE(value)) {\n \t\t/* true (whitespace) */\n"},{"id":"473631","messageId":"BE20C11F-135C-4A5B-9148-A88C3B2A6DF5@gmail.com","threadId":"59394","inReplyTo":"xmqqcz58i9ud.fsf@gitster.g","subject":"Re: [PATCH 1/2] diff: use HEAD for attributes when using bare repository","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2023-03-17T14:11:46Z","receivedAt":"2023-03-17T14:11:53Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Junio,\n\nOn 16 Mar 2023, at 18:56, Junio C Hamano wrote:\n\n> John Cai writes:\n>\n>> Ah I see--in that case, what would be a good object to put this state into? Mabe\n>> repo_settings?\n>\n> I personally think a global is good enough for a case like this. It\n> is not like the code in the attribute subsystem is structured to\n> work in multiple repositories at the same time in the first place, so\n> in repo_settings, the_repository, or elsewhere is just as good as\n> having it in a plain old file-scope static, and even after moving it\n> into the_repository or something, the access will initially be done\n> by referring to the_repository global (which may not even exist by\n> the time \"git --attr-source=\" gets parsed), and not by plumbing\n> a \"struct repository *\" parameter through the callchain.\n\nMakes sense.\n\n>\n> Another thing to note is that there needs a mechanism to convey the\n> tree object used to read the attributes from down to subprocesses,\n> e.g. exporting the GIT_ATTR_SOURCE environment variable and make Git\n> behave as if it saw the \"git --attr-source=\" option when such\n> an environment variable exists, or something. Otherwise, the custom\n> attribute source would only take effect inside built-in commands.\n>\n> In any case, here is what I have now, stealing some tests from your\n> patch. I do not think I'll be working on it further at least for\n> now, so feel free to run with it. I am not adding the \"in a bare\n> repository, always read from HEAD unless told otherwise\", as I do\n> not see a good way to countermand it from a command line.\n\nThanks for the patch! Looks like you took it most of the way. I'll take a look\nand make some additions if necessary.\n\nthanks\nJohn\n\n>\n> ----- >8 ----- cut here ----- >8 ----- cut here ----- >8 -----\n> Subject: [PATCH] attr: teach \"--attr-source=\" global option to \"git\"\n>\n> Earlier, 47cfc9bd (attr: add flag `--source` to work with tree-ish,\n> 2023-01-14) taught \"git check-attr\" the \"--source=\" option to\n> allow it to read attribute files from a tree-ish, but did so only\n> for the command. Just like \"check-attr\" users wanted a way to use\n> attributes from a tree-ish and not from the working tree files,\n> users of other commands (like \"git diff\") would benefit from the\n> same.\n>\n> Undo most of the UI change the commit made, while keeping the\n> internal logic to read attributes from a given tree-ish. Expose the\n> internal logic via a new \"--attr-source=\" command line option\n> given to \"git\", so that it can be used with any git command that\n> runs internally.\n>\n> The tests were stolen and adjusted from John Cai's effort where only\n> \"git diff\" learned the \"--attr-source\" option to read from a\n> tree-ish.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> archive.c | 2 +-\n> attr.c | 34 ++++++++++++++++++++++++++++++++--\n> attr.h | 13 +++++++++----\n> builtin/check-attr.c | 17 ++++++++---------\n> builtin/pack-objects.c | 2 +-\n> convert.c | 2 +-\n> git.c | 3 +++\n> ll-merge.c | 4 ++--\n> pathspec.c | 2 +-\n> t/lib-diff-alternative.sh | 31 ++++++++++++++++++++++++++-----\n> t/t0003-attributes.sh | 7 ++++++-\n> t/t4018-diff-funcname.sh | 19 +++++++++++++++++++\n> userdiff.c | 2 +-\n> ws.c | 2 +-\n> 14 files changed, 111 insertions(+), 29 deletions(-)\n>\n> diff --git c/archive.c w/archive.c\n> index 9aeaf2bd87..385aa5f248 100644\n> --- c/archive.c\n> +++ w/archive.c\n> @@ -120,7 +120,7 @@ static const struct attr_check *get_archive_attrs(struct index_state *istate,\n> \tstatic struct attr_check *check;\n> \tif (!check)\n> \t\tcheck = attr_check_initl(\"export-ignore\", \"export-subst\", NULL);\n> -\tgit_check_attr(istate, NULL, path, check);\n> +\tgit_check_attr(istate, path, check);\n> \treturn check;\n> }\n>\n> diff --git c/attr.c w/attr.c\n> index 1053dfcd4b..d34c7e9d54 100644\n> --- c/attr.c\n> +++ w/attr.c\n> @@ -1165,11 +1165,40 @@ static void collect_some_attrs(struct index_state *istate,\n> \tfill(path, pathlen, basename_offset, check->stack, check->all_attrs, rem);\n> }\n>\n> +static const char *default_attr_source_tree_object_name;\n> +\n> +void set_git_attr_source(const char *tree_object_name)\n> +{\n> +\tdefault_attr_source_tree_object_name = xstrdup(tree_object_name);\n> +}\n> +\n> +\n> +static void compute_default_attr_source(struct object_id *attr_source)\n> +{\n> +\tif (!default_attr_source_tree_object_name || !is_null_oid(attr_source))\n> +\t\treturn;\n> +\n> +\tif (get_oid_treeish(default_attr_source_tree_object_name, attr_source))\n> +\t\tdie(_(\"bad --attr-source object\"));\n> +}\n> +\n> +static struct object_id *default_attr_source(void)\n> +{\n> +\tstatic struct object_id attr_source;\n> +\n> +\tif (is_null_oid(&attr_source))\n> +\t\tcompute_default_attr_source(&attr_source);\n> +\tif (is_null_oid(&attr_source))\n> +\t\treturn NULL;\n> +\treturn &attr_source;\n> +}\n> +\n> void git_check_attr(struct index_state *istate,\n> -\t\t const struct object_id *tree_oid, const char *path,\n> +\t\t const char *path,\n> \t\t struct attr_check *check)\n> {\n> \tint i;\n> +\tconst struct object_id *tree_oid = default_attr_source();\n>\n> \tcollect_some_attrs(istate, tree_oid, path, check);\n>\n> @@ -1182,10 +1211,11 @@ void git_check_attr(struct index_state *istate,\n> \t}\n> }\n>\n> -void git_all_attrs(struct index_state *istate, const struct object_id *tree_oid,\n> +void git_all_attrs(struct index_state *istate,\n> \t\t const char *path, struct attr_check *check)\n> {\n> \tint i;\n> +\tconst struct object_id *tree_oid = default_attr_source();\n>\n> \tattr_check_reset(check);\n> \tcollect_some_attrs(istate, tree_oid, path, check);\n> diff --git c/attr.h w/attr.h\n> index 9884ea2bc6..676bd17ce2 100644\n> --- c/attr.h\n> +++ w/attr.h\n> @@ -45,7 +45,7 @@\n> * const char *path;\n> *\n> * setup_check();\n> - * git_check_attr(&the_index, tree_oid, path, check);\n> + * git_check_attr(&the_index, path, check);\n> * ------------\n> *\n> * - Act on `.value` member of the result, left in `check->items[]`:\n> @@ -120,7 +120,6 @@\n> #define ATTR_MAX_FILE_SIZE (100 * 1024 * 1024)\n>\n> struct index_state;\n> -struct object_id;\n>\n> /**\n> * An attribute is an opaque object that is identified by its name. Pass the\n> @@ -135,6 +134,12 @@ struct git_attr;\n> struct all_attrs_item;\n> struct attr_stack;\n>\n> +/*\n> + * The textual object name for the tree-ish used by git_check_attr()\n> + * to read attributes from (instead of from the working tree).\n> + */\n> +void set_git_attr_source(const char *);\n> +\n> /*\n> * Given a string, return the gitattribute object that\n> * corresponds to it.\n> @@ -203,14 +208,14 @@ void attr_check_free(struct attr_check *check);\n> const char *git_attr_name(const struct git_attr *);\n>\n> void git_check_attr(struct index_state *istate,\n> -\t\t const struct object_id *tree_oid, const char *path,\n> +\t\t const char *path,\n> \t\t struct attr_check *check);\n>\n> /*\n> * Retrieve all attributes that apply to the specified path.\n> * check holds the attributes and their values.\n> */\n> -void git_all_attrs(struct index_state *istate, const struct object_id *tree_oid,\n> +void git_all_attrs(struct index_state *istate,\n> \t\t const char *path, struct attr_check *check);\n>\n> enum git_attr_direction {\n> diff --git c/builtin/check-attr.c w/builtin/check-attr.c\n> index d7a40e674c..1a7929c980 100644\n> --- c/builtin/check-attr.c\n> +++ w/builtin/check-attr.c\n> @@ -58,7 +58,7 @@ static void output_attr(struct attr_check *check, const char *file)\n> }\n>\n> static void check_attr(const char *prefix, struct attr_check *check,\n> -\t\t const struct object_id *tree_oid, int collect_all,\n> +\t\t int collect_all,\n> \t\t const char *file)\n>\n> {\n> @@ -66,9 +66,9 @@ static void check_attr(const char *prefix, struct attr_check *check,\n> \t\tprefix_path(prefix, prefix ? strlen(prefix) : 0, file);\n>\n> \tif (collect_all) {\n> -\t\tgit_all_attrs(&the_index, tree_oid, full_path, check);\n> +\t\tgit_all_attrs(&the_index, full_path, check);\n> \t} else {\n> -\t\tgit_check_attr(&the_index, tree_oid, full_path, check);\n> +\t\tgit_check_attr(&the_index, full_path, check);\n> \t}\n> \toutput_attr(check, file);\n>\n> @@ -76,7 +76,7 @@ static void check_attr(const char *prefix, struct attr_check *check,\n> }\n>\n> static void check_attr_stdin_paths(const char *prefix, struct attr_check *check,\n> -\t\t\t\t const struct object_id *tree_oid, int collect_all)\n> +\t\t\t\t int collect_all)\n> {\n> \tstruct strbuf buf = STRBUF_INIT;\n> \tstruct strbuf unquoted = STRBUF_INIT;\n> @@ -90,7 +90,7 @@ static void check_attr_stdin_paths(const char *prefix, struct attr_check *check,\n> \t\t\t\tdie(\"line is badly quoted\");\n> \t\t\tstrbuf_swap(&buf, &unquoted);\n> \t\t}\n> -\t\tcheck_attr(prefix, check, tree_oid, collect_all, buf.buf);\n> +\t\tcheck_attr(prefix, check, collect_all, buf.buf);\n> \t\tmaybe_flush_or_die(stdout, \"attribute to stdout\");\n> \t}\n> \tstrbuf_release(&buf);\n> @@ -106,7 +106,6 @@ static NORETURN void error_with_usage(const char *msg)\n> int cmd_check_attr(int argc, const char **argv, const char *prefix)\n> {\n> \tstruct attr_check *check;\n> -\tstruct object_id *tree_oid = NULL;\n> \tstruct object_id initialized_oid;\n> \tint cnt, i, doubledash, filei;\n>\n> @@ -182,14 +181,14 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)\n> \tif (source) {\n> \t\tif (repo_get_oid_tree(the_repository, source, &initialized_oid))\n> \t\t\tdie(\"%s: not a valid tree-ish source\", source);\n> -\t\ttree_oid = &initialized_oid;\n> +\t\tset_git_attr_source(source);\n> \t}\n>\n> \tif (stdin_paths)\n> -\t\tcheck_attr_stdin_paths(prefix, check, tree_oid, all_attrs);\n> +\t\tcheck_attr_stdin_paths(prefix, check, all_attrs);\n> \telse {\n> \t\tfor (i = filei; i < argc; i++)\n> -\t\t\tcheck_attr(prefix, check, tree_oid, all_attrs, argv[i]);\n> +\t\t\tcheck_attr(prefix, check, all_attrs, argv[i]);\n> \t\tmaybe_flush_or_die(stdout, \"attribute to stdout\");\n> \t}\n>\n> diff --git c/builtin/pack-objects.c w/builtin/pack-objects.c\n> index 74a167a180..d561541b8c 100644\n> --- c/builtin/pack-objects.c\n> +++ w/builtin/pack-objects.c\n> @@ -1320,7 +1320,7 @@ static int no_try_delta(const char *path)\n>\n> \tif (!check)\n> \t\tcheck = attr_check_initl(\"delta\", NULL);\n> -\tgit_check_attr(the_repository->index, NULL, path, check);\n> +\tgit_check_attr(the_repository->index, path, check);\n> \tif (ATTR_FALSE(check->items[0].value))\n> \t\treturn 1;\n> \treturn 0;\n> diff --git c/convert.c w/convert.c\n> index a54d1690c0..9b67649032 100644\n> --- c/convert.c\n> +++ w/convert.c\n> @@ -1308,7 +1308,7 @@ void convert_attrs(struct index_state *istate,\n> \t\tgit_config(read_convert_config, NULL);\n> \t}\n>\n> -\tgit_check_attr(istate, NULL, path, check);\n> +\tgit_check_attr(istate, path, check);\n> \tccheck = check->items;\n> \tca->crlf_action = git_path_check_crlf(ccheck + 4);\n> \tif (ca->crlf_action == CRLF_UNDEFINED)\n> diff --git c/git.c w/git.c\n> index 6171fd6769..21bddc5718 100644\n> --- c/git.c\n> +++ w/git.c\n> @@ -4,6 +4,7 @@\n> #include \"help.h\"\n> #include \"run-command.h\"\n> #include \"alias.h\"\n> +#include \"attr.h\"\n> #include \"shallow.h\"\n>\n> #define RUN_SETUP\t\t(1<<0)\n> @@ -307,6 +308,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n> \t\t\t} else {\n> \t\t\t\texit(list_cmds(cmd));\n> \t\t\t}\n> +\t\t} else if (skip_prefix(cmd, \"--attr-source=\", &cmd)) {\n> +\t\t\tset_git_attr_source(cmd);\n> \t\t} else {\n> \t\t\tfprintf(stderr, _(\"unknown option: %s\\n\"), cmd);\n> \t\t\tusage(git_usage_string);\n> diff --git c/ll-merge.c w/ll-merge.c\n> index 130d26501c..22a603e8af 100644\n> --- c/ll-merge.c\n> +++ w/ll-merge.c\n> @@ -391,7 +391,7 @@ enum ll_merge_result ll_merge(mmbuffer_t *result_buf,\n> \t\tnormalize_file(theirs, path, istate);\n> \t}\n>\n> -\tgit_check_attr(istate, NULL, path, check);\n> +\tgit_check_attr(istate, path, check);\n> \tll_driver_name = check->items[0].value;\n> \tif (check->items[1].value) {\n> \t\tmarker_size = atoi(check->items[1].value);\n> @@ -419,7 +419,7 @@ int ll_merge_marker_size(struct index_state *istate, const char *path)\n>\n> \tif (!check)\n> \t\tcheck = attr_check_initl(\"conflict-marker-size\", NULL);\n> -\tgit_check_attr(istate, NULL, path, check);\n> +\tgit_check_attr(istate, path, check);\n> \tif (check->items[0].value) {\n> \t\tmarker_size = atoi(check->items[0].value);\n> \t\tif (marker_size <= 0)\n> diff --git c/pathspec.c w/pathspec.c\n> index ab70fcbe61..74e02c75fc 100644\n> --- c/pathspec.c\n> +++ w/pathspec.c\n> @@ -730,7 +730,7 @@ int match_pathspec_attrs(struct index_state *istate,\n> \tif (name[namelen])\n> \t\tname = to_free = xmemdupz(name, namelen);\n>\n> -\tgit_check_attr(istate, NULL, name, item->attr_check);\n> +\tgit_check_attr(istate, name, item->attr_check);\n>\n> \tfree(to_free);\n>\n> diff --git c/t/lib-diff-alternative.sh w/t/lib-diff-alternative.sh\n> index a8f5d3274a..02381eb7f1 100644\n> --- c/t/lib-diff-alternative.sh\n> +++ w/t/lib-diff-alternative.sh\n> @@ -112,15 +112,36 @@ EOF\n>\n> \tSTRATEGY=$1\n>\n> -\ttest_expect_success \"$STRATEGY diff from attributes\" '\n> +\ttest_expect_success \"setup attributes files for tests with $STRATEGY\" '\n> +\t\tgit checkout -b master &&\n> \t\techo \"file* diff=driver\" >.gitattributes &&\n> -\t\tgit config diff.driver.algorithm \"$STRATEGY\" &&\n> -\t\ttest_must_fail git diff --no-index file1 file2 > output &&\n> -\t\tcat expect &&\n> -\t\tcat output &&\n> +\t\tgit add file1 file2 .gitattributes &&\n> +\t\tgit commit -m \"adding files\" &&\n> +\t\tgit checkout -b branchA &&\n> +\t\techo \"file* diff=driverA\" >.gitattributes &&\n> +\t\tgit add .gitattributes &&\n> +\t\tgit commit -m \"adding driverA as diff driver\" &&\n> +\t\tgit checkout master &&\n> +\t\tgit clone --bare --no-local . bare.git\n> +\t'\n> +\n> +\ttest_expect_success \"$STRATEGY diff from attributes\" '\n> +\t\ttest_must_fail git -c diff.driver.algorithm=$STRATEGY diff --no-index file1 file2 > output &&\n> \t\ttest_cmp expect output\n> \t'\n>\n> +\ttest_expect_success \"diff from attributes with bare repo when branch different than HEAD\" '\n> +\t\tgit -C bare.git --attr-source=branchA -c diff.driver.algorithm=myers \\\n> +\t\t\t-c diff.driverA.algorithm=$STRATEGY \\\n> +\t\t\tdiff HEAD:file1 HEAD:file2 >output &&\n> +\t\ttest_cmp expect output\n> +\t'\n> +\n> +\ttest_expect_success \"diff from attributes with bare repo with invalid source\" '\n> +\t\ttest_must_fail git -C bare.git --attr-source=invalid-branch diff \\\n> +\t\t\tHEAD:file1 HEAD:file2\n> +\t'\n> +\n> \ttest_expect_success \"$STRATEGY diff from attributes has valid diffstat\" '\n> \t\techo \"file* diff=driver\" >.gitattributes &&\n> \t\tgit config diff.driver.algorithm \"$STRATEGY\" &&\n> diff --git c/t/t0003-attributes.sh w/t/t0003-attributes.sh\n> index 89b306cb11..73db37a7f3 100755\n> --- c/t/t0003-attributes.sh\n> +++ w/t/t0003-attributes.sh\n> @@ -30,8 +30,13 @@ attr_check_quote () {\n> attr_check_source () {\n> \tpath=\"$1\" expect=\"$2\" source=\"$3\" git_opts=\"$4\" &&\n>\n> -\tgit $git_opts check-attr --source $source test -- \"$path\" >actual 2>err &&\n> \techo \"$path: test: $expect\" >expect &&\n> +\n> +\tgit $git_opts check-attr --source $source test -- \"$path\" >actual 2>err &&\n> +\ttest_cmp expect actual &&\n> +\ttest_must_be_empty err &&\n> +\n> +\tgit $git_opts --attr-source=\"$source\" check-attr test -- \"$path\" >actual 2>err &&\n> \ttest_cmp expect actual &&\n> \ttest_must_be_empty err\n> }\n> diff --git c/t/t4018-diff-funcname.sh w/t/t4018-diff-funcname.sh\n> index 42a2b9a13b..c8d555771d 100755\n> --- c/t/t4018-diff-funcname.sh\n> +++ w/t/t4018-diff-funcname.sh\n> @@ -63,6 +63,25 @@ do\n> \t\ttest_i18ngrep ! fatal msg &&\n> \t\ttest_i18ngrep ! error msg\n> \t'\n> +\n> +\ttest_expect_success \"builtin $p pattern compiles on bare repo with --attr-source\" '\n> +\t\ttest_when_finished \"rm -rf bare.git\" &&\n> +\t\tgit checkout -B master &&\n> +\t\tgit add . &&\n> +\t\techo \"*.java diff=notexist\" >.gitattributes &&\n> +\t\tgit add .gitattributes &&\n> +\t\tgit commit -am \"changing gitattributes\" &&\n> +\t\tgit checkout -B branchA &&\n> +\t\techo \"*.java diff=$p\" >.gitattributes &&\n> +\t\tgit add .gitattributes &&\n> +\t\tgit commit -am \"changing gitattributes\" &&\n> +\t\tgit clone --bare --no-local . bare.git &&\n> +\t\tgit -C bare.git symbolic-ref HEAD refs/heads/master &&\n> +\t\ttest_expect_code 1 git -C bare.git --attr-source=branchA \\\n> +\t\t\tdiff --exit-code HEAD:A.java HEAD:B.java 2>msg &&\n> +\t\ttest_i18ngrep ! fatal msg &&\n> +\t\ttest_i18ngrep ! error msg\n> +\t'\n> done\n>\n> test_expect_success 'last regexp must not be negated' '\n> diff --git c/userdiff.c w/userdiff.c\n> index 58a3d59ef8..156660eaca 100644\n> --- c/userdiff.c\n> +++ w/userdiff.c\n> @@ -415,7 +415,7 @@ struct userdiff_driver *userdiff_find_by_path(struct index_state *istate,\n> \t\tcheck = attr_check_initl(\"diff\", NULL);\n> \tif (!path)\n> \t\treturn NULL;\n> -\tgit_check_attr(istate, NULL, path, check);\n> +\tgit_check_attr(istate, path, check);\n>\n> \tif (ATTR_TRUE(check->items[0].value))\n> \t\treturn &driver_true;\n> diff --git c/ws.c w/ws.c\n> index da3d0e28cb..903bfcd53e 100644\n> --- c/ws.c\n> +++ w/ws.c\n> @@ -79,7 +79,7 @@ unsigned whitespace_rule(struct index_state *istate, const char *pathname)\n> \tif (!attr_whitespace_rule)\n> \t\tattr_whitespace_rule = attr_check_initl(\"whitespace\", NULL);\n>\n> -\tgit_check_attr(istate, NULL, pathname, attr_whitespace_rule);\n> +\tgit_check_attr(istate, pathname, attr_whitespace_rule);\n> \tvalue = attr_whitespace_rule->items[0].value;\n> \tif (ATTR_TRUE(value)) {\n> \t\t/* true (whitespace) */\n"}]}