{"thread":{"id":"60250","subject":"[PATCH] attr: attr.allowInvalidSource config to allow invalid revision","startedAt":"2023-09-20T14:00:39Z","lastAt":"2023-10-19T15:43:30Z","messageCount":34,"participants":["John Cai via GitGitGadget","Junio C Hamano","Jeff King","John Cai","Jonathan Tan","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"482066","messageId":"pull.1577.git.git.1695218431033.gitgitgadget@gmail.com","threadId":"60250","inReplyTo":null,"subject":"[PATCH] attr: attr.allowInvalidSource config to allow invalid revision","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-09-20T14:00:30Z","receivedAt":"2023-09-20T14:00:39Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\n44451a2e5e (attr: teach \"--attr-source=\" global option to \"git\",\n2023-05-06) provided the ability to pass in a treeish as the attr\nsource. When a revision does not resolve to a valid tree is passed, Git\nwill die. GitLab keeps bare repositories and always reads attributes\nfrom the default branch, so we pass in HEAD to --attr-source.\n\nWith empty repositories however, HEAD does not point to a valid treeish,\ncausing Git to die. This means we would need to check for a valid\ntreeish each time. To avoid this, let's add a configuration that allows\nGit to simply ignore --attr-source if it does not resolve to a valid\ntree.\n\nSigned-off-by: John Cai \n---\n attr: attr.allowInvalidSource config to allow empty revision\n \n 44451a2e5e (attr: teach \"--attr-source=\" global option to \"git\",\n 2023-05-06) provided the ability to pass in a treeish as the attr\n source. When a revision does not resolve to a valid tree is passed, Git\n will die. GitLab keeps bare repositories and always reads attributes\n from the default branch, so we pass in HEAD to --attr-source.\n \n With empty repositories however, HEAD does not point to a valid treeish,\n causing Git to die. This means we would need to check for a valid\n treeish each time. To avoid this, let's add a configuration that allows\n Git to simply ignore --attr-source if it does not resolve to a valid\n tree.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1577%2Fjohn-cai%2Fjc%2Fconfig-attr-invalid-source-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1577/john-cai/jc/config-attr-invalid-source-v1\nPull-Request: https://github.com/git/git/pull/1577\n\n Documentation/config.txt | 2 ++\n Documentation/config/attr.txt | 6 ++++++\n attr.c | 11 +++++++++--\n t/t0003-attributes.sh | 36 +++++++++++++++++++++++++++++++++++\n 4 files changed, 53 insertions(+), 2 deletions(-)\n create mode 100644 Documentation/config/attr.txt\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 229b63a454c..b1891c2b5af 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -371,6 +371,8 @@ other popular tools, and describe them in your documentation.\n \n include::config/advice.txt[]\n \n+include::config/attr.txt[]\n+\n include::config/core.txt[]\n \n include::config/add.txt[]\ndiff --git a/Documentation/config/attr.txt b/Documentation/config/attr.txt\nnew file mode 100644\nindex 00000000000..2218f0c982a\n--- /dev/null\n+++ b/Documentation/config/attr.txt\n@@ -0,0 +1,6 @@\n+attr.allowInvalidSource::\n+\tIf `--attr-source` cannot resolve to a valid tree object, ignore\n+\t`--attr-source` instead of erroring out, and fall back to looking for\n+\tattributes in the default locations. Useful when passing `HEAD` into\n+\t`attr-source` since it allows `HEAD` to point to an unborn branch in\n+\tcases like an empty repository.\ndiff --git a/attr.c b/attr.c\nindex 71c84fbcf86..854a3720e3f 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -1208,8 +1208,15 @@ static void compute_default_attr_source(struct object_id *attr_source)\n \tif (!default_attr_source_tree_object_name || !is_null_oid(attr_source))\n \t\treturn;\n \n-\tif (repo_get_oid_treeish(the_repository, default_attr_source_tree_object_name, attr_source))\n-\t\tdie(_(\"bad --attr-source or GIT_ATTR_SOURCE\"));\n+\n+\tif (repo_get_oid_treeish(the_repository, default_attr_source_tree_object_name, attr_source)) {\n+\t\tint allow_invalid_attr_source = 0;\n+\n+\t\tgit_config_get_bool(\"attr.allowinvalidsource\", &allow_invalid_attr_source);\n+\n+\t\tif (!allow_invalid_attr_source)\n+\t\t\tdie(_(\"bad --attr-source or GIT_ATTR_SOURCE\"));\n+\t}\n }\n \n static struct object_id *default_attr_source(void)\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 26e082f05b4..3272237ee2b 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -342,6 +342,42 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '\n \t)\n '\n \n+bad_attr_source_err=\"fatal: bad --attr-source or GIT_ATTR_SOURCE\"\n+\n+test_expect_success 'attr.allowInvalidSource when HEAD is unborn' '\n+\ttest_when_finished rm -rf empty &&\n+\techo $bad_attr_source_err >expect_err &&\n+\techo \"f/path: test: unspecified\" >expect &&\n+\tgit init empty &&\n+\ttest_must_fail git -C empty --attr-source=HEAD check-attr test -- f/path 2>err &&\n+\ttest_cmp expect_err err &&\n+\tgit -C empty -c attr.allowInvalidSource=true --attr-source=HEAD check-attr test -- f/path >actual 2>err &&\n+\ttest_must_be_empty err &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'attr.allowInvalidSource when --attr-source points to non-existing ref' '\n+\ttest_when_finished rm -rf empty &&\n+\techo $bad_attr_source_err >expect_err &&\n+\techo \"f/path: test: unspecified\" >expect &&\n+\tgit init empty &&\n+\ttest_must_fail git -C empty --attr-source=refs/does/not/exist check-attr test -- f/path 2>err &&\n+\ttest_cmp expect_err err &&\n+\tgit -C empty -c attr.allowInvalidSource=true --attr-source=refs/does/not/exist check-attr test -- f/path >actual 2>err &&\n+\ttest_must_be_empty err &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'bad attr source defaults to reading .gitattributes file' '\n+\ttest_when_finished rm -rf empty &&\n+\tgit init empty &&\n+\techo \"f/path test=val\" >empty/.gitattributes &&\n+\techo \"f/path: test: val\" >expect &&\n+\tgit -C empty -c attr.allowInvalidSource=true --attr-source=HEAD check-attr test -- f/path >actual 2>err &&\n+\ttest_must_be_empty err &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'bare repository: with --source' '\n \t(\n \t\tcd bare.git &&\n\nbase-commit: 1fc548b2d6a3596f3e1c1f8b1930d8dbd1e30bf3\n-- \ngitgitgadget\n"},{"id":"482078","messageId":"xmqqfs38akx5.fsf@gitster.g","threadId":"60250","inReplyTo":"pull.1577.git.git.1695218431033.gitgitgadget@gmail.com","subject":"Re: [PATCH] attr: attr.allowInvalidSource config to allow invalid revision","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-20T16:06:46Z","receivedAt":"2023-09-20T16:06:56Z","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> 44451a2e5e (attr: teach \"--attr-source=\" global option to \"git\",\n> 2023-05-06) provided the ability to pass in a treeish as the attr\n> source. When a revision does not resolve to a valid tree is passed, Git\n> will die. GitLab keeps bare repositories and always reads attributes\n> from the default branch, so we pass in HEAD to --attr-source.\n\nMakes sense.\n\n> With empty repositories however, HEAD does not point to a valid treeish,\n> causing Git to die. This means we would need to check for a valid\n> treeish each time.\n\nNaturally.\n\n> To avoid this, let's add a configuration that allows\n> Git to simply ignore --attr-source if it does not resolve to a valid\n> tree.\n\nNot convincing at all as to the reason why we want to do anything\n\"to avoid this\". \"git log\" in a repository whose HEAD does not\npoint to a valid treeish. \"git blame\" dies with \"no such ref:\nHEAD\". An empty repository (more precisely, an unborn history)\nneeds special casing if you want to present it if you do not want to\nspew underlying error messages to the end users *anyway*. It is\nunclear why seeing what commit the HEAD pointer points at (or which\nbranch it points at for that matter) is *an* *extra* and *otherwise*\n*unnecessary* overhead that need to be avoided.\n\n\n"},{"id":"482096","messageId":"xmqq1qer7vrv.fsf@gitster.g","threadId":"60250","inReplyTo":"20230921041545.GA2338791@coredump.intra.peff.net","subject":"Re: [PATCH] attr: attr.allowInvalidSource config to allow invalid revision","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-21T08:52:52Z","receivedAt":"2023-09-21T18:14:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> In an empty repository, \"git log\" will die anyway. So I think the more\n> interesting case is \"I have a repository with stuff in it, but HEAD\n> points to an unborn branch\". So:\n>\n> git --attr-source=HEAD diff foo^ foo\n\nThis still looks like a made-up example. Who in the right mind\nwould specify HEAD when both of the revs involved in the operation\nare from branch 'foo'? The history of HEAD may not have anything\ncommon with the operand of the operation 'foo' (or its parent), or\nworse, it may not even exist.\n\nBut your \"in this repository we never trust attributes from working\ntree, take it instead from this file or from this blob\" example does\nmake a lot more sense as a use case.\n\n> And there you really are saying \"if there are attributes in HEAD, use\n> them; otherwise, don't worry about it\". This is exactly what we do with\n> mailmap.blob: in a bare repository it is set to HEAD by default, but if\n> HEAD does not resolve, we just ignore it (just like a HEAD that does not\n> contain a .mailmap file). And those match the non-bare cases, where we'd\n> read those files from the working tree instead.\n\n\"HEAD\" -> \"HEAD:.mailmap\" if I recall correctly.\n\nAnd if HEAD does not resolve, we pretend as if HEAD is an empty\ntree-ish (hence HEAD:.mailmap is missing). It becomes very tempting\nto do the same for the attribute sources and treat unborn HEAD as if\nit specifies an empty tree-ish, without any configuration or an\nextra option.\n\nSuch a change would be an end-user observable behaviour change, but\nnobody sane would be running \"git --attr-source=HEAD diff HEAD^ HEAD\"\nto check and detect an unborn HEAD for its error exit code, so I do\nnot think it is a horribly wrong thing to do.\n\nBut again, as you said, --attr-source= does not sound like\na good fit for bare-repository hosted environment and a tentative\nhack waiting for a proper attr.blob support, or something like that,\nto appear.\n\n> But what is weird about this patch is that we are using a config option\n> to change how a command-line option is interpreted. If the idea is that\n> some invocations care about the validity of the source and some do not,\n> then the config option is much too blunt. It is set once long ago, but\n> it can't distinguish between times you care about invalid sources and\n> times you don't.\n>\n> It would make much more sense to me to have another command-line option,\n> like:\n>\n> git --attr-source=HEAD --allow-invalid-attr-source\n\nYeah, if we were to make it configurable without changing the\ndefault behaviour, I agree that would be more correct approach. A\nconfiguration does not sound like a good fit.\n\n> ... And I really think attr.blob is a better match for what GitLab\n> is trying to do here, because it is set once and applies to all\n> commands, rather than having to teach every invocation to pass it\n> (though I guess maybe they use it as an environment variable).\n\nTrue, too.\n\nThanks.\n"},{"id":"482107","messageId":"20230921041545.GA2338791@coredump.intra.peff.net","threadId":"60250","inReplyTo":"xmqqfs38akx5.fsf@gitster.g","subject":"Re: [PATCH] attr: attr.allowInvalidSource config to allow invalid revision","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-21T04:15:45Z","receivedAt":"2023-09-21T20:51:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 20, 2023 at 09:06:46AM -0700, Junio C Hamano wrote:\n\n> > With empty repositories however, HEAD does not point to a valid treeish,\n> > causing Git to die. This means we would need to check for a valid\n> > treeish each time.\n> \n> Naturally.\n> \n> > To avoid this, let's add a configuration that allows\n> > Git to simply ignore --attr-source if it does not resolve to a valid\n> > tree.\n> \n> Not convincing at all as to the reason why we want to do anything\n> \"to avoid this\". \"git log\" in a repository whose HEAD does not\n> point to a valid treeish. \"git blame\" dies with \"no such ref:\n> HEAD\". An empty repository (more precisely, an unborn history)\n> needs special casing if you want to present it if you do not want to\n> spew underlying error messages to the end users *anyway*. It is\n> unclear why seeing what commit the HEAD pointer points at (or which\n> branch it points at for that matter) is *an* *extra* and *otherwise*\n> *unnecessary* overhead that need to be avoided.\n\nIn an empty repository, \"git log\" will die anyway. So I think the more\ninteresting case is \"I have a repository with stuff in it, but HEAD\npoints to an unborn branch\". So:\n\n git --attr-source=HEAD diff foo^ foo\n\nAnd there you really are saying \"if there are attributes in HEAD, use\nthem; otherwise, don't worry about it\". This is exactly what we do with\nmailmap.blob: in a bare repository it is set to HEAD by default, but if\nHEAD does not resolve, we just ignore it (just like a HEAD that does not\ncontain a .mailmap file). And those match the non-bare cases, where we'd\nread those files from the working tree instead.\n\nSo I think the same notion applies here. You want to be able to point it\nat HEAD by default, but if there is no HEAD, that is the same as if HEAD\nsimply did not contain any attributes. If we had attr.blob, that is\nexactly how I would expect it to work.\n\nMy gut feeling is that --attr-source should do the same, and just\nquietly ignore a ref that does not resolve. But I think an argument can\nbe made that because the caller explicitly gave us a ref, they expect it\nto work (and that would catch misspellings, etc). Like:\n\n git --attr-source=my-barnch diff foo^ foo\n\nSo I'm OK with not changing that behavior.\n\nBut what is weird about this patch is that we are using a config option\nto change how a command-line option is interpreted. If the idea is that\nsome invocations care about the validity of the source and some do not,\nthen the config option is much too blunt. It is set once long ago, but\nit can't distinguish between times you care about invalid sources and\ntimes you don't.\n\nIt would make much more sense to me to have another command-line option,\nlike:\n\n git --attr-source=HEAD --allow-invalid-attr-source\n\nObviously that is horrible to type, but I think the point is that you'd\nonly do this from a script anyway (because it's those automated cases\nwhere you want to say \"use HEAD only if it exists\").\n\nIf there were an attr.blob config option and it complained about an\ninvalid HEAD, _then_ I think attr.allowInvalidSource might make sense\n(though again, I would just argue for switching the behavior by\ndefault). And I really think attr.blob is a better match for what GitLab\nis trying to do here, because it is set once and applies to all\ncommands, rather than having to teach every invocation to pass it\n(though I guess maybe they use it as an environment variable).\n\nOf course I would think that, as the person who solved GitHub's exact\nsame problem for mailmap by adding mailmap.blob. So you may ingest the\nappropriate grain of salt. :)\n\n-Peff\n"},{"id":"482120","messageId":"20230921214021.GB302338@coredump.intra.peff.net","threadId":"60250","inReplyTo":"xmqq1qer7vrv.fsf@gitster.g","subject":"Re: [PATCH] attr: attr.allowInvalidSource config to allow invalid revision","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-21T21:40:21Z","receivedAt":"2023-09-21T22:25:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 21, 2023 at 01:52:52AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > In an empty repository, \"git log\" will die anyway. So I think the more\n> > interesting case is \"I have a repository with stuff in it, but HEAD\n> > points to an unborn branch\". So:\n> >\n> > git --attr-source=HEAD diff foo^ foo\n> \n> This still looks like a made-up example. Who in the right mind\n> would specify HEAD when both of the revs involved in the operation\n> are from branch 'foo'? The history of HEAD may not have anything\n> common with the operand of the operation 'foo' (or its parent), or\n> worse, it may not even exist.\n\nI think it's unlikely for a user to write that. But if you are running a\nserver full of bare repositories that does diffs, you might end up with\na script that sticks \"--attr-source=HEAD\" at the beginning of every\ncommand.\n\nIt is true that HEAD may not be related. But that is what we use if you\nare in a non-bare repository and run \"git diff foo^ foo\".\n\nArguably:\n\n git --attr-source=$to diff $from $to\n\nis a better default for this command. But something like \"git log -p\" is\ntrickier, as you have many commits to show. You can try to use the tip\nof the traversal, but there may be multiple. Using the attributes from\nthe destination of each commit is the most likely to avoid divergence\nbetween the attributes and the code, but it may also not be what people\nwant. Using the modern attributes from the working tree often makes\nshowing historical commits much nicer.\n\nSo I dunno. I could see arguments in both directions, but I think in\ngeneral people have been OK with pulling attributes from the working\ntree. And --attr-source=HEAD is the bare equivalent.\n\n> But your \"in this repository we never trust attributes from working\n> tree, take it instead from this file or from this blob\" example does\n> make a lot more sense as a use case.\n\nI don't know that it was my example. :) But yes, if you do\n\"--attr-source=$to\", you're overriding even the non-bare case. That may\nbe what you want for some cases, but as above, I think it's hard to\napply consistently (or even what you'd want for the general case).\n\n> > And there you really are saying \"if there are attributes in HEAD, use\n> > them; otherwise, don't worry about it\". This is exactly what we do with\n> > mailmap.blob: in a bare repository it is set to HEAD by default, but if\n> > HEAD does not resolve, we just ignore it (just like a HEAD that does not\n> > contain a .mailmap file). And those match the non-bare cases, where we'd\n> > read those files from the working tree instead.\n> \n> \"HEAD\" -> \"HEAD:.mailmap\" if I recall correctly.\n\nTrue, yeah. We can't do that here because attributes are spread across\nthe tree. So all my mentions of attr.blob would really be attr.tree.\n\n> And if HEAD does not resolve, we pretend as if HEAD is an empty\n> tree-ish (hence HEAD:.mailmap is missing). It becomes very tempting\n> to do the same for the attribute sources and treat unborn HEAD as if\n> it specifies an empty tree-ish, without any configuration or an\n> extra option.\n> \n> Such a change would be an end-user observable behaviour change, but\n> nobody sane would be running \"git --attr-source=HEAD diff HEAD^ HEAD\"\n> to check and detect an unborn HEAD for its error exit code, so I do\n> not think it is a horribly wrong thing to do.\n\nYeah, that is basically what I am proposing. It sounds from the\ndiscussion here that there are two interesting cases:\n\n 1. You want to use --attr-source=HEAD because you are trying to make a\n bare repo behave like a non-bare one. You probably want the \"don't\n complain if it is missing\" behavior.\n\n 2. You are trying to use the attributes from one side of the diff to\n override the worktree ones (because the two trees are unrelated).\n In which case it does not really matter if --attr-source complains,\n because the diff will likewise complain if the tree cannot be\n resolved.\n\nJust trying to play devil's advocate, though, I guess you could run a\nnon-diff operation like say:\n\n git --attr-source=my-branch check-attr foo\n\nand then you probably _do_ want to know if that source was ignored or\ntypo'd.\n\n> But again, as you said, --attr-source= does not sound like\n> a good fit for bare-repository hosted environment and a tentative\n> hack waiting for a proper attr.blob support, or something like that,\n> to appear.\n\nI think folks mentioned mailmap.blob in the original discussion for\n--attr-source. I don't remember why the patch went with --attr-source\nthere. Maybe John can speak to that.\n\n-Peff\n"},{"id":"482351","messageId":"F825AAFE-E55E-402F-8116-34D6FC61034C@gmail.com","threadId":"60250","inReplyTo":"20230921041545.GA2338791@coredump.intra.peff.net","subject":"Re: [PATCH] attr: attr.allowInvalidSource config to allow invalid revision","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2023-09-26T18:23:26Z","receivedAt":"2023-09-26T18:23:32Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Peff,\n\nOn 21 Sep 2023, at 0:15, Jeff King wrote:\n\n> On Wed, Sep 20, 2023 at 09:06:46AM -0700, Junio C Hamano wrote:\n>\n>>> With empty repositories however, HEAD does not point to a valid treeish,\n>>> causing Git to die. This means we would need to check for a valid\n>>> treeish each time.\n>>\n>> Naturally.\n>>\n>>> To avoid this, let's add a configuration that allows\n>>> Git to simply ignore --attr-source if it does not resolve to a valid\n>>> tree.\n>>\n>> Not convincing at all as to the reason why we want to do anything\n>> \"to avoid this\". \"git log\" in a repository whose HEAD does not\n>> point to a valid treeish. \"git blame\" dies with \"no such ref:\n>> HEAD\". An empty repository (more precisely, an unborn history)\n>> needs special casing if you want to present it if you do not want to\n>> spew underlying error messages to the end users *anyway*. It is\n>> unclear why seeing what commit the HEAD pointer points at (or which\n>> branch it points at for that matter) is *an* *extra* and *otherwise*\n>> *unnecessary* overhead that need to be avoided.\n>\n> In an empty repository, \"git log\" will die anyway. So I think the more\n> interesting case is \"I have a repository with stuff in it, but HEAD\n> points to an unborn branch\". So:\n>\n> git --attr-source=HEAD diff foo^ foo\n>\n> And there you really are saying \"if there are attributes in HEAD, use\n> them; otherwise, don't worry about it\". This is exactly what we do with\n> mailmap.blob: in a bare repository it is set to HEAD by default, but if\n> HEAD does not resolve, we just ignore it (just like a HEAD that does not\n> contain a .mailmap file). And those match the non-bare cases, where we'd\n> read those files from the working tree instead.\n>\n> So I think the same notion applies here. You want to be able to point it\n> at HEAD by default, but if there is no HEAD, that is the same as if HEAD\n> simply did not contain any attributes. If we had attr.blob, that is\n> exactly how I would expect it to work.\n\nYou captured this quite well!\n\n>\n> My gut feeling is that --attr-source should do the same, and just\n> quietly ignore a ref that does not resolve. But I think an argument can\n> be made that because the caller explicitly gave us a ref, they expect it\n> to work (and that would catch misspellings, etc). Like:\n>\n> git --attr-source=my-barnch diff foo^ foo\n>\n> So I'm OK with not changing that behavior.\n>\n> But what is weird about this patch is that we are using a config option\n> to change how a command-line option is interpreted. If the idea is that\n> some invocations care about the validity of the source and some do not,\n> then the config option is much too blunt. It is set once long ago, but\n> it can't distinguish between times you care about invalid sources and\n> times you don't.\n>\n> It would make much more sense to me to have another command-line option,\n> like:\n>\n> git --attr-source=HEAD --allow-invalid-attr-source\n>\n> Obviously that is horrible to type, but I think the point is that you'd\n> only do this from a script anyway (because it's those automated cases\n> where you want to say \"use HEAD only if it exists\").\n\nYeah, I see the point about using a config to change default behavior of a\ncommand leading to confusion.\n\n>\n> If there were an attr.blob config option and it complained about an\n> invalid HEAD, _then_ I think attr.allowInvalidSource might make sense\n> (though again, I would just argue for switching the behavior by\n> default). And I really think attr.blob is a better match for what GitLab\n> is trying to do here, because it is set once and applies to all\n> commands, rather than having to teach every invocation to pass it\n> (though I guess maybe they use it as an environment variable).\n\nIn retrospect perhaps a config would have been better here. I think this\npatch started with improving an existing command line flag [1] by making\nit global. So I think we were just thinking about command line flags and\ndidn't consider configs.\n\nThat being said, for GitLab at least there's not a lot of difference since\nwe pass in configs through the commandline anyways rather than relying on the config\nstate itself on disk for our bare server-side repositories.\n\n\n1. https://lore.kernel.org/git/pull.1470.git.git.1679109928556.gitgitgadget@gmail.com/\n\n>\n> Of course I would think that, as the person who solved GitHub's exact\n> same problem for mailmap by adding mailmap.blob. So you may ingest the\n> appropriate grain of salt. :)\n>\n> -Peff\n\n\nthanks\nJohn\n\n"},{"id":"482352","messageId":"92BDE017-77A1-444A-A156-7CEC73456268@gmail.com","threadId":"60250","inReplyTo":"20230921214021.GB302338@coredump.intra.peff.net","subject":"Re: [PATCH] attr: attr.allowInvalidSource config to allow invalid revision","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2023-09-26T18:27:17Z","receivedAt":"2023-09-26T18:27:27Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Peff,\t\n\nOn 21 Sep 2023, at 17:40, Jeff King wrote:\n\n> On Thu, Sep 21, 2023 at 01:52:52AM -0700, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>>\n>>> In an empty repository, \"git log\" will die anyway. So I think the more\n>>> interesting case is \"I have a repository with stuff in it, but HEAD\n>>> points to an unborn branch\". So:\n>>>\n>>> git --attr-source=HEAD diff foo^ foo\n>>\n>> This still looks like a made-up example. Who in the right mind\n>> would specify HEAD when both of the revs involved in the operation\n>> are from branch 'foo'? The history of HEAD may not have anything\n>> common with the operand of the operation 'foo' (or its parent), or\n>> worse, it may not even exist.\n>\n> I think it's unlikely for a user to write that. But if you are running a\n> server full of bare repositories that does diffs, you might end up with\n> a script that sticks \"--attr-source=HEAD\" at the beginning of every\n> command.\n>\n> It is true that HEAD may not be related. But that is what we use if you\n> are in a non-bare repository and run \"git diff foo^ foo\".\n>\n> Arguably:\n>\n> git --attr-source=$to diff $from $to\n>\n> is a better default for this command. But something like \"git log -p\" is\n> trickier, as you have many commits to show. You can try to use the tip\n> of the traversal, but there may be multiple. Using the attributes from\n> the destination of each commit is the most likely to avoid divergence\n> between the attributes and the code, but it may also not be what people\n> want. Using the modern attributes from the working tree often makes\n> showing historical commits much nicer.\n>\n> So I dunno. I could see arguments in both directions, but I think in\n> general people have been OK with pulling attributes from the working\n> tree. And --attr-source=HEAD is the bare equivalent.\n>\n>> But your \"in this repository we never trust attributes from working\n>> tree, take it instead from this file or from this blob\" example does\n>> make a lot more sense as a use case.\n>\n> I don't know that it was my example. :) But yes, if you do\n> \"--attr-source=$to\", you're overriding even the non-bare case. That may\n> be what you want for some cases, but as above, I think it's hard to\n> apply consistently (or even what you'd want for the general case).\n>\n>>> And there you really are saying \"if there are attributes in HEAD, use\n>>> them; otherwise, don't worry about it\". This is exactly what we do with\n>>> mailmap.blob: in a bare repository it is set to HEAD by default, but if\n>>> HEAD does not resolve, we just ignore it (just like a HEAD that does not\n>>> contain a .mailmap file). And those match the non-bare cases, where we'd\n>>> read those files from the working tree instead.\n>>\n>> \"HEAD\" -> \"HEAD:.mailmap\" if I recall correctly.\n>\n> True, yeah. We can't do that here because attributes are spread across\n> the tree. So all my mentions of attr.blob would really be attr.tree.\n>\n>> And if HEAD does not resolve, we pretend as if HEAD is an empty\n>> tree-ish (hence HEAD:.mailmap is missing). It becomes very tempting\n>> to do the same for the attribute sources and treat unborn HEAD as if\n>> it specifies an empty tree-ish, without any configuration or an\n>> extra option.\n>>\n>> Such a change would be an end-user observable behaviour change, but\n>> nobody sane would be running \"git --attr-source=HEAD diff HEAD^ HEAD\"\n>> to check and detect an unborn HEAD for its error exit code, so I do\n>> not think it is a horribly wrong thing to do.\n>\n> Yeah, that is basically what I am proposing. It sounds from the\n> discussion here that there are two interesting cases:\n>\n> 1. You want to use --attr-source=HEAD because you are trying to make a\n> bare repo behave like a non-bare one. You probably want the \"don't\n> complain if it is missing\" behavior.\n\nYep, this is the primary use case for us.\n\n>\n> 2. You are trying to use the attributes from one side of the diff to\n> override the worktree ones (because the two trees are unrelated).\n> In which case it does not really matter if --attr-source complains,\n> because the diff will likewise complain if the tree cannot be\n> resolved.\n>\n> Just trying to play devil's advocate, though, I guess you could run a\n> non-diff operation like say:\n>\n> git --attr-source=my-branch check-attr foo\n>\n> and then you probably _do_ want to know if that source was ignored or\n> typo'd.\n>\n>> But again, as you said, --attr-source= does not sound like\n>> a good fit for bare-repository hosted environment and a tentative\n>> hack waiting for a proper attr.blob support, or something like that,\n>> to appear.\n>\n> I think folks mentioned mailmap.blob in the original discussion for\n> --attr-source. I don't remember why the patch went with --attr-source\n> there. Maybe John can speak to that.\n\nYeah I was looking for this, and I think I found it in [1], which was part of a\nseparate patch having to do with gitattributes. If someone did mention it in the\npatch for --attr-source, then I can't remember why we decided to go with the\ncomand line flag rather than the config.\n\n1. https://lore.kernel.org/git/ZBMn5T6zfKK+PYUe@coredump.intra.peff.net/\n\n>\n> -Peff\n"},{"id":"482353","messageId":"000AEFB0-0694-415D-94CB-9BB437E1A9FA@gmail.com","threadId":"60250","inReplyTo":"xmqq1qer7vrv.fsf@gitster.g","subject":"Re: [PATCH] attr: attr.allowInvalidSource config to allow invalid revision","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2023-09-26T18:30:14Z","receivedAt":"2023-09-26T18:30:21Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Junio,\n\nOn 21 Sep 2023, at 4:52, Junio C Hamano wrote:\n\n> Jeff King writes:\n>\n>> In an empty repository, \"git log\" will die anyway. So I think the more\n>> interesting case is \"I have a repository with stuff in it, but HEAD\n>> points to an unborn branch\". So:\n>>\n>> git --attr-source=HEAD diff foo^ foo\n>\n> This still looks like a made-up example. Who in the right mind\n> would specify HEAD when both of the revs involved in the operation\n> are from branch 'foo'? The history of HEAD may not have anything\n> common with the operand of the operation 'foo' (or its parent), or\n> worse, it may not even exist.\n>\n> But your \"in this repository we never trust attributes from working\n> tree, take it instead from this file or from this blob\" example does\n> make a lot more sense as a use case.\n>\n>> And there you really are saying \"if there are attributes in HEAD, use\n>> them; otherwise, don't worry about it\". This is exactly what we do with\n>> mailmap.blob: in a bare repository it is set to HEAD by default, but if\n>> HEAD does not resolve, we just ignore it (just like a HEAD that does not\n>> contain a .mailmap file). And those match the non-bare cases, where we'd\n>> read those files from the working tree instead.\n>\n> \"HEAD\" -> \"HEAD:.mailmap\" if I recall correctly.\n>\n> And if HEAD does not resolve, we pretend as if HEAD is an empty\n> tree-ish (hence HEAD:.mailmap is missing). It becomes very tempting\n> to do the same for the attribute sources and treat unborn HEAD as if\n> it specifies an empty tree-ish, without any configuration or an\n> extra option.\n>\n> Such a change would be an end-user observable behaviour change, but\n> nobody sane would be running \"git --attr-source=HEAD diff HEAD^ HEAD\"\n> to check and detect an unborn HEAD for its error exit code, so I do\n> not think it is a horribly wrong thing to do.\n>\n> But again, as you said, --attr-source= does not sound like\n> a good fit for bare-repository hosted environment and a tentative\n> hack waiting for a proper attr.blob support, or something like that,\n> to appear.\n>\n>> But what is weird about this patch is that we are using a config option\n>> to change how a command-line option is interpreted. If the idea is that\n>> some invocations care about the validity of the source and some do not,\n>> then the config option is much too blunt. It is set once long ago, but\n>> it can't distinguish between times you care about invalid sources and\n>> times you don't.\n>>\n>> It would make much more sense to me to have another command-line option,\n>> like:\n>>\n>> git --attr-source=HEAD --allow-invalid-attr-source\n>\n> Yeah, if we were to make it configurable without changing the\n> default behaviour, I agree that would be more correct approach. A\n> configuration does not sound like a good fit.\n>\n>> ... And I really think attr.blob is a better match for what GitLab\n>> is trying to do here, because it is set once and applies to all\n>> commands, rather than having to teach every invocation to pass it\n>> (though I guess maybe they use it as an environment variable).\n\nBetween adding an --allow-invalid-attr-source, and adding attr.blob and\nattr.allowInvalidSource I think I like adding the attr.blob config more.\n\n>\n> True, too.\n>\n> Thanks.\n\nthanks\nJohn\n"},{"id":"482663","messageId":"pull.1577.v2.git.git.1696443502.gitgitgadget@gmail.com","threadId":"60250","inReplyTo":"pull.1577.git.git.1695218431033.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] attr: add attr.tree and attr.allowInvalidSource configs","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-10-04T18:18:19Z","receivedAt":"2023-10-04T18:18:28Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"44451a2e5e (attr: teach \"--attr-source=\" global option to \"git\", 2023-05-06)\nprovided the ability to pass in a treeish as the attr source. When a\nrevision does not resolve to a valid tree is passed, Git will die. At\nGitLab, we server repositories as bare repos and would like to always read\nattributes from the default branch, so we'd like to pass in HEAD as the\ntreeish to read gitattributes from on every command. In this context we\nwould not want Git to die if HEAD is unborn, like in the case of empty\nrepositories.\n\nInstead of modifying the default behavior of --attr-source, create a pair of\nconfigs attr.tree and attr.allowInvalidSource whereby an admin can configure\na ref for all commands to read gitattributes from, and another config that\nrelaxes the requirement that this treeish resolve to a valid tree.\n\nChanges since v1:\n\n * Added a commit to add attr.tree config\n\nJohn Cai (2):\n attr: add attr.tree for setting the treeish to read attributes from\n attr: add attr.allowInvalidSource config to allow invalid revision\n\n Documentation/config.txt | 2 ++\n Documentation/config/attr.txt | 12 +++++++++\n attr.c | 21 +++++++++++++--\n t/t0003-attributes.sh | 49 +++++++++++++++++++++++++++++++++++\n 4 files changed, 82 insertions(+), 2 deletions(-)\n create mode 100644 Documentation/config/attr.txt\n\n\nbase-commit: 1fc548b2d6a3596f3e1c1f8b1930d8dbd1e30bf3\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1577%2Fjohn-cai%2Fjc%2Fconfig-attr-invalid-source-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1577/john-cai/jc/config-attr-invalid-source-v2\nPull-Request: https://github.com/git/git/pull/1577\n\nRange-diff vs v1:\n\n -: ----------- > 1: 446bce03a96 attr: add attr.tree for setting the treeish to read attributes from\n 1: 06b9acf24ca ! 2: 52d9e180352 attr: attr.allowInvalidSource config to allow invalid revision\n @@ Metadata\n Author: John Cai \n \n ## Commit message ##\n - attr: attr.allowInvalidSource config to allow invalid revision\n + attr: add attr.allowInvalidSource config to allow invalid revision\n \n - 44451a2e5e (attr: teach \"--attr-source=\" global option to \"git\",\n - 2023-05-06) provided the ability to pass in a treeish as the attr\n - source. When a revision does not resolve to a valid tree is passed, Git\n - will die. GitLab keeps bare repositories and always reads attributes\n - from the default branch, so we pass in HEAD to --attr-source.\n + The previous commit provided the ability to pass in a treeish as the\n + attr source via the attr.tree config. The default behavior is that when\n + a revision does not resolve to a valid tree is passed, Git will die.\n \n - With empty repositories however, HEAD does not point to a valid treeish,\n - causing Git to die. This means we would need to check for a valid\n - treeish each time. To avoid this, let's add a configuration that allows\n - Git to simply ignore --attr-source if it does not resolve to a valid\n - tree.\n + When HEAD is unborn however, it does not point to a valid treeish,\n + causing Git to die. In the context of serving repositories through bare\n + repositories, we'd like to be able to set attr.tree to HEAD and allow\n + cases where HEAD does not resolve to a valid tree to be treated as if\n + there were no treeish provided.\n +\n + Add attr.allowInvalidSource that allows this.\n \n Signed-off-by: John Cai \n \n - ## Documentation/config.txt ##\n -@@ Documentation/config.txt: other popular tools, and describe them in your documentation.\n - \n - include::config/advice.txt[]\n - \n -+include::config/attr.txt[]\n + ## Documentation/config/attr.txt ##\n +@@ Documentation/config/attr.txt: attr.tree:\n + \tlinkgit:gitattributes[5]. This is equivalent to setting the\n + \t`GIT_ATTR_SOURCE` environment variable, or passing in --attr-source to\n + \tthe Git command.\n +\n - include::config/core.txt[]\n - \n - include::config/add.txt[]\n -\n - ## Documentation/config/attr.txt (new) ##\n -@@\n +attr.allowInvalidSource::\n -+\tIf `--attr-source` cannot resolve to a valid tree object, ignore\n -+\t`--attr-source` instead of erroring out, and fall back to looking for\n ++\tIf `attr.tree` cannot resolve to a valid tree object, ignore\n ++\t`attr.tree` instead of erroring out, and fall back to looking for\n +\tattributes in the default locations. Useful when passing `HEAD` into\n +\t`attr-source` since it allows `HEAD` to point to an unborn branch in\n +\tcases like an empty repository.\n \n ## attr.c ##\n -@@ attr.c: static void compute_default_attr_source(struct object_id *attr_source)\n +@@ attr.c: void set_git_attr_source(const char *tree_object_name)\n + \n + static void compute_default_attr_source(struct object_id *attr_source)\n + {\n ++\tint attr_source_from_config = 0;\n ++\n + \tif (!default_attr_source_tree_object_name)\n + \t\tdefault_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);\n + \n + \tif (!default_attr_source_tree_object_name) {\n + \t\tchar *attr_tree;\n + \n +-\t\tif (!git_config_get_string(\"attr.tree\", &attr_tree))\n ++\t\tif (!git_config_get_string(\"attr.tree\", &attr_tree)) {\n ++\t\t\tattr_source_from_config = 1;\n + \t\t\tdefault_attr_source_tree_object_name = attr_tree;\n ++\t\t}\n + \t}\n + \n \tif (!default_attr_source_tree_object_name || !is_null_oid(attr_source))\n \t\treturn;\n \n -\tif (repo_get_oid_treeish(the_repository, default_attr_source_tree_object_name, attr_source))\n -\t\tdie(_(\"bad --attr-source or GIT_ATTR_SOURCE\"));\n -+\n +\tif (repo_get_oid_treeish(the_repository, default_attr_source_tree_object_name, attr_source)) {\n +\t\tint allow_invalid_attr_source = 0;\n +\n +\t\tgit_config_get_bool(\"attr.allowinvalidsource\", &allow_invalid_attr_source);\n +\n -+\t\tif (!allow_invalid_attr_source)\n ++\t\tif (!(allow_invalid_attr_source && attr_source_from_config))\n +\t\t\tdie(_(\"bad --attr-source or GIT_ATTR_SOURCE\"));\n +\t}\n }\n @@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr\n +\tgit init empty &&\n +\ttest_must_fail git -C empty --attr-source=HEAD check-attr test -- f/path 2>err &&\n +\ttest_cmp expect_err err &&\n -+\tgit -C empty -c attr.allowInvalidSource=true --attr-source=HEAD check-attr test -- f/path >actual 2>err &&\n ++\tgit -C empty -c attr.tree=HEAD -c attr.allowInvalidSource=true check-attr test -- f/path >actual 2>err &&\n +\ttest_must_be_empty err &&\n +\ttest_cmp expect actual\n +'\n @@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr\n +\tgit init empty &&\n +\ttest_must_fail git -C empty --attr-source=refs/does/not/exist check-attr test -- f/path 2>err &&\n +\ttest_cmp expect_err err &&\n -+\tgit -C empty -c attr.allowInvalidSource=true --attr-source=refs/does/not/exist check-attr test -- f/path >actual 2>err &&\n ++\tgit -C empty -c attr.tree=refs/does/not/exist -c attr.allowInvalidSource=true check-attr test -- f/path >actual 2>err &&\n +\ttest_must_be_empty err &&\n +\ttest_cmp expect actual\n +'\n @@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr\n +\tgit init empty &&\n +\techo \"f/path test=val\" >empty/.gitattributes &&\n +\techo \"f/path: test: val\" >expect &&\n -+\tgit -C empty -c attr.allowInvalidSource=true --attr-source=HEAD check-attr test -- f/path >actual 2>err &&\n ++\tgit -C empty -c attr.tree=HEAD -c attr.allowInvalidSource=true check-attr test -- f/path >actual 2>err &&\n +\ttest_must_be_empty err &&\n +\ttest_cmp expect actual\n +'\n ++\n ++test_expect_success 'attr.allowInvalidSource has no effect on --attr-source' '\n ++\ttest_when_finished rm -rf empty &&\n ++\techo $bad_attr_source_err >expect_err &&\n ++\techo \"f/path: test: unspecified\" >expect &&\n ++\tgit init empty &&\n ++\ttest_must_fail git -C empty -c attr.allowInvalidSource=true --attr-source=HEAD check-attr test -- f/path 2>err &&\n ++\ttest_cmp expect_err err\n ++'\n +\n test_expect_success 'bare repository: with --source' '\n \t(\n\n-- \ngitgitgadget\n"},{"id":"482664","messageId":"446bce03a96836f35f94e9ef8548cf4a2b041ba8.1696443502.git.gitgitgadget@gmail.com","threadId":"60250","inReplyTo":"pull.1577.v2.git.git.1696443502.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] attr: add attr.tree for setting the treeish to read attributes from","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-10-04T18:18:20Z","receivedAt":"2023-10-04T18:18:29Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\n44451a2e5e (attr: teach \"--attr-source=\" global option to \"git\",\n2023-05-06) provided the ability to pass in a treeish as the attr\nsource. In the context of serving Git repositories as bare repos like we\ndo at GitLab however, it would be easier to point --attr-source to HEAD\nfor all commands by setting it once.\n\nAdd a new config attr.tree that allows this.\n\nSigned-off-by: John Cai \n---\n Documentation/config.txt | 2 ++\n Documentation/config/attr.txt | 5 +++++\n attr.c | 7 +++++++\n t/t0003-attributes.sh | 4 ++++\n 4 files changed, 18 insertions(+)\n create mode 100644 Documentation/config/attr.txt\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 229b63a454c..b1891c2b5af 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -371,6 +371,8 @@ other popular tools, and describe them in your documentation.\n \n include::config/advice.txt[]\n \n+include::config/attr.txt[]\n+\n include::config/core.txt[]\n \n include::config/add.txt[]\ndiff --git a/Documentation/config/attr.txt b/Documentation/config/attr.txt\nnew file mode 100644\nindex 00000000000..e4f2122b7ab\n--- /dev/null\n+++ b/Documentation/config/attr.txt\n@@ -0,0 +1,5 @@\n+attr.tree:\n+\tA to read gitattributes from instead of the worktree. See\n+\tlinkgit:gitattributes[5]. This is equivalent to setting the\n+\t`GIT_ATTR_SOURCE` environment variable, or passing in --attr-source to\n+\tthe Git command.\ndiff --git a/attr.c b/attr.c\nindex 71c84fbcf86..bb0d54eb967 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -1205,6 +1205,13 @@ static void compute_default_attr_source(struct object_id *attr_source)\n \tif (!default_attr_source_tree_object_name)\n \t\tdefault_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);\n \n+\tif (!default_attr_source_tree_object_name) {\n+\t\tchar *attr_tree;\n+\n+\t\tif (!git_config_get_string(\"attr.tree\", &attr_tree))\n+\t\t\tdefault_attr_source_tree_object_name = attr_tree;\n+\t}\n+\n \tif (!default_attr_source_tree_object_name || !is_null_oid(attr_source))\n \t\treturn;\n \ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 26e082f05b4..6342187c751 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -40,6 +40,10 @@ attr_check_source () {\n \ttest_cmp expect actual &&\n \ttest_must_be_empty err\n \n+\tgit $git_opts -c \"attr.tree=$source\" check-attr test -- \"$path\" >actual 2>err &&\n+\ttest_cmp expect actual &&\n+\ttest_must_be_empty err\n+\n \tGIT_ATTR_SOURCE=\"$source\" git $git_opts check-attr test -- \"$path\" >actual 2>err &&\n \ttest_cmp expect actual &&\n \ttest_must_be_empty err\n-- \ngitgitgadget\n\n"},{"id":"482665","messageId":"52d9e1803526bf1f267d23f6ef163049f8be77b7.1696443502.git.gitgitgadget@gmail.com","threadId":"60250","inReplyTo":"pull.1577.v2.git.git.1696443502.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] attr: add attr.allowInvalidSource config to allow invalid revision","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-10-04T18:18:21Z","receivedAt":"2023-10-04T18:18:36Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nThe previous commit provided the ability to pass in a treeish as the\nattr source via the attr.tree config. The default behavior is that when\na revision does not resolve to a valid tree is passed, Git will die.\n\nWhen HEAD is unborn however, it does not point to a valid treeish,\ncausing Git to die. In the context of serving repositories through bare\nrepositories, we'd like to be able to set attr.tree to HEAD and allow\ncases where HEAD does not resolve to a valid tree to be treated as if\nthere were no treeish provided.\n\nAdd attr.allowInvalidSource that allows this.\n\nSigned-off-by: John Cai \n---\n Documentation/config/attr.txt | 7 ++++++\n attr.c | 16 ++++++++++---\n t/t0003-attributes.sh | 45 +++++++++++++++++++++++++++++++++++\n 3 files changed, 65 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config/attr.txt b/Documentation/config/attr.txt\nindex e4f2122b7ab..00113a4950e 100644\n--- a/Documentation/config/attr.txt\n+++ b/Documentation/config/attr.txt\n@@ -3,3 +3,10 @@ attr.tree:\n \tlinkgit:gitattributes[5]. This is equivalent to setting the\n \t`GIT_ATTR_SOURCE` environment variable, or passing in --attr-source to\n \tthe Git command.\n+\n+attr.allowInvalidSource::\n+\tIf `attr.tree` cannot resolve to a valid tree object, ignore\n+\t`attr.tree` instead of erroring out, and fall back to looking for\n+\tattributes in the default locations. Useful when passing `HEAD` into\n+\t`attr-source` since it allows `HEAD` to point to an unborn branch in\n+\tcases like an empty repository.\ndiff --git a/attr.c b/attr.c\nindex bb0d54eb967..1a7ac39b9d1 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -1202,21 +1202,31 @@ void set_git_attr_source(const char *tree_object_name)\n \n static void compute_default_attr_source(struct object_id *attr_source)\n {\n+\tint attr_source_from_config = 0;\n+\n \tif (!default_attr_source_tree_object_name)\n \t\tdefault_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);\n \n \tif (!default_attr_source_tree_object_name) {\n \t\tchar *attr_tree;\n \n-\t\tif (!git_config_get_string(\"attr.tree\", &attr_tree))\n+\t\tif (!git_config_get_string(\"attr.tree\", &attr_tree)) {\n+\t\t\tattr_source_from_config = 1;\n \t\t\tdefault_attr_source_tree_object_name = attr_tree;\n+\t\t}\n \t}\n \n \tif (!default_attr_source_tree_object_name || !is_null_oid(attr_source))\n \t\treturn;\n \n-\tif (repo_get_oid_treeish(the_repository, default_attr_source_tree_object_name, attr_source))\n-\t\tdie(_(\"bad --attr-source or GIT_ATTR_SOURCE\"));\n+\tif (repo_get_oid_treeish(the_repository, default_attr_source_tree_object_name, attr_source)) {\n+\t\tint allow_invalid_attr_source = 0;\n+\n+\t\tgit_config_get_bool(\"attr.allowinvalidsource\", &allow_invalid_attr_source);\n+\n+\t\tif (!(allow_invalid_attr_source && attr_source_from_config))\n+\t\t\tdie(_(\"bad --attr-source or GIT_ATTR_SOURCE\"));\n+\t}\n }\n \n static struct object_id *default_attr_source(void)\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 6342187c751..972b64496e7 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -346,6 +346,51 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '\n \t)\n '\n \n+bad_attr_source_err=\"fatal: bad --attr-source or GIT_ATTR_SOURCE\"\n+\n+test_expect_success 'attr.allowInvalidSource when HEAD is unborn' '\n+\ttest_when_finished rm -rf empty &&\n+\techo $bad_attr_source_err >expect_err &&\n+\techo \"f/path: test: unspecified\" >expect &&\n+\tgit init empty &&\n+\ttest_must_fail git -C empty --attr-source=HEAD check-attr test -- f/path 2>err &&\n+\ttest_cmp expect_err err &&\n+\tgit -C empty -c attr.tree=HEAD -c attr.allowInvalidSource=true check-attr test -- f/path >actual 2>err &&\n+\ttest_must_be_empty err &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'attr.allowInvalidSource when --attr-source points to non-existing ref' '\n+\ttest_when_finished rm -rf empty &&\n+\techo $bad_attr_source_err >expect_err &&\n+\techo \"f/path: test: unspecified\" >expect &&\n+\tgit init empty &&\n+\ttest_must_fail git -C empty --attr-source=refs/does/not/exist check-attr test -- f/path 2>err &&\n+\ttest_cmp expect_err err &&\n+\tgit -C empty -c attr.tree=refs/does/not/exist -c attr.allowInvalidSource=true check-attr test -- f/path >actual 2>err &&\n+\ttest_must_be_empty err &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'bad attr source defaults to reading .gitattributes file' '\n+\ttest_when_finished rm -rf empty &&\n+\tgit init empty &&\n+\techo \"f/path test=val\" >empty/.gitattributes &&\n+\techo \"f/path: test: val\" >expect &&\n+\tgit -C empty -c attr.tree=HEAD -c attr.allowInvalidSource=true check-attr test -- f/path >actual 2>err &&\n+\ttest_must_be_empty err &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'attr.allowInvalidSource has no effect on --attr-source' '\n+\ttest_when_finished rm -rf empty &&\n+\techo $bad_attr_source_err >expect_err &&\n+\techo \"f/path: test: unspecified\" >expect &&\n+\tgit init empty &&\n+\ttest_must_fail git -C empty -c attr.allowInvalidSource=true --attr-source=HEAD check-attr test -- f/path 2>err &&\n+\ttest_cmp expect_err err\n+'\n+\n test_expect_success 'bare repository: with --source' '\n \t(\n \t\tcd bare.git &&\n-- \ngitgitgadget\n"},{"id":"482668","messageId":"xmqqfs2qp3bg.fsf@gitster.g","threadId":"60250","inReplyTo":"446bce03a96836f35f94e9ef8548cf4a2b041ba8.1696443502.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] attr: add attr.tree for setting the treeish to read attributes from","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-04T19:58:43Z","receivedAt":"2023-10-04T19:58:52Z","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> 44451a2e5e (attr: teach \"--attr-source=\" global option to \"git\",\n> 2023-05-06) provided the ability to pass in a treeish as the attr\n> source. In the context of serving Git repositories as bare repos like we\n> do at GitLab however, it would be easier to point --attr-source to HEAD\n> for all commands by setting it once.\n>\n> Add a new config attr.tree that allows this.\n\nHmph, I wonder if we want to go all the way to emulate how the\nmailmap.blob was done, including\n\n - Default the value of attr.tree to HEAD in a bare repository;\n\n - Notice but ignore errors if the attr.tree does not point at a\n tree object, and pretend as if attr.tree specified an empty tree;\n\nwhich does not seem to be in this patch. With such a change,\nprobably we do not even need [2/2] of the series, perhaps?\n\n\n"},{"id":"482676","messageId":"xmqqv8bmlzoi.fsf@gitster.g","threadId":"60250","inReplyTo":"446bce03a96836f35f94e9ef8548cf4a2b041ba8.1696443502.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] attr: add attr.tree for setting the treeish to read attributes from","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-04T23:45:33Z","receivedAt":"2023-10-04T23:45:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"[jc: JTan CC'ed as he seems to have took over the polishing of\nb1bda751 (parse: separate out parsing functions from config.h,\n2023-09-29)]\n\n\"John Cai via GitGitGadget\" writes:\n\n> diff --git a/attr.c b/attr.c\n> index 71c84fbcf86..bb0d54eb967 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -1205,6 +1205,13 @@ static void compute_default_attr_source(struct object_id *attr_source)\n> \tif (!default_attr_source_tree_object_name)\n> \t\tdefault_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);\n> \n> +\tif (!default_attr_source_tree_object_name) {\n> +\t\tchar *attr_tree;\n> +\n> +\t\tif (!git_config_get_string(\"attr.tree\", &attr_tree))\n> +\t\t\tdefault_attr_source_tree_object_name = attr_tree;\n> +\t}\n> +\n> \tif (!default_attr_source_tree_object_name || !is_null_oid(attr_source))\n> \t\treturn;\n\nAs this adds a new call to git_config_get_string(), which will only\nbe available by including , a merge-fix into 'seen' of\nthis topic needs to revert what b1bda751 (parse: separate out\nparsing functions from config.h, 2023-09-29) did, which made this\nfile include only .\n\nAs this configuration variable was invented to improve the way the\nattribute source tree is supported by emulating how mailmap.blob is\ndone, it deserves a bit of comparison.\n\nThe way mailmap.c does this is not have any code that reads or\nparses configuration in mailmap.c (which is a rather library-ish\nplace), and leaves it up to callers to pre-populate the global\nvariable git_mailmap_blob with config.c:git_default_config(). That\nway, they do not need to include (nor ) that is\ncloser to the UI layer. I am wondering why we are not doing the\nsame, and instead making an ad-hoc call to git_config_get_string()\nin this code, and if it is a good direction to move the codebase to\n(in which case we may want to make sure that the same pattern is\nfollowed in other places).\n\nFolks interested in libification, as to the direction of that\neffort, what's your plan on where to draw a line between \"library\"\nand \"userland\"? Should library-ish code be allowed to call\ngit_config_anything()? I somehow suspect that it might be cleaner\nif they didn't, and instead have the user of the \"attr\" module to\nsupply the necessary values from outside.\n\nOn the other hand, once the part we have historically called\n\"config\" API gets a reasonably solid abstraction so that they become\npluggable and replaceable, random ad-hoc calls from library code\noutside the \"config\" library code may not be a huge problem, as long\nas we plumb the necessary object handles around (so \"attr\" library\nwould need to be told which \"config\" backend is in use, probably in\nthe form of a struct that holds the various states in to replace\nthe current use of globals, plus a vtable to point at\nimplementations of the \"config\" service, and git_config_get_string()\ncall in such a truly libified world would grab the value of the named\nvariable transparently from whichever \"config\" backend is currently\nin use).\n\nAnyway, I think I wiggled this patch into 'seen' so I'll push out\ntoday's integration result shortly.\n\n"},{"id":"482694","messageId":"20231005170703.GA975921@coredump.intra.peff.net","threadId":"60250","inReplyTo":"xmqqfs2qp3bg.fsf@gitster.g","subject":"Re: [PATCH v2 1/2] attr: add attr.tree for setting the treeish to read attributes from","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-05T17:07:03Z","receivedAt":"2023-10-05T17:31:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 04, 2023 at 12:58:43PM -0700, Junio C Hamano wrote:\n\n> \"John Cai via GitGitGadget\" writes:\n> \n> > From: John Cai \n> >\n> > 44451a2e5e (attr: teach \"--attr-source=\" global option to \"git\",\n> > 2023-05-06) provided the ability to pass in a treeish as the attr\n> > source. In the context of serving Git repositories as bare repos like we\n> > do at GitLab however, it would be easier to point --attr-source to HEAD\n> > for all commands by setting it once.\n> >\n> > Add a new config attr.tree that allows this.\n> \n> Hmph, I wonder if we want to go all the way to emulate how the\n> mailmap.blob was done, including\n> \n> - Default the value of attr.tree to HEAD in a bare repository;\n> \n> - Notice but ignore errors if the attr.tree does not point at a\n> tree object, and pretend as if attr.tree specified an empty tree;\n> \n> which does not seem to be in this patch. With such a change,\n> probably we do not even need [2/2] of the series, perhaps?\n\nOh good, this was exactly what I was going to write in a review, so now\nI don't have to. :)\n\nEven though it creates behavior differences between attr.tree and\n--attr-source, I think that is justifiable. Config options apply across\na wider set of contexts, so loosening the error handling may make sense\nwhere it would not for a command-line option. But we should document\nboth that, and also how the two interact (I assume \"git --attr-source\"\nwould override attr.tree completely).\n\n-Peff\n"},{"id":"482707","messageId":"BC4BEE47-2BE1-44F8-B368-6E858D98D416@gmail.com","threadId":"60250","inReplyTo":"20231005170703.GA975921@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/2] attr: add attr.tree for setting the treeish to read attributes from","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2023-10-05T19:46:46Z","receivedAt":"2023-10-05T19:46:57Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Peff,\n\nOn 5 Oct 2023, at 13:07, Jeff King wrote:\n\n> On Wed, Oct 04, 2023 at 12:58:43PM -0700, Junio C Hamano wrote:\n>\n>> \"John Cai via GitGitGadget\" writes:\n>>\n>>> From: John Cai \n>>>\n>>> 44451a2e5e (attr: teach \"--attr-source=\" global option to \"git\",\n>>> 2023-05-06) provided the ability to pass in a treeish as the attr\n>>> source. In the context of serving Git repositories as bare repos like we\n>>> do at GitLab however, it would be easier to point --attr-source to HEAD\n>>> for all commands by setting it once.\n>>>\n>>> Add a new config attr.tree that allows this.\n>>\n>> Hmph, I wonder if we want to go all the way to emulate how the\n>> mailmap.blob was done, including\n>>\n>> - Default the value of attr.tree to HEAD in a bare repository;\n>>\n>> - Notice but ignore errors if the attr.tree does not point at a\n>> tree object, and pretend as if attr.tree specified an empty tree;\n>>\n>> which does not seem to be in this patch. With such a change,\n>> probably we do not even need [2/2] of the series, perhaps?\n>\n> Oh good, this was exactly what I was going to write in a review, so now\n> I don't have to. :)\n>\n> Even though it creates behavior differences between attr.tree and\n> --attr-source, I think that is justifiable. Config options apply across\n> a wider set of contexts, so loosening the error handling may make sense\n> where it would not for a command-line option.\n\nMakes sense. Will adjust in the next version.\n\n> But we should document both that, and also how the two interact (I assume\n> \"git --attr-source\" would override attr.tree completely).\n\nYes, --attr-source would take precedence over attr.tree\n\n>\n> -Peff\n\nthanks!\nJohn\n"},{"id":"482748","messageId":"20231006172022.1153171-1-jonathantanmy@google.com","threadId":"60250","inReplyTo":"xmqqv8bmlzoi.fsf@gitster.g","subject":"Re: [PATCH v2 1/2] attr: add attr.tree for setting the treeish to read attributes from","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2023-10-06T17:20:22Z","receivedAt":"2023-10-06T17:20:34Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Junio C Hamano writes:\n> As this adds a new call to git_config_get_string(), which will only\n> be available by including , a merge-fix into 'seen' of\n> this topic needs to revert what b1bda751 (parse: separate out\n> parsing functions from config.h, 2023-09-29) did, which made this\n> file include only .\n> \n> As this configuration variable was invented to improve the way the\n> attribute source tree is supported by emulating how mailmap.blob is\n> done, it deserves a bit of comparison.\n> \n> The way mailmap.c does this is not have any code that reads or\n> parses configuration in mailmap.c (which is a rather library-ish\n> place), and leaves it up to callers to pre-populate the global\n> variable git_mailmap_blob with config.c:git_default_config(). That\n> way, they do not need to include (nor ) that is\n> closer to the UI layer. I am wondering why we are not doing the\n> same, and instead making an ad-hoc call to git_config_get_string()\n> in this code, and if it is a good direction to move the codebase to\n> (in which case we may want to make sure that the same pattern is\n> followed in other places).\n> \n> Folks interested in libification, as to the direction of that\n> effort, what's your plan on where to draw a line between \"library\"\n> and \"userland\"? Should library-ish code be allowed to call\n> git_config_anything()? I somehow suspect that it might be cleaner\n> if they didn't, and instead have the user of the \"attr\" module to\n> supply the necessary values from outside.\n\nI think that ideally library-ish code shouldn't be allowed to call\nconfig, yes. However I think what's practical would be for libraries\nthat use very few config variables to get the necessary values from\noutside, and libraries that use many config variables (e.g. fetch, if it\nbecomes a library) to call config.\n\n> On the other hand, once the part we have historically called\n> \"config\" API gets a reasonably solid abstraction so that they become\n> pluggable and replaceable, random ad-hoc calls from library code\n> outside the \"config\" library code may not be a huge problem, as long\n> as we plumb the necessary object handles around (so \"attr\" library\n> would need to be told which \"config\" backend is in use, probably in\n> the form of a struct that holds the various states in to replace\n> the current use of globals, plus a vtable to point at\n> implementations of the \"config\" service, and git_config_get_string()\n> call in such a truly libified world would grab the value of the named\n> variable transparently from whichever \"config\" backend is currently\n> in use).\n\nThis is true, but if we were ever to use the attr library elsewhere\n(whether in the git.git repo itself to unit test this library, or\nin another software project), we would need to supply a mock/stub of\nconfig. If attr uses very few config variables, I think it's clearer if\nit takes in the information from outside.\n"},{"id":"483000","messageId":"pull.1577.v3.git.git.1696967380.gitgitgadget@gmail.com","threadId":"60250","inReplyTo":"pull.1577.v2.git.git.1696443502.gitgitgadget@gmail.com","subject":"[PATCH v3 0/2] attr: add attr.tree config","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-10-10T19:49:38Z","receivedAt":"2023-10-10T19:49:48Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"44451a2e5e (attr: teach \"--attr-source=\" global option to \"git\", 2023-05-06)\nprovided the ability to pass in a treeish as the attr source. When a\nrevision does not resolve to a valid tree is passed, Git will die. At\nGitLab, we server repositories as bare repos and would like to always read\nattributes from the default branch, so we'd like to pass in HEAD as the\ntreeish to read gitattributes from on every command. In this context we\nwould not want Git to die if HEAD is unborn, like in the case of empty\nrepositories.\n\nInstead of modifying the default behavior of --attr-source, create a new\nconfig attr.tree with which an admin can configure a ref for all commands to\nread gitattributes from. Also make the default tree to read from HEAD on\nbare repositories.\n\nChanges since v2:\n\n * relax the restrictions around attr.tree so that if it does not resolve to\n a valid treeish, ignore it.\n * add a commit to default to HEAD in bare repositories\n\nChanges since v1:\n\n * Added a commit to add attr.tree config\n\nJohn Cai (2):\n attr: read attributes from HEAD when bare repo\n attr: add attr.tree for setting the treeish to read attributes from\n\n Documentation/config.txt | 2 +\n Documentation/config/attr.txt | 5 +++\n attr.c | 19 ++++++++-\n attr.h | 2 +\n config.c | 14 +++++++\n t/t0003-attributes.sh | 76 +++++++++++++++++++++++++++++++++++\n t/t5001-archive-attr.sh | 2 +-\n 7 files changed, 118 insertions(+), 2 deletions(-)\n create mode 100644 Documentation/config/attr.txt\n\n\nbase-commit: 1fc548b2d6a3596f3e1c1f8b1930d8dbd1e30bf3\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1577%2Fjohn-cai%2Fjc%2Fconfig-attr-invalid-source-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1577/john-cai/jc/config-attr-invalid-source-v3\nPull-Request: https://github.com/git/git/pull/1577\n\nRange-diff vs v2:\n\n 2: 52d9e180352 ! 1: cef206d47c7 attr: add attr.allowInvalidSource config to allow invalid revision\n @@ Metadata\n Author: John Cai \n \n ## Commit message ##\n - attr: add attr.allowInvalidSource config to allow invalid revision\n + attr: read attributes from HEAD when bare repo\n \n - The previous commit provided the ability to pass in a treeish as the\n - attr source via the attr.tree config. The default behavior is that when\n - a revision does not resolve to a valid tree is passed, Git will die.\n + The motivation for 44451a2e5e (attr: teach \"--attr-source=\" global\n + option to \"git\" , 2023-05-06), was to make it possible to use\n + gitattributes with bare repositories.\n \n - When HEAD is unborn however, it does not point to a valid treeish,\n - causing Git to die. In the context of serving repositories through bare\n - repositories, we'd like to be able to set attr.tree to HEAD and allow\n - cases where HEAD does not resolve to a valid tree to be treated as if\n - there were no treeish provided.\n -\n - Add attr.allowInvalidSource that allows this.\n + To make it easier to read gitattributes in bare repositories however,\n + let's just make HEAD:.gitattributes the default. This is in line with\n + how mailmap works, 8c473cecfd (mailmap: default mailmap.blob in bare\n + repositories, 2012-12-13).\n \n Signed-off-by: John Cai \n \n - ## Documentation/config/attr.txt ##\n -@@ Documentation/config/attr.txt: attr.tree:\n - \tlinkgit:gitattributes[5]. This is equivalent to setting the\n - \t`GIT_ATTR_SOURCE` environment variable, or passing in --attr-source to\n - \tthe Git command.\n -+\n -+attr.allowInvalidSource::\n -+\tIf `attr.tree` cannot resolve to a valid tree object, ignore\n -+\t`attr.tree` instead of erroring out, and fall back to looking for\n -+\tattributes in the default locations. Useful when passing `HEAD` into\n -+\t`attr-source` since it allows `HEAD` to point to an unborn branch in\n -+\tcases like an empty repository.\n -\n ## attr.c ##\n -@@ attr.c: void set_git_attr_source(const char *tree_object_name)\n +@@ attr.c: static void collect_some_attrs(struct index_state *istate,\n + }\n \n - static void compute_default_attr_source(struct object_id *attr_source)\n + static const char *default_attr_source_tree_object_name;\n ++static int ignore_bad_attr_tree;\n + \n + void set_git_attr_source(const char *tree_object_name)\n {\n -+\tint attr_source_from_config = 0;\n -+\n +@@ attr.c: static void compute_default_attr_source(struct object_id *attr_source)\n \tif (!default_attr_source_tree_object_name)\n \t\tdefault_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);\n \n - \tif (!default_attr_source_tree_object_name) {\n - \t\tchar *attr_tree;\n - \n --\t\tif (!git_config_get_string(\"attr.tree\", &attr_tree))\n -+\t\tif (!git_config_get_string(\"attr.tree\", &attr_tree)) {\n -+\t\t\tattr_source_from_config = 1;\n - \t\t\tdefault_attr_source_tree_object_name = attr_tree;\n -+\t\t}\n - \t}\n - \n ++\tif (!default_attr_source_tree_object_name &&\n ++\t startup_info->have_repository &&\n ++\t is_bare_repository()) {\n ++\t\tdefault_attr_source_tree_object_name = \"HEAD\";\n ++\t\tignore_bad_attr_tree = 1;\n ++\t}\n ++\n \tif (!default_attr_source_tree_object_name || !is_null_oid(attr_source))\n \t\treturn;\n \n -\tif (repo_get_oid_treeish(the_repository, default_attr_source_tree_object_name, attr_source))\n --\t\tdie(_(\"bad --attr-source or GIT_ATTR_SOURCE\"));\n -+\tif (repo_get_oid_treeish(the_repository, default_attr_source_tree_object_name, attr_source)) {\n -+\t\tint allow_invalid_attr_source = 0;\n -+\n -+\t\tgit_config_get_bool(\"attr.allowinvalidsource\", &allow_invalid_attr_source);\n -+\n -+\t\tif (!(allow_invalid_attr_source && attr_source_from_config))\n -+\t\t\tdie(_(\"bad --attr-source or GIT_ATTR_SOURCE\"));\n -+\t}\n ++\tif (repo_get_oid_treeish(the_repository,\n ++\t\t\t\t default_attr_source_tree_object_name,\n ++\t\t\t\t attr_source) && !ignore_bad_attr_tree)\n + \t\tdie(_(\"bad --attr-source or GIT_ATTR_SOURCE\"));\n }\n \n - static struct object_id *default_attr_source(void)\n \n ## t/t0003-attributes.sh ##\n @@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattribute is ignored' '\n \t)\n '\n \n -+bad_attr_source_err=\"fatal: bad --attr-source or GIT_ATTR_SOURCE\"\n +\n -+test_expect_success 'attr.allowInvalidSource when HEAD is unborn' '\n -+\ttest_when_finished rm -rf empty &&\n -+\techo $bad_attr_source_err >expect_err &&\n -+\techo \"f/path: test: unspecified\" >expect &&\n -+\tgit init empty &&\n -+\ttest_must_fail git -C empty --attr-source=HEAD check-attr test -- f/path 2>err &&\n -+\ttest_cmp expect_err err &&\n -+\tgit -C empty -c attr.tree=HEAD -c attr.allowInvalidSource=true check-attr test -- f/path >actual 2>err &&\n -+\ttest_must_be_empty err &&\n -+\ttest_cmp expect actual\n -+'\n -+\n -+test_expect_success 'attr.allowInvalidSource when --attr-source points to non-existing ref' '\n -+\ttest_when_finished rm -rf empty &&\n -+\techo $bad_attr_source_err >expect_err &&\n -+\techo \"f/path: test: unspecified\" >expect &&\n -+\tgit init empty &&\n -+\ttest_must_fail git -C empty --attr-source=refs/does/not/exist check-attr test -- f/path 2>err &&\n -+\ttest_cmp expect_err err &&\n -+\tgit -C empty -c attr.tree=refs/does/not/exist -c attr.allowInvalidSource=true check-attr test -- f/path >actual 2>err &&\n -+\ttest_must_be_empty err &&\n -+\ttest_cmp expect actual\n -+'\n -+\n -+test_expect_success 'bad attr source defaults to reading .gitattributes file' '\n -+\ttest_when_finished rm -rf empty &&\n -+\tgit init empty &&\n -+\techo \"f/path test=val\" >empty/.gitattributes &&\n ++test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '\n ++\ttest_when_finished rm -rf test bare_with_gitattribute &&\n ++\tgit init test &&\n ++\t(\n ++\t\tcd test &&\n ++\t\ttest_commit gitattributes .gitattributes \"f/path test=val\"\n ++\t) &&\n ++\tgit clone --bare test bare_with_gitattribute &&\n +\techo \"f/path: test: val\" >expect &&\n -+\tgit -C empty -c attr.tree=HEAD -c attr.allowInvalidSource=true check-attr test -- f/path >actual 2>err &&\n -+\ttest_must_be_empty err &&\n ++\tgit -C bare_with_gitattribute check-attr test -- f/path >actual &&\n +\ttest_cmp expect actual\n +'\n -+\n -+test_expect_success 'attr.allowInvalidSource has no effect on --attr-source' '\n -+\ttest_when_finished rm -rf empty &&\n -+\techo $bad_attr_source_err >expect_err &&\n -+\techo \"f/path: test: unspecified\" >expect &&\n -+\tgit init empty &&\n -+\ttest_must_fail git -C empty -c attr.allowInvalidSource=true --attr-source=HEAD check-attr test -- f/path 2>err &&\n -+\ttest_cmp expect_err err\n -+'\n +\n test_expect_success 'bare repository: with --source' '\n \t(\n \t\tcd bare.git &&\n +\n + ## t/t5001-archive-attr.sh ##\n +@@ t/t5001-archive-attr.sh: test_expect_success 'git archive with worktree attributes, bare' '\n + '\n + \n + test_expect_missing\tbare-worktree/ignored\n +-test_expect_exists\tbare-worktree/ignored-by-tree\n ++test_expect_missing\tbare-worktree/ignored-by-tree\n + test_expect_exists\tbare-worktree/ignored-by-worktree\n + \n + test_expect_success 'export-subst' '\n 1: 446bce03a96 ! 2: dadb822da99 attr: add attr.tree for setting the treeish to read attributes from\n @@ Metadata\n ## Commit message ##\n attr: add attr.tree for setting the treeish to read attributes from\n \n - 44451a2e5e (attr: teach \"--attr-source=\" global option to \"git\",\n + 44451a2 (attr: teach \"--attr-source=\" global option to \"git\",\n 2023-05-06) provided the ability to pass in a treeish as the attr\n source. In the context of serving Git repositories as bare repos like we\n do at GitLab however, it would be easier to point --attr-source to HEAD\n @@ Documentation/config/attr.txt (new)\n @@\n +attr.tree:\n +\tA to read gitattributes from instead of the worktree. See\n -+\tlinkgit:gitattributes[5]. This is equivalent to setting the\n -+\t`GIT_ATTR_SOURCE` environment variable, or passing in --attr-source to\n -+\tthe Git command.\n ++\tlinkgit:gitattributes[5]. If `attr.tree` does not resolve to a valid tree,\n ++\ttreat it as an empty tree. --attr-source and GIT_ATTR_SOURCE take\n ++\tprecedence over attr.tree.\n \n ## attr.c ##\n +@@\n + #include \"tree-walk.h\"\n + #include \"object-name.h\"\n + \n ++const char *git_attr_tree;\n ++\n + const char git_attr__true[] = \"(builtin)true\";\n + const char git_attr__false[] = \"\\0(builtin)false\";\n + static const char git_attr__unknown[] = \"(builtin)unknown\";\n @@ attr.c: static void compute_default_attr_source(struct object_id *attr_source)\n \tif (!default_attr_source_tree_object_name)\n \t\tdefault_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);\n \n +\tif (!default_attr_source_tree_object_name) {\n -+\t\tchar *attr_tree;\n -+\n -+\t\tif (!git_config_get_string(\"attr.tree\", &attr_tree))\n -+\t\t\tdefault_attr_source_tree_object_name = attr_tree;\n ++\t\tdefault_attr_source_tree_object_name = git_attr_tree;\n ++\t\tignore_bad_attr_tree = 1;\n +\t}\n +\n - \tif (!default_attr_source_tree_object_name || !is_null_oid(attr_source))\n - \t\treturn;\n + \tif (!default_attr_source_tree_object_name &&\n + \t startup_info->have_repository &&\n + \t is_bare_repository()) {\n +\n + ## attr.h ##\n +@@ attr.h: const char *git_attr_global_file(void);\n + /* Return whether the system gitattributes file is enabled and should be used. */\n + int git_attr_system_is_enabled(void);\n + \n ++extern const char *git_attr_tree;\n ++\n + #endif /* ATTR_H */\n +\n + ## config.c ##\n +@@\n + #include \"repository.h\"\n + #include \"lockfile.h\"\n + #include \"mailmap.h\"\n ++#include \"attr.h\"\n + #include \"exec-cmd.h\"\n + #include \"strbuf.h\"\n + #include \"quote.h\"\n +@@ config.c: static int git_default_mailmap_config(const char *var, const char *value)\n + \treturn 0;\n + }\n + \n ++static int git_default_attr_config(const char *var, const char *value)\n ++{\n ++\tif (!strcmp(var, \"attr.tree\"))\n ++\t\treturn git_config_string(&git_attr_tree, var, value);\n ++\n ++\t/* Add other attribute related config variables here and to\n ++\t Documentation/config/attr.txt. */\n ++\treturn 0;\n ++}\n ++\n + int git_default_config(const char *var, const char *value,\n + \t\t const struct config_context *ctx, void *cb)\n + {\n +@@ config.c: int git_default_config(const char *var, const char *value,\n + \tif (starts_with(var, \"mailmap.\"))\n + \t\treturn git_default_mailmap_config(var, value);\n + \n ++\tif (starts_with(var, \"attr.\"))\n ++\t\treturn git_default_attr_config(var, value);\n ++\n + \tif (starts_with(var, \"advice.\") || starts_with(var, \"color.advice\"))\n + \t\treturn git_default_advice_config(var, value);\n \n \n ## t/t0003-attributes.sh ##\n @@ t/t0003-attributes.sh: attr_check_source () {\n \tGIT_ATTR_SOURCE=\"$source\" git $git_opts check-attr test -- \"$path\" >actual 2>err &&\n \ttest_cmp expect actual &&\n \ttest_must_be_empty err\n +@@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattribute is ignored' '\n + \t)\n + '\n + \n ++bad_attr_source_err=\"fatal: bad --attr-source or GIT_ATTR_SOURCE\"\n ++\n ++test_expect_success 'attr.tree when HEAD is unborn' '\n ++\ttest_when_finished rm -rf empty &&\n ++\tgit init empty &&\n ++\t(\n ++\t\tcd empty &&\n ++\t\techo $bad_attr_source_err >expect_err &&\n ++\t\techo \"f/path: test: unspecified\" >expect &&\n ++\t\tgit -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&\n ++\t\ttest_must_be_empty err &&\n ++\t\ttest_cmp expect actual\n ++\t)\n ++'\n ++\n ++test_expect_success 'attr.tree points to non-existing ref' '\n ++\ttest_when_finished rm -rf empty &&\n ++\tgit init empty &&\n ++\t(\n ++\t\tcd empty &&\n ++\t\techo $bad_attr_source_err >expect_err &&\n ++\t\techo \"f/path: test: unspecified\" >expect &&\n ++\t\tgit -c attr.tree=refs/does/not/exist check-attr test -- f/path >actual 2>err &&\n ++\t\ttest_must_be_empty err &&\n ++\t\ttest_cmp expect actual\n ++\t)\n ++'\n ++\n ++test_expect_success 'bad attr source defaults to reading .gitattributes file' '\n ++\ttest_when_finished rm -rf empty &&\n ++\tgit init empty &&\n ++\t(\n ++\t\tcd empty &&\n ++\t\techo \"f/path test=val\" >.gitattributes &&\n ++\t\techo \"f/path: test: val\" >expect &&\n ++\t\tgit -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&\n ++\t\ttest_must_be_empty err &&\n ++\t\ttest_cmp expect actual\n ++\t)\n ++'\n + \n + test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '\n + \ttest_when_finished rm -rf test bare_with_gitattribute &&\n +@@ t/t0003-attributes.sh: test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '\n + \ttest_cmp expect actual\n + '\n + \n ++test_expect_success '--attr-source and GIT_ATTR_SOURCE take precedence over attr.tree' '\n ++\ttest_when_finished rm -rf empty &&\n ++\tgit init empty &&\n ++\t(\n ++\t\tcd empty &&\n ++\t\tgit checkout -b attr-source &&\n ++\t\ttest_commit \"val1\" .gitattributes \"f/path test=val1\" &&\n ++\t\tgit checkout -b attr-tree &&\n ++\t\ttest_commit \"val2\" .gitattributes \"f/path test=val2\" &&\n ++\t\tgit checkout attr-source &&\n ++\t\techo \"f/path: test: val1\" >expect &&\n ++\t\tgit -c attr.tree=attr-tree --attr-source=attr-source check-attr test -- f/path >actual &&\n ++\t\ttest_cmp expect actual &&\n ++\t\tGIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree check-attr test -- f/path >actual &&\n ++\t\ttest_cmp expect actual\n ++\t)\n ++'\n ++\n + test_expect_success 'bare repository: with --source' '\n + \t(\n + \t\tcd bare.git &&\n\n-- \ngitgitgadget\n"},{"id":"483001","messageId":"cef206d47c724f54220b0b915e5405b48f5eb2cb.1696967380.git.gitgitgadget@gmail.com","threadId":"60250","inReplyTo":"pull.1577.v3.git.git.1696967380.gitgitgadget@gmail.com","subject":"[PATCH v3 1/2] attr: read attributes from HEAD when bare repo","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-10-10T19:49:39Z","receivedAt":"2023-10-10T19:49:50Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nThe motivation for 44451a2e5e (attr: teach \"--attr-source=\" global\noption to \"git\" , 2023-05-06), was to make it possible to use\ngitattributes with bare repositories.\n\nTo make it easier to read gitattributes in bare repositories however,\nlet's just make HEAD:.gitattributes the default. This is in line with\nhow mailmap works, 8c473cecfd (mailmap: default mailmap.blob in bare\nrepositories, 2012-12-13).\n\nSigned-off-by: John Cai \n---\n attr.c | 12 +++++++++++-\n t/t0003-attributes.sh | 14 ++++++++++++++\n t/t5001-archive-attr.sh | 2 +-\n 3 files changed, 26 insertions(+), 2 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 71c84fbcf86..bf2ea1626a6 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -1194,6 +1194,7 @@ static void collect_some_attrs(struct index_state *istate,\n }\n \n static const char *default_attr_source_tree_object_name;\n+static int ignore_bad_attr_tree;\n \n void set_git_attr_source(const char *tree_object_name)\n {\n@@ -1205,10 +1206,19 @@ static void compute_default_attr_source(struct object_id *attr_source)\n \tif (!default_attr_source_tree_object_name)\n \t\tdefault_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);\n \n+\tif (!default_attr_source_tree_object_name &&\n+\t startup_info->have_repository &&\n+\t is_bare_repository()) {\n+\t\tdefault_attr_source_tree_object_name = \"HEAD\";\n+\t\tignore_bad_attr_tree = 1;\n+\t}\n+\n \tif (!default_attr_source_tree_object_name || !is_null_oid(attr_source))\n \t\treturn;\n \n-\tif (repo_get_oid_treeish(the_repository, default_attr_source_tree_object_name, attr_source))\n+\tif (repo_get_oid_treeish(the_repository,\n+\t\t\t\t default_attr_source_tree_object_name,\n+\t\t\t\t attr_source) && !ignore_bad_attr_tree)\n \t\tdie(_(\"bad --attr-source or GIT_ATTR_SOURCE\"));\n }\n \ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 26e082f05b4..e6b1a117228 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -342,6 +342,20 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '\n \t)\n '\n \n+\n+test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '\n+\ttest_when_finished rm -rf test bare_with_gitattribute &&\n+\tgit init test &&\n+\t(\n+\t\tcd test &&\n+\t\ttest_commit gitattributes .gitattributes \"f/path test=val\"\n+\t) &&\n+\tgit clone --bare test bare_with_gitattribute &&\n+\techo \"f/path: test: val\" >expect &&\n+\tgit -C bare_with_gitattribute check-attr test -- f/path >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'bare repository: with --source' '\n \t(\n \t\tcd bare.git &&\ndiff --git a/t/t5001-archive-attr.sh b/t/t5001-archive-attr.sh\nindex 0ff47a239db..eaf959d8f63 100755\n--- a/t/t5001-archive-attr.sh\n+++ b/t/t5001-archive-attr.sh\n@@ -138,7 +138,7 @@ test_expect_success 'git archive with worktree attributes, bare' '\n '\n \n test_expect_missing\tbare-worktree/ignored\n-test_expect_exists\tbare-worktree/ignored-by-tree\n+test_expect_missing\tbare-worktree/ignored-by-tree\n test_expect_exists\tbare-worktree/ignored-by-worktree\n \n test_expect_success 'export-subst' '\n-- \ngitgitgadget\n\n"},{"id":"483002","messageId":"dadb822da99772cd277417f564cf672f65d1cc24.1696967380.git.gitgitgadget@gmail.com","threadId":"60250","inReplyTo":"pull.1577.v3.git.git.1696967380.gitgitgadget@gmail.com","subject":"[PATCH v3 2/2] attr: add attr.tree for setting the treeish to read attributes from","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-10-10T19:49:40Z","receivedAt":"2023-10-10T19:49:51Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\n44451a2 (attr: teach \"--attr-source=\" global option to \"git\",\n2023-05-06) provided the ability to pass in a treeish as the attr\nsource. In the context of serving Git repositories as bare repos like we\ndo at GitLab however, it would be easier to point --attr-source to HEAD\nfor all commands by setting it once.\n\nAdd a new config attr.tree that allows this.\n\nSigned-off-by: John Cai \n---\n Documentation/config.txt | 2 ++\n Documentation/config/attr.txt | 5 +++\n attr.c | 7 ++++\n attr.h | 2 ++\n config.c | 14 ++++++++\n t/t0003-attributes.sh | 62 +++++++++++++++++++++++++++++++++++\n 6 files changed, 92 insertions(+)\n create mode 100644 Documentation/config/attr.txt\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 229b63a454c..b1891c2b5af 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -371,6 +371,8 @@ other popular tools, and describe them in your documentation.\n \n include::config/advice.txt[]\n \n+include::config/attr.txt[]\n+\n include::config/core.txt[]\n \n include::config/add.txt[]\ndiff --git a/Documentation/config/attr.txt b/Documentation/config/attr.txt\nnew file mode 100644\nindex 00000000000..be882523f8b\n--- /dev/null\n+++ b/Documentation/config/attr.txt\n@@ -0,0 +1,5 @@\n+attr.tree:\n+\tA to read gitattributes from instead of the worktree. See\n+\tlinkgit:gitattributes[5]. If `attr.tree` does not resolve to a valid tree,\n+\ttreat it as an empty tree. --attr-source and GIT_ATTR_SOURCE take\n+\tprecedence over attr.tree.\ndiff --git a/attr.c b/attr.c\nindex bf2ea1626a6..0ae6852d12b 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -24,6 +24,8 @@\n #include \"tree-walk.h\"\n #include \"object-name.h\"\n \n+const char *git_attr_tree;\n+\n const char git_attr__true[] = \"(builtin)true\";\n const char git_attr__false[] = \"\\0(builtin)false\";\n static const char git_attr__unknown[] = \"(builtin)unknown\";\n@@ -1206,6 +1208,11 @@ static void compute_default_attr_source(struct object_id *attr_source)\n \tif (!default_attr_source_tree_object_name)\n \t\tdefault_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);\n \n+\tif (!default_attr_source_tree_object_name) {\n+\t\tdefault_attr_source_tree_object_name = git_attr_tree;\n+\t\tignore_bad_attr_tree = 1;\n+\t}\n+\n \tif (!default_attr_source_tree_object_name &&\n \t startup_info->have_repository &&\n \t is_bare_repository()) {\ndiff --git a/attr.h b/attr.h\nindex 2b745df4054..127998ae013 100644\n--- a/attr.h\n+++ b/attr.h\n@@ -236,4 +236,6 @@ const char *git_attr_global_file(void);\n /* Return whether the system gitattributes file is enabled and should be used. */\n int git_attr_system_is_enabled(void);\n \n+extern const char *git_attr_tree;\n+\n #endif /* ATTR_H */\ndiff --git a/config.c b/config.c\nindex 3846a37be97..21a1590b505 100644\n--- a/config.c\n+++ b/config.c\n@@ -18,6 +18,7 @@\n #include \"repository.h\"\n #include \"lockfile.h\"\n #include \"mailmap.h\"\n+#include \"attr.h\"\n #include \"exec-cmd.h\"\n #include \"strbuf.h\"\n #include \"quote.h\"\n@@ -1904,6 +1905,16 @@ static int git_default_mailmap_config(const char *var, const char *value)\n \treturn 0;\n }\n \n+static int git_default_attr_config(const char *var, const char *value)\n+{\n+\tif (!strcmp(var, \"attr.tree\"))\n+\t\treturn git_config_string(&git_attr_tree, var, value);\n+\n+\t/* Add other attribute related config variables here and to\n+\t Documentation/config/attr.txt. */\n+\treturn 0;\n+}\n+\n int git_default_config(const char *var, const char *value,\n \t\t const struct config_context *ctx, void *cb)\n {\n@@ -1927,6 +1938,9 @@ int git_default_config(const char *var, const char *value,\n \tif (starts_with(var, \"mailmap.\"))\n \t\treturn git_default_mailmap_config(var, value);\n \n+\tif (starts_with(var, \"attr.\"))\n+\t\treturn git_default_attr_config(var, value);\n+\n \tif (starts_with(var, \"advice.\") || starts_with(var, \"color.advice\"))\n \t\treturn git_default_advice_config(var, value);\n \ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex e6b1a117228..b0949125f26 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -40,6 +40,10 @@ attr_check_source () {\n \ttest_cmp expect actual &&\n \ttest_must_be_empty err\n \n+\tgit $git_opts -c \"attr.tree=$source\" check-attr test -- \"$path\" >actual 2>err &&\n+\ttest_cmp expect actual &&\n+\ttest_must_be_empty err\n+\n \tGIT_ATTR_SOURCE=\"$source\" git $git_opts check-attr test -- \"$path\" >actual 2>err &&\n \ttest_cmp expect actual &&\n \ttest_must_be_empty err\n@@ -342,6 +346,46 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '\n \t)\n '\n \n+bad_attr_source_err=\"fatal: bad --attr-source or GIT_ATTR_SOURCE\"\n+\n+test_expect_success 'attr.tree when HEAD is unborn' '\n+\ttest_when_finished rm -rf empty &&\n+\tgit init empty &&\n+\t(\n+\t\tcd empty &&\n+\t\techo $bad_attr_source_err >expect_err &&\n+\t\techo \"f/path: test: unspecified\" >expect &&\n+\t\tgit -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&\n+\t\ttest_must_be_empty err &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'attr.tree points to non-existing ref' '\n+\ttest_when_finished rm -rf empty &&\n+\tgit init empty &&\n+\t(\n+\t\tcd empty &&\n+\t\techo $bad_attr_source_err >expect_err &&\n+\t\techo \"f/path: test: unspecified\" >expect &&\n+\t\tgit -c attr.tree=refs/does/not/exist check-attr test -- f/path >actual 2>err &&\n+\t\ttest_must_be_empty err &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'bad attr source defaults to reading .gitattributes file' '\n+\ttest_when_finished rm -rf empty &&\n+\tgit init empty &&\n+\t(\n+\t\tcd empty &&\n+\t\techo \"f/path test=val\" >.gitattributes &&\n+\t\techo \"f/path: test: val\" >expect &&\n+\t\tgit -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&\n+\t\ttest_must_be_empty err &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n \n test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '\n \ttest_when_finished rm -rf test bare_with_gitattribute &&\n@@ -356,6 +400,24 @@ test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--attr-source and GIT_ATTR_SOURCE take precedence over attr.tree' '\n+\ttest_when_finished rm -rf empty &&\n+\tgit init empty &&\n+\t(\n+\t\tcd empty &&\n+\t\tgit checkout -b attr-source &&\n+\t\ttest_commit \"val1\" .gitattributes \"f/path test=val1\" &&\n+\t\tgit checkout -b attr-tree &&\n+\t\ttest_commit \"val2\" .gitattributes \"f/path test=val2\" &&\n+\t\tgit checkout attr-source &&\n+\t\techo \"f/path: test: val1\" >expect &&\n+\t\tgit -c attr.tree=attr-tree --attr-source=attr-source check-attr test -- f/path >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\tGIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree check-attr test -- f/path >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'bare repository: with --source' '\n \t(\n \t\tcd bare.git &&\n-- \ngitgitgadget\n"},{"id":"483003","messageId":"CAPig+cQHSiVbsNTDQWJZcyEA3uAHu5EzbCkd4CSJ7+Sno+Az-Q@mail.gmail.com","threadId":"60250","inReplyTo":"cef206d47c724f54220b0b915e5405b48f5eb2cb.1696967380.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/2] attr: read attributes from HEAD when bare repo","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-10-10T19:58:16Z","receivedAt":"2023-10-10T19:58:32Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Oct 10, 2023 at 3:49 PM John Cai via GitGitGadget\n wrote:\n> The motivation for 44451a2e5e (attr: teach \"--attr-source=\" global\n> option to \"git\" , 2023-05-06), was to make it possible to use\n> gitattributes with bare repositories.\n>\n> To make it easier to read gitattributes in bare repositories however,\n> let's just make HEAD:.gitattributes the default. This is in line with\n> how mailmap works, 8c473cecfd (mailmap: default mailmap.blob in bare\n> repositories, 2012-12-13).\n>\n> Signed-off-by: John Cai \n> ---\n> diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\n> @@ -342,6 +342,20 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '\n> +test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '\n> + test_when_finished rm -rf test bare_with_gitattribute &&\n> + git init test &&\n> + (\n> + cd test &&\n> + test_commit gitattributes .gitattributes \"f/path test=val\"\n> + ) &&\n\nNot at all worth a reroll, rather just for future reference... these\ndays test_commit() supports -C, so the same could be accomplished\nwithout the subshell or `cd`:\n\n test_commit -C test gitattributes .gitattributes \"f/path test=val\" &&\n\n> + git clone --bare test bare_with_gitattribute &&\n> + echo \"f/path: test: val\" >expect &&\n> + git -C bare_with_gitattribute check-attr test -- f/path >actual &&\n> + test_cmp expect actual\n> +'\n"},{"id":"483025","messageId":"xmqqfs2iqg4k.fsf@gitster.g","threadId":"60250","inReplyTo":"dadb822da99772cd277417f564cf672f65d1cc24.1696967380.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/2] attr: add attr.tree for setting the treeish to read attributes from","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-10T22:14:51Z","receivedAt":"2023-10-10T22:15:00Z","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> 44451a2 (attr: teach \"--attr-source=\" global option to \"git\",\n> 2023-05-06) provided the ability to pass in a treeish as the attr\n> source. In the context of serving Git repositories as bare repos like we\n> do at GitLab however, it would be easier to point --attr-source to HEAD\n> for all commands by setting it once.\n>\n> Add a new config attr.tree that allows this.\n>\n> Signed-off-by: John Cai \n> ---\n> Documentation/config.txt | 2 ++\n> Documentation/config/attr.txt | 5 +++\n> attr.c | 7 ++++\n> attr.h | 2 ++\n> config.c | 14 ++++++++\n> t/t0003-attributes.sh | 62 +++++++++++++++++++++++++++++++++++\n> 6 files changed, 92 insertions(+)\n> create mode 100644 Documentation/config/attr.txt\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 229b63a454c..b1891c2b5af 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -371,6 +371,8 @@ other popular tools, and describe them in your documentation.\n> \n> include::config/advice.txt[]\n> \n> +include::config/attr.txt[]\n> +\n> include::config/core.txt[]\n> \n> include::config/add.txt[]\n> diff --git a/Documentation/config/attr.txt b/Documentation/config/attr.txt\n> new file mode 100644\n> index 00000000000..be882523f8b\n> --- /dev/null\n> +++ b/Documentation/config/attr.txt\n> @@ -0,0 +1,5 @@\n> +attr.tree:\n> +\tA to read gitattributes from instead of the worktree. See\n> +\tlinkgit:gitattributes[5]. If `attr.tree` does not resolve to a valid tree,\n> +\ttreat it as an empty tree. --attr-source and GIT_ATTR_SOURCE take\n> +\tprecedence over attr.tree.\n\nProperly typeset `--attr-source` and `GIT_ATTR_SOURCE`.\n\nA quick \"git grep\" in Documentation/config/*.txt tells me that\nnobody refers to an object type like . Imitate what this\nwas modeled after, namely Documentation/config/mailmap.txt, which\nsays just\n\n\t... a reference to a blob in the repository.\n\nwithout any half mark-up.\n\nMore importantly, the description makes one wonder what the\nprecedence rule between these two (the general rule would be for\ncommand line parameter to override environment, if I recall\ncorrectly).\n\nI think the enumeration header usually is followed by double-colons\namong Documentation/config/*.txt files. Let's be consistent.\n\nIn the context of this expression, \"worktree\" is a wrong noun to\nuse---the term of art refers to an instance of \"working tree\",\ntogether with some \"per worktree\" administrative files inside .git/\ndirectory. On the other hand, \"working tree\" refers to the \"files\nmeant to be visible to build tools and editors part of the non-bare\nrepository\", which is what you want to use here.\n\nattr.tree::\n\tA tree object to read the attributes from, instead of the\n\t`.gitattributes` file in the working tree. In a bare\n\trepository, this defaults to HEAD:.gitattributes\". If a\n\tgiven value does not resolve to a valid tree object, an\n\tempty tree is used instead. When `GIT_ATTR_SOURCE`\n\tenvironment variable or `--attr-source` command line option\n\tis used, this configuration variable has no effect.\n\nor something along that line, perhaps.\n\n> diff --git a/attr.c b/attr.c\n> index bf2ea1626a6..0ae6852d12b 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -24,6 +24,8 @@\n> #include \"tree-walk.h\"\n> #include \"object-name.h\"\n> \n> +const char *git_attr_tree;\n> +\n> const char git_attr__true[] = \"(builtin)true\";\n> const char git_attr__false[] = \"\\0(builtin)false\";\n> static const char git_attr__unknown[] = \"(builtin)unknown\";\n> @@ -1206,6 +1208,11 @@ static void compute_default_attr_source(struct object_id *attr_source)\n> \tif (!default_attr_source_tree_object_name)\n> \t\tdefault_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);\n> \n> +\tif (!default_attr_source_tree_object_name) {\n> +\t\tdefault_attr_source_tree_object_name = git_attr_tree;\n> +\t\tignore_bad_attr_tree = 1;\n> +\t}\n\nAs long as \"attr.tree\" was read by calling git_default_attr_config()\nbefore we come here, git_attr_tree is not NULL and we allow bad attr\ntree in default_attr_source_tree_object_name. But stepping back a\nbit, even if \"attr.tree\" is unspecified, i.e., git_attr_tree is\nNULL, we set ignore_bad_attr_tree to true here.\n\nWhat it means is that after the above if() statement, if\ndefault_attr_source_tree_object_name is still NULL, we know that\nignore_bad_attr_tree is already set to true. \n\n> \tif (!default_attr_source_tree_object_name &&\n> \t startup_info->have_repository &&\n> \t is_bare_repository()) {\n\nSo would it make more sense to remove the assignment to the same\nvariable we made in [1/2] around here (not seen in the post\ncontext)?\n\nAlternatively, even though it makes the code a bit more verbose, the\nlogic might become clearer if you wrote the \"assign from the config\"\npart like so:\n\n\tif (!default_attr_source_tree_object_name && git_attr_tree) {\n \tdefault_attr_source_tree_object_name = git_attr_tree;\n\t\tignore_bad_attr_tree = 1;\n\t}\n\nIt would leave more flexibility to the code around here. You could\nfor example add code that assigns a different value, a tree object\nthat is required to exist, to default_attr_source_tree_object_name\nafter this point, for example, without having to wonder what the\n\"current\" value of ignore_bad_attr_tree is.\n\n> +static int git_default_attr_config(const char *var, const char *value)\n> +{\n> +\tif (!strcmp(var, \"attr.tree\"))\n> +\t\treturn git_config_string(&git_attr_tree, var, value);\n> +\n> +\t/* Add other attribute related config variables here and to\n> +\t Documentation/config/attr.txt. */\n\n\t/*\n\t * Our multi-line comments should look\n * more like this; opening slash-asterisk\n\t * and closing asterisk-slash sit on a line\n\t * on its own.\n\t */\n\n> @@ -342,6 +346,46 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '\n> \t)\n> '\n> \n> +bad_attr_source_err=\"fatal: bad --attr-source or GIT_ATTR_SOURCE\"\n\nNot a fault of this two-patch series, but we probably should refine\nthis error reporting so that the reader can tell which one is being\ncomplained about, and optionally what the offending value was.\n\n\n> +test_expect_success 'attr.tree when HEAD is unborn' '\n> +\ttest_when_finished rm -rf empty &&\n> +\tgit init empty &&\n> +\t(\n> +\t\tcd empty &&\n> +\t\techo $bad_attr_source_err >expect_err &&\n\nLet's not rely on words in the error message split with exactly one\nwhitespace each and instead quote the variable properly. I.e.,\n\n\t\techo \"$bad_attr_source_err\" >expect_err &&\n\nBut this is not even used, as we do not expect it to fail. Perhaps\nremove it altogether?\n\n> +\t\techo \"f/path: test: unspecified\" >expect &&\n> +\t\tgit -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&\n> +\t\ttest_must_be_empty err &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n> +test_expect_success 'attr.tree points to non-existing ref' '\n> +\ttest_when_finished rm -rf empty &&\n> +\tgit init empty &&\n> +\t(\n> +\t\tcd empty &&\n> +\t\techo $bad_attr_source_err >expect_err &&\n\nDitto.\n\n> +\t\techo \"f/path: test: unspecified\" >expect &&\n> +\t\tgit -c attr.tree=refs/does/not/exist check-attr test -- f/path >actual 2>err &&\n> +\t\ttest_must_be_empty err &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n> +test_expect_success 'bad attr source defaults to reading .gitattributes file' '\n> +\ttest_when_finished rm -rf empty &&\n> +\tgit init empty &&\n> +\t(\n> +\t\tcd empty &&\n> +\t\techo \"f/path test=val\" >.gitattributes &&\n> +\t\techo \"f/path: test: val\" >expect &&\n> +\t\tgit -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&\n> +\t\ttest_must_be_empty err &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n\nIn other words, with the additional tests, we do not check error\ncases (which may be perfectly OK, if they are covered by existing\ntests). A bit curious.\n\n> @@ -356,6 +400,24 @@ test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success '--attr-source and GIT_ATTR_SOURCE take precedence over attr.tree' '\n\nDo we want to ensure which one takes precedence between the command\nline option and the environment? It's not like the one that is\ngiven the last takes effect.\n\n> +\ttest_when_finished rm -rf empty &&\n> +\tgit init empty &&\n> +\t(\n> +\t\tcd empty &&\n> +\t\tgit checkout -b attr-source &&\n> +\t\ttest_commit \"val1\" .gitattributes \"f/path test=val1\" &&\n> +\t\tgit checkout -b attr-tree &&\n> +\t\ttest_commit \"val2\" .gitattributes \"f/path test=val2\" &&\n> +\t\tgit checkout attr-source &&\n> +\t\techo \"f/path: test: val1\" >expect &&\n> +\t\tgit -c attr.tree=attr-tree --attr-source=attr-source check-attr test -- f/path >actual &&\n> +\t\ttest_cmp expect actual &&\n> +\t\tGIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree check-attr test -- f/path >actual &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n> test_expect_success 'bare repository: with --source' '\n> \t(\n> \t\tcd bare.git &&\n\nOther than that, looking great. Thanks.\n"},{"id":"483033","messageId":"2A3060D0-C551-47F6-B27A-049B52B3E0F3@gmail.com","threadId":"60250","inReplyTo":"xmqqfs2iqg4k.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] attr: add attr.tree for setting the treeish to read attributes from","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2023-10-11T02:19:45Z","receivedAt":"2023-10-11T02:19:51Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Junio,\n\nOn 10 Oct 2023, at 18:14, Junio C Hamano wrote:\n\n> \"John Cai via GitGitGadget\" writes:\n>\n>> From: John Cai \n>>\n>> 44451a2 (attr: teach \"--attr-source=\" global option to \"git\",\n>> 2023-05-06) provided the ability to pass in a treeish as the attr\n>> source. In the context of serving Git repositories as bare repos like we\n>> do at GitLab however, it would be easier to point --attr-source to HEAD\n>> for all commands by setting it once.\n>>\n>> Add a new config attr.tree that allows this.\n>>\n>> Signed-off-by: John Cai \n>> ---\n>> Documentation/config.txt | 2 ++\n>> Documentation/config/attr.txt | 5 +++\n>> attr.c | 7 ++++\n>> attr.h | 2 ++\n>> config.c | 14 ++++++++\n>> t/t0003-attributes.sh | 62 +++++++++++++++++++++++++++++++++++\n>> 6 files changed, 92 insertions(+)\n>> create mode 100644 Documentation/config/attr.txt\n>>\n>> diff --git a/Documentation/config.txt b/Documentation/config.txt\n>> index 229b63a454c..b1891c2b5af 100644\n>> --- a/Documentation/config.txt\n>> +++ b/Documentation/config.txt\n>> @@ -371,6 +371,8 @@ other popular tools, and describe them in your documentation.\n>>\n>> include::config/advice.txt[]\n>>\n>> +include::config/attr.txt[]\n>> +\n>> include::config/core.txt[]\n>>\n>> include::config/add.txt[]\n>> diff --git a/Documentation/config/attr.txt b/Documentation/config/attr.txt\n>> new file mode 100644\n>> index 00000000000..be882523f8b\n>> --- /dev/null\n>> +++ b/Documentation/config/attr.txt\n>> @@ -0,0 +1,5 @@\n>> +attr.tree:\n>> +\tA to read gitattributes from instead of the worktree. See\n>> +\tlinkgit:gitattributes[5]. If `attr.tree` does not resolve to a valid tree,\n>> +\ttreat it as an empty tree. --attr-source and GIT_ATTR_SOURCE take\n>> +\tprecedence over attr.tree.\n>\n> Properly typeset `--attr-source` and `GIT_ATTR_SOURCE`.\n>\n> A quick \"git grep\" in Documentation/config/*.txt tells me that\n> nobody refers to an object type like . Imitate what this\n> was modeled after, namely Documentation/config/mailmap.txt, which\n> says just\n>\n> \t... a reference to a blob in the repository.\n>\n> without any half mark-up.\n\nsounds good\n\n>\n> More importantly, the description makes one wonder what the\n> precedence rule between these two (the general rule would be for\n> command line parameter to override environment, if I recall\n> correctly).\n\nyes, that should be the case.\n\n>\n> I think the enumeration header usually is followed by double-colons\n> among Documentation/config/*.txt files. Let's be consistent.\n>\n> In the context of this expression, \"worktree\" is a wrong noun to\n> use---the term of art refers to an instance of \"working tree\",\n> together with some \"per worktree\" administrative files inside .git/\n> directory. On the other hand, \"working tree\" refers to the \"files\n> meant to be visible to build tools and editors part of the non-bare\n> repository\", which is what you want to use here.\n>\n> attr.tree::\n> \tA tree object to read the attributes from, instead of the\n> \t`.gitattributes` file in the working tree. In a bare\n> \trepository, this defaults to HEAD:.gitattributes\". If a\n> \tgiven value does not resolve to a valid tree object, an\n> \tempty tree is used instead. When `GIT_ATTR_SOURCE`\n> \tenvironment variable or `--attr-source` command line option\n> \tis used, this configuration variable has no effect.\n>\n> or something along that line, perhaps.\n\nsounds good to me, thanks\n\n>\n>> diff --git a/attr.c b/attr.c\n>> index bf2ea1626a6..0ae6852d12b 100644\n>> --- a/attr.c\n>> +++ b/attr.c\n>> @@ -24,6 +24,8 @@\n>> #include \"tree-walk.h\"\n>> #include \"object-name.h\"\n>>\n>> +const char *git_attr_tree;\n>> +\n>> const char git_attr__true[] = \"(builtin)true\";\n>> const char git_attr__false[] = \"\\0(builtin)false\";\n>> static const char git_attr__unknown[] = \"(builtin)unknown\";\n>> @@ -1206,6 +1208,11 @@ static void compute_default_attr_source(struct object_id *attr_source)\n>> \tif (!default_attr_source_tree_object_name)\n>> \t\tdefault_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);\n>>\n>> +\tif (!default_attr_source_tree_object_name) {\n>> +\t\tdefault_attr_source_tree_object_name = git_attr_tree;\n>> +\t\tignore_bad_attr_tree = 1;\n>> +\t}\n>\n> As long as \"attr.tree\" was read by calling git_default_attr_config()\n> before we come here, git_attr_tree is not NULL and we allow bad attr\n> tree in default_attr_source_tree_object_name. But stepping back a\n> bit, even if \"attr.tree\" is unspecified, i.e., git_attr_tree is\n> NULL, we set ignore_bad_attr_tree to true here.\n>\n> What it means is that after the above if() statement, if\n> default_attr_source_tree_object_name is still NULL, we know that\n> ignore_bad_attr_tree is already set to true.\n>\n>> \tif (!default_attr_source_tree_object_name &&\n>> \t startup_info->have_repository &&\n>> \t is_bare_repository()) {\n>\n> So would it make more sense to remove the assignment to the same\n> variable we made in [1/2] around here (not seen in the post\n> context)?\n>\n> Alternatively, even though it makes the code a bit more verbose, the\n> logic might become clearer if you wrote the \"assign from the config\"\n> part like so:\n>\n> \tif (!default_attr_source_tree_object_name && git_attr_tree) {\n> \tdefault_attr_source_tree_object_name = git_attr_tree;\n> \t\tignore_bad_attr_tree = 1;\n> \t}\n>\n> It would leave more flexibility to the code around here. You could\n> for example add code that assigns a different value, a tree object\n> that is required to exist, to default_attr_source_tree_object_name\n> after this point, for example, without having to wonder what the\n> \"current\" value of ignore_bad_attr_tree is.\n\ngood point--I think this logic is easier to follow.\n\n>\n>> +static int git_default_attr_config(const char *var, const char *value)\n>> +{\n>> +\tif (!strcmp(var, \"attr.tree\"))\n>> +\t\treturn git_config_string(&git_attr_tree, var, value);\n>> +\n>> +\t/* Add other attribute related config variables here and to\n>> +\t Documentation/config/attr.txt. */\n>\n> \t/*\n> \t * Our multi-line comments should look\n> * more like this; opening slash-asterisk\n> \t * and closing asterisk-slash sit on a line\n> \t * on its own.\n> \t */\n>\n>> @@ -342,6 +346,46 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '\n>> \t)\n>> '\n>>\n>> +bad_attr_source_err=\"fatal: bad --attr-source or GIT_ATTR_SOURCE\"\n>\n> Not a fault of this two-patch series, but we probably should refine\n> this error reporting so that the reader can tell which one is being\n> complained about, and optionally what the offending value was.\n>\n>\n>> +test_expect_success 'attr.tree when HEAD is unborn' '\n>> +\ttest_when_finished rm -rf empty &&\n>> +\tgit init empty &&\n>> +\t(\n>> +\t\tcd empty &&\n>> +\t\techo $bad_attr_source_err >expect_err &&\n>\n> Let's not rely on words in the error message split with exactly one\n> whitespace each and instead quote the variable properly. I.e.,\n>\n> \t\techo \"$bad_attr_source_err\" >expect_err &&\n>\n> But this is not even used, as we do not expect it to fail. Perhaps\n> remove it altogether?\n>\n>> +\t\techo \"f/path: test: unspecified\" >expect &&\n>> +\t\tgit -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&\n>> +\t\ttest_must_be_empty err &&\n>> +\t\ttest_cmp expect actual\n>> +\t)\n>> +'\n>> +\n>> +test_expect_success 'attr.tree points to non-existing ref' '\n>> +\ttest_when_finished rm -rf empty &&\n>> +\tgit init empty &&\n>> +\t(\n>> +\t\tcd empty &&\n>> +\t\techo $bad_attr_source_err >expect_err &&\n>\n> Ditto.\n>\n>> +\t\techo \"f/path: test: unspecified\" >expect &&\n>> +\t\tgit -c attr.tree=refs/does/not/exist check-attr test -- f/path >actual 2>err &&\n>> +\t\ttest_must_be_empty err &&\n>> +\t\ttest_cmp expect actual\n>> +\t)\n>> +'\n>> +\n>> +test_expect_success 'bad attr source defaults to reading .gitattributes file' '\n>> +\ttest_when_finished rm -rf empty &&\n>> +\tgit init empty &&\n>> +\t(\n>> +\t\tcd empty &&\n>> +\t\techo \"f/path test=val\" >.gitattributes &&\n>> +\t\techo \"f/path: test: val\" >expect &&\n>> +\t\tgit -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&\n>> +\t\ttest_must_be_empty err &&\n>> +\t\ttest_cmp expect actual\n>> +\t)\n>> +'\n>\n> In other words, with the additional tests, we do not check error\n> cases (which may be perfectly OK, if they are covered by existing\n> tests). A bit curious.\n\nJust checked and it does seem like we don't have a test case that covers the\nerror case. Will add in the next version.\n\n>\n>> @@ -356,6 +400,24 @@ test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '\n>> \ttest_cmp expect actual\n>> '\n>>\n>> +test_expect_success '--attr-source and GIT_ATTR_SOURCE take precedence over attr.tree' '\n>\n> Do we want to ensure which one takes precedence between the command\n> line option and the environment? It's not like the one that is\n> given the last takes effect.\n\nyeah, might as well.\n\n>\n>> +\ttest_when_finished rm -rf empty &&\n>> +\tgit init empty &&\n>> +\t(\n>> +\t\tcd empty &&\n>> +\t\tgit checkout -b attr-source &&\n>> +\t\ttest_commit \"val1\" .gitattributes \"f/path test=val1\" &&\n>> +\t\tgit checkout -b attr-tree &&\n>> +\t\ttest_commit \"val2\" .gitattributes \"f/path test=val2\" &&\n>> +\t\tgit checkout attr-source &&\n>> +\t\techo \"f/path: test: val1\" >expect &&\n>> +\t\tgit -c attr.tree=attr-tree --attr-source=attr-source check-attr test -- f/path >actual &&\n>> +\t\ttest_cmp expect actual &&\n>> +\t\tGIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree check-attr test -- f/path >actual &&\n>> +\t\ttest_cmp expect actual\n>> +\t)\n>> +'\n>> +\n>> test_expect_success 'bare repository: with --source' '\n>> \t(\n>> \t\tcd bare.git &&\n>\n> Other than that, looking great. Thanks.\n\nthanks!\nJohn\n"},{"id":"483059","messageId":"pull.1577.v4.git.git.1697044422.gitgitgadget@gmail.com","threadId":"60250","inReplyTo":"pull.1577.v3.git.git.1696967380.gitgitgadget@gmail.com","subject":"[PATCH v4 0/2] attr: add attr.tree config","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-10-11T17:13:40Z","receivedAt":"2023-10-11T17:13:49Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"44451a2e5e (attr: teach \"--attr-source=\" global option to \"git\", 2023-05-06)\nprovided the ability to pass in a treeish as the attr source. When a\nrevision does not resolve to a valid tree is passed, Git will die. At\nGitLab, we server repositories as bare repos and would like to always read\nattributes from the default branch, so we'd like to pass in HEAD as the\ntreeish to read gitattributes from on every command. In this context we\nwould not want Git to die if HEAD is unborn, like in the case of empty\nrepositories.\n\nInstead of modifying the default behavior of --attr-source, create a new\nconfig attr.tree with which an admin can configure a ref for all commands to\nread gitattributes from. Also make the default tree to read from HEAD on\nbare repositories.\n\nChanges since v2:\n\n * relax the restrictions around attr.tree so that if it does not resolve to\n a valid treeish, ignore it.\n * add a commit to default to HEAD in bare repositories\n\nChanges since v1:\n\n * Added a commit to add attr.tree config\n\nJohn Cai (2):\n attr: read attributes from HEAD when bare repo\n attr: add attr.tree for setting the treeish to read attributes from\n\n Documentation/config.txt | 2 +\n Documentation/config/attr.txt | 7 +++\n attr.c | 19 +++++++-\n attr.h | 2 +\n config.c | 16 +++++++\n t/t0003-attributes.sh | 84 +++++++++++++++++++++++++++++++++++\n t/t5001-archive-attr.sh | 2 +-\n 7 files changed, 130 insertions(+), 2 deletions(-)\n create mode 100644 Documentation/config/attr.txt\n\n\nbase-commit: 1fc548b2d6a3596f3e1c1f8b1930d8dbd1e30bf3\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1577%2Fjohn-cai%2Fjc%2Fconfig-attr-invalid-source-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1577/john-cai/jc/config-attr-invalid-source-v4\nPull-Request: https://github.com/git/git/pull/1577\n\nRange-diff vs v3:\n\n 1: cef206d47c7 ! 1: eaa27c47810 attr: read attributes from HEAD when bare repo\n @@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr\n +test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '\n +\ttest_when_finished rm -rf test bare_with_gitattribute &&\n +\tgit init test &&\n -+\t(\n -+\t\tcd test &&\n -+\t\ttest_commit gitattributes .gitattributes \"f/path test=val\"\n -+\t) &&\n ++\ttest_commit -C test gitattributes .gitattributes \"f/path test=val\" &&\n +\tgit clone --bare test bare_with_gitattribute &&\n +\techo \"f/path: test: val\" >expect &&\n +\tgit -C bare_with_gitattribute check-attr test -- f/path >actual &&\n 2: dadb822da99 ! 2: 749d8a8082e attr: add attr.tree for setting the treeish to read attributes from\n @@ Documentation/config.txt: other popular tools, and describe them in your documen\n \n ## Documentation/config/attr.txt (new) ##\n @@\n -+attr.tree:\n -+\tA to read gitattributes from instead of the worktree. See\n -+\tlinkgit:gitattributes[5]. If `attr.tree` does not resolve to a valid tree,\n -+\ttreat it as an empty tree. --attr-source and GIT_ATTR_SOURCE take\n -+\tprecedence over attr.tree.\n ++attr.tree::\n ++\tA reference to a tree in the repository from which to read attributes,\n ++\tinstead of the `.gitattributes` file in the working tree. In a bare\n ++\trepository, this defaults to `HEAD:.gitattributes`. If the value does\n ++\tnot resolve to a valid tree object, an empty tree is used instead.\n ++\tWhen the `GIT_ATTR_SOURCE` environment variable or `--attr-source`\n ++\tcommand line option are used, this configuration variable has no effect.\n \n ## attr.c ##\n @@\n @@ attr.c: static void compute_default_attr_source(struct object_id *attr_source)\n \tif (!default_attr_source_tree_object_name)\n \t\tdefault_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);\n \n -+\tif (!default_attr_source_tree_object_name) {\n ++\tif (!default_attr_source_tree_object_name && git_attr_tree) {\n +\t\tdefault_attr_source_tree_object_name = git_attr_tree;\n +\t\tignore_bad_attr_tree = 1;\n +\t}\n @@ config.c: static int git_default_mailmap_config(const char *var, const char *val\n +\tif (!strcmp(var, \"attr.tree\"))\n +\t\treturn git_config_string(&git_attr_tree, var, value);\n +\n -+\t/* Add other attribute related config variables here and to\n -+\t Documentation/config/attr.txt. */\n ++\t/*\n ++\t * Add other attribute related config variables here and to\n ++\t * Documentation/config/attr.txt.\n ++\t */\n +\treturn 0;\n +}\n +\n @@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr\n \n +bad_attr_source_err=\"fatal: bad --attr-source or GIT_ATTR_SOURCE\"\n +\n ++test_expect_success '--attr-source is bad' '\n ++\ttest_when_finished rm -rf empty &&\n ++\tgit init empty &&\n ++\t(\n ++\t\tcd empty &&\n ++\t\techo \"$bad_attr_source_err\" >expect_err &&\n ++\t\ttest_must_fail git --attr-source=HEAD check-attr test -- f/path 2>err &&\n ++\t\ttest_cmp expect_err err\n ++\t)\n ++'\n ++\n +test_expect_success 'attr.tree when HEAD is unborn' '\n +\ttest_when_finished rm -rf empty &&\n +\tgit init empty &&\n +\t(\n +\t\tcd empty &&\n -+\t\techo $bad_attr_source_err >expect_err &&\n +\t\techo \"f/path: test: unspecified\" >expect &&\n +\t\tgit -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&\n +\t\ttest_must_be_empty err &&\n @@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr\n +\tgit init empty &&\n +\t(\n +\t\tcd empty &&\n -+\t\techo $bad_attr_source_err >expect_err &&\n +\t\techo \"f/path: test: unspecified\" >expect &&\n +\t\tgit -c attr.tree=refs/does/not/exist check-attr test -- f/path >actual 2>err &&\n +\t\ttest_must_be_empty err &&\n @@ t/t0003-attributes.sh: test_expect_success 'bare repo defaults to reading .gitat\n \ttest_cmp expect actual\n '\n \n -+test_expect_success '--attr-source and GIT_ATTR_SOURCE take precedence over attr.tree' '\n ++test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tree' '\n +\ttest_when_finished rm -rf empty &&\n +\tgit init empty &&\n +\t(\n @@ t/t0003-attributes.sh: test_expect_success 'bare repo defaults to reading .gitat\n +\t\ttest_commit \"val2\" .gitattributes \"f/path test=val2\" &&\n +\t\tgit checkout attr-source &&\n +\t\techo \"f/path: test: val1\" >expect &&\n -+\t\tgit -c attr.tree=attr-tree --attr-source=attr-source check-attr test -- f/path >actual &&\n ++\t\tGIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree --attr-source=attr-source \\\n ++\t\tcheck-attr test -- f/path >actual &&\n +\t\ttest_cmp expect actual &&\n -+\t\tGIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree check-attr test -- f/path >actual &&\n ++\t\tGIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree \\\n ++\t\tcheck-attr test -- f/path >actual &&\n +\t\ttest_cmp expect actual\n +\t)\n +'\n\n-- \ngitgitgadget\n"},{"id":"483060","messageId":"eaa27c478105606b39917bdadbdcfdce2b1b3521.1697044422.git.gitgitgadget@gmail.com","threadId":"60250","inReplyTo":"pull.1577.v4.git.git.1697044422.gitgitgadget@gmail.com","subject":"[PATCH v4 1/2] attr: read attributes from HEAD when bare repo","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-10-11T17:13:41Z","receivedAt":"2023-10-11T17:13:50Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nThe motivation for 44451a2e5e (attr: teach \"--attr-source=\" global\noption to \"git\" , 2023-05-06), was to make it possible to use\ngitattributes with bare repositories.\n\nTo make it easier to read gitattributes in bare repositories however,\nlet's just make HEAD:.gitattributes the default. This is in line with\nhow mailmap works, 8c473cecfd (mailmap: default mailmap.blob in bare\nrepositories, 2012-12-13).\n\nSigned-off-by: John Cai \n---\n attr.c | 12 +++++++++++-\n t/t0003-attributes.sh | 11 +++++++++++\n t/t5001-archive-attr.sh | 2 +-\n 3 files changed, 23 insertions(+), 2 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 71c84fbcf86..bf2ea1626a6 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -1194,6 +1194,7 @@ static void collect_some_attrs(struct index_state *istate,\n }\n \n static const char *default_attr_source_tree_object_name;\n+static int ignore_bad_attr_tree;\n \n void set_git_attr_source(const char *tree_object_name)\n {\n@@ -1205,10 +1206,19 @@ static void compute_default_attr_source(struct object_id *attr_source)\n \tif (!default_attr_source_tree_object_name)\n \t\tdefault_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);\n \n+\tif (!default_attr_source_tree_object_name &&\n+\t startup_info->have_repository &&\n+\t is_bare_repository()) {\n+\t\tdefault_attr_source_tree_object_name = \"HEAD\";\n+\t\tignore_bad_attr_tree = 1;\n+\t}\n+\n \tif (!default_attr_source_tree_object_name || !is_null_oid(attr_source))\n \t\treturn;\n \n-\tif (repo_get_oid_treeish(the_repository, default_attr_source_tree_object_name, attr_source))\n+\tif (repo_get_oid_treeish(the_repository,\n+\t\t\t\t default_attr_source_tree_object_name,\n+\t\t\t\t attr_source) && !ignore_bad_attr_tree)\n \t\tdie(_(\"bad --attr-source or GIT_ATTR_SOURCE\"));\n }\n \ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 26e082f05b4..5665cdc079f 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -342,6 +342,17 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '\n \t)\n '\n \n+\n+test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '\n+\ttest_when_finished rm -rf test bare_with_gitattribute &&\n+\tgit init test &&\n+\ttest_commit -C test gitattributes .gitattributes \"f/path test=val\" &&\n+\tgit clone --bare test bare_with_gitattribute &&\n+\techo \"f/path: test: val\" >expect &&\n+\tgit -C bare_with_gitattribute check-attr test -- f/path >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'bare repository: with --source' '\n \t(\n \t\tcd bare.git &&\ndiff --git a/t/t5001-archive-attr.sh b/t/t5001-archive-attr.sh\nindex 0ff47a239db..eaf959d8f63 100755\n--- a/t/t5001-archive-attr.sh\n+++ b/t/t5001-archive-attr.sh\n@@ -138,7 +138,7 @@ test_expect_success 'git archive with worktree attributes, bare' '\n '\n \n test_expect_missing\tbare-worktree/ignored\n-test_expect_exists\tbare-worktree/ignored-by-tree\n+test_expect_missing\tbare-worktree/ignored-by-tree\n test_expect_exists\tbare-worktree/ignored-by-worktree\n \n test_expect_success 'export-subst' '\n-- \ngitgitgadget\n\n"},{"id":"483061","messageId":"749d8a8082edf05fe184ab73acb7583cf00e2f0a.1697044422.git.gitgitgadget@gmail.com","threadId":"60250","inReplyTo":"pull.1577.v4.git.git.1697044422.gitgitgadget@gmail.com","subject":"[PATCH v4 2/2] attr: add attr.tree for setting the treeish to read attributes from","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-10-11T17:13:42Z","receivedAt":"2023-10-11T17:14:05Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\n44451a2 (attr: teach \"--attr-source=\" global option to \"git\",\n2023-05-06) provided the ability to pass in a treeish as the attr\nsource. In the context of serving Git repositories as bare repos like we\ndo at GitLab however, it would be easier to point --attr-source to HEAD\nfor all commands by setting it once.\n\nAdd a new config attr.tree that allows this.\n\nSigned-off-by: John Cai \n---\n Documentation/config.txt | 2 +\n Documentation/config/attr.txt | 7 ++++\n attr.c | 7 ++++\n attr.h | 2 +\n config.c | 16 ++++++++\n t/t0003-attributes.sh | 73 +++++++++++++++++++++++++++++++++++\n 6 files changed, 107 insertions(+)\n create mode 100644 Documentation/config/attr.txt\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 229b63a454c..b1891c2b5af 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -371,6 +371,8 @@ other popular tools, and describe them in your documentation.\n \n include::config/advice.txt[]\n \n+include::config/attr.txt[]\n+\n include::config/core.txt[]\n \n include::config/add.txt[]\ndiff --git a/Documentation/config/attr.txt b/Documentation/config/attr.txt\nnew file mode 100644\nindex 00000000000..1a482d6af2b\n--- /dev/null\n+++ b/Documentation/config/attr.txt\n@@ -0,0 +1,7 @@\n+attr.tree::\n+\tA reference to a tree in the repository from which to read attributes,\n+\tinstead of the `.gitattributes` file in the working tree. In a bare\n+\trepository, this defaults to `HEAD:.gitattributes`. If the value does\n+\tnot resolve to a valid tree object, an empty tree is used instead.\n+\tWhen the `GIT_ATTR_SOURCE` environment variable or `--attr-source`\n+\tcommand line option are used, this configuration variable has no effect.\ndiff --git a/attr.c b/attr.c\nindex bf2ea1626a6..e62876dfd3e 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -24,6 +24,8 @@\n #include \"tree-walk.h\"\n #include \"object-name.h\"\n \n+const char *git_attr_tree;\n+\n const char git_attr__true[] = \"(builtin)true\";\n const char git_attr__false[] = \"\\0(builtin)false\";\n static const char git_attr__unknown[] = \"(builtin)unknown\";\n@@ -1206,6 +1208,11 @@ static void compute_default_attr_source(struct object_id *attr_source)\n \tif (!default_attr_source_tree_object_name)\n \t\tdefault_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);\n \n+\tif (!default_attr_source_tree_object_name && git_attr_tree) {\n+\t\tdefault_attr_source_tree_object_name = git_attr_tree;\n+\t\tignore_bad_attr_tree = 1;\n+\t}\n+\n \tif (!default_attr_source_tree_object_name &&\n \t startup_info->have_repository &&\n \t is_bare_repository()) {\ndiff --git a/attr.h b/attr.h\nindex 2b745df4054..127998ae013 100644\n--- a/attr.h\n+++ b/attr.h\n@@ -236,4 +236,6 @@ const char *git_attr_global_file(void);\n /* Return whether the system gitattributes file is enabled and should be used. */\n int git_attr_system_is_enabled(void);\n \n+extern const char *git_attr_tree;\n+\n #endif /* ATTR_H */\ndiff --git a/config.c b/config.c\nindex 3846a37be97..fb6a2db1d9b 100644\n--- a/config.c\n+++ b/config.c\n@@ -18,6 +18,7 @@\n #include \"repository.h\"\n #include \"lockfile.h\"\n #include \"mailmap.h\"\n+#include \"attr.h\"\n #include \"exec-cmd.h\"\n #include \"strbuf.h\"\n #include \"quote.h\"\n@@ -1904,6 +1905,18 @@ static int git_default_mailmap_config(const char *var, const char *value)\n \treturn 0;\n }\n \n+static int git_default_attr_config(const char *var, const char *value)\n+{\n+\tif (!strcmp(var, \"attr.tree\"))\n+\t\treturn git_config_string(&git_attr_tree, var, value);\n+\n+\t/*\n+\t * Add other attribute related config variables here and to\n+\t * Documentation/config/attr.txt.\n+\t */\n+\treturn 0;\n+}\n+\n int git_default_config(const char *var, const char *value,\n \t\t const struct config_context *ctx, void *cb)\n {\n@@ -1927,6 +1940,9 @@ int git_default_config(const char *var, const char *value,\n \tif (starts_with(var, \"mailmap.\"))\n \t\treturn git_default_mailmap_config(var, value);\n \n+\tif (starts_with(var, \"attr.\"))\n+\t\treturn git_default_attr_config(var, value);\n+\n \tif (starts_with(var, \"advice.\") || starts_with(var, \"color.advice\"))\n \t\treturn git_default_advice_config(var, value);\n \ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 5665cdc079f..e9953ce19c5 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -40,6 +40,10 @@ attr_check_source () {\n \ttest_cmp expect actual &&\n \ttest_must_be_empty err\n \n+\tgit $git_opts -c \"attr.tree=$source\" check-attr test -- \"$path\" >actual 2>err &&\n+\ttest_cmp expect actual &&\n+\ttest_must_be_empty err\n+\n \tGIT_ATTR_SOURCE=\"$source\" git $git_opts check-attr test -- \"$path\" >actual 2>err &&\n \ttest_cmp expect actual &&\n \ttest_must_be_empty err\n@@ -342,6 +346,55 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '\n \t)\n '\n \n+bad_attr_source_err=\"fatal: bad --attr-source or GIT_ATTR_SOURCE\"\n+\n+test_expect_success '--attr-source is bad' '\n+\ttest_when_finished rm -rf empty &&\n+\tgit init empty &&\n+\t(\n+\t\tcd empty &&\n+\t\techo \"$bad_attr_source_err\" >expect_err &&\n+\t\ttest_must_fail git --attr-source=HEAD check-attr test -- f/path 2>err &&\n+\t\ttest_cmp expect_err err\n+\t)\n+'\n+\n+test_expect_success 'attr.tree when HEAD is unborn' '\n+\ttest_when_finished rm -rf empty &&\n+\tgit init empty &&\n+\t(\n+\t\tcd empty &&\n+\t\techo \"f/path: test: unspecified\" >expect &&\n+\t\tgit -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&\n+\t\ttest_must_be_empty err &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'attr.tree points to non-existing ref' '\n+\ttest_when_finished rm -rf empty &&\n+\tgit init empty &&\n+\t(\n+\t\tcd empty &&\n+\t\techo \"f/path: test: unspecified\" >expect &&\n+\t\tgit -c attr.tree=refs/does/not/exist check-attr test -- f/path >actual 2>err &&\n+\t\ttest_must_be_empty err &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'bad attr source defaults to reading .gitattributes file' '\n+\ttest_when_finished rm -rf empty &&\n+\tgit init empty &&\n+\t(\n+\t\tcd empty &&\n+\t\techo \"f/path test=val\" >.gitattributes &&\n+\t\techo \"f/path: test: val\" >expect &&\n+\t\tgit -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&\n+\t\ttest_must_be_empty err &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n \n test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '\n \ttest_when_finished rm -rf test bare_with_gitattribute &&\n@@ -353,6 +406,26 @@ test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tree' '\n+\ttest_when_finished rm -rf empty &&\n+\tgit init empty &&\n+\t(\n+\t\tcd empty &&\n+\t\tgit checkout -b attr-source &&\n+\t\ttest_commit \"val1\" .gitattributes \"f/path test=val1\" &&\n+\t\tgit checkout -b attr-tree &&\n+\t\ttest_commit \"val2\" .gitattributes \"f/path test=val2\" &&\n+\t\tgit checkout attr-source &&\n+\t\techo \"f/path: test: val1\" >expect &&\n+\t\tGIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree --attr-source=attr-source \\\n+\t\tcheck-attr test -- f/path >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\tGIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree \\\n+\t\tcheck-attr test -- f/path >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'bare repository: with --source' '\n \t(\n \t\tcd bare.git &&\n-- \ngitgitgadget\n"},{"id":"483095","messageId":"xmqqjzrskdzq.fsf@gitster.g","threadId":"60250","inReplyTo":"pull.1577.v4.git.git.1697044422.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 0/2] attr: add attr.tree config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-11T22:09:45Z","receivedAt":"2023-10-11T22:09:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> 44451a2e5e (attr: teach \"--attr-source=\" global option to \"git\", 2023-05-06)\n> provided the ability to pass in a treeish as the attr source. When a\n> revision does not resolve to a valid tree is passed, Git will die. At\n> GitLab, we server repositories as bare repos and would like to always read\n> attributes from the default branch, so we'd like to pass in HEAD as the\n> treeish to read gitattributes from on every command. In this context we\n> would not want Git to die if HEAD is unborn, like in the case of empty\n> repositories.\n>\n> Instead of modifying the default behavior of --attr-source, create a new\n> config attr.tree with which an admin can configure a ref for all commands to\n> read gitattributes from. Also make the default tree to read from HEAD on\n> bare repositories.\n>\n> Changes since v2:\n>\n> * relax the restrictions around attr.tree so that if it does not resolve to\n> a valid treeish, ignore it.\n> * add a commit to default to HEAD in bare repositories\n>\n> Changes since v1:\n>\n> * Added a commit to add attr.tree config\n\nTHis is v4 so there must be some changes since v3 that we are missing?\n\n> Range-diff vs v3:\n>\n> 1: cef206d47c7 ! 1: eaa27c47810 attr: read attributes from HEAD when bare repo\n> @@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr\n> +test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '\n> +\ttest_when_finished rm -rf test bare_with_gitattribute &&\n> +\tgit init test &&\n> -+\t(\n> -+\t\tcd test &&\n> -+\t\ttest_commit gitattributes .gitattributes \"f/path test=val\"\n> -+\t) &&\n> ++\ttest_commit -C test gitattributes .gitattributes \"f/path test=val\" &&\n\nOK.\n\n> 2: dadb822da99 ! 2: 749d8a8082e attr: add attr.tree for setting the treeish to read attributes from\n> @@ Documentation/config.txt: other popular tools, and describe them in your documen\n> \n> ## Documentation/config/attr.txt (new) ##\n> @@\n> -+attr.tree:\n> -+\tA to read gitattributes from instead of the worktree. See\n> -+\tlinkgit:gitattributes[5]. If `attr.tree` does not resolve to a valid tree,\n> -+\ttreat it as an empty tree. --attr-source and GIT_ATTR_SOURCE take\n> -+\tprecedence over attr.tree.\n> ++attr.tree::\n> ++\tA reference to a tree in the repository from which to read attributes,\n> ++\tinstead of the `.gitattributes` file in the working tree. In a bare\n> ++\trepository, this defaults to `HEAD:.gitattributes`. If the value does\n> ++\tnot resolve to a valid tree object, an empty tree is used instead.\n> ++\tWhen the `GIT_ATTR_SOURCE` environment variable or `--attr-source`\n> ++\tcommand line option are used, this configuration variable has no effect.\n\nOK.\n\n> -+\tif (!default_attr_source_tree_object_name) {\n> ++\tif (!default_attr_source_tree_object_name && git_attr_tree) {\n> +\t\tdefault_attr_source_tree_object_name = git_attr_tree;\n> +\t\tignore_bad_attr_tree = 1;\n> +\t}\n\nMakes sense.\n\n> @@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr\n> \n> +bad_attr_source_err=\"fatal: bad --attr-source or GIT_ATTR_SOURCE\"\n> +\n> ++test_expect_success '--attr-source is bad' '\n> ++\ttest_when_finished rm -rf empty &&\n> ++\tgit init empty &&\n> ++\t(\n> ++\t\tcd empty &&\n> ++\t\techo \"$bad_attr_source_err\" >expect_err &&\n> ++\t\ttest_must_fail git --attr-source=HEAD check-attr test -- f/path 2>err &&\n> ++\t\ttest_cmp expect_err err\n> ++\t)\n> ++'\n\nOK. We fail when explicitly given a bad attr-source.\n\n> +test_expect_success 'attr.tree when HEAD is unborn' '\n> +\ttest_when_finished rm -rf empty &&\n> +\tgit init empty &&\n> +\t(\n> +\t\tcd empty &&\n> -+\t\techo $bad_attr_source_err >expect_err &&\n> +\t\techo \"f/path: test: unspecified\" >expect &&\n> +\t\tgit -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&\n> +\t\ttest_must_be_empty err &&\n\nBut we silently ignore when given via a configuration variable.\n\n> @@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr\n> +\tgit init empty &&\n> +\t(\n> +\t\tcd empty &&\n> -+\t\techo $bad_attr_source_err >expect_err &&\n> +\t\techo \"f/path: test: unspecified\" >expect &&\n> +\t\tgit -c attr.tree=refs/does/not/exist check-attr test -- f/path >actual 2>err &&\n> +\t\ttest_must_be_empty err &&\n\nDitto. Is this any different from the above? Both points at an\nobject that does not exist. If one were pointing at an object that\ndoes not exist (e.g., HEAD before the initial commit) and the other\nwere pointing at an object that is not a tree-ish (e.g., a blob),\nthen having two separate tests may make sense, but otherwise, I am\nnot sure about the value proposition of the second test.\n\n> @@ t/t0003-attributes.sh: test_expect_success 'bare repo defaults to reading .gitat\n> \ttest_cmp expect actual\n> '\n> \n> -+test_expect_success '--attr-source and GIT_ATTR_SOURCE take precedence over attr.tree' '\n> ++test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tree' '\n> +\ttest_when_finished rm -rf empty &&\n> +\tgit init empty &&\n> +\t(\n> @@ t/t0003-attributes.sh: test_expect_success 'bare repo defaults to reading .gitat\n> +\t\ttest_commit \"val2\" .gitattributes \"f/path test=val2\" &&\n> +\t\tgit checkout attr-source &&\n> +\t\techo \"f/path: test: val1\" >expect &&\n> -+\t\tgit -c attr.tree=attr-tree --attr-source=attr-source check-attr test -- f/path >actual &&\n> ++\t\tGIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree --attr-source=attr-source \\\n> ++\t\tcheck-attr test -- f/path >actual &&\n> +\t\ttest_cmp expect actual &&\n> -+\t\tGIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree check-attr test -- f/path >actual &&\n> ++\t\tGIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree \\\n> ++\t\tcheck-attr test -- f/path >actual &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n\nLooking good.\n\nThanks. Queued.\n"},{"id":"483201","messageId":"DFC5DF88-B832-44DC-A0EF-7FFB26B8730C@gmail.com","threadId":"60250","inReplyTo":"xmqqjzrskdzq.fsf@gitster.g","subject":"Re: [PATCH v4 0/2] attr: add attr.tree config","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2023-10-13T15:30:56Z","receivedAt":"2023-10-13T15:31:02Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Junio,\n\nOn 11 Oct 2023, at 18:09, Junio C Hamano wrote:\n\n> \"John Cai via GitGitGadget\" writes:\n>\n>> 44451a2e5e (attr: teach \"--attr-source=\" global option to \"git\", 2023-05-06)\n>> provided the ability to pass in a treeish as the attr source. When a\n>> revision does not resolve to a valid tree is passed, Git will die. At\n>> GitLab, we server repositories as bare repos and would like to always read\n>> attributes from the default branch, so we'd like to pass in HEAD as the\n>> treeish to read gitattributes from on every command. In this context we\n>> would not want Git to die if HEAD is unborn, like in the case of empty\n>> repositories.\n>>\n>> Instead of modifying the default behavior of --attr-source, create a new\n>> config attr.tree with which an admin can configure a ref for all commands to\n>> read gitattributes from. Also make the default tree to read from HEAD on\n>> bare repositories.\n>>\n>> Changes since v2:\n>>\n>> * relax the restrictions around attr.tree so that if it does not resolve to\n>> a valid treeish, ignore it.\n>> * add a commit to default to HEAD in bare repositories\n>>\n>> Changes since v1:\n>>\n>> * Added a commit to add attr.tree config\n>\n> THis is v4 so there must be some changes since v3 that we are missing?\n\nOops I messed up the ordering of changes here. I'll fix in the (hopefully) final\nre-roll\n\n>\n>> Range-diff vs v3:\n>>\n>> 1: cef206d47c7 ! 1: eaa27c47810 attr: read attributes from HEAD when bare repo\n>> @@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr\n>> +test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '\n>> +\ttest_when_finished rm -rf test bare_with_gitattribute &&\n>> +\tgit init test &&\n>> -+\t(\n>> -+\t\tcd test &&\n>> -+\t\ttest_commit gitattributes .gitattributes \"f/path test=val\"\n>> -+\t) &&\n>> ++\ttest_commit -C test gitattributes .gitattributes \"f/path test=val\" &&\n>\n> OK.\n>\n>> 2: dadb822da99 ! 2: 749d8a8082e attr: add attr.tree for setting the treeish to read attributes from\n>> @@ Documentation/config.txt: other popular tools, and describe them in your documen\n>>\n>> ## Documentation/config/attr.txt (new) ##\n>> @@\n>> -+attr.tree:\n>> -+\tA to read gitattributes from instead of the worktree. See\n>> -+\tlinkgit:gitattributes[5]. If `attr.tree` does not resolve to a valid tree,\n>> -+\ttreat it as an empty tree. --attr-source and GIT_ATTR_SOURCE take\n>> -+\tprecedence over attr.tree.\n>> ++attr.tree::\n>> ++\tA reference to a tree in the repository from which to read attributes,\n>> ++\tinstead of the `.gitattributes` file in the working tree. In a bare\n>> ++\trepository, this defaults to `HEAD:.gitattributes`. If the value does\n>> ++\tnot resolve to a valid tree object, an empty tree is used instead.\n>> ++\tWhen the `GIT_ATTR_SOURCE` environment variable or `--attr-source`\n>> ++\tcommand line option are used, this configuration variable has no effect.\n>\n> OK.\n>\n>> -+\tif (!default_attr_source_tree_object_name) {\n>> ++\tif (!default_attr_source_tree_object_name && git_attr_tree) {\n>> +\t\tdefault_attr_source_tree_object_name = git_attr_tree;\n>> +\t\tignore_bad_attr_tree = 1;\n>> +\t}\n>\n> Makes sense.\n>\n>> @@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr\n>>\n>> +bad_attr_source_err=\"fatal: bad --attr-source or GIT_ATTR_SOURCE\"\n>> +\n>> ++test_expect_success '--attr-source is bad' '\n>> ++\ttest_when_finished rm -rf empty &&\n>> ++\tgit init empty &&\n>> ++\t(\n>> ++\t\tcd empty &&\n>> ++\t\techo \"$bad_attr_source_err\" >expect_err &&\n>> ++\t\ttest_must_fail git --attr-source=HEAD check-attr test -- f/path 2>err &&\n>> ++\t\ttest_cmp expect_err err\n>> ++\t)\n>> ++'\n>\n> OK. We fail when explicitly given a bad attr-source.\n>\n>> +test_expect_success 'attr.tree when HEAD is unborn' '\n>> +\ttest_when_finished rm -rf empty &&\n>> +\tgit init empty &&\n>> +\t(\n>> +\t\tcd empty &&\n>> -+\t\techo $bad_attr_source_err >expect_err &&\n>> +\t\techo \"f/path: test: unspecified\" >expect &&\n>> +\t\tgit -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&\n>> +\t\ttest_must_be_empty err &&\n>\n> But we silently ignore when given via a configuration variable.\n>\n>> @@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr\n>> +\tgit init empty &&\n>> +\t(\n>> +\t\tcd empty &&\n>> -+\t\techo $bad_attr_source_err >expect_err &&\n>> +\t\techo \"f/path: test: unspecified\" >expect &&\n>> +\t\tgit -c attr.tree=refs/does/not/exist check-attr test -- f/path >actual 2>err &&\n>> +\t\ttest_must_be_empty err &&\n>\n> Ditto. Is this any different from the above? Both points at an\n> object that does not exist. If one were pointing at an object that\n> does not exist (e.g., HEAD before the initial commit) and the other\n> were pointing at an object that is not a tree-ish (e.g., a blob),\n> then having two separate tests may make sense, but otherwise, I am\n> not sure about the value proposition of the second test.\n\nYeah looking at it now, this test seems like it doesn't add much.\n\n>\n>> @@ t/t0003-attributes.sh: test_expect_success 'bare repo defaults to reading .gitat\n>> \ttest_cmp expect actual\n>> '\n>>\n>> -+test_expect_success '--attr-source and GIT_ATTR_SOURCE take precedence over attr.tree' '\n>> ++test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tree' '\n>> +\ttest_when_finished rm -rf empty &&\n>> +\tgit init empty &&\n>> +\t(\n>> @@ t/t0003-attributes.sh: test_expect_success 'bare repo defaults to reading .gitat\n>> +\t\ttest_commit \"val2\" .gitattributes \"f/path test=val2\" &&\n>> +\t\tgit checkout attr-source &&\n>> +\t\techo \"f/path: test: val1\" >expect &&\n>> -+\t\tgit -c attr.tree=attr-tree --attr-source=attr-source check-attr test -- f/path >actual &&\n>> ++\t\tGIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree --attr-source=attr-source \\\n>> ++\t\tcheck-attr test -- f/path >actual &&\n>> +\t\ttest_cmp expect actual &&\n>> -+\t\tGIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree check-attr test -- f/path >actual &&\n>> ++\t\tGIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree \\\n>> ++\t\tcheck-attr test -- f/path >actual &&\n>> +\t\ttest_cmp expect actual\n>> +\t)\n>> +'\n>\n> Looking good.\n>\n> Thanks. Queued.\n\nthanks!\nJohn\n"},{"id":"483208","messageId":"eaa27c478105606b39917bdadbdcfdce2b1b3521.1697218770.git.gitgitgadget@gmail.com","threadId":"60250","inReplyTo":"pull.1577.v5.git.git.1697218770.gitgitgadget@gmail.com","subject":"[PATCH v5 1/2] attr: read attributes from HEAD when bare repo","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-10-13T17:39:29Z","receivedAt":"2023-10-13T17:39:42Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nThe motivation for 44451a2e5e (attr: teach \"--attr-source=\" global\noption to \"git\" , 2023-05-06), was to make it possible to use\ngitattributes with bare repositories.\n\nTo make it easier to read gitattributes in bare repositories however,\nlet's just make HEAD:.gitattributes the default. This is in line with\nhow mailmap works, 8c473cecfd (mailmap: default mailmap.blob in bare\nrepositories, 2012-12-13).\n\nSigned-off-by: John Cai \n---\n attr.c | 12 +++++++++++-\n t/t0003-attributes.sh | 11 +++++++++++\n t/t5001-archive-attr.sh | 2 +-\n 3 files changed, 23 insertions(+), 2 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 71c84fbcf86..bf2ea1626a6 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -1194,6 +1194,7 @@ static void collect_some_attrs(struct index_state *istate,\n }\n \n static const char *default_attr_source_tree_object_name;\n+static int ignore_bad_attr_tree;\n \n void set_git_attr_source(const char *tree_object_name)\n {\n@@ -1205,10 +1206,19 @@ static void compute_default_attr_source(struct object_id *attr_source)\n \tif (!default_attr_source_tree_object_name)\n \t\tdefault_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);\n \n+\tif (!default_attr_source_tree_object_name &&\n+\t startup_info->have_repository &&\n+\t is_bare_repository()) {\n+\t\tdefault_attr_source_tree_object_name = \"HEAD\";\n+\t\tignore_bad_attr_tree = 1;\n+\t}\n+\n \tif (!default_attr_source_tree_object_name || !is_null_oid(attr_source))\n \t\treturn;\n \n-\tif (repo_get_oid_treeish(the_repository, default_attr_source_tree_object_name, attr_source))\n+\tif (repo_get_oid_treeish(the_repository,\n+\t\t\t\t default_attr_source_tree_object_name,\n+\t\t\t\t attr_source) && !ignore_bad_attr_tree)\n \t\tdie(_(\"bad --attr-source or GIT_ATTR_SOURCE\"));\n }\n \ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 26e082f05b4..5665cdc079f 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -342,6 +342,17 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '\n \t)\n '\n \n+\n+test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '\n+\ttest_when_finished rm -rf test bare_with_gitattribute &&\n+\tgit init test &&\n+\ttest_commit -C test gitattributes .gitattributes \"f/path test=val\" &&\n+\tgit clone --bare test bare_with_gitattribute &&\n+\techo \"f/path: test: val\" >expect &&\n+\tgit -C bare_with_gitattribute check-attr test -- f/path >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'bare repository: with --source' '\n \t(\n \t\tcd bare.git &&\ndiff --git a/t/t5001-archive-attr.sh b/t/t5001-archive-attr.sh\nindex 0ff47a239db..eaf959d8f63 100755\n--- a/t/t5001-archive-attr.sh\n+++ b/t/t5001-archive-attr.sh\n@@ -138,7 +138,7 @@ test_expect_success 'git archive with worktree attributes, bare' '\n '\n \n test_expect_missing\tbare-worktree/ignored\n-test_expect_exists\tbare-worktree/ignored-by-tree\n+test_expect_missing\tbare-worktree/ignored-by-tree\n test_expect_exists\tbare-worktree/ignored-by-worktree\n \n test_expect_success 'export-subst' '\n-- \ngitgitgadget\n\n"},{"id":"483209","messageId":"pull.1577.v5.git.git.1697218770.gitgitgadget@gmail.com","threadId":"60250","inReplyTo":"pull.1577.v4.git.git.1697044422.gitgitgadget@gmail.com","subject":"[PATCH v5 0/2] attr: add attr.tree config","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-10-13T17:39:28Z","receivedAt":"2023-10-13T17:39:42Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"44451a2e5e (attr: teach \"--attr-source=\" global option to \"git\", 2023-05-06)\nprovided the ability to pass in a treeish as the attr source. When a\nrevision does not resolve to a valid tree is passed, Git will die. At\nGitLab, we server repositories as bare repos and would like to always read\nattributes from the default branch, so we'd like to pass in HEAD as the\ntreeish to read gitattributes from on every command. In this context we\nwould not want Git to die if HEAD is unborn, like in the case of empty\nrepositories.\n\nInstead of modifying the default behavior of --attr-source, create a new\nconfig attr.tree with which an admin can configure a ref for all commands to\nread gitattributes from. Also make the default tree to read from HEAD on\nbare repositories.\n\nChanges since v4:\n\n * removed superfluous test\n\nChanges since v3:\n\n * clarified attr logic around ignoring errors if source set by attr.tree is\n invalid\n * refactored tests by using helpers\n * modified test to check for precedence between --attr-source, attr.tree,\n GIT_ATTR_SOURCE\n\nChanges since v2:\n\n * relax the restrictions around attr.tree so that if it does not resolve to\n a valid treeish, ignore it.\n * add a commit to default to HEAD in bare repositories\n * remove commit that adds attr.allowInvalidSource\n\nChanges since v1:\n\n * Added a commit to add attr.tree config\n\nJohn Cai (2):\n attr: read attributes from HEAD when bare repo\n attr: add attr.tree for setting the treeish to read attributes from\n\n Documentation/config.txt | 2 +\n Documentation/config/attr.txt | 7 ++++\n attr.c | 19 ++++++++-\n attr.h | 2 +\n config.c | 16 ++++++++\n t/t0003-attributes.sh | 72 +++++++++++++++++++++++++++++++++++\n t/t5001-archive-attr.sh | 2 +-\n 7 files changed, 118 insertions(+), 2 deletions(-)\n create mode 100644 Documentation/config/attr.txt\n\n\nbase-commit: 1fc548b2d6a3596f3e1c1f8b1930d8dbd1e30bf3\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1577%2Fjohn-cai%2Fjc%2Fconfig-attr-invalid-source-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1577/john-cai/jc/config-attr-invalid-source-v5\nPull-Request: https://github.com/git/git/pull/1577\n\nRange-diff vs v4:\n\n 1: eaa27c47810 = 1: eaa27c47810 attr: read attributes from HEAD when bare repo\n 2: 749d8a8082e ! 2: df4b3f53309 attr: add attr.tree for setting the treeish to read attributes from\n @@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr\n +\t)\n +'\n +\n -+test_expect_success 'attr.tree points to non-existing ref' '\n -+\ttest_when_finished rm -rf empty &&\n -+\tgit init empty &&\n -+\t(\n -+\t\tcd empty &&\n -+\t\techo \"f/path: test: unspecified\" >expect &&\n -+\t\tgit -c attr.tree=refs/does/not/exist check-attr test -- f/path >actual 2>err &&\n -+\t\ttest_must_be_empty err &&\n -+\t\ttest_cmp expect actual\n -+\t)\n -+'\n -+\n +test_expect_success 'bad attr source defaults to reading .gitattributes file' '\n +\ttest_when_finished rm -rf empty &&\n +\tgit init empty &&\n\n-- \ngitgitgadget\n"},{"id":"483210","messageId":"df4b3f5330987558ea1c04d8523489daef719c77.1697218770.git.gitgitgadget@gmail.com","threadId":"60250","inReplyTo":"pull.1577.v5.git.git.1697218770.gitgitgadget@gmail.com","subject":"[PATCH v5 2/2] attr: add attr.tree for setting the treeish to read attributes from","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-10-13T17:39:30Z","receivedAt":"2023-10-13T17:39:44Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\n44451a2 (attr: teach \"--attr-source=\" global option to \"git\",\n2023-05-06) provided the ability to pass in a treeish as the attr\nsource. In the context of serving Git repositories as bare repos like we\ndo at GitLab however, it would be easier to point --attr-source to HEAD\nfor all commands by setting it once.\n\nAdd a new config attr.tree that allows this.\n\nSigned-off-by: John Cai \n---\n Documentation/config.txt | 2 ++\n Documentation/config/attr.txt | 7 ++++\n attr.c | 7 ++++\n attr.h | 2 ++\n config.c | 16 +++++++++\n t/t0003-attributes.sh | 61 +++++++++++++++++++++++++++++++++++\n 6 files changed, 95 insertions(+)\n create mode 100644 Documentation/config/attr.txt\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 229b63a454c..b1891c2b5af 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -371,6 +371,8 @@ other popular tools, and describe them in your documentation.\n \n include::config/advice.txt[]\n \n+include::config/attr.txt[]\n+\n include::config/core.txt[]\n \n include::config/add.txt[]\ndiff --git a/Documentation/config/attr.txt b/Documentation/config/attr.txt\nnew file mode 100644\nindex 00000000000..1a482d6af2b\n--- /dev/null\n+++ b/Documentation/config/attr.txt\n@@ -0,0 +1,7 @@\n+attr.tree::\n+\tA reference to a tree in the repository from which to read attributes,\n+\tinstead of the `.gitattributes` file in the working tree. In a bare\n+\trepository, this defaults to `HEAD:.gitattributes`. If the value does\n+\tnot resolve to a valid tree object, an empty tree is used instead.\n+\tWhen the `GIT_ATTR_SOURCE` environment variable or `--attr-source`\n+\tcommand line option are used, this configuration variable has no effect.\ndiff --git a/attr.c b/attr.c\nindex bf2ea1626a6..e62876dfd3e 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -24,6 +24,8 @@\n #include \"tree-walk.h\"\n #include \"object-name.h\"\n \n+const char *git_attr_tree;\n+\n const char git_attr__true[] = \"(builtin)true\";\n const char git_attr__false[] = \"\\0(builtin)false\";\n static const char git_attr__unknown[] = \"(builtin)unknown\";\n@@ -1206,6 +1208,11 @@ static void compute_default_attr_source(struct object_id *attr_source)\n \tif (!default_attr_source_tree_object_name)\n \t\tdefault_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);\n \n+\tif (!default_attr_source_tree_object_name && git_attr_tree) {\n+\t\tdefault_attr_source_tree_object_name = git_attr_tree;\n+\t\tignore_bad_attr_tree = 1;\n+\t}\n+\n \tif (!default_attr_source_tree_object_name &&\n \t startup_info->have_repository &&\n \t is_bare_repository()) {\ndiff --git a/attr.h b/attr.h\nindex 2b745df4054..127998ae013 100644\n--- a/attr.h\n+++ b/attr.h\n@@ -236,4 +236,6 @@ const char *git_attr_global_file(void);\n /* Return whether the system gitattributes file is enabled and should be used. */\n int git_attr_system_is_enabled(void);\n \n+extern const char *git_attr_tree;\n+\n #endif /* ATTR_H */\ndiff --git a/config.c b/config.c\nindex 3846a37be97..fb6a2db1d9b 100644\n--- a/config.c\n+++ b/config.c\n@@ -18,6 +18,7 @@\n #include \"repository.h\"\n #include \"lockfile.h\"\n #include \"mailmap.h\"\n+#include \"attr.h\"\n #include \"exec-cmd.h\"\n #include \"strbuf.h\"\n #include \"quote.h\"\n@@ -1904,6 +1905,18 @@ static int git_default_mailmap_config(const char *var, const char *value)\n \treturn 0;\n }\n \n+static int git_default_attr_config(const char *var, const char *value)\n+{\n+\tif (!strcmp(var, \"attr.tree\"))\n+\t\treturn git_config_string(&git_attr_tree, var, value);\n+\n+\t/*\n+\t * Add other attribute related config variables here and to\n+\t * Documentation/config/attr.txt.\n+\t */\n+\treturn 0;\n+}\n+\n int git_default_config(const char *var, const char *value,\n \t\t const struct config_context *ctx, void *cb)\n {\n@@ -1927,6 +1940,9 @@ int git_default_config(const char *var, const char *value,\n \tif (starts_with(var, \"mailmap.\"))\n \t\treturn git_default_mailmap_config(var, value);\n \n+\tif (starts_with(var, \"attr.\"))\n+\t\treturn git_default_attr_config(var, value);\n+\n \tif (starts_with(var, \"advice.\") || starts_with(var, \"color.advice\"))\n \t\treturn git_default_advice_config(var, value);\n \ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 5665cdc079f..ecf43ab5454 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -40,6 +40,10 @@ attr_check_source () {\n \ttest_cmp expect actual &&\n \ttest_must_be_empty err\n \n+\tgit $git_opts -c \"attr.tree=$source\" check-attr test -- \"$path\" >actual 2>err &&\n+\ttest_cmp expect actual &&\n+\ttest_must_be_empty err\n+\n \tGIT_ATTR_SOURCE=\"$source\" git $git_opts check-attr test -- \"$path\" >actual 2>err &&\n \ttest_cmp expect actual &&\n \ttest_must_be_empty err\n@@ -342,6 +346,43 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '\n \t)\n '\n \n+bad_attr_source_err=\"fatal: bad --attr-source or GIT_ATTR_SOURCE\"\n+\n+test_expect_success '--attr-source is bad' '\n+\ttest_when_finished rm -rf empty &&\n+\tgit init empty &&\n+\t(\n+\t\tcd empty &&\n+\t\techo \"$bad_attr_source_err\" >expect_err &&\n+\t\ttest_must_fail git --attr-source=HEAD check-attr test -- f/path 2>err &&\n+\t\ttest_cmp expect_err err\n+\t)\n+'\n+\n+test_expect_success 'attr.tree when HEAD is unborn' '\n+\ttest_when_finished rm -rf empty &&\n+\tgit init empty &&\n+\t(\n+\t\tcd empty &&\n+\t\techo \"f/path: test: unspecified\" >expect &&\n+\t\tgit -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&\n+\t\ttest_must_be_empty err &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'bad attr source defaults to reading .gitattributes file' '\n+\ttest_when_finished rm -rf empty &&\n+\tgit init empty &&\n+\t(\n+\t\tcd empty &&\n+\t\techo \"f/path test=val\" >.gitattributes &&\n+\t\techo \"f/path: test: val\" >expect &&\n+\t\tgit -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&\n+\t\ttest_must_be_empty err &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n \n test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '\n \ttest_when_finished rm -rf test bare_with_gitattribute &&\n@@ -353,6 +394,26 @@ test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tree' '\n+\ttest_when_finished rm -rf empty &&\n+\tgit init empty &&\n+\t(\n+\t\tcd empty &&\n+\t\tgit checkout -b attr-source &&\n+\t\ttest_commit \"val1\" .gitattributes \"f/path test=val1\" &&\n+\t\tgit checkout -b attr-tree &&\n+\t\ttest_commit \"val2\" .gitattributes \"f/path test=val2\" &&\n+\t\tgit checkout attr-source &&\n+\t\techo \"f/path: test: val1\" >expect &&\n+\t\tGIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree --attr-source=attr-source \\\n+\t\tcheck-attr test -- f/path >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\tGIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree \\\n+\t\tcheck-attr test -- f/path >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'bare repository: with --source' '\n \t(\n \t\tcd bare.git &&\n-- \ngitgitgadget\n"},{"id":"483216","messageId":"xmqqmswmz76w.fsf@gitster.g","threadId":"60250","inReplyTo":"pull.1577.v5.git.git.1697218770.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 0/2] attr: add attr.tree config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-13T18:52:07Z","receivedAt":"2023-10-13T18:52:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> Changes since v4:\n>\n> * removed superfluous test\n\nAn alternative would have been to point with the ref some non-tree\nobject like a blob, but as the outcome should be the same as missing\ncase (from the code --- which is not exactly kosher), it should be\nOK.\n\n\tif (repo_get_oid_treeish(the_repository,\n\t\t\t\t default_attr_source_tree_object_name,\n\t\t\t\t attr_source) && !ignore_bad_attr_tree)\n\t\tdie(_(\"bad --attr-source or GIT_ATTR_SOURCE\"));\n\nOOPS! Sorry for not noticing earlier, but repo_get_oid_treeish()\ndoes *NOT* error out when the discovered object is not a treeish, as\nthe suggested object type is merely supplied for disambiguation\npurposes (e.g., with objects 012345 that is a tree and 012346 that\nis a blob, you can still ask for treeish \"01234\" but if you ask for\nan object \"01234\" it will fail).\n\nSo, the alternative test would have caught this bug, no? Instead of\nsilently treating the non-treeish as an empty tree, we would have\ndied much later when the object supposedly a tree-ish turns out to\nbe a blob, or something?\n\n\n\n"},{"id":"483231","messageId":"xmqqa5smz2lk.fsf@gitster.g","threadId":"60250","inReplyTo":"xmqqmswmz76w.fsf@gitster.g","subject":"Re: [PATCH v5 0/2] attr: add attr.tree config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-13T20:31:19Z","receivedAt":"2023-10-13T20:31:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \tif (repo_get_oid_treeish(the_repository,\n> \t\t\t\t default_attr_source_tree_object_name,\n> \t\t\t\t attr_source) && !ignore_bad_attr_tree)\n> \t\tdie(_(\"bad --attr-source or GIT_ATTR_SOURCE\"));\n>\n> OOPS! Sorry for not noticing earlier, but repo_get_oid_treeish()\n> does *NOT* error out when the discovered object is not a treeish, as\n> the suggested object type is merely supplied for disambiguation\n> purposes (e.g., with objects 012345 that is a tree and 012346 that\n> is a blob, you can still ask for treeish \"01234\" but if you ask for\n> an object \"01234\" it will fail).\n>\n> So, the alternative test would have caught this bug, no? Instead of\n> silently treating the non-treeish as an empty tree, we would have\n> died much later when the object supposedly a tree-ish turns out to\n> be a blob, or something?\n\nThere indeed is a bug, but not really. If we add this test:\n\ntest_expect_success 'attr.tree that points at a non-treeish' '\n\ttest_when_finished rm -rf empty &&\n\tgit init empty &&\n\t(\n\t\tcd empty &&\n\t\techo \"f/path: test: unspecified\" >expect &&\n\t\tH=$(git hash-object -t blob --stdin -w actual 2>err &&\n\t\ttest_must_be_empty err &&\n\t\ttest_cmp expect actual\n\t)\n'\n\nrepo_get_oid_treeish() returns a blob object name and we end up\nstoring a blob object name in \"attr_source\" static variable of\ndefault_attr_source() function.\n\nLater this is fed to read_attr() by bootstrap_attr_stack() and then\nto read_attr_from_blob() that uses it to call get_tree_entry(),\nwhich fails for any path because it is a blob. We do not give any\nerrors or error messages during the whole process.\n\nSo in a sense, for !!ignore_bad_attr_tree case, the code ends up\ndoing the right thing. But if !ignore_bad_attr_tree is true, i.e.,\na blob object name is given via --attr-source or GIT_ATTR_SOURCE,\nthen the bug will be uncovered.\n\n t/t0003-attributes.sh | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git c/t/t0003-attributes.sh w/t/t0003-attributes.sh\nindex ecf43ab545..0f02f22171 100755\n--- c/t/t0003-attributes.sh\n+++ w/t/t0003-attributes.sh\n@@ -394,6 +394,18 @@ test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--attr-source that points at a non-treeish' '\n+\ttest_when_finished rm -rf empty &&\n+\tgit init empty &&\n+\t(\n+\t\tcd empty &&\n+\t\techo \"$bad_attr_source_err\" >expect_err &&\n+\t\tH=$(git hash-object -t blob --stdin -w err &&\n+\t\ttest_cmp expect_err err\n+\t)\n+'\n+\n test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tree' '\n \ttest_when_finished rm -rf empty &&\n \tgit init empty &&\n\n"},{"id":"483233","messageId":"xmqqzg0mxnaj.fsf@gitster.g","threadId":"60250","inReplyTo":"xmqqa5smz2lk.fsf@gitster.g","subject":"Re: [PATCH v5 0/2] attr: add attr.tree config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-13T20:47:16Z","receivedAt":"2023-10-13T20:47:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> So in a sense, for !!ignore_bad_attr_tree case, the code ends up\n> doing the right thing. But if !ignore_bad_attr_tree is true, i.e.,\n> a blob object name is given via --attr-source or GIT_ATTR_SOURCE,\n> then the bug will be uncovered.\n\nHaving said all that, I suspect that this problem is not new and\ncertainly not caused by this topic. We should have unconditionally\ndied when GIT_ATTR_SOURCE gave a blob object name, but pretended as\nif an empty tree was given. There may even be existing users who\nnow assume that is working as intended and depend on this bug.\n\nSo, let's leave it as a \"possible bug\" that we might want to fix in\nthe future, outside the scope of this series.\n\nThanks.\n\n\n> t/t0003-attributes.sh | 12 ++++++++++++\n> 1 file changed, 12 insertions(+)\n>\n> diff --git c/t/t0003-attributes.sh w/t/t0003-attributes.sh\n> index ecf43ab545..0f02f22171 100755\n> --- c/t/t0003-attributes.sh\n> +++ w/t/t0003-attributes.sh\n> @@ -394,6 +394,18 @@ test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success '--attr-source that points at a non-treeish' '\n> +\ttest_when_finished rm -rf empty &&\n> +\tgit init empty &&\n> +\t(\n> +\t\tcd empty &&\n> +\t\techo \"$bad_attr_source_err\" >expect_err &&\n> +\t\tH=$(git hash-object -t blob --stdin -w +\t\ttest_must_fail git --attr-source=$H check-attr test -- f/path 2>err &&\n> +\t\ttest_cmp expect_err err\n> +\t)\n> +'\n> +\n> test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tree' '\n> \ttest_when_finished rm -rf empty &&\n> \tgit init empty &&\n"},{"id":"483495","messageId":"C8977B9C-6875-40D1-BEFA-6468379F4028@gmail.com","threadId":"60250","inReplyTo":"xmqqzg0mxnaj.fsf@gitster.g","subject":"Re: [PATCH v5 0/2] attr: add attr.tree config","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2023-10-19T15:43:09Z","receivedAt":"2023-10-19T15:43:30Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Junio,\n\nOn 13 Oct 2023, at 16:47, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n>\n>> So in a sense, for !!ignore_bad_attr_tree case, the code ends up\n>> doing the right thing. But if !ignore_bad_attr_tree is true, i.e.,\n>> a blob object name is given via --attr-source or GIT_ATTR_SOURCE,\n>> then the bug will be uncovered.\n>\n> Having said all that, I suspect that this problem is not new and\n> certainly not caused by this topic. We should have unconditionally\n> died when GIT_ATTR_SOURCE gave a blob object name, but pretended as\n> if an empty tree was given. There may even be existing users who\n> now assume that is working as intended and depend on this bug.\n>\n> So, let's leave it as a \"possible bug\" that we might want to fix in\n> the future, outside the scope of this series.\n>\n> Thanks.\n\nThat sounds good--let's fix in a separate series. Thanks for the careful review!\n\nthanks\nJohn\n\n>\n>\n>> t/t0003-attributes.sh | 12 ++++++++++++\n>> 1 file changed, 12 insertions(+)\n>>\n>> diff --git c/t/t0003-attributes.sh w/t/t0003-attributes.sh\n>> index ecf43ab545..0f02f22171 100755\n>> --- c/t/t0003-attributes.sh\n>> +++ w/t/t0003-attributes.sh\n>> @@ -394,6 +394,18 @@ test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '\n>> \ttest_cmp expect actual\n>> '\n>>\n>> +test_expect_success '--attr-source that points at a non-treeish' '\n>> +\ttest_when_finished rm -rf empty &&\n>> +\tgit init empty &&\n>> +\t(\n>> +\t\tcd empty &&\n>> +\t\techo \"$bad_attr_source_err\" >expect_err &&\n>> +\t\tH=$(git hash-object -t blob --stdin -w > +\t\ttest_must_fail git --attr-source=$H check-attr test -- f/path 2>err &&\n>> +\t\ttest_cmp expect_err err\n>> +\t)\n>> +'\n>> +\n>> test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tree' '\n>> \ttest_when_finished rm -rf empty &&\n>> \tgit init empty &&\n"}]}