attr: attr.allowInvalidSource config to allow invalid revision
PATCH 1/1 attr: attr.allowInvalidSource config to allow invalid revision
From: John Cai <[email protected]>
44451a2e5e (attr: teach "--attr-source=<tree>" global option to "git", 2023-05-06) provided the ability to pass in a treeish as the attr source. When a revision does not resolve to a valid tree is passed, Git will die. GitLab keeps bare repositories and always reads attributes from the default branch, so we pass in HEAD to --attr-source.
With empty repositories however, HEAD does not point to a valid treeish, causing Git to die. This means we would need to check for a valid treeish each time. To avoid this, let's add a configuration that allows Git to simply ignore --attr-source if it does not resolve to a valid tree.
---
attr: attr.allowInvalidSource config to allow empty revision
44451a2e5e (attr: teach "--attr-source=" global option to "git", 2023-05-06) provided the ability to pass in a treeish as the attr source. When a revision does not resolve to a valid tree is passed, Git will die. GitLab keeps bare repositories and always reads attributes from the default branch, so we pass in HEAD to --attr-source.
With empty repositories however, HEAD does not point to a valid treeish, causing Git to die. This means we would need to check for a valid treeish each time. To avoid this, let's add a configuration that allows Git to simply ignore --attr-source if it does not resolve to a valid tree.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1577%2Fjohn-cai%2Fjc%2Fconfig-attr-invalid-source-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1577/john-cai/jc/config-attr-invalid-source-v1
Pull-Request: https://github.com/git/git/pull/1577
Documentation/config.txt | 2 ++ Documentation/config/attr.txt | 6 ++++++ attr.c | 11 +++++++++-- t/t0003-attributes.sh | 36 +++++++++++++++++++++++++++++++++++ 4 files changed, 53 insertions(+), 2 deletions(-) create mode 100644 Documentation/config/attr.txt
Documentation/config.txt +2 -0
@@ -371,6 +371,8 @@ other popular tools, and describe them in your documentation. include::config/advice.txt[] +include::config/attr.txt[] + include::config/core.txt[] include::config/add.txt[]
Documentation/config/attr.txt new file +6 -0
@@ -0,0 +1,6 @@ +attr.allowInvalidSource:: + If `--attr-source` cannot resolve to a valid tree object, ignore + `--attr-source` instead of erroring out, and fall back to looking for + attributes in the default locations. Useful when passing `HEAD` into + `attr-source` since it allows `HEAD` to point to an unborn branch in + cases like an empty repository.
attr.c +9 -2
@@ -1208,8 +1208,15 @@ static void compute_default_attr_source(struct object_id *attr_source)if (!default_attr_source_tree_object_name || !is_null_oid (attr_source))return ; -if (repo_get_oid_treeish (the_repository, default_attr_source_tree_object_name, attr_source)) -die (_ ("bad --attr-source or GIT_ATTR_SOURCE" )); + +if (repo_get_oid_treeish (the_repository, default_attr_source_tree_object_name, attr_source)) { +int allow_invalid_attr_source =0 ; + +git_config_get_bool ("attr.allowinvalidsource" , &allow_invalid_attr_source); + +if (!allow_invalid_attr_source) +die (_ ("bad --attr-source or GIT_ATTR_SOURCE" )); + } }static struct object_id *default_attr_source (void )
t/t0003-attributes.sh +36 -0
@@ -342,6 +342,42 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' ' ) ' +bad_attr_source_err="fatal: bad --attr-source or GIT_ATTR_SOURCE" + +test_expect_success 'attr.allowInvalidSource when HEAD is unborn' ' + test_when_finished rm -rf empty && + echo $bad_attr_source_err >expect_err && + echo "f/path: test: unspecified" >expect && + git init empty && + test_must_fail git -C empty --attr-source=HEAD check-attr test -- f/path 2>err && + test_cmp expect_err err && + git -C empty -c attr.allowInvalidSource=true --attr-source=HEAD check-attr test -- f/path >actual 2>err && + test_must_be_empty err && + test_cmp expect actual +' + +test_expect_success 'attr.allowInvalidSource when --attr-source points to non-existing ref' ' + test_when_finished rm -rf empty && + echo $bad_attr_source_err >expect_err && + echo "f/path: test: unspecified" >expect && + git init empty && + test_must_fail git -C empty --attr-source=refs/does/not/exist check-attr test -- f/path 2>err && + test_cmp expect_err err && + git -C empty -c attr.allowInvalidSource=true --attr-source=refs/does/not/exist check-attr test -- f/path >actual 2>err && + test_must_be_empty err && + test_cmp expect actual +' + +test_expect_success 'bad attr source defaults to reading .gitattributes file' ' + test_when_finished rm -rf empty && + git init empty && + echo "f/path test=val" >empty/.gitattributes && + echo "f/path: test: val" >expect && + git -C empty -c attr.allowInvalidSource=true --attr-source=HEAD check-attr test -- f/path >actual 2>err && + test_must_be_empty err && + test_cmp expect actual +' + test_expect_success 'bare repository: with --source' ' ( cd bare.git &&
base-commit: 1fc548b2d6a3596f3e1c1f8b1930d8dbd1e30bf3
Signature · 1 line
gitgitgadget
Quoted 7 lines · John Cai via GitGitGadget
From: John Cai <[email protected]>
44451a2e5e(attr: teach "--attr-source=<tree>" global option to "git", 2023-05-06) provided the ability to pass in a treeish as the attr source. When a revision does not resolve to a valid tree is passed, Git will die. GitLab keeps bare repositories and always reads attributes from the default branch, so we pass in HEAD to --attr-source.
Makes sense.
Quoted 3 lines
With empty repositories however, HEAD does not point to a valid treeish, causing Git to die. This means we would need to check for a valid treeish each time.
Naturally.
Quoted 3 lines
To avoid this, let's add a configuration that allows
Git to simply ignore --attr-source if it does not resolve to a valid tree.
Not convincing at all as to the reason why we want to do anything "to avoid this". "git log" in a repository whose HEAD does not point to a valid treeish. "git blame" dies with "no such ref:
HEAD". An empty repository (more precisely, an unborn history) needs special casing if you want to present it if you do not want to spew underlying error messages to the end users *anyway*. It is unclear why seeing what commit the HEAD pointer points at (or which branch it points at for that matter) is *an* *extra* and *otherwise* *unnecessary* overhead that need to be avoided.
Quoted 10 lines · Junio C Hamano
Quoted 3 lines
With empty repositories however, HEAD does not point to a valid treeish, causing Git to die. This means we would need to check for a valid treeish each time.
Naturally.
Quoted 3 lines
To avoid this, let's add a configuration that allows
Git to simply ignore --attr-source if it does not resolve to a valid tree.
Quoted 9 lines
Not convincing at all as to the reason why we want to do anything "to avoid this". "git log" in a repository whose HEAD does not point to a valid treeish. "git blame" dies with "no such ref:
HEAD". An empty repository (more precisely, an unborn history) needs special casing if you want to present it if you do not want to spew underlying error messages to the end users *anyway*. It is unclear why seeing what commit the HEAD pointer points at (or which branch it points at for that matter) is *an* *extra* and *otherwise* *unnecessary* overhead that need to be avoided.
In an empty repository, "git log" will die anyway. So I think the more interesting case is "I have a repository with stuff in it, but HEAD points to an unborn branch". So:
git --attr-source=HEAD diff foo^ foo
And there you really are saying "if there are attributes in HEAD, use them; otherwise, don't worry about it". This is exactly what we do with mailmap.blob: in a bare repository it is set to HEAD by default, but if HEAD does not resolve, we just ignore it (just like a HEAD that does not contain a .mailmap file). And those match the non-bare cases, where we'd read those files from the working tree instead.
So I think the same notion applies here. You want to be able to point it at HEAD by default, but if there is no HEAD, that is the same as if HEAD simply did not contain any attributes. If we had attr.blob, that is exactly how I would expect it to work.
My gut feeling is that --attr-source should do the same, and just quietly ignore a ref that does not resolve. But I think an argument can be made that because the caller explicitly gave us a ref, they expect it to work (and that would catch misspellings, etc). Like:
git --attr-source=my-barnch diff foo^ foo
So I'm OK with not changing that behavior.
But what is weird about this patch is that we are using a config option to change how a command-line option is interpreted. If the idea is that some invocations care about the validity of the source and some do not, then the config option is much too blunt. It is set once long ago, but it can't distinguish between times you care about invalid sources and times you don't.
It would make much more sense to me to have another command-line option, like:
git --attr-source=HEAD --allow-invalid-attr-source
Obviously that is horrible to type, but I think the point is that you'd only do this from a script anyway (because it's those automated cases where you want to say "use HEAD only if it exists").
If there were an attr.blob config option and it complained about an invalid HEAD, _then_ I think attr.allowInvalidSource might make sense (though again, I would just argue for switching the behavior by default). And I really think attr.blob is a better match for what GitLab is trying to do here, because it is set once and applies to all commands, rather than having to teach every invocation to pass it (though I guess maybe they use it as an environment variable).
Of course I would think that, as the person who solved GitHub's exact same problem for mailmap by adding mailmap.blob. So you may ingest the appropriate grain of salt. :)
-Peff
Quoted 5 lines · Jeff King
In an empty repository, "git log" will die anyway. So I think the more interesting case is "I have a repository with stuff in it, but HEAD points to an unborn branch". So:
git --attr-source=HEAD diff foo^ foo
This still looks like a made-up example. Who in the right mind would specify HEAD when both of the revs involved in the operation are from branch 'foo'? The history of HEAD may not have anything common with the operand of the operation 'foo' (or its parent), or worse, it may not even exist.
But your "in this repository we never trust attributes from working tree, take it instead from this file or from this blob" example does make a lot more sense as a use case.
Quoted 6 lines
And there you really are saying "if there are attributes in HEAD, use them; otherwise, don't worry about it". This is exactly what we do with mailmap.blob: in a bare repository it is set to HEAD by default, but if HEAD does not resolve, we just ignore it (just like a HEAD that does not contain a .mailmap file). And those match the non-bare cases, where we'd read those files from the working tree instead.
"HEAD" -> "HEAD:.mailmap" if I recall correctly.
And if HEAD does not resolve, we pretend as if HEAD is an empty tree-ish (hence HEAD:.mailmap is missing). It becomes very tempting to do the same for the attribute sources and treat unborn HEAD as if it specifies an empty tree-ish, without any configuration or an extra option.
Such a change would be an end-user observable behaviour change, but nobody sane would be running "git --attr-source=HEAD diff HEAD^ HEAD" to check and detect an unborn HEAD for its error exit code, so I do not think it is a horribly wrong thing to do.
But again, as you said, --attr-source=<tree-ish> does not sound like a good fit for bare-repository hosted environment and a tentative hack waiting for a proper attr.blob support, or something like that, to appear.
Quoted 11 lines
But what is weird about this patch is that we are using a config option to change how a command-line option is interpreted. If the idea is that some invocations care about the validity of the source and some do not, then the config option is much too blunt. It is set once long ago, but it can't distinguish between times you care about invalid sources and times you don't.
It would make much more sense to me to have another command-line option, like:
git --attr-source=HEAD --allow-invalid-attr-source
Yeah, if we were to make it configurable without changing the default behaviour, I agree that would be more correct approach. A configuration does not sound like a good fit.
Quoted 4 lines
... And I really think attr.blob is a better match for what GitLab is trying to do here, because it is set once and applies to all commands, rather than having to teach every invocation to pass it (though I guess maybe they use it as an environment variable).
True, too.
Thanks.
Quoted 6 lines · Junio C Hamano
Quoted 4 lines · Jeff King
In an empty repository, "git log" will die anyway. So I think the more interesting case is "I have a repository with stuff in it, but HEAD points to an unborn branch". So:
Quoted 7 lines
Quoted 1 line
git --attr-source=HEAD diff foo^ fooThis still looks like a made-up example. Who in the right mind would specify HEAD when both of the revs involved in the operation are from branch 'foo'? The history of HEAD may not have anything common with the operand of the operation 'foo' (or its parent), or worse, it may not even exist.
I think it's unlikely for a user to write that. But if you are running a server full of bare repositories that does diffs, you might end up with a script that sticks "--attr-source=HEAD" at the beginning of every command.
It is true that HEAD may not be related. But that is what we use if you are in a non-bare repository and run "git diff foo^ foo".
Arguably:
git --attr-source=$to diff $from $to
is a better default for this command. But something like "git log -p" is trickier, as you have many commits to show. You can try to use the tip of the traversal, but there may be multiple. Using the attributes from the destination of each commit is the most likely to avoid divergence between the attributes and the code, but it may also not be what people want. Using the modern attributes from the working tree often makes showing historical commits much nicer.
So I dunno. I could see arguments in both directions, but I think in general people have been OK with pulling attributes from the working tree. And --attr-source=HEAD is the bare equivalent.
Quoted 3 lines
But your "in this repository we never trust attributes from working tree, take it instead from this file or from this blob" example does make a lot more sense as a use case.
I don't know that it was my example. :) But yes, if you do "--attr-source=$to", you're overriding even the non-bare case. That may be what you want for some cases, but as above, I think it's hard to apply consistently (or even what you'd want for the general case).
Quoted 8 lines
Quoted 6 lines
And there you really are saying "if there are attributes in HEAD, use them; otherwise, don't worry about it". This is exactly what we do with mailmap.blob: in a bare repository it is set to HEAD by default, but if HEAD does not resolve, we just ignore it (just like a HEAD that does not contain a .mailmap file). And those match the non-bare cases, where we'd read those files from the working tree instead.
"HEAD" -> "HEAD:.mailmap" if I recall correctly.
True, yeah. We can't do that here because attributes are spread across the tree. So all my mentions of attr.blob would really be attr.tree.
Quoted 10 lines
And if HEAD does not resolve, we pretend as if HEAD is an empty tree-ish (hence HEAD:.mailmap is missing). It becomes very tempting to do the same for the attribute sources and treat unborn HEAD as if it specifies an empty tree-ish, without any configuration or an extra option.
Such a change would be an end-user observable behaviour change, but nobody sane would be running "git --attr-source=HEAD diff HEAD^ HEAD" to check and detect an unborn HEAD for its error exit code, so I do not think it is a horribly wrong thing to do.
Yeah, that is basically what I am proposing. It sounds from the discussion here that there are two interesting cases:
1. You want to use --attr-source=HEAD because you are trying to make a bare repo behave like a non-bare one. You probably want the "don't complain if it is missing" behavior.
2. You are trying to use the attributes from one side of the diff to override the worktree ones (because the two trees are unrelated). In which case it does not really matter if --attr-source complains, because the diff will likewise complain if the tree cannot be resolved.
Just trying to play devil's advocate, though, I guess you could run a non-diff operation like say:
git --attr-source=my-branch check-attr foo
and then you probably _do_ want to know if that source was ignored or typo'd.
Quoted 4 lines
But again, as you said, --attr-source=<tree-ish> does not sound like a good fit for bare-repository hosted environment and a tentative hack waiting for a proper attr.blob support, or something like that, to appear.
I think folks mentioned mailmap.blob in the original discussion for --attr-source. I don't remember why the patch went with --attr-source there. Maybe John can speak to that.
-Peff
Hi Peff,
Quoted 72 lines · Jeff King
Quoted 6 lines · Junio C Hamano
Quoted 4 lines · Jeff King
In an empty repository, "git log" will die anyway. So I think the more interesting case is "I have a repository with stuff in it, but HEAD points to an unborn branch". So:
Quoted 7 lines
Quoted 1 line
git --attr-source=HEAD diff foo^ fooThis still looks like a made-up example. Who in the right mind would specify HEAD when both of the revs involved in the operation are from branch 'foo'? The history of HEAD may not have anything common with the operand of the operation 'foo' (or its parent), or worse, it may not even exist.
I think it's unlikely for a user to write that. But if you are running a server full of bare repositories that does diffs, you might end up with a script that sticks "--attr-source=HEAD" at the beginning of every command.
It is true that HEAD may not be related. But that is what we use if you are in a non-bare repository and run "git diff foo^ foo".
Arguably:
git --attr-source=$to diff $from $tois a better default for this command. But something like "git log -p" is trickier, as you have many commits to show. You can try to use the tip of the traversal, but there may be multiple. Using the attributes from the destination of each commit is the most likely to avoid divergence between the attributes and the code, but it may also not be what people want. Using the modern attributes from the working tree often makes showing historical commits much nicer.
So I dunno. I could see arguments in both directions, but I think in general people have been OK with pulling attributes from the working tree. And --attr-source=HEAD is the bare equivalent.
Quoted 3 lines
But your "in this repository we never trust attributes from working tree, take it instead from this file or from this blob" example does make a lot more sense as a use case.
I don't know that it was my example. :) But yes, if you do "--attr-source=$to", you're overriding even the non-bare case. That may be what you want for some cases, but as above, I think it's hard to apply consistently (or even what you'd want for the general case).
Quoted 8 lines
Quoted 6 lines
And there you really are saying "if there are attributes in HEAD, use them; otherwise, don't worry about it". This is exactly what we do with mailmap.blob: in a bare repository it is set to HEAD by default, but if HEAD does not resolve, we just ignore it (just like a HEAD that does not contain a .mailmap file). And those match the non-bare cases, where we'd read those files from the working tree instead.
"HEAD" -> "HEAD:.mailmap" if I recall correctly.
True, yeah. We can't do that here because attributes are spread across the tree. So all my mentions of attr.blob would really be attr.tree.
Quoted 10 lines
And if HEAD does not resolve, we pretend as if HEAD is an empty tree-ish (hence HEAD:.mailmap is missing). It becomes very tempting to do the same for the attribute sources and treat unborn HEAD as if it specifies an empty tree-ish, without any configuration or an extra option.
Such a change would be an end-user observable behaviour change, but nobody sane would be running "git --attr-source=HEAD diff HEAD^ HEAD" to check and detect an unborn HEAD for its error exit code, so I do not think it is a horribly wrong thing to do.
Quoted 6 lines
Yeah, that is basically what I am proposing. It sounds from the discussion here that there are two interesting cases:
1. You want to use --attr-source=HEAD because you are trying to make a bare repo behave like a non-bare one. You probably want the "don't complain if it is missing" behavior.
Yep, this is the primary use case for us.
Quoted 15 lines
2. You are trying to use the attributes from one side of the diff to override the worktree ones (because the two trees are unrelated). In which case it does not really matter if --attr-source complains, because the diff will likewise complain if the tree cannot be resolved.
Just trying to play devil's advocate, though, I guess you could run a non-diff operation like say:
git --attr-source=my-branch check-attr fooand then you probably _do_ want to know if that source was ignored or typo'd.
Quoted 8 lines
Quoted 4 lines
But again, as you said, --attr-source=<tree-ish> does not sound like a good fit for bare-repository hosted environment and a tentative hack waiting for a proper attr.blob support, or something like that, to appear.
I think folks mentioned mailmap.blob in the original discussion for --attr-source. I don't remember why the patch went with --attr-source there. Maybe John can speak to that.
Yeah I was looking for this, and I think I found it in [1], which was part of a separate patch having to do with gitattributes. If someone did mention it in the patch for --attr-source, then I can't remember why we decided to go with the comand line flag rather than the config.
1. https://lore.kernel.org/git/[email protected]/
Quoted 2 lines
-Peff
Hi Junio,
Quoted 50 lines · Junio C Hamano
Quoted 5 lines · Jeff King
In an empty repository, "git log" will die anyway. So I think the more interesting case is "I have a repository with stuff in it, but HEAD points to an unborn branch". So:
git --attr-source=HEAD diff foo^ fooThis still looks like a made-up example. Who in the right mind would specify HEAD when both of the revs involved in the operation are from branch 'foo'? The history of HEAD may not have anything common with the operand of the operation 'foo' (or its parent), or worse, it may not even exist.
But your "in this repository we never trust attributes from working tree, take it instead from this file or from this blob" example does make a lot more sense as a use case.
Quoted 6 lines
And there you really are saying "if there are attributes in HEAD, use them; otherwise, don't worry about it". This is exactly what we do with mailmap.blob: in a bare repository it is set to HEAD by default, but if HEAD does not resolve, we just ignore it (just like a HEAD that does not contain a .mailmap file). And those match the non-bare cases, where we'd read those files from the working tree instead.
"HEAD" -> "HEAD:.mailmap" if I recall correctly.
And if HEAD does not resolve, we pretend as if HEAD is an empty tree-ish (hence HEAD:.mailmap is missing). It becomes very tempting to do the same for the attribute sources and treat unborn HEAD as if it specifies an empty tree-ish, without any configuration or an extra option.
Such a change would be an end-user observable behaviour change, but nobody sane would be running "git --attr-source=HEAD diff HEAD^ HEAD" to check and detect an unborn HEAD for its error exit code, so I do not think it is a horribly wrong thing to do.
But again, as you said, --attr-source=<tree-ish> does not sound like a good fit for bare-repository hosted environment and a tentative hack waiting for a proper attr.blob support, or something like that, to appear.
Quoted 7 lines
But what is weird about this patch is that we are using a config option to change how a command-line option is interpreted. If the idea is that some invocations care about the validity of the source and some do not, then the config option is much too blunt. It is set once long ago, but it can't distinguish between times you care about invalid sources and times you don't.
Quoted 13 lines
Quoted 4 lines
It would make much more sense to me to have another command-line option, like:
git --attr-source=HEAD --allow-invalid-attr-sourceYeah, if we were to make it configurable without changing the default behaviour, I agree that would be more correct approach. A configuration does not sound like a good fit.
Quoted 4 lines
... And I really think attr.blob is a better match for what GitLab is trying to do here, because it is set once and applies to all commands, rather than having to teach every invocation to pass it (though I guess maybe they use it as an environment variable).
Between adding an --allow-invalid-attr-source, and adding attr.blob and attr.allowInvalidSource I think I like adding the attr.blob config more.
Quoted 4 lines
True, too.
Thanks.
thanks
John
Hi Peff,
Quoted 35 lines · Jeff King
Quoted 10 lines · Junio C Hamano
Quoted 3 lines
With empty repositories however, HEAD does not point to a valid treeish, causing Git to die. This means we would need to check for a valid treeish each time.
Naturally.
Quoted 3 lines
To avoid this, let's add a configuration that allows
Git to simply ignore --attr-source if it does not resolve to a valid tree.Quoted 9 lines
Not convincing at all as to the reason why we want to do anything "to avoid this". "git log" in a repository whose HEAD does not point to a valid treeish. "git blame" dies with "no such ref:
HEAD". An empty repository (more precisely, an unborn history) needs special casing if you want to present it if you do not want to spew underlying error messages to the end users *anyway*. It is unclear why seeing what commit the HEAD pointer points at (or which branch it points at for that matter) is *an* *extra* and *otherwise* *unnecessary* overhead that need to be avoided.In an empty repository, "git log" will die anyway. So I think the more interesting case is "I have a repository with stuff in it, but HEAD points to an unborn branch". So:
git --attr-source=HEAD diff foo^ fooAnd there you really are saying "if there are attributes in HEAD, use them; otherwise, don't worry about it". This is exactly what we do with mailmap.blob: in a bare repository it is set to HEAD by default, but if HEAD does not resolve, we just ignore it (just like a HEAD that does not contain a .mailmap file). And those match the non-bare cases, where we'd read those files from the working tree instead.
Quoted 4 lines
So I think the same notion applies here. You want to be able to point it at HEAD by default, but if there is no HEAD, that is the same as if HEAD simply did not contain any attributes. If we had attr.blob, that is exactly how I would expect it to work.
You captured this quite well!
Quoted 17 lines
My gut feeling is that --attr-source should do the same, and just quietly ignore a ref that does not resolve. But I think an argument can be made that because the caller explicitly gave us a ref, they expect it to work (and that would catch misspellings, etc). Like:
git --attr-source=my-barnch diff foo^ fooSo I'm OK with not changing that behavior.
But what is weird about this patch is that we are using a config option to change how a command-line option is interpreted. If the idea is that some invocations care about the validity of the source and some do not, then the config option is much too blunt. It is set once long ago, but it can't distinguish between times you care about invalid sources and times you don't.
Quoted 8 lines
It would make much more sense to me to have another command-line option, like:
git --attr-source=HEAD --allow-invalid-attr-sourceObviously that is horrible to type, but I think the point is that you'd only do this from a script anyway (because it's those automated cases where you want to say "use HEAD only if it exists").
Yeah, I see the point about using a config to change default behavior of a command leading to confusion.
Quoted 8 lines
If there were an attr.blob config option and it complained about an invalid HEAD, _then_ I think attr.allowInvalidSource might make sense (though again, I would just argue for switching the behavior by default). And I really think attr.blob is a better match for what GitLab is trying to do here, because it is set once and applies to all commands, rather than having to teach every invocation to pass it (though I guess maybe they use it as an environment variable).
In retrospect perhaps a config would have been better here. I think this patch started with improving an existing command line flag [1] by making it global. So I think we were just thinking about command line flags and didn't consider configs.
That being said, for GitLab at least there's not a lot of difference since we pass in configs through the commandline anyways rather than relying on the config state itself on disk for our bare server-side repositories.
1. https://lore.kernel.org/git/[email protected]/
Quoted 6 lines
Of course I would think that, as the person who solved GitHub's exact same problem for mailmap by adding mailmap.blob. So you may ingest the appropriate grain of salt. :)
-Peff
thanks
John
v2 cover attr: add attr.tree and attr.allowInvalidSource configs
In reply to John Cai via GitGitGadget44451a2e5e (attr: teach "--attr-source=" global option to "git", 2023-05-06) provided the ability to pass in a treeish as the attr source. When a revision does not resolve to a valid tree is passed, Git will die. At GitLab, we server repositories as bare repos and would like to always read attributes from the default branch, so we'd like to pass in HEAD as the treeish to read gitattributes from on every command. In this context we would not want Git to die if HEAD is unborn, like in the case of empty repositories.
Instead of modifying the default behavior of --attr-source, create a pair of configs attr.tree and attr.allowInvalidSource whereby an admin can configure a ref for all commands to read gitattributes from, and another config that relaxes the requirement that this treeish resolve to a valid tree.
Changes since v1:
* Added a commit to add attr.tree config
John Cai (2):
attr: add attr.tree for setting the treeish to read attributes from
attr: add attr.allowInvalidSource config to allow invalid revision
Documentation/config.txt | 2 ++ Documentation/config/attr.txt | 12 +++++++++ attr.c | 21 +++++++++++++-- t/t0003-attributes.sh | 49 +++++++++++++++++++++++++++++++++++ 4 files changed, 82 insertions(+), 2 deletions(-) create mode 100644 Documentation/config/attr.txt
base-commit: 1fc548b2d6a3596f3e1c1f8b1930d8dbd1e30bf3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1577%2Fjohn-cai%2Fjc%2Fconfig-attr-invalid-source-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1577/john-cai/jc/config-attr-invalid-source-v2
Pull-Request: https://github.com/git/git/pull/1577
Range-diff vs v1:
-: ----------- > 1: 446bce03a96 attr: add attr.tree for setting the treeish to read attributes from
1: 06b9acf24ca ! 2: 52d9e180352 attr: attr.allowInvalidSource config to allow invalid revision
@@ Metadata
Author: John Cai <[email protected]>
## Commit message ##
- attr: attr.allowInvalidSource config to allow invalid revision
+ attr: add attr.allowInvalidSource config to allow invalid revision
- 44451a2e5e (attr: teach "--attr-source=<tree>" global option to "git",
- 2023-05-06) provided the ability to pass in a treeish as the attr
- source. When a revision does not resolve to a valid tree is passed, Git
- will die. GitLab keeps bare repositories and always reads attributes
- from the default branch, so we pass in HEAD to --attr-source.
+ The previous commit provided the ability to pass in a treeish as the
+ attr source via the attr.tree config. The default behavior is that when
+ a revision does not resolve to a valid tree is passed, Git will die.
- With empty repositories however, HEAD does not point to a valid treeish,
- causing Git to die. This means we would need to check for a valid
- treeish each time. To avoid this, let's add a configuration that allows
- Git to simply ignore --attr-source if it does not resolve to a valid
- tree.
+ When HEAD is unborn however, it does not point to a valid treeish,
+ causing Git to die. In the context of serving repositories through bare
+ repositories, we'd like to be able to set attr.tree to HEAD and allow
+ cases where HEAD does not resolve to a valid tree to be treated as if
+ there were no treeish provided.
+
+ Add attr.allowInvalidSource that allows this.
Signed-off-by: John Cai <[email protected]>
- ## Documentation/config.txt ##
-@@ Documentation/config.txt: other popular tools, and describe them in your documentation.
-
- include::config/advice.txt[]
-
-+include::config/attr.txt[]
+ ## Documentation/config/attr.txt ##
+@@ Documentation/config/attr.txt: attr.tree:
+ linkgit:gitattributes[5]. This is equivalent to setting the
+ `GIT_ATTR_SOURCE` environment variable, or passing in --attr-source to
+ the Git command.
+
- include::config/core.txt[]
-
- include::config/add.txt[]
-
- ## Documentation/config/attr.txt (new) ##
-@@
+attr.allowInvalidSource::
-+ If `--attr-source` cannot resolve to a valid tree object, ignore
-+ `--attr-source` instead of erroring out, and fall back to looking for
++ If `attr.tree` cannot resolve to a valid tree object, ignore
++ `attr.tree` instead of erroring out, and fall back to looking for
+ attributes in the default locations. Useful when passing `HEAD` into
+ `attr-source` since it allows `HEAD` to point to an unborn branch in
+ cases like an empty repository.
## attr.c ##
-@@ attr.c: static void compute_default_attr_source(struct object_id *attr_source)
+@@ attr.c: void set_git_attr_source(const char *tree_object_name)
+
+ static void compute_default_attr_source(struct object_id *attr_source)
+ {
++ int attr_source_from_config = 0;
++
+ if (!default_attr_source_tree_object_name)
+ default_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);
+
+ if (!default_attr_source_tree_object_name) {
+ char *attr_tree;
+
+- if (!git_config_get_string("attr.tree", &attr_tree))
++ if (!git_config_get_string("attr.tree", &attr_tree)) {
++ attr_source_from_config = 1;
+ default_attr_source_tree_object_name = attr_tree;
++ }
+ }
+
if (!default_attr_source_tree_object_name || !is_null_oid(attr_source))
return;
- if (repo_get_oid_treeish(the_repository, default_attr_source_tree_object_name, attr_source))
- die(_("bad --attr-source or GIT_ATTR_SOURCE"));
-+
+ if (repo_get_oid_treeish(the_repository, default_attr_source_tree_object_name, attr_source)) {
+ int allow_invalid_attr_source = 0;
+
+ git_config_get_bool("attr.allowinvalidsource", &allow_invalid_attr_source);
+
-+ if (!allow_invalid_attr_source)
++ if (!(allow_invalid_attr_source && attr_source_from_config))
+ die(_("bad --attr-source or GIT_ATTR_SOURCE"));
+ }
}
@@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
+ git init empty &&
+ test_must_fail git -C empty --attr-source=HEAD check-attr test -- f/path 2>err &&
+ test_cmp expect_err err &&
-+ git -C empty -c attr.allowInvalidSource=true --attr-source=HEAD check-attr test -- f/path >actual 2>err &&
++ git -C empty -c attr.tree=HEAD -c attr.allowInvalidSource=true check-attr test -- f/path >actual 2>err &&
+ test_must_be_empty err &&
+ test_cmp expect actual
+'
@@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
+ git init empty &&
+ test_must_fail git -C empty --attr-source=refs/does/not/exist check-attr test -- f/path 2>err &&
+ test_cmp expect_err err &&
-+ git -C empty -c attr.allowInvalidSource=true --attr-source=refs/does/not/exist check-attr test -- f/path >actual 2>err &&
++ git -C empty -c attr.tree=refs/does/not/exist -c attr.allowInvalidSource=true check-attr test -- f/path >actual 2>err &&
+ test_must_be_empty err &&
+ test_cmp expect actual
+'
@@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
+ git init empty &&
+ echo "f/path test=val" >empty/.gitattributes &&
+ echo "f/path: test: val" >expect &&
-+ git -C empty -c attr.allowInvalidSource=true --attr-source=HEAD check-attr test -- f/path >actual 2>err &&
++ git -C empty -c attr.tree=HEAD -c attr.allowInvalidSource=true check-attr test -- f/path >actual 2>err &&
+ test_must_be_empty err &&
+ test_cmp expect actual
+'
++
++test_expect_success 'attr.allowInvalidSource has no effect on --attr-source' '
++ test_when_finished rm -rf empty &&
++ echo $bad_attr_source_err >expect_err &&
++ echo "f/path: test: unspecified" >expect &&
++ git init empty &&
++ test_must_fail git -C empty -c attr.allowInvalidSource=true --attr-source=HEAD check-attr test -- f/path 2>err &&
++ test_cmp expect_err err
++'
+
test_expect_success 'bare repository: with --source' '
(Signature · 1 line
gitgitgadget
PATCH v2 1/2 attr: add attr.tree for setting the treeish to read attributes from
From: John Cai <[email protected]>
44451a2e5e (attr: teach "--attr-source=<tree>" global option to "git", 2023-05-06) provided the ability to pass in a treeish as the attr source. In the context of serving Git repositories as bare repos like we do at GitLab however, it would be easier to point --attr-source to HEAD for all commands by setting it once.
Add a new config attr.tree that allows this.
--- Documentation/config.txt | 2 ++ Documentation/config/attr.txt | 5 +++++ attr.c | 7 +++++++ t/t0003-attributes.sh | 4 ++++ 4 files changed, 18 insertions(+) create mode 100644 Documentation/config/attr.txt
Documentation/config.txt +2 -0
@@ -371,6 +371,8 @@ other popular tools, and describe them in your documentation. include::config/advice.txt[] +include::config/attr.txt[] + include::config/core.txt[] include::config/add.txt[]
Documentation/config/attr.txt new file +5 -0
@@ -0,0 +1,5 @@ +attr.tree: + A <tree-ish> to read gitattributes from instead of the worktree. See + linkgit:gitattributes[5]. This is equivalent to setting the + `GIT_ATTR_SOURCE` environment variable, or passing in --attr-source to + the Git command.
attr.c +7 -0
@@ -1205,6 +1205,13 @@ static void compute_default_attr_source(struct object_id *attr_source)if (!default_attr_source_tree_object_name) default_attr_source_tree_object_name =getenv (GIT_ATTR_SOURCE_ENVIRONMENT ); +if (!default_attr_source_tree_object_name) { +char *attr_tree; + +if (!git_config_get_string ("attr.tree" , &attr_tree)) + default_attr_source_tree_object_name = attr_tree; + } +if (!default_attr_source_tree_object_name || !is_null_oid (attr_source))return ;
t/t0003-attributes.sh +4 -0
@@ -40,6 +40,10 @@ attr_check_source () { test_cmp expect actual && test_must_be_empty err + git $git_opts -c "attr.tree=$source" check-attr test -- "$path" >actual 2>err && + test_cmp expect actual && + test_must_be_empty err + GIT_ATTR_SOURCE="$source" git $git_opts check-attr test -- "$path" >actual 2>err && test_cmp expect actual && test_must_be_empty err
Signature · 1 line
gitgitgadget
Re: [PATCH v2 1/2] attr: add attr.tree for setting the treeish to read attributes from
Quoted 9 lines · John Cai via GitGitGadget
From: John Cai <[email protected]>
44451a2e5e(attr: teach "--attr-source=<tree>" global option to "git", 2023-05-06) provided the ability to pass in a treeish as the attr source. In the context of serving Git repositories as bare repos like we do at GitLab however, it would be easier to point --attr-source to HEAD for all commands by setting it once.Add a new config attr.tree that allows this.
Hmph, I wonder if we want to go all the way to emulate how the mailmap.blob was done, including
- Default the value of attr.tree to HEAD in a bare repository;
- Notice but ignore errors if the attr.tree does not point at a tree object, and pretend as if attr.tree specified an empty tree;
which does not seem to be in this patch. With such a change, probably we do not even need [2/2] of the series, perhaps?
Re: [PATCH v2 1/2] attr: add attr.tree for setting the treeish to read attributes from
Quoted 15 lines · Junio C Hamano
Quoted 9 lines · John Cai via GitGitGadget
From: John Cai <[email protected]>
44451a2e5e(attr: teach "--attr-source=<tree>" global option to "git", 2023-05-06) provided the ability to pass in a treeish as the attr source. In the context of serving Git repositories as bare repos like we do at GitLab however, it would be easier to point --attr-source to HEAD for all commands by setting it once.Add a new config attr.tree that allows this.
Hmph, I wonder if we want to go all the way to emulate how the mailmap.blob was done, including
Quoted 7 lines
- Default the value of attr.tree to HEAD in a bare repository;
- Notice but ignore errors if the attr.tree does not point at a tree object, and pretend as if attr.tree specified an empty tree;
which does not seem to be in this patch. With such a change, probably we do not even need [2/2] of the series, perhaps?
Oh good, this was exactly what I was going to write in a review, so now I don't have to. :)
Even though it creates behavior differences between attr.tree and --attr-source, I think that is justifiable. Config options apply across a wider set of contexts, so loosening the error handling may make sense where it would not for a command-line option. But we should document both that, and also how the two interact (I assume "git --attr-source" would override attr.tree completely).
-Peff
Re: [PATCH v2 1/2] attr: add attr.tree for setting the treeish to read attributes from
Hi Peff,
Quoted 25 lines · Jeff King
Quoted 15 lines · Junio C Hamano
Quoted 9 lines · John Cai via GitGitGadget
From: John Cai <[email protected]>
44451a2e5e(attr: teach "--attr-source=<tree>" global option to "git", 2023-05-06) provided the ability to pass in a treeish as the attr source. In the context of serving Git repositories as bare repos like we do at GitLab however, it would be easier to point --attr-source to HEAD for all commands by setting it once.Add a new config attr.tree that allows this.
Hmph, I wonder if we want to go all the way to emulate how the mailmap.blob was done, including
Quoted 7 lines
- Default the value of attr.tree to HEAD in a bare repository;
- Notice but ignore errors if the attr.tree does not point at a tree object, and pretend as if attr.tree specified an empty tree;
which does not seem to be in this patch. With such a change, probably we do not even need [2/2] of the series, perhaps?
Quoted 7 lines
Oh good, this was exactly what I was going to write in a review, so now I don't have to. :)
Even though it creates behavior differences between attr.tree and --attr-source, I think that is justifiable. Config options apply across a wider set of contexts, so loosening the error handling may make sense where it would not for a command-line option.
Makes sense. Will adjust in the next version.
Quoted 2 lines
But we should document both that, and also how the two interact (I assume "git --attr-source" would override attr.tree completely).
Yes, --attr-source would take precedence over attr.tree
Quoted 2 lines
-Peff
thanks!
John
Re: [PATCH v2 1/2] attr: add attr.tree for setting the treeish to read attributes from
In reply to John Cai via GitGitGadget[jc: JTan CC'ed as he seems to have took over the polishing of b1bda751 (parse: separate out parsing functions from config.h, 2023-09-29)]
Quoted 7 lines · John Cai via GitGitGadget
attr.c +0
-0@@ -1205,6 +1205,13 @@ static void compute_default_attr_source(struct object_id *attr_source)if (!default_attr_source_tree_object_name) default_attr_source_tree_object_name =getenv (GIT_ATTR_SOURCE_ENVIRONMENT );
attr.c static void compute_default_attr_source(struct object_id *attr_source) · line 1216
Quoted 10 lines
attr.c +7
-0+if (!default_attr_source_tree_object_name) { +char *attr_tree; + +if (!git_config_get_string ("attr.tree" , &attr_tree)) + default_attr_source_tree_object_name = attr_tree; + } +if (!default_attr_source_tree_object_name || !is_null_oid (attr_source))return ;
As this adds a new call to git_config_get_string(), which will only be available by including <config.h>, a merge-fix into 'seen' of this topic needs to revert what b1bda751 (parse: separate out parsing functions from config.h, 2023-09-29) did, which made this file include only <parse.h>.
As this configuration variable was invented to improve the way the attribute source tree is supported by emulating how mailmap.blob is done, it deserves a bit of comparison.
The way mailmap.c does this is not have any code that reads or parses configuration in mailmap.c (which is a rather library-ish place), and leaves it up to callers to pre-populate the global variable git_mailmap_blob with config.c:git_default_config(). That way, they do not need to include <config.h> (nor <parse.h>) that is closer to the UI layer. I am wondering why we are not doing the same, and instead making an ad-hoc call to git_config_get_string() in this code, and if it is a good direction to move the codebase to (in which case we may want to make sure that the same pattern is followed in other places).
Folks interested in libification, as to the direction of that effort, what's your plan on where to draw a line between "library" and "userland"? Should library-ish code be allowed to call git_config_anything()? I somehow suspect that it might be cleaner if they didn't, and instead have the user of the "attr" module to supply the necessary values from outside.
On the other hand, once the part we have historically called "config" API gets a reasonably solid abstraction so that they become pluggable and replaceable, random ad-hoc calls from library code outside the "config" library code may not be a huge problem, as long as we plumb the necessary object handles around (so "attr" library would need to be told which "config" backend is in use, probably in the form of a struct that holds the various states in to replace the current use of globals, plus a vtable to point at implementations of the "config" service, and git_config_get_string() call in such a truly libified world would grab the value of the named variable transparently from whichever "config" backend is currently in use).
Anyway, I think I wiggled this patch into 'seen' so I'll push out today's integration result shortly.
Re: [PATCH v2 1/2] attr: add attr.tree for setting the treeish to read attributes from
Quoted 21 lines · Junio C Hamano
As this adds a new call to
git_config_get_string(), which will only be available by including <config.h>, a merge-fix into 'seen' of this topic needs to revert whatb1bda751(parse: separate out parsing functions fromconfig.h, 2023-09-29) did, which made this file include only <parse.h>.As this configuration variable was invented to improve the way the attribute source tree is supported by emulating how mailmap.blob is done, it deserves a bit of comparison.
The way
mailmap.cdoes this is not have any code that reads or parses configuration inmailmap.c(which is a rather library-ish place), and leaves it up to callers to pre-populate the global variablegit_mailmap_blobwith config.c:git_default_config(). That way, they do not need to include <config.h> (nor <parse.h>) that is closer to the UI layer. I am wondering why we are not doing the same, and instead making an ad-hoc call togit_config_get_string()in this code, and if it is a good direction to move the codebase to (in which case we may want to make sure that the same pattern is followed in other places).
Quoted 6 lines
Folks interested in libification, as to the direction of that effort, what's your plan on where to draw a line between "library" and "userland"? Should library-ish code be allowed to call
git_config_anything()? I somehow suspect that it might be cleaner if they didn't, and instead have the user of the "attr" module to supply the necessary values from outside.
I think that ideally library-ish code shouldn't be allowed to call config, yes. However I think what's practical would be for libraries that use very few config variables to get the necessary values from outside, and libraries that use many config variables (e.g. fetch, if it becomes a library) to call config.
Quoted 4 lines
On the other hand, once the part we have historically called "config" API gets a reasonably solid abstraction so that they become pluggable and replaceable, random ad-hoc calls from library code outside the "config" library code may not be a huge problem, as long
Quoted 8 lines
as we plumb the necessary object handles around (so "attr" library would need to be told which "config" backend is in use, probably in the form of a struct that holds the various states in to replace the current use of globals, plus a vtable to point at implementations of the "config" service, and
git_config_get_string()call in such a truly libified world would grab the value of the named variable transparently from whichever "config" backend is currently in use).
This is true, but if we were ever to use the attr library elsewhere (whether in the git.git repo itself to unit test this library, or in another software project), we would need to supply a mock/stub of config. If attr uses very few config variables, I think it's clearer if it takes in the information from outside.
PATCH v2 2/2 attr: add attr.allowInvalidSource config to allow invalid revision
In reply to John Cai via GitGitGadgetFrom: John Cai <[email protected]>
The previous commit provided the ability to pass in a treeish as the attr source via the attr.tree config. The default behavior is that when a revision does not resolve to a valid tree is passed, Git will die.
When HEAD is unborn however, it does not point to a valid treeish, causing Git to die. In the context of serving repositories through bare repositories, we'd like to be able to set attr.tree to HEAD and allow cases where HEAD does not resolve to a valid tree to be treated as if there were no treeish provided.
Add attr.allowInvalidSource that allows this.
--- Documentation/config/attr.txt | 7 ++++++ attr.c | 16 ++++++++++--- t/t0003-attributes.sh | 45 +++++++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 3 deletions(-)
Documentation/config/attr.txt +7 -0
@@ -3,3 +3,10 @@ attr.tree: linkgit:gitattributes[5]. This is equivalent to setting the `GIT_ATTR_SOURCE` environment variable, or passing in --attr-source to the Git command. + +attr.allowInvalidSource:: + If `attr.tree` cannot resolve to a valid tree object, ignore + `attr.tree` instead of erroring out, and fall back to looking for + attributes in the default locations. Useful when passing `HEAD` into + `attr-source` since it allows `HEAD` to point to an unborn branch in + cases like an empty repository.
attr.c +13 -3
@@ -1202,21 +1202,31 @@ void set_git_attr_source(const char *tree_object_name)static void compute_default_attr_source (struct object_id *attr_source) { +int attr_source_from_config =0 ; +if (!default_attr_source_tree_object_name) default_attr_source_tree_object_name =getenv (GIT_ATTR_SOURCE_ENVIRONMENT );if (!default_attr_source_tree_object_name) {char *attr_tree; -if (!git_config_get_string ("attr.tree" , &attr_tree)) +if (!git_config_get_string ("attr.tree" , &attr_tree)) { + attr_source_from_config =1 ; default_attr_source_tree_object_name = attr_tree; + } }if (!default_attr_source_tree_object_name || !is_null_oid (attr_source))return ; -if (repo_get_oid_treeish (the_repository, default_attr_source_tree_object_name, attr_source)) -die (_ ("bad --attr-source or GIT_ATTR_SOURCE" )); +if (repo_get_oid_treeish (the_repository, default_attr_source_tree_object_name, attr_source)) { +int allow_invalid_attr_source =0 ; + +git_config_get_bool ("attr.allowinvalidsource" , &allow_invalid_attr_source); + +if (!(allow_invalid_attr_source && attr_source_from_config)) +die (_ ("bad --attr-source or GIT_ATTR_SOURCE" )); + } }static struct object_id *default_attr_source (void )
t/t0003-attributes.sh +45 -0
51 lines not on this page · raw message
Signature · 1 line
gitgitgadget
v3 cover attr: add attr.tree config
In reply to John Cai via GitGitGadget44451a2e5e (attr: teach "--attr-source=" global option to "git", 2023-05-06) provided the ability to pass in a treeish as the attr source. When a revision does not resolve to a valid tree is passed, Git will die. At GitLab, we server repositories as bare repos and would like to always read attributes from the default branch, so we'd like to pass in HEAD as the treeish to read gitattributes from on every command. In this context we would not want Git to die if HEAD is unborn, like in the case of empty repositories.
Instead of modifying the default behavior of --attr-source, create a new config attr.tree with which an admin can configure a ref for all commands to read gitattributes from. Also make the default tree to read from HEAD on bare repositories.
Changes since v2:
* relax the restrictions around attr.tree so that if it does not resolve to a valid treeish, ignore it.
* add a commit to default to HEAD in bare repositories
Changes since v1:
* Added a commit to add attr.tree config
John Cai (2):
attr: read attributes from HEAD when bare repo
attr: add attr.tree for setting the treeish to read attributes from
Documentation/config.txt | 2 + Documentation/config/attr.txt | 5 +++ attr.c | 19 ++++++++- attr.h | 2 + config.c | 14 +++++++ t/t0003-attributes.sh | 76 +++++++++++++++++++++++++++++++++++ t/t5001-archive-attr.sh | 2 +- 7 files changed, 118 insertions(+), 2 deletions(-) create mode 100644 Documentation/config/attr.txt
base-commit: 1fc548b2d6a3596f3e1c1f8b1930d8dbd1e30bf3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1577%2Fjohn-cai%2Fjc%2Fconfig-attr-invalid-source-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1577/john-cai/jc/config-attr-invalid-source-v3
Pull-Request: https://github.com/git/git/pull/1577
Range-diff vs v2:
2: 52d9e180352 ! 1: cef206d47c7 attr: add attr.allowInvalidSource config to allow invalid revision
@@ Metadata
Author: John Cai <[email protected]>
## Commit message ##
- attr: add attr.allowInvalidSource config to allow invalid revision
+ attr: read attributes from HEAD when bare repo
- The previous commit provided the ability to pass in a treeish as the
- attr source via the attr.tree config. The default behavior is that when
- a revision does not resolve to a valid tree is passed, Git will die.
+ The motivation for 44451a2e5e (attr: teach "--attr-source=<tree>" global
+ option to "git" , 2023-05-06), was to make it possible to use
+ gitattributes with bare repositories.
- When HEAD is unborn however, it does not point to a valid treeish,
- causing Git to die. In the context of serving repositories through bare
- repositories, we'd like to be able to set attr.tree to HEAD and allow
- cases where HEAD does not resolve to a valid tree to be treated as if
- there were no treeish provided.
-
- Add attr.allowInvalidSource that allows this.
+ To make it easier to read gitattributes in bare repositories however,
+ let's just make HEAD:.gitattributes the default. This is in line with
+ how mailmap works, 8c473cecfd (mailmap: default mailmap.blob in bare
+ repositories, 2012-12-13).
Signed-off-by: John Cai <[email protected]>
- ## Documentation/config/attr.txt ##
-@@ Documentation/config/attr.txt: attr.tree:
- linkgit:gitattributes[5]. This is equivalent to setting the
- `GIT_ATTR_SOURCE` environment variable, or passing in --attr-source to
- the Git command.
-+
-+attr.allowInvalidSource::
-+ If `attr.tree` cannot resolve to a valid tree object, ignore
-+ `attr.tree` instead of erroring out, and fall back to looking for
-+ attributes in the default locations. Useful when passing `HEAD` into
-+ `attr-source` since it allows `HEAD` to point to an unborn branch in
-+ cases like an empty repository.
-
## attr.c ##
-@@ attr.c: void set_git_attr_source(const char *tree_object_name)
+@@ attr.c: static void collect_some_attrs(struct index_state *istate,
+ }
- static void compute_default_attr_source(struct object_id *attr_source)
+ static const char *default_attr_source_tree_object_name;
++static int ignore_bad_attr_tree;
+
+ void set_git_attr_source(const char *tree_object_name)
{
-+ int attr_source_from_config = 0;
-+
+@@ attr.c: static void compute_default_attr_source(struct object_id *attr_source)
if (!default_attr_source_tree_object_name)
default_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);
- if (!default_attr_source_tree_object_name) {
- char *attr_tree;
-
-- if (!git_config_get_string("attr.tree", &attr_tree))
-+ if (!git_config_get_string("attr.tree", &attr_tree)) {
-+ attr_source_from_config = 1;
- default_attr_source_tree_object_name = attr_tree;
-+ }
- }
-
++ if (!default_attr_source_tree_object_name &&
++ startup_info->have_repository &&
++ is_bare_repository()) {
++ default_attr_source_tree_object_name = "HEAD";
++ ignore_bad_attr_tree = 1;
++ }
++
if (!default_attr_source_tree_object_name || !is_null_oid(attr_source))
return;
- if (repo_get_oid_treeish(the_repository, default_attr_source_tree_object_name, attr_source))
-- die(_("bad --attr-source or GIT_ATTR_SOURCE"));
-+ if (repo_get_oid_treeish(the_repository, default_attr_source_tree_object_name, attr_source)) {
-+ int allow_invalid_attr_source = 0;
-+
-+ git_config_get_bool("attr.allowinvalidsource", &allow_invalid_attr_source);
-+
-+ if (!(allow_invalid_attr_source && attr_source_from_config))
-+ die(_("bad --attr-source or GIT_ATTR_SOURCE"));
-+ }
++ if (repo_get_oid_treeish(the_repository,
++ default_attr_source_tree_object_name,
++ attr_source) && !ignore_bad_attr_tree)
+ die(_("bad --attr-source or GIT_ATTR_SOURCE"));
}
- static struct object_id *default_attr_source(void)
## t/t0003-attributes.sh ##
@@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattribute is ignored' '
)
'
-+bad_attr_source_err="fatal: bad --attr-source or GIT_ATTR_SOURCE"
+
-+test_expect_success 'attr.allowInvalidSource when HEAD is unborn' '
-+ test_when_finished rm -rf empty &&
-+ echo $bad_attr_source_err >expect_err &&
-+ echo "f/path: test: unspecified" >expect &&
-+ git init empty &&
-+ test_must_fail git -C empty --attr-source=HEAD check-attr test -- f/path 2>err &&
-+ test_cmp expect_err err &&
-+ git -C empty -c attr.tree=HEAD -c attr.allowInvalidSource=true check-attr test -- f/path >actual 2>err &&
-+ test_must_be_empty err &&
-+ test_cmp expect actual
-+'
-+
-+test_expect_success 'attr.allowInvalidSource when --attr-source points to non-existing ref' '
-+ test_when_finished rm -rf empty &&
-+ echo $bad_attr_source_err >expect_err &&
-+ echo "f/path: test: unspecified" >expect &&
-+ git init empty &&
-+ test_must_fail git -C empty --attr-source=refs/does/not/exist check-attr test -- f/path 2>err &&
-+ test_cmp expect_err err &&
-+ git -C empty -c attr.tree=refs/does/not/exist -c attr.allowInvalidSource=true check-attr test -- f/path >actual 2>err &&
-+ test_must_be_empty err &&
-+ test_cmp expect actual
-+'
-+
-+test_expect_success 'bad attr source defaults to reading .gitattributes file' '
-+ test_when_finished rm -rf empty &&
-+ git init empty &&
-+ echo "f/path test=val" >empty/.gitattributes &&
++test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '
++ test_when_finished rm -rf test bare_with_gitattribute &&
++ git init test &&
++ (
++ cd test &&
++ test_commit gitattributes .gitattributes "f/path test=val"
++ ) &&
++ git clone --bare test bare_with_gitattribute &&
+ echo "f/path: test: val" >expect &&
-+ git -C empty -c attr.tree=HEAD -c attr.allowInvalidSource=true check-attr test -- f/path >actual 2>err &&
-+ test_must_be_empty err &&
++ git -C bare_with_gitattribute check-attr test -- f/path >actual &&
+ test_cmp expect actual
+'
-+
-+test_expect_success 'attr.allowInvalidSource has no effect on --attr-source' '
-+ test_when_finished rm -rf empty &&
-+ echo $bad_attr_source_err >expect_err &&
-+ echo "f/path: test: unspecified" >expect &&
-+ git init empty &&
-+ test_must_fail git -C empty -c attr.allowInvalidSource=true --attr-source=HEAD check-attr test -- f/path 2>err &&
-+ test_cmp expect_err err
-+'
+
test_expect_success 'bare repository: with --source' '
(
cd bare.git &&
+
+ ## t/t5001-archive-attr.sh ##
+@@ t/t5001-archive-attr.sh: test_expect_success 'git archive with worktree attributes, bare' '
+ '
+
+ test_expect_missing bare-worktree/ignored
+-test_expect_exists bare-worktree/ignored-by-tree
++test_expect_missing bare-worktree/ignored-by-tree
+ test_expect_exists bare-worktree/ignored-by-worktree
+
+ test_expect_success 'export-subst' '
1: 446bce03a96 ! 2: dadb822da99 attr: add attr.tree for setting the treeish to read attributes from
@@ Metadata
## Commit message ##
attr: add attr.tree for setting the treeish to read attributes from
- 44451a2e5e (attr: teach "--attr-source=<tree>" global option to "git",
+ 44451a2 (attr: teach "--attr-source=<tree>" global option to "git",
2023-05-06) provided the ability to pass in a treeish as the attr
source. In the context of serving Git repositories as bare repos like we
do at GitLab however, it would be easier to point --attr-source to HEAD
@@ Documentation/config/attr.txt (new)
@@
+attr.tree:
+ A <tree-ish> to read gitattributes from instead of the worktree. See
-+ linkgit:gitattributes[5]. This is equivalent to setting the
-+ `GIT_ATTR_SOURCE` environment variable, or passing in --attr-source to
-+ the Git command.
++ linkgit:gitattributes[5]. If `attr.tree` does not resolve to a valid tree,
++ treat it as an empty tree. --attr-source and GIT_ATTR_SOURCE take
++ precedence over attr.tree.
## attr.c ##
+@@
+ #include "tree-walk.h"
+ #include "object-name.h"
+
++const char *git_attr_tree;
++
+ const char git_attr__true[] = "(builtin)true";
+ const char git_attr__false[] = "\0(builtin)false";
+ static const char git_attr__unknown[] = "(builtin)unknown";
@@ attr.c: static void compute_default_attr_source(struct object_id *attr_source)
if (!default_attr_source_tree_object_name)
default_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);
+ if (!default_attr_source_tree_object_name) {
-+ char *attr_tree;
-+
-+ if (!git_config_get_string("attr.tree", &attr_tree))
-+ default_attr_source_tree_object_name = attr_tree;
++ default_attr_source_tree_object_name = git_attr_tree;
++ ignore_bad_attr_tree = 1;
+ }
+
- if (!default_attr_source_tree_object_name || !is_null_oid(attr_source))
- return;
+ if (!default_attr_source_tree_object_name &&
+ startup_info->have_repository &&
+ is_bare_repository()) {
+
+ ## attr.h ##
+@@ attr.h: const char *git_attr_global_file(void);
+ /* Return whether the system gitattributes file is enabled and should be used. */
+ int git_attr_system_is_enabled(void);
+
++extern const char *git_attr_tree;
++
+ #endif /* ATTR_H */
+
+ ## config.c ##
+@@
+ #include "repository.h"
+ #include "lockfile.h"
+ #include "mailmap.h"
++#include "attr.h"
+ #include "exec-cmd.h"
+ #include "strbuf.h"
+ #include "quote.h"
+@@ config.c: static int git_default_mailmap_config(const char *var, const char *value)
+ return 0;
+ }
+
++static int git_default_attr_config(const char *var, const char *value)
++{
++ if (!strcmp(var, "attr.tree"))
++ return git_config_string(&git_attr_tree, var, value);
++
++ /* Add other attribute related config variables here and to
++ Documentation/config/attr.txt. */
++ return 0;
++}
++
+ int git_default_config(const char *var, const char *value,
+ const struct config_context *ctx, void *cb)
+ {
+@@ config.c: int git_default_config(const char *var, const char *value,
+ if (starts_with(var, "mailmap."))
+ return git_default_mailmap_config(var, value);
+
++ if (starts_with(var, "attr."))
++ return git_default_attr_config(var, value);
++
+ if (starts_with(var, "advice.") || starts_with(var, "color.advice"))
+ return git_default_advice_config(var, value);
## t/t0003-attributes.sh ##
@@ t/t0003-attributes.sh: attr_check_source () {
GIT_ATTR_SOURCE="$source" git $git_opts check-attr test -- "$path" >actual 2>err &&
test_cmp expect actual &&
test_must_be_empty err
+@@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattribute is ignored' '
+ )
+ '
+
++bad_attr_source_err="fatal: bad --attr-source or GIT_ATTR_SOURCE"
++
++test_expect_success 'attr.tree when HEAD is unborn' '
++ test_when_finished rm -rf empty &&
++ git init empty &&
++ (
++ cd empty &&
++ echo $bad_attr_source_err >expect_err &&
++ echo "f/path: test: unspecified" >expect &&
++ git -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&
++ test_must_be_empty err &&
++ test_cmp expect actual
++ )
++'
++
++test_expect_success 'attr.tree points to non-existing ref' '
++ test_when_finished rm -rf empty &&
++ git init empty &&
++ (
++ cd empty &&
++ echo $bad_attr_source_err >expect_err &&
++ echo "f/path: test: unspecified" >expect &&
++ git -c attr.tree=refs/does/not/exist check-attr test -- f/path >actual 2>err &&
++ test_must_be_empty err &&
++ test_cmp expect actual
++ )
++'
++
++test_expect_success 'bad attr source defaults to reading .gitattributes file' '
++ test_when_finished rm -rf empty &&
++ git init empty &&
++ (
++ cd empty &&
++ echo "f/path test=val" >.gitattributes &&
++ echo "f/path: test: val" >expect &&
++ git -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&
++ test_must_be_empty err &&
++ test_cmp expect actual
++ )
++'
+
+ test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '
+ test_when_finished rm -rf test bare_with_gitattribute &&
+@@ t/t0003-attributes.sh: test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '
+ test_cmp expect actual
+ '
+
++test_expect_success '--attr-source and GIT_ATTR_SOURCE take precedence over attr.tree' '
++ test_when_finished rm -rf empty &&
++ git init empty &&
++ (
++ cd empty &&
++ git checkout -b attr-source &&
++ test_commit "val1" .gitattributes "f/path test=val1" &&
++ git checkout -b attr-tree &&
++ test_commit "val2" .gitattributes "f/path test=val2" &&
++ git checkout attr-source &&
++ echo "f/path: test: val1" >expect &&
++ git -c attr.tree=attr-tree --attr-source=attr-source check-attr test -- f/path >actual &&
++ test_cmp expect actual &&
++ GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree check-attr test -- f/path >actual &&
++ test_cmp expect actual
++ )
++'
++
+ test_expect_success 'bare repository: with --source' '
+ (
+ cd bare.git &&Signature · 1 line
gitgitgadget
PATCH v3 1/2 attr: read attributes from HEAD when bare repo
From: John Cai <[email protected]>
The motivation for 44451a2e5e (attr: teach "--attr-source=<tree>" global option to "git" , 2023-05-06), was to make it possible to use gitattributes with bare repositories.
To make it easier to read gitattributes in bare repositories however, let's just make HEAD:.gitattributes the default. This is in line with how mailmap works, 8c473cecfd (mailmap: default mailmap.blob in bare repositories, 2012-12-13).
--- attr.c | 12 +++++++++++- t/t0003-attributes.sh | 14 ++++++++++++++ t/t5001-archive-attr.sh | 2 +- 3 files changed, 26 insertions(+), 2 deletions(-)
attr.c +11 -1
@@ -1194,6 +1194,7 @@ static void collect_some_attrs(struct index_state *istate, }static const char *default_attr_source_tree_object_name; +static int ignore_bad_attr_tree;void set_git_attr_source (const char *tree_object_name) {
@@ -1205,10 +1206,19 @@ static void compute_default_attr_source(struct object_id *attr_source)if (!default_attr_source_tree_object_name) default_attr_source_tree_object_name =getenv (GIT_ATTR_SOURCE_ENVIRONMENT ); +if (!default_attr_source_tree_object_name && + startup_info->have_repository && +is_bare_repository ()) { + default_attr_source_tree_object_name ="HEAD" ; + ignore_bad_attr_tree =1 ; + } +if (!default_attr_source_tree_object_name || !is_null_oid (attr_source))return ; -if (repo_get_oid_treeish (the_repository, default_attr_source_tree_object_name, attr_source)) +if (repo_get_oid_treeish (the_repository, + default_attr_source_tree_object_name, + attr_source) && !ignore_bad_attr_tree)die (_ ("bad --attr-source or GIT_ATTR_SOURCE" )); }
t/t0003-attributes.sh +14 -0
@@ -342,6 +342,20 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' ' ) ' + +test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' ' + test_when_finished rm -rf test bare_with_gitattribute && + git init test && + ( + cd test && + test_commit gitattributes .gitattributes "f/path test=val" + ) && + git clone --bare test bare_with_gitattribute && + echo "f/path: test: val" >expect && + git -C bare_with_gitattribute check-attr test -- f/path >actual && + test_cmp expect actual +' + test_expect_success 'bare repository: with --source' ' ( cd bare.git &&
t/t5001-archive-attr.sh +1 -1
@@ -138,7 +138,7 @@ test_expect_success 'git archive with worktree attributes, bare' ' ' test_expect_missing bare-worktree/ignored -test_expect_exists bare-worktree/ignored-by-tree +test_expect_missing bare-worktree/ignored-by-tree test_expect_exists bare-worktree/ignored-by-worktree test_expect_success 'export-subst' '
Signature · 1 line
gitgitgadget
Re: [PATCH v3 1/2] attr: read attributes from HEAD when bare repo
Quoted 13 lines · John Cai via GitGitGadget
The motivation for
44451a2e5e(attr: teach "--attr-source=<tree>" global option to "git" , 2023-05-06), was to make it possible to use gitattributes with bare repositories.To make it easier to read gitattributes in bare repositories however, let's just make HEAD:.gitattributes the default. This is in line with how mailmap works,
8c473cecfd(mailmap: default mailmap.blob in bare repositories, 2012-12-13).Signed-off-by John Cai <[email protected]>---
t/t0003-attributes.sh test_expect_success 'bare repository: check that .gitattribute is ignored' ' · line 348
Quoted 7 lines
t/t0003-attributes.sh +7
-0@@ -342,6 +342,20 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' ' +test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' ' + test_when_finished rm -rf test bare_with_gitattribute && + git init test && + ( + cd test && + test_commit gitattributes .gitattributes "f/path test=val" + ) &&
Not at all worth a reroll, rather just for future reference... these days test_commit() supports -C, so the same could be accomplished without the subshell or cd:
test_commit -C test gitattributes .gitattributes "f/path test=val" &&
Quoted 5 lines
t/t0003-attributes.sh +5
-0+ git clone --bare test bare_with_gitattribute && + echo "f/path: test: val" >expect && + git -C bare_with_gitattribute check-attr test -- f/path >actual && + test_cmp expect actual +'
PATCH v3 2/2 attr: add attr.tree for setting the treeish to read attributes from
In reply to John Cai via GitGitGadgetFrom: John Cai <[email protected]>
44451a2 (attr: teach "--attr-source=<tree>" global option to "git", 2023-05-06) provided the ability to pass in a treeish as the attr source. In the context of serving Git repositories as bare repos like we do at GitLab however, it would be easier to point --attr-source to HEAD for all commands by setting it once.
Add a new config attr.tree that allows this.
--- Documentation/config.txt | 2 ++ Documentation/config/attr.txt | 5 +++ attr.c | 7 ++++ attr.h | 2 ++ config.c | 14 ++++++++ t/t0003-attributes.sh | 62 +++++++++++++++++++++++++++++++++++ 6 files changed, 92 insertions(+) create mode 100644 Documentation/config/attr.txt
Documentation/config.txt +2 -0
@@ -371,6 +371,8 @@ other popular tools, and describe them in your documentation. include::config/advice.txt[] +include::config/attr.txt[] + include::config/core.txt[] include::config/add.txt[]
Documentation/config/attr.txt new file +5 -0
@@ -0,0 +1,5 @@ +attr.tree: + A <tree-ish> to read gitattributes from instead of the worktree. See + linkgit:gitattributes[5]. If `attr.tree` does not resolve to a valid tree, + treat it as an empty tree. --attr-source and GIT_ATTR_SOURCE take + precedence over attr.tree.
attr.c +7 -0
@@ -24,6 +24,8 @@#include "tree-walk.h" #include "object-name.h" +const char *git_attr_tree; +const char git_attr__true[] ="(builtin)true" ;const char git_attr__false[] ="\0(builtin)false" ;static const char git_attr__unknown[] ="(builtin)unknown" ;
@@ -1206,6 +1208,11 @@ static void compute_default_attr_source(struct object_id *attr_source)if (!default_attr_source_tree_object_name) default_attr_source_tree_object_name =getenv (GIT_ATTR_SOURCE_ENVIRONMENT ); +if (!default_attr_source_tree_object_name) { + default_attr_source_tree_object_name = git_attr_tree; + ignore_bad_attr_tree =1 ; + } +if (!default_attr_source_tree_object_name && startup_info->have_repository &&is_bare_repository ()) {
attr.h +2 -0
@@ -236,4 +236,6 @@ const char *git_attr_global_file(void);/* Return whether the system gitattributes file is enabled and should be used. */ int git_attr_system_is_enabled (void ); +extern const char *git_attr_tree; +#endif /* ATTR_H */
config.c +14 -0
@@ -18,6 +18,7 @@#include "repository.h" #include "lockfile.h" #include "mailmap.h" +#include "attr.h" #include "exec-cmd.h" #include "strbuf.h" #include "quote.h"
@@ -1904,6 +1905,16 @@ static int git_default_mailmap_config(const char *var, const char *value)return 0 ; } +static int git_default_attr_config (const char *var,const char *value) +{ +if (!strcmp (var,"attr.tree" )) +return git_config_string (&git_attr_tree, var, value); + +/* Add other attribute related config variables here and to +Documentation/config/attr.txt. */ +return 0 ; +} +int git_default_config (const char *var,const char *value,const struct config_context *ctx,void *cb) {
@@ -1927,6 +1938,9 @@ int git_default_config(const char *var, const char *value,if (starts_with (var,"mailmap." ))return git_default_mailmap_config (var, value); +if (starts_with (var,"attr." )) +return git_default_attr_config (var, value); +if (starts_with (var,"advice." ) ||starts_with (var,"color.advice" ))return git_default_advice_config (var, value);
t/t0003-attributes.sh +62 -0
80 lines not on this page · raw message
Signature · 1 line
gitgitgadget
Re: [PATCH v3 2/2] attr: add attr.tree for setting the treeish to read attributes from
Quoted 40 lines · John Cai via GitGitGadget
From: John Cai <[email protected]>
44451a2 (attr: teach "--attr-source=<tree>" global option to "git", 2023-05-06) provided the ability to pass in a treeish as the attr source. In the context of serving Git repositories as bare repos like we do at GitLab however, it would be easier to point --attr-source to HEAD for all commands by setting it once.
Add a new config attr.tree that allows this.
Signed-off-by John Cai <[email protected]>--- Documentation/config.txt | 2 ++ Documentation/config/attr.txt | 5 +++ attr.c | 7 ++++ attr.h | 2 ++ config.c | 14 ++++++++ t/t0003-attributes.sh | 62 +++++++++++++++++++++++++++++++++++ 6 files changed, 92 insertions(+) create mode 100644 Documentation/config/attr.txtDocumentation/config.txt +2
-0@@ -371,6 +371,8 @@ other popular tools, and describe them in your documentation. include::config/advice.txt[] +include::config/attr.txt[] + include::config/core.txt[] include::config/add.txt[]Documentation/config/attr.txt new file +0
-0
Documentation/config/attr.txt line 5
Quoted 5 lines
Documentation/config/attr.txt new file +5
-0@@ -0,0 +1,5 @@ +attr.tree: + A <tree-ish> to read gitattributes from instead of the worktree. See + linkgit:gitattributes[5]. If `attr.tree` does not resolve to a valid tree, + treat it as an empty tree. --attr-source and GIT_ATTR_SOURCE take + precedence over attr.tree.
Properly typeset --attr-source and GIT_ATTR_SOURCE.
A quick "git grep" in Documentation/config/*.txt tells me that nobody refers to an object type like <tree-ish>. Imitate what this was modeled after, namely Documentation/config/mailmap.txt, which says just
... a reference to a blob in the repository.
without any half mark-up.
More importantly, the description makes one wonder what the precedence rule between these two (the general rule would be for command line parameter to override environment, if I recall correctly).
I think the enumeration header usually is followed by double-colons among Documentation/config/*.txt files. Let's be consistent.
In the context of this expression, "worktree" is a wrong noun to use---the term of art refers to an instance of "working tree", together with some "per worktree" administrative files inside .git/ directory. On the other hand, "working tree" refers to the "files meant to be visible to build tools and editors part of the non-bare repository", which is what you want to use here.
attr.tree::
A tree object to read the attributes from, instead of the .gitattributes file in the working tree. In a bare repository, this defaults to HEAD:.gitattributes". If a given value does not resolve to a valid tree object, an empty tree is used instead. When GIT_ATTR_SOURCE environment variable or --attr-source command line option is used, this configuration variable has no effect.
or something along that line, perhaps.
Quoted 14 lines
attr.c +2
-0@@ -24,6 +24,8 @@#include "tree-walk.h" #include "object-name.h" +const char *git_attr_tree; +const char git_attr__true[] ="(builtin)true" ;const char git_attr__false[] ="\0(builtin)false" ;static const char git_attr__unknown[] ="(builtin)unknown" ;
attr.c static void compute_default_attr_source(struct object_id *attr_source) · line 1214
Quoted 7 lines
attr.c +4
-0@@ -1206,6 +1208,11 @@ static void compute_default_attr_source(struct object_id *attr_source)if (!default_attr_source_tree_object_name) default_attr_source_tree_object_name =getenv (GIT_ATTR_SOURCE_ENVIRONMENT ); +if (!default_attr_source_tree_object_name) { + default_attr_source_tree_object_name = git_attr_tree; + ignore_bad_attr_tree =1 ; + }
As long as "attr.tree" was read by calling git_default_attr_config() before we come here, git_attr_tree is not NULL and we allow bad attr tree in default_attr_source_tree_object_name. But stepping back a bit, even if "attr.tree" is unspecified, i.e., git_attr_tree is NULL, we set ignore_bad_attr_tree to true here.
What it means is that after the above if() statement, if default_attr_source_tree_object_name is still NULL, we know that ignore_bad_attr_tree is already set to true.
attr.c static void compute_default_attr_source(struct object_id *attr_source) · line 1218
Quoted 3 lines
attr.c +0
-0if (!default_attr_source_tree_object_name && startup_info->have_repository &&is_bare_repository ()) {
So would it make more sense to remove the assignment to the same variable we made in [1/2] around here (not seen in the post context)?
Alternatively, even though it makes the code a bit more verbose, the logic might become clearer if you wrote the "assign from the config" part like so:
if (!default_attr_source_tree_object_name && git_attr_tree) {
default_attr_source_tree_object_name = git_attr_tree;
ignore_bad_attr_tree = 1;
}It would leave more flexibility to the code around here. You could for example add code that assigns a different value, a tree object that is required to exist, to default_attr_source_tree_object_name after this point, for example, without having to wonder what the "current" value of ignore_bad_attr_tree is.
attr.c static void compute_default_attr_source(struct object_id *attr_source)
Quoted 7 lines
attr.c +7
-0+static int git_default_attr_config (const char *var,const char *value) +{ +if (!strcmp (var,"attr.tree" )) +return git_config_string (&git_attr_tree, var, value); + +/* Add other attribute related config variables here and to +Documentation/config/attr.txt. */
/*
* Our multi-line comments should look
* more like this; opening slash-asterisk
* and closing asterisk-slash sit on a line
* on its own.
*/
attr.c test_expect_success 'bare repository: check that .gitattribute is ignored' ' · line 349
Quoted 5 lines
attr.c +1
-0@@ -342,6 +346,46 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' ' ) ' +bad_attr_source_err="fatal: bad --attr-source or GIT_ATTR_SOURCE"
Not a fault of this two-patch series, but we probably should refine this error reporting so that the reader can tell which one is being complained about, and optionally what the offending value was.
attr.c test_expect_success 'bare repository: check that .gitattribute is ignored' ' · line 355
Quoted 6 lines
attr.c +6
-0+test_expect_success 'attr.tree when HEAD is unborn' ' + test_when_finished rm -rf empty && + git init empty && + ( + cd empty && + echo $bad_attr_source_err >expect_err &&
Let's not rely on words in the error message split with exactly one whitespace each and instead quote the variable properly. I.e.,
echo "$bad_attr_source_err" >expect_err &&
But this is not even used, as we do not expect it to fail. Perhaps remove it altogether?
attr.c test_expect_success 'bare repository: check that .gitattribute is ignored' ' · line 368
Quoted 13 lines
attr.c +13
-0+ echo"f/path: test: unspecified" >expect && + git -c attr.tree=HEAD check-attr test -- f/path >actual 2>err && + test_must_be_empty err && + test_cmp expect actual + ) +' + +test_expect_success 'attr.tree points to non-existing ref' ' + test_when_finished rm -rf empty && + git init empty && + ( + cd empty && + echo $bad_attr_source_err >expect_err &&
Ditto.
Quoted 9 lines
attr.c +9
-0+ echo"f/path: test: unspecified" >expect && + git -c attr.tree=refs/does/not/exist check-attr test -- f/path >actual 2>err && + test_must_be_empty err && + test_cmp expect actual + ) +' + +test_expect_success 'bad attr source defaults to reading .gitattributes file' ' + test_when_finished rm -rf empty &&
attr.c test_expect_success 'bare repository: check that .gitattribute is ignored' ' · line 387
Quoted 10 lines
attr.c +10
-0+ git init empty && + ( + cd empty && + echo "f/path test=val" >.gitattributes && + echo"f/path: test: val" >expect && + git -c attr.tree=HEAD check-attr test -- f/path >actual 2>err && + test_must_be_empty err && + test_cmp expect actual + ) +'
In other words, with the additional tests, we do not check error cases (which may be perfectly OK, if they are covered by existing tests). A bit curious.
attr.c test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' ' · line 403
Quoted 5 lines
attr.c +1
-0@@ -356,6 +400,24 @@ test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' ' test_cmp expect actual ' +test_expect_success '--attr-source and GIT_ATTR_SOURCE take precedence over attr.tree' '
Do we want to ensure which one takes precedence between the command line option and the environment? It's not like the one that is given the last takes effect.
Quoted 10 lines
attr.c +10
-0+ test_when_finished rm -rf empty && + git init empty && + ( + cd empty && + git checkout -b attr-source && + test_commit "val1" .gitattributes "f/path test=val1" && + git checkout -b attr-tree && + test_commit "val2" .gitattributes "f/path test=val2" && + git checkout attr-source && + echo "f/path: test: val1" >expect &&
attr.c test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' ' · line 423
Quoted 10 lines
attr.c +7
-0+ git -c attr.tree=attr-tree --attr-source=attr-source check-attr test -- f/path >actual && + test_cmp expect actual && + GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree check-attr test -- f/path >actual && + test_cmp expect actual + ) +' + test_expect_success 'bare repository: with --source' ' ( cd bare.git &&
Other than that, looking great. Thanks.
Re: [PATCH v3 2/2] attr: add attr.tree for setting the treeish to read attributes from
Hi Junio,
Quoted 50 lines · Junio C Hamano
Quoted 40 lines · John Cai via GitGitGadget
From: John Cai <[email protected]>
44451a2 (attr: teach "--attr-source=<tree>" global option to "git", 2023-05-06) provided the ability to pass in a treeish as the attr source. In the context of serving Git repositories as bare repos like we do at GitLab however, it would be easier to point --attr-source to HEAD for all commands by setting it once.
Add a new config attr.tree that allows this.
Signed-off-by John Cai <[email protected]>--- Documentation/config.txt | 2 ++ Documentation/config/attr.txt | 5 +++ attr.c | 7 ++++ attr.h | 2 ++ config.c | 14 ++++++++ t/t0003-attributes.sh | 62 +++++++++++++++++++++++++++++++++++ 6 files changed, 92 insertions(+) create mode 100644 Documentation/config/attr.txtDocumentation/config.txt +2
-0@@ -371,6 +371,8 @@ other popular tools, and describe them in your documentation. include::config/advice.txt[] +include::config/attr.txt[] + include::config/core.txt[] include::config/add.txt[]Documentation/config/attr.txt new file +0
-0Documentation/config/attr.txt line 5
Quoted 5 lines
Documentation/config/attr.txt new file +5
-0@@ -0,0 +1,5 @@ +attr.tree: + A <tree-ish> to read gitattributes from instead of the worktree. See + linkgit:gitattributes[5]. If `attr.tree` does not resolve to a valid tree, + treat it as an empty tree. --attr-source and GIT_ATTR_SOURCE take + precedence over attr.tree.Properly typeset
--attr-sourceandGIT_ATTR_SOURCE.
Quoted 8 lines
A quick "git grep" in
Documentation/config/*.txttells me that nobody refers to an object type like <tree-ish>. Imitate what this was modeled after, namelyDocumentation/config/mailmap.txt, which says just... a reference to a blob in the repository.
without any half mark-up.
sounds good
Quoted 5 lines
More importantly, the description makes one wonder what the precedence rule between these two (the general rule would be for command line parameter to override environment, if I recall correctly).
yes, that should be the case.
Quoted 20 lines
I think the enumeration header usually is followed by double-colons among
Documentation/config/*.txtfiles. Let's be consistent.In the context of this expression, "worktree" is a wrong noun to use---the term of art refers to an instance of "working tree", together with some "per worktree" administrative files inside .git/ directory. On the other hand, "working tree" refers to the "files meant to be visible to build tools and editors part of the non-bare repository", which is what you want to use here.
attr.tree::
A tree object to read the attributes from, instead of the.gitattributesfile in the working tree. In a bare repository, this defaults to HEAD:.gitattributes". If a given value does not resolve to a valid tree object, an empty tree is used instead. WhenGIT_ATTR_SOURCEenvironment variable or--attr-sourcecommand line option is used, this configuration variable has no effect.
Quoted 1 line
or something along that line, perhaps.
sounds good to me, thanks
Quoted 50 lines
Quoted 15 lines
attr.c +2
-0@@ -24,6 +24,8 @@#include "tree-walk.h" #include "object-name.h" +const char *git_attr_tree; +const char git_attr__true[] ="(builtin)true" ;const char git_attr__false[] ="\0(builtin)false" ;static const char git_attr__unknown[] ="(builtin)unknown" ;attr.c static void compute_default_attr_source(struct object_id *attr_source) · line 1214
Quoted 7 lines
attr.c +4
-0@@ -1206,6 +1208,11 @@ static void compute_default_attr_source(struct object_id *attr_source)if (!default_attr_source_tree_object_name) default_attr_source_tree_object_name =getenv (GIT_ATTR_SOURCE_ENVIRONMENT ); +if (!default_attr_source_tree_object_name) { + default_attr_source_tree_object_name = git_attr_tree; + ignore_bad_attr_tree =1 ; + }As long as "attr.tree" was read by calling
git_default_attr_config()before we come here,git_attr_treeis not NULL and we allow bad attr tree indefault_attr_source_tree_object_name. But stepping back a bit, even if "attr.tree" is unspecified, i.e.,git_attr_treeis NULL, we setignore_bad_attr_treeto true here.What it means is that after the above
if()statement, ifdefault_attr_source_tree_object_nameis still NULL, we know thatignore_bad_attr_treeis already set to true.attr.c static void compute_default_attr_source(struct object_id *attr_source) · line 1218
Quoted 3 lines
attr.c +0
-0if (!default_attr_source_tree_object_name && startup_info->have_repository &&is_bare_repository ()) {So would it make more sense to remove the assignment to the same variable we made in [1/2] around here (not seen in the post context)?
Alternatively, even though it makes the code a bit more verbose, the logic might become clearer if you wrote the "assign from the config" part like so:
if (!default_attr_source_tree_object_name && git_attr_tree) { default_attr_source_tree_object_name = git_attr_tree; ignore_bad_attr_tree = 1; }
Quoted 5 lines
It would leave more flexibility to the code around here. You could for example add code that assigns a different value, a tree object that is required to exist, to
default_attr_source_tree_object_nameafter this point, for example, without having to wonder what the "current" value ofignore_bad_attr_treeis.
good point--I think this logic is easier to follow.
Quoted 73 lines
attr.c static void compute_default_attr_source(struct object_id *attr_source)
Quoted 8 lines
attr.c +7
-0+static int git_default_attr_config (const char *var,const char *value) +{ +if (!strcmp (var,"attr.tree" )) +return git_config_string (&git_attr_tree, var, value); + +/* Add other attribute related config variables here and to +Documentation/config/attr.txt. */ /*
* Our multi-line comments should look
* more like this; opening slash-asterisk
* and closing asterisk-slash sit on a line
* on its own.
*/attr.c test_expect_success 'bare repository: check that .gitattribute is ignored' ' · line 349
Quoted 5 lines
attr.c +1
-0@@ -342,6 +346,46 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' ' ) ' +bad_attr_source_err="fatal: bad --attr-source or GIT_ATTR_SOURCE" Not a fault of this two-patch series, but we probably should refine this error reporting so that the reader can tell which one is being complained about, and optionally what the offending value was.
attr.c test_expect_success 'bare repository: check that .gitattribute is ignored' ' · line 355
Quoted 6 lines
attr.c +6
-0+test_expect_success 'attr.tree when HEAD is unborn' ' + test_when_finished rm -rf empty && + git init empty && + ( + cd empty && + echo $bad_attr_source_err >expect_err &&Let's not rely on words in the error message split with exactly one whitespace each and instead quote the variable properly. I.e.,
echo "$bad_attr_source_err" >expect_err &&But this is not even used, as we do not expect it to fail. Perhaps remove it altogether?
attr.c test_expect_success 'bare repository: check that .gitattribute is ignored' ' · line 368
Quoted 13 lines
attr.c +13
-0+ echo"f/path: test: unspecified" >expect && + git -c attr.tree=HEAD check-attr test -- f/path >actual 2>err && + test_must_be_empty err && + test_cmp expect actual + ) +' + +test_expect_success 'attr.tree points to non-existing ref' ' + test_when_finished rm -rf empty && + git init empty && + ( + cd empty && + echo $bad_attr_source_err >expect_err &&Ditto.
Quoted 15 lines
attr.c +15
-0+ echo"f/path: test: unspecified" >expect && + git -c attr.tree=refs/does/not/exist check-attr test -- f/path >actual 2>err && + test_must_be_empty err && + test_cmp expect actual + ) +' + +test_expect_success 'bad attr source defaults to reading .gitattributes file' ' + test_when_finished rm -rf empty && + git init empty && + ( + cd empty && + echo "f/path test=val" >.gitattributes && + echo "f/path: test: val" >expect && + git -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&
Quoted 8 lines
attr.c test_expect_success 'bare repository: check that .gitattribute is ignored' ' · line 387
Quoted 4 lines
attr.c +4
-0+ test_must_be_empty err && + test_cmp expect actual + ) +'In other words, with the additional tests, we do not check error cases (which may be perfectly OK, if they are covered by existing tests). A bit curious.
Just checked and it does seem like we don't have a test case that covers the error case. Will add in the next version.
Quoted 10 lines
attr.c test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' ' · line 403
Quoted 6 lines
attr.c +1
-0@@ -356,6 +400,24 @@ test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' ' test_cmp expect actual ' +test_expect_success '--attr-source and GIT_ATTR_SOURCE take precedence over attr.tree' 'Do we want to ensure which one takes precedence between the command line option and the environment? It's not like the one that is given the last takes effect.
yeah, might as well.
Quoted 15 lines
Quoted 15 lines
attr.c +14
-0+ test_when_finished rm -rf empty && + git init empty && + ( + cd empty && + git checkout -b attr-source && + test_commit "val1" .gitattributes "f/path test=val1" && + git checkout -b attr-tree && + test_commit "val2" .gitattributes "f/path test=val2" && + git checkout attr-source && + echo "f/path: test: val1" >expect && + git -c attr.tree=attr-tree --attr-source=attr-source check-attr test -- f/path >actual && + test_cmp expect actual && + GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree check-attr test -- f/path >actual && + test_cmp expect actual
Quoted 8 lines
attr.c test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' ' · line 423
Quoted 6 lines
attr.c +3
-0+ ) +' + test_expect_success 'bare repository: with --source' ' ( cd bare.git &&Other than that, looking great. Thanks.
thanks!
John
v4 cover attr: add attr.tree config
In reply to John Cai via GitGitGadget44451a2e5e (attr: teach "--attr-source=" global option to "git", 2023-05-06) provided the ability to pass in a treeish as the attr source. When a revision does not resolve to a valid tree is passed, Git will die. At GitLab, we server repositories as bare repos and would like to always read attributes from the default branch, so we'd like to pass in HEAD as the treeish to read gitattributes from on every command. In this context we would not want Git to die if HEAD is unborn, like in the case of empty repositories.
Instead of modifying the default behavior of --attr-source, create a new config attr.tree with which an admin can configure a ref for all commands to read gitattributes from. Also make the default tree to read from HEAD on bare repositories.
Changes since v2:
* relax the restrictions around attr.tree so that if it does not resolve to a valid treeish, ignore it.
* add a commit to default to HEAD in bare repositories
Changes since v1:
* Added a commit to add attr.tree config
John Cai (2):
attr: read attributes from HEAD when bare repo
attr: add attr.tree for setting the treeish to read attributes from
Documentation/config.txt | 2 + Documentation/config/attr.txt | 7 +++ attr.c | 19 +++++++- attr.h | 2 + config.c | 16 +++++++ t/t0003-attributes.sh | 84 +++++++++++++++++++++++++++++++++++ t/t5001-archive-attr.sh | 2 +- 7 files changed, 130 insertions(+), 2 deletions(-) create mode 100644 Documentation/config/attr.txt
base-commit: 1fc548b2d6a3596f3e1c1f8b1930d8dbd1e30bf3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1577%2Fjohn-cai%2Fjc%2Fconfig-attr-invalid-source-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1577/john-cai/jc/config-attr-invalid-source-v4
Pull-Request: https://github.com/git/git/pull/1577
Range-diff vs v3:
1: cef206d47c7 ! 1: eaa27c47810 attr: read attributes from HEAD when bare repo
@@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
+test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '
+ test_when_finished rm -rf test bare_with_gitattribute &&
+ git init test &&
-+ (
-+ cd test &&
-+ test_commit gitattributes .gitattributes "f/path test=val"
-+ ) &&
++ test_commit -C test gitattributes .gitattributes "f/path test=val" &&
+ git clone --bare test bare_with_gitattribute &&
+ echo "f/path: test: val" >expect &&
+ git -C bare_with_gitattribute check-attr test -- f/path >actual &&
2: dadb822da99 ! 2: 749d8a8082e attr: add attr.tree for setting the treeish to read attributes from
@@ Documentation/config.txt: other popular tools, and describe them in your documen
## Documentation/config/attr.txt (new) ##
@@
-+attr.tree:
-+ A <tree-ish> to read gitattributes from instead of the worktree. See
-+ linkgit:gitattributes[5]. If `attr.tree` does not resolve to a valid tree,
-+ treat it as an empty tree. --attr-source and GIT_ATTR_SOURCE take
-+ precedence over attr.tree.
++attr.tree::
++ A reference to a tree in the repository from which to read attributes,
++ instead of the `.gitattributes` file in the working tree. In a bare
++ repository, this defaults to `HEAD:.gitattributes`. If the value does
++ not resolve to a valid tree object, an empty tree is used instead.
++ When the `GIT_ATTR_SOURCE` environment variable or `--attr-source`
++ command line option are used, this configuration variable has no effect.
## attr.c ##
@@
@@ attr.c: static void compute_default_attr_source(struct object_id *attr_source)
if (!default_attr_source_tree_object_name)
default_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);
-+ if (!default_attr_source_tree_object_name) {
++ if (!default_attr_source_tree_object_name && git_attr_tree) {
+ default_attr_source_tree_object_name = git_attr_tree;
+ ignore_bad_attr_tree = 1;
+ }
@@ config.c: static int git_default_mailmap_config(const char *var, const char *val
+ if (!strcmp(var, "attr.tree"))
+ return git_config_string(&git_attr_tree, var, value);
+
-+ /* Add other attribute related config variables here and to
-+ Documentation/config/attr.txt. */
++ /*
++ * Add other attribute related config variables here and to
++ * Documentation/config/attr.txt.
++ */
+ return 0;
+}
+
@@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
+bad_attr_source_err="fatal: bad --attr-source or GIT_ATTR_SOURCE"
+
++test_expect_success '--attr-source is bad' '
++ test_when_finished rm -rf empty &&
++ git init empty &&
++ (
++ cd empty &&
++ echo "$bad_attr_source_err" >expect_err &&
++ test_must_fail git --attr-source=HEAD check-attr test -- f/path 2>err &&
++ test_cmp expect_err err
++ )
++'
++
+test_expect_success 'attr.tree when HEAD is unborn' '
+ test_when_finished rm -rf empty &&
+ git init empty &&
+ (
+ cd empty &&
-+ echo $bad_attr_source_err >expect_err &&
+ echo "f/path: test: unspecified" >expect &&
+ git -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&
+ test_must_be_empty err &&
@@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
+ git init empty &&
+ (
+ cd empty &&
-+ echo $bad_attr_source_err >expect_err &&
+ echo "f/path: test: unspecified" >expect &&
+ git -c attr.tree=refs/does/not/exist check-attr test -- f/path >actual 2>err &&
+ test_must_be_empty err &&
@@ t/t0003-attributes.sh: test_expect_success 'bare repo defaults to reading .gitat
test_cmp expect actual
'
-+test_expect_success '--attr-source and GIT_ATTR_SOURCE take precedence over attr.tree' '
++test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tree' '
+ test_when_finished rm -rf empty &&
+ git init empty &&
+ (
@@ t/t0003-attributes.sh: test_expect_success 'bare repo defaults to reading .gitat
+ test_commit "val2" .gitattributes "f/path test=val2" &&
+ git checkout attr-source &&
+ echo "f/path: test: val1" >expect &&
-+ git -c attr.tree=attr-tree --attr-source=attr-source check-attr test -- f/path >actual &&
++ GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree --attr-source=attr-source \
++ check-attr test -- f/path >actual &&
+ test_cmp expect actual &&
-+ GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree check-attr test -- f/path >actual &&
++ GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree \
++ check-attr test -- f/path >actual &&
+ test_cmp expect actual
+ )
+'Signature · 1 line
gitgitgadget
PATCH v4 1/2 attr: read attributes from HEAD when bare repo
From: John Cai <[email protected]>
The motivation for 44451a2e5e (attr: teach "--attr-source=<tree>" global option to "git" , 2023-05-06), was to make it possible to use gitattributes with bare repositories.
To make it easier to read gitattributes in bare repositories however, let's just make HEAD:.gitattributes the default. This is in line with how mailmap works, 8c473cecfd (mailmap: default mailmap.blob in bare repositories, 2012-12-13).
--- attr.c | 12 +++++++++++- t/t0003-attributes.sh | 11 +++++++++++ t/t5001-archive-attr.sh | 2 +- 3 files changed, 23 insertions(+), 2 deletions(-)
attr.c +11 -1
@@ -1194,6 +1194,7 @@ static void collect_some_attrs(struct index_state *istate, }static const char *default_attr_source_tree_object_name; +static int ignore_bad_attr_tree;void set_git_attr_source (const char *tree_object_name) {
@@ -1205,10 +1206,19 @@ static void compute_default_attr_source(struct object_id *attr_source)if (!default_attr_source_tree_object_name) default_attr_source_tree_object_name =getenv (GIT_ATTR_SOURCE_ENVIRONMENT ); +if (!default_attr_source_tree_object_name && + startup_info->have_repository && +is_bare_repository ()) { + default_attr_source_tree_object_name ="HEAD" ; + ignore_bad_attr_tree =1 ; + } +if (!default_attr_source_tree_object_name || !is_null_oid (attr_source))return ; -if (repo_get_oid_treeish (the_repository, default_attr_source_tree_object_name, attr_source)) +if (repo_get_oid_treeish (the_repository, + default_attr_source_tree_object_name, + attr_source) && !ignore_bad_attr_tree)die (_ ("bad --attr-source or GIT_ATTR_SOURCE" )); }
t/t0003-attributes.sh +11 -0
@@ -342,6 +342,17 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' ' ) ' + +test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' ' + test_when_finished rm -rf test bare_with_gitattribute && + git init test && + test_commit -C test gitattributes .gitattributes "f/path test=val" && + git clone --bare test bare_with_gitattribute && + echo "f/path: test: val" >expect && + git -C bare_with_gitattribute check-attr test -- f/path >actual && + test_cmp expect actual +' + test_expect_success 'bare repository: with --source' ' ( cd bare.git &&
t/t5001-archive-attr.sh +1 -1
@@ -138,7 +138,7 @@ test_expect_success 'git archive with worktree attributes, bare' ' ' test_expect_missing bare-worktree/ignored -test_expect_exists bare-worktree/ignored-by-tree +test_expect_missing bare-worktree/ignored-by-tree test_expect_exists bare-worktree/ignored-by-worktree test_expect_success 'export-subst' '
Signature · 1 line
gitgitgadget
PATCH v4 2/2 attr: add attr.tree for setting the treeish to read attributes from
In reply to John Cai via GitGitGadgetFrom: John Cai <[email protected]>
44451a2 (attr: teach "--attr-source=<tree>" global option to "git", 2023-05-06) provided the ability to pass in a treeish as the attr source. In the context of serving Git repositories as bare repos like we do at GitLab however, it would be easier to point --attr-source to HEAD for all commands by setting it once.
Add a new config attr.tree that allows this.
--- Documentation/config.txt | 2 + Documentation/config/attr.txt | 7 ++++ attr.c | 7 ++++ attr.h | 2 + config.c | 16 ++++++++ t/t0003-attributes.sh | 73 +++++++++++++++++++++++++++++++++++ 6 files changed, 107 insertions(+) create mode 100644 Documentation/config/attr.txt
Documentation/config.txt +2 -0
@@ -371,6 +371,8 @@ other popular tools, and describe them in your documentation. include::config/advice.txt[] +include::config/attr.txt[] + include::config/core.txt[] include::config/add.txt[]
Documentation/config/attr.txt new file +7 -0
@@ -0,0 +1,7 @@ +attr.tree:: + A reference to a tree in the repository from which to read attributes, + instead of the `.gitattributes` file in the working tree. In a bare + repository, this defaults to `HEAD:.gitattributes`. If the value does + not resolve to a valid tree object, an empty tree is used instead. + When the `GIT_ATTR_SOURCE` environment variable or `--attr-source` + command line option are used, this configuration variable has no effect.
attr.c +7 -0
@@ -24,6 +24,8 @@#include "tree-walk.h" #include "object-name.h" +const char *git_attr_tree; +const char git_attr__true[] ="(builtin)true" ;const char git_attr__false[] ="\0(builtin)false" ;static const char git_attr__unknown[] ="(builtin)unknown" ;
@@ -1206,6 +1208,11 @@ static void compute_default_attr_source(struct object_id *attr_source)if (!default_attr_source_tree_object_name) default_attr_source_tree_object_name =getenv (GIT_ATTR_SOURCE_ENVIRONMENT ); +if (!default_attr_source_tree_object_name && git_attr_tree) { + default_attr_source_tree_object_name = git_attr_tree; + ignore_bad_attr_tree =1 ; + } +if (!default_attr_source_tree_object_name && startup_info->have_repository &&is_bare_repository ()) {
attr.h +2 -0
@@ -236,4 +236,6 @@ const char *git_attr_global_file(void);/* Return whether the system gitattributes file is enabled and should be used. */ int git_attr_system_is_enabled (void ); +extern const char *git_attr_tree; +#endif /* ATTR_H */
config.c +16 -0
@@ -18,6 +18,7 @@#include "repository.h" #include "lockfile.h" #include "mailmap.h" +#include "attr.h" #include "exec-cmd.h" #include "strbuf.h" #include "quote.h"
@@ -1904,6 +1905,18 @@ static int git_default_mailmap_config(const char *var, const char *value)return 0 ; } +static int git_default_attr_config (const char *var,const char *value) +{ +if (!strcmp (var,"attr.tree" )) +return git_config_string (&git_attr_tree, var, value); + +/* +* Add other attribute related config variables here and to +* Documentation/config/attr.txt. +*/ +return 0 ; +} +int git_default_config (const char *var,const char *value,const struct config_context *ctx,void *cb) {
@@ -1927,6 +1940,9 @@ int git_default_config(const char *var, const char *value,if (starts_with (var,"mailmap." ))return git_default_mailmap_config (var, value); +if (starts_with (var,"attr." )) +return git_default_attr_config (var, value); +if (starts_with (var,"advice." ) ||starts_with (var,"color.advice" ))return git_default_advice_config (var, value);
t/t0003-attributes.sh +73 -0
91 lines not on this page · raw message
Signature · 1 line
gitgitgadget
Re: [PATCH v4 0/2] attr: add attr.tree config
In reply to John Cai via GitGitGadgetQuoted 16 lines · John Cai via GitGitGadget
44451a2e5e(attr: teach "--attr-source=" global option to "git", 2023-05-06) provided the ability to pass in a treeish as the attr source. When a revision does not resolve to a valid tree is passed, Git will die. At GitLab, we server repositories as bare repos and would like to always read attributes from the default branch, so we'd like to pass in HEAD as the treeish to read gitattributes from on every command. In this context we would not want Git to die if HEAD is unborn, like in the case of empty repositories.Instead of modifying the default behavior of --attr-source, create a new config attr.tree with which an admin can configure a ref for all commands to read gitattributes from. Also make the default tree to read from HEAD on bare repositories.
Changes since v2:
Quoted 7 lines
* relax the restrictions around attr.tree so that if it does not resolve to a valid treeish, ignore it.
* add a commit to default to HEAD in bare repositoriesChanges since v1:
* Added a commit to add attr.tree config
THis is v4 so there must be some changes since v3 that we are missing?
Quoted 12 lines
Range-diff vs v3:
1:
cef206d47c7! 1:eaa27c47810attr: read attributes from HEAD when bare repo@@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr +test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' ' + test_when_finished rm -rf test bare_with_gitattribute && + git init test && -+ ( -+ cd test && -+ test_commit gitattributes .gitattributes "f/path test=val" -+ ) && ++ test_commit -C test gitattributes .gitattributes "f/path test=val" &&
OK.
Quoted 17 lines
2:
dadb822da99! 2:749d8a8082eattr: add attr.tree for setting the treeish to read attributes from
@@Documentation/config.txt: other popular tools, and describe them in your documen##
Documentation/config/attr.txt(new) ##
@@
-+attr.tree:
-+ A <tree-ish> to read gitattributes from instead of the worktree. See
-+ linkgit:gitattributes[5]. Ifattr.treedoes not resolve to a valid tree,
-+ treat it as an empty tree. --attr-source andGIT_ATTR_SOURCEtake
-+ precedence over attr.tree.
++attr.tree::
++ A reference to a tree in the repository from which to read attributes,
++ instead of the.gitattributesfile in the working tree. In a bare
++ repository, this defaults toHEAD:.gitattributes. If the value does
++ not resolve to a valid tree object, an empty tree is used instead.
++ When theGIT_ATTR_SOURCEenvironment variable or--attr-source
++ command line option are used, this configuration variable has no effect.
OK.
Quoted 5 lines
-+ if (!default_attr_source_tree_object_name) {
++ if (!default_attr_source_tree_object_name && git_attr_tree) {
+ default_attr_source_tree_object_name = git_attr_tree;
+ ignore_bad_attr_tree = 1;
+ }Makes sense.
Quoted 6 lines
@@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
+bad_attr_source_err="fatal: bad --attr-source or GIT_ATTR_SOURCE"
+
++test_expect_success '--attr-source is bad' '
++ test_when_finished rm -rf empty &&Quoted 8 lines
++ git init empty &&
++ (
++ cd empty &&
++ echo "$bad_attr_source_err" >expect_err &&
++ test_must_fail git --attr-source=HEAD check-attr test -- f/path 2>err &&
++ test_cmp expect_err err
++ )
++'OK. We fail when explicitly given a bad attr-source.
Quoted 9 lines
+test_expect_success 'attr.tree when HEAD is unborn' '
+ test_when_finished rm -rf empty &&
+ git init empty &&
+ (
+ cd empty &&
-+ echo $bad_attr_source_err >expect_err &&
+ echo "f/path: test: unspecified" >expect &&
+ git -c attr.tree=HEAD check-attr test -- f/path >actual 2>err &&
+ test_must_be_empty err &&But we silently ignore when given via a configuration variable.
Quoted 8 lines
@@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
+ git init empty &&
+ (
+ cd empty &&
-+ echo $bad_attr_source_err >expect_err &&
+ echo "f/path: test: unspecified" >expect &&
+ git -c attr.tree=refs/does/not/exist check-attr test -- f/path >actual 2>err &&
+ test_must_be_empty err &&Ditto. Is this any different from the above? Both points at an object that does not exist. If one were pointing at an object that does not exist (e.g., HEAD before the initial commit) and the other were pointing at an object that is not a tree-ish (e.g., a blob), then having two separate tests may make sense, but otherwise, I am not sure about the value proposition of the second test.
Quoted 15 lines
@@ t/t0003-attributes.sh: test_expect_success 'bare repo defaults to reading .gitat
test_cmp expect actual
'
-+test_expect_success '--attr-source and GIT_ATTR_SOURCE take precedence over attr.tree' '
++test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tree' '
+ test_when_finished rm -rf empty &&
+ git init empty &&
+ (
@@ t/t0003-attributes.sh: test_expect_success 'bare repo defaults to reading .gitat
+ test_commit "val2" .gitattributes "f/path test=val2" &&
+ git checkout attr-source &&
+ echo "f/path: test: val1" >expect &&
-+ git -c attr.tree=attr-tree --attr-source=attr-source check-attr test -- f/path >actual &&
++ GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree --attr-source=attr-source \Quoted 8 lines
++ check-attr test -- f/path >actual &&
+ test_cmp expect actual &&
-+ GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree check-attr test -- f/path >actual &&
++ GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree \
++ check-attr test -- f/path >actual &&
+ test_cmp expect actual
+ )
+'Looking good.
Thanks. Queued.
Re: [PATCH v4 0/2] attr: add attr.tree config
Hi Junio,
Quoted 22 lines · Junio C Hamano
Quoted 20 lines · John Cai via GitGitGadget
44451a2e5e(attr: teach "--attr-source=" global option to "git", 2023-05-06) provided the ability to pass in a treeish as the attr source. When a revision does not resolve to a valid tree is passed, Git will die. At GitLab, we server repositories as bare repos and would like to always read attributes from the default branch, so we'd like to pass in HEAD as the treeish to read gitattributes from on every command. In this context we would not want Git to die if HEAD is unborn, like in the case of empty repositories.Instead of modifying the default behavior of --attr-source, create a new config attr.tree with which an admin can configure a ref for all commands to read gitattributes from. Also make the default tree to read from HEAD on bare repositories.
Changes since v2:
* relax the restrictions around attr.tree so that if it does not resolve to a valid treeish, ignore it.
* add a commit to default to HEAD in bare repositories
Quoted 5 lines
Quoted 3 lines
Changes since v1:
* Added a commit to add attr.tree config
THis is v4 so there must be some changes since v3 that we are missing?
Oops I messed up the ordering of changes here. I'll fix in the (hopefully) final re-roll
Quoted 82 lines
Quoted 4 lines
Range-diff vs v3:
1:
cef206d47c7! 1:eaa27c47810attr: read attributes from HEAD when bare repoQuoted 9 lines
@@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr +test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' ' + test_when_finished rm -rf test bare_with_gitattribute && + git init test && -+ ( -+ cd test && -+ test_commit gitattributes .gitattributes "f/path test=val" -+ ) && ++ test_commit -C test gitattributes .gitattributes "f/path test=val" &&OK.
Quoted 17 lines
2:
dadb822da99! 2:749d8a8082eattr: add attr.tree for setting the treeish to read attributes from
@@Documentation/config.txt: other popular tools, and describe them in your documen##
Documentation/config/attr.txt(new) ##
@@
-+attr.tree:
-+ A <tree-ish> to read gitattributes from instead of the worktree. See
-+ linkgit:gitattributes[5]. Ifattr.treedoes not resolve to a valid tree,
-+ treat it as an empty tree. --attr-source andGIT_ATTR_SOURCEtake
-+ precedence over attr.tree.
++attr.tree::
++ A reference to a tree in the repository from which to read attributes,
++ instead of the.gitattributesfile in the working tree. In a bare
++ repository, this defaults toHEAD:.gitattributes. If the value does
++ not resolve to a valid tree object, an empty tree is used instead.
++ When theGIT_ATTR_SOURCEenvironment variable or--attr-source
++ command line option are used, this configuration variable has no effect.OK.
Quoted 5 lines
-+ if (!default_attr_source_tree_object_name) { ++ if (!default_attr_source_tree_object_name && git_attr_tree) { + default_attr_source_tree_object_name = git_attr_tree; + ignore_bad_attr_tree = 1; + }Makes sense.
Quoted 6 lines
@@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr +bad_attr_source_err="fatal: bad --attr-source or GIT_ATTR_SOURCE" + ++test_expect_success '--attr-source is bad' ' ++ test_when_finished rm -rf empty &&Quoted 8 lines
++ git init empty && ++ ( ++ cd empty && ++ echo "$bad_attr_source_err" >expect_err && ++ test_must_fail git --attr-source=HEAD check-attr test -- f/path 2>err && ++ test_cmp expect_err err ++ ) ++'OK. We fail when explicitly given a bad attr-source.
Quoted 9 lines
+test_expect_success 'attr.tree when HEAD is unborn' ' + test_when_finished rm -rf empty && + git init empty && + ( + cd empty && -+ echo $bad_attr_source_err >expect_err && + echo "f/path: test: unspecified" >expect && + git -c attr.tree=HEAD check-attr test -- f/path >actual 2>err && + test_must_be_empty err &&But we silently ignore when given via a configuration variable.
Quoted 8 lines
@@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr + git init empty && + ( + cd empty && -+ echo $bad_attr_source_err >expect_err && + echo "f/path: test: unspecified" >expect && + git -c attr.tree=refs/does/not/exist check-attr test -- f/path >actual 2>err && + test_must_be_empty err &&
Quoted 6 lines
Ditto. Is this any different from the above? Both points at an object that does not exist. If one were pointing at an object that does not exist (e.g., HEAD before the initial commit) and the other were pointing at an object that is not a tree-ish (e.g., a blob), then having two separate tests may make sense, but otherwise, I am not sure about the value proposition of the second test.
Yeah looking at it now, this test seems like it doesn't add much.
Quoted 25 lines
Quoted 16 lines
@@ t/t0003-attributes.sh: test_expect_success 'bare repo defaults to reading .gitat test_cmp expect actual ' -+test_expect_success '--attr-source and GIT_ATTR_SOURCE take precedence over attr.tree' ' ++test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tree' ' + test_when_finished rm -rf empty && + git init empty && + ( @@ t/t0003-attributes.sh: test_expect_success 'bare repo defaults to reading .gitat + test_commit "val2" .gitattributes "f/path test=val2" && + git checkout attr-source && + echo "f/path: test: val1" >expect && -+ git -c attr.tree=attr-tree --attr-source=attr-source check-attr test -- f/path >actual && ++ GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree --attr-source=attr-source \Quoted 8 lines
++ check-attr test -- f/path >actual && + test_cmp expect actual && -+ GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree check-attr test -- f/path >actual && ++ GIT_ATTR_SOURCE=attr-source git -c attr.tree=attr-tree \ ++ check-attr test -- f/path >actual && + test_cmp expect actual + ) +'
Quoted 3 lines
Looking good.
Thanks. Queued.
thanks!
John
v5 cover attr: add attr.tree config
In reply to John Cai via GitGitGadget44451a2e5e (attr: teach "--attr-source=" global option to "git", 2023-05-06) provided the ability to pass in a treeish as the attr source. When a revision does not resolve to a valid tree is passed, Git will die. At GitLab, we server repositories as bare repos and would like to always read attributes from the default branch, so we'd like to pass in HEAD as the treeish to read gitattributes from on every command. In this context we would not want Git to die if HEAD is unborn, like in the case of empty repositories.
Instead of modifying the default behavior of --attr-source, create a new config attr.tree with which an admin can configure a ref for all commands to read gitattributes from. Also make the default tree to read from HEAD on bare repositories.
Changes since v4:
* removed superfluous test
Changes since v3:
* clarified attr logic around ignoring errors if source set by attr.tree is invalid
* refactored tests by using helpers
* modified test to check for precedence between --attr-source, attr.tree, GIT_ATTR_SOURCE
Changes since v2:
* relax the restrictions around attr.tree so that if it does not resolve to a valid treeish, ignore it.
* add a commit to default to HEAD in bare repositories
* remove commit that adds attr.allowInvalidSource
Changes since v1:
* Added a commit to add attr.tree config
John Cai (2):
attr: read attributes from HEAD when bare repo
attr: add attr.tree for setting the treeish to read attributes from
Documentation/config.txt | 2 + Documentation/config/attr.txt | 7 ++++ attr.c | 19 ++++++++- attr.h | 2 + config.c | 16 ++++++++ t/t0003-attributes.sh | 72 +++++++++++++++++++++++++++++++++++ t/t5001-archive-attr.sh | 2 +- 7 files changed, 118 insertions(+), 2 deletions(-) create mode 100644 Documentation/config/attr.txt
base-commit: 1fc548b2d6a3596f3e1c1f8b1930d8dbd1e30bf3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1577%2Fjohn-cai%2Fjc%2Fconfig-attr-invalid-source-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1577/john-cai/jc/config-attr-invalid-source-v5
Pull-Request: https://github.com/git/git/pull/1577
Range-diff vs v4:
1: eaa27c47810 = 1: eaa27c47810 attr: read attributes from HEAD when bare repo
2: 749d8a8082e ! 2: df4b3f53309 attr: add attr.tree for setting the treeish to read attributes from
@@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
+ )
+'
+
-+test_expect_success 'attr.tree points to non-existing ref' '
-+ test_when_finished rm -rf empty &&
-+ git init empty &&
-+ (
-+ cd empty &&
-+ echo "f/path: test: unspecified" >expect &&
-+ git -c attr.tree=refs/does/not/exist check-attr test -- f/path >actual 2>err &&
-+ test_must_be_empty err &&
-+ test_cmp expect actual
-+ )
-+'
-+
+test_expect_success 'bad attr source defaults to reading .gitattributes file' '
+ test_when_finished rm -rf empty &&
+ git init empty &&Signature · 1 line
gitgitgadget
PATCH v5 1/2 attr: read attributes from HEAD when bare repo
From: John Cai <[email protected]>
The motivation for 44451a2e5e (attr: teach "--attr-source=<tree>" global option to "git" , 2023-05-06), was to make it possible to use gitattributes with bare repositories.
To make it easier to read gitattributes in bare repositories however, let's just make HEAD:.gitattributes the default. This is in line with how mailmap works, 8c473cecfd (mailmap: default mailmap.blob in bare repositories, 2012-12-13).
--- attr.c | 12 +++++++++++- t/t0003-attributes.sh | 11 +++++++++++ t/t5001-archive-attr.sh | 2 +- 3 files changed, 23 insertions(+), 2 deletions(-)
attr.c +11 -1
@@ -1194,6 +1194,7 @@ static void collect_some_attrs(struct index_state *istate, }static const char *default_attr_source_tree_object_name; +static int ignore_bad_attr_tree;void set_git_attr_source (const char *tree_object_name) {
@@ -1205,10 +1206,19 @@ static void compute_default_attr_source(struct object_id *attr_source)if (!default_attr_source_tree_object_name) default_attr_source_tree_object_name =getenv (GIT_ATTR_SOURCE_ENVIRONMENT ); +if (!default_attr_source_tree_object_name && + startup_info->have_repository && +is_bare_repository ()) { + default_attr_source_tree_object_name ="HEAD" ; + ignore_bad_attr_tree =1 ; + } +if (!default_attr_source_tree_object_name || !is_null_oid (attr_source))return ; -if (repo_get_oid_treeish (the_repository, default_attr_source_tree_object_name, attr_source)) +if (repo_get_oid_treeish (the_repository, + default_attr_source_tree_object_name, + attr_source) && !ignore_bad_attr_tree)die (_ ("bad --attr-source or GIT_ATTR_SOURCE" )); }
t/t0003-attributes.sh +11 -0
@@ -342,6 +342,17 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' ' ) ' + +test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' ' + test_when_finished rm -rf test bare_with_gitattribute && + git init test && + test_commit -C test gitattributes .gitattributes "f/path test=val" && + git clone --bare test bare_with_gitattribute && + echo "f/path: test: val" >expect && + git -C bare_with_gitattribute check-attr test -- f/path >actual && + test_cmp expect actual +' + test_expect_success 'bare repository: with --source' ' ( cd bare.git &&
t/t5001-archive-attr.sh +1 -1
@@ -138,7 +138,7 @@ test_expect_success 'git archive with worktree attributes, bare' ' ' test_expect_missing bare-worktree/ignored -test_expect_exists bare-worktree/ignored-by-tree +test_expect_missing bare-worktree/ignored-by-tree test_expect_exists bare-worktree/ignored-by-worktree test_expect_success 'export-subst' '
Signature · 1 line
gitgitgadget
PATCH v5 2/2 attr: add attr.tree for setting the treeish to read attributes from
In reply to John Cai via GitGitGadgetFrom: John Cai <[email protected]>
44451a2 (attr: teach "--attr-source=<tree>" global option to "git", 2023-05-06) provided the ability to pass in a treeish as the attr source. In the context of serving Git repositories as bare repos like we do at GitLab however, it would be easier to point --attr-source to HEAD for all commands by setting it once.
Add a new config attr.tree that allows this.
--- Documentation/config.txt | 2 ++ Documentation/config/attr.txt | 7 ++++ attr.c | 7 ++++ attr.h | 2 ++ config.c | 16 +++++++++ t/t0003-attributes.sh | 61 +++++++++++++++++++++++++++++++++++ 6 files changed, 95 insertions(+) create mode 100644 Documentation/config/attr.txt
Documentation/config.txt +2 -0
@@ -371,6 +371,8 @@ other popular tools, and describe them in your documentation. include::config/advice.txt[] +include::config/attr.txt[] + include::config/core.txt[] include::config/add.txt[]
Documentation/config/attr.txt new file +7 -0
@@ -0,0 +1,7 @@ +attr.tree:: + A reference to a tree in the repository from which to read attributes, + instead of the `.gitattributes` file in the working tree. In a bare + repository, this defaults to `HEAD:.gitattributes`. If the value does + not resolve to a valid tree object, an empty tree is used instead. + When the `GIT_ATTR_SOURCE` environment variable or `--attr-source` + command line option are used, this configuration variable has no effect.
attr.c +7 -0
@@ -24,6 +24,8 @@#include "tree-walk.h" #include "object-name.h" +const char *git_attr_tree; +const char git_attr__true[] ="(builtin)true" ;const char git_attr__false[] ="\0(builtin)false" ;static const char git_attr__unknown[] ="(builtin)unknown" ;
@@ -1206,6 +1208,11 @@ static void compute_default_attr_source(struct object_id *attr_source)if (!default_attr_source_tree_object_name) default_attr_source_tree_object_name =getenv (GIT_ATTR_SOURCE_ENVIRONMENT ); +if (!default_attr_source_tree_object_name && git_attr_tree) { + default_attr_source_tree_object_name = git_attr_tree; + ignore_bad_attr_tree =1 ; + } +if (!default_attr_source_tree_object_name && startup_info->have_repository &&is_bare_repository ()) {
attr.h +2 -0
@@ -236,4 +236,6 @@ const char *git_attr_global_file(void);/* Return whether the system gitattributes file is enabled and should be used. */ int git_attr_system_is_enabled (void ); +extern const char *git_attr_tree; +#endif /* ATTR_H */
config.c +16 -0
@@ -18,6 +18,7 @@#include "repository.h" #include "lockfile.h" #include "mailmap.h" +#include "attr.h" #include "exec-cmd.h" #include "strbuf.h" #include "quote.h"
@@ -1904,6 +1905,18 @@ static int git_default_mailmap_config(const char *var, const char *value)return 0 ; } +static int git_default_attr_config (const char *var,const char *value) +{ +if (!strcmp (var,"attr.tree" )) +return git_config_string (&git_attr_tree, var, value); + +/* +* Add other attribute related config variables here and to +* Documentation/config/attr.txt. +*/ +return 0 ; +} +int git_default_config (const char *var,const char *value,const struct config_context *ctx,void *cb) {
@@ -1927,6 +1940,9 @@ int git_default_config(const char *var, const char *value,if (starts_with (var,"mailmap." ))return git_default_mailmap_config (var, value); +if (starts_with (var,"attr." )) +return git_default_attr_config (var, value); +if (starts_with (var,"advice." ) ||starts_with (var,"color.advice" ))return git_default_advice_config (var, value);
t/t0003-attributes.sh +61 -0
79 lines not on this page · raw message
Signature · 1 line
gitgitgadget
Re: [PATCH v5 0/2] attr: add attr.tree config
In reply to John Cai via GitGitGadgetQuoted 3 lines · John Cai via GitGitGadget
Changes since v4:
* removed superfluous test
An alternative would have been to point with the ref some non-tree object like a blob, but as the outcome should be the same as missing case (from the code --- which is not exactly kosher), it should be OK.
if (repo_get_oid_treeish(the_repository,
default_attr_source_tree_object_name,
attr_source) && !ignore_bad_attr_tree)
die(_("bad --attr-source or GIT_ATTR_SOURCE"));OOPS! Sorry for not noticing earlier, but repo_get_oid_treeish() does *NOT* error out when the discovered object is not a treeish, as the suggested object type is merely supplied for disambiguation purposes (e.g., with objects 012345 that is a tree and 012346 that is a blob, you can still ask for treeish "01234" but if you ask for an object "01234" it will fail).
So, the alternative test would have caught this bug, no? Instead of silently treating the non-treeish as an empty tree, we would have died much later when the object supposedly a tree-ish turns out to be a blob, or something?
Re: [PATCH v5 0/2] attr: add attr.tree config
Quoted 12 lines · Junio C Hamano
if (repo_get_oid_treeish(the_repository, default_attr_source_tree_object_name, attr_source) && !ignore_bad_attr_tree) die(_("bad --attr-source or GIT_ATTR_SOURCE"));OOPS! Sorry for not noticing earlier, but
repo_get_oid_treeish()does *NOT* error out when the discovered object is not a treeish, as the suggested object type is merely supplied for disambiguation purposes (e.g., with objects 012345 that is a tree and 012346 that is a blob, you can still ask for treeish "01234" but if you ask for an object "01234" it will fail).
Quoted 4 lines
So, the alternative test would have caught this bug, no? Instead of silently treating the non-treeish as an empty tree, we would have died much later when the object supposedly a tree-ish turns out to be a blob, or something?
There indeed is a bug, but not really. If we add this test:
test_expect_success 'attr.tree that points at a non-treeish' '
test_when_finished rm -rf empty && git init empty && ( cd empty && echo "f/path: test: unspecified" >expect && H=$(git hash-object -t blob --stdin -w </dev/null) && git -c attr.tree=$H check-attr test -- f/path >actual 2>err && test_must_be_empty err && test_cmp expect actual )
'
repo_get_oid_treeish() returns a blob object name and we end up storing a blob object name in "attr_source" static variable of default_attr_source() function.
Later this is fed to read_attr() by bootstrap_attr_stack() and then to read_attr_from_blob() that uses it to call get_tree_entry(), which fails for any path because it is a blob. We do not give any errors or error messages during the whole process.
So in a sense, for !!ignore_bad_attr_tree case, the code ends up doing the right thing. But if !ignore_bad_attr_tree is true, i.e., a blob object name is given via --attr-source or GIT_ATTR_SOURCE, then the bug will be uncovered.
t/t0003-attributes.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+)
t/t0003-attributes.sh +12 -0
@@ -394,6 +394,18 @@ test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' ' test_cmp expect actual ' +test_expect_success '--attr-source that points at a non-treeish' ' + test_when_finished rm -rf empty && + git init empty && + ( + cd empty && + echo "$bad_attr_source_err" >expect_err && + H=$(git hash-object -t blob --stdin -w </dev/null) && + test_must_fail git --attr-source=$H check-attr test -- f/path 2>err && + test_cmp expect_err err + ) +' + test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tree' ' test_when_finished rm -rf empty && git init empty &&
Re: [PATCH v5 0/2] attr: add attr.tree config
Quoted 4 lines · Junio C Hamano
So in a sense, for !!ignore_bad_attr_tree case, the code ends up doing the right thing. But if !ignore_bad_attr_tree is true, i.e., a blob object name is given via --attr-source or
GIT_ATTR_SOURCE, then the bug will be uncovered.
Having said all that, I suspect that this problem is not new and certainly not caused by this topic. We should have unconditionally died when GIT_ATTR_SOURCE gave a blob object name, but pretended as if an empty tree was given. There may even be existing users who now assume that is working as intended and depend on this bug.
So, let's leave it as a "possible bug" that we might want to fix in the future, outside the scope of this series.
Thanks.
Quoted 26 lines
t/t0003-attributes.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+)t/t0003-attributes.sh +12
-0@@ -394,6 +394,18 @@ test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' ' test_cmp expect actual ' +test_expect_success '--attr-source that points at a non-treeish' ' + test_when_finished rm -rf empty && + git init empty && + ( + cd empty && + echo "$bad_attr_source_err" >expect_err && + H=$(git hash-object -t blob --stdin -w </dev/null) && + test_must_fail git --attr-source=$H check-attr test -- f/path 2>err && + test_cmp expect_err err + ) +' + test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tree' ' test_when_finished rm -rf empty && git init empty &&
Re: [PATCH v5 0/2] attr: add attr.tree config
Hi Junio,
Quoted 13 lines · Junio C Hamano
Quoted 4 lines · Junio C Hamano
So in a sense, for !!ignore_bad_attr_tree case, the code ends up doing the right thing. But if !ignore_bad_attr_tree is true, i.e., a blob object name is given via --attr-source or
GIT_ATTR_SOURCE, then the bug will be uncovered.Having said all that, I suspect that this problem is not new and certainly not caused by this topic. We should have unconditionally died when
GIT_ATTR_SOURCEgave a blob object name, but pretended as if an empty tree was given. There may even be existing users who now assume that is working as intended and depend on this bug.
Quoted 4 lines
So, let's leave it as a "possible bug" that we might want to fix in the future, outside the scope of this series.
Thanks.
That sounds good--let's fix in a separate series. Thanks for the careful review!
thanks
John
Quoted 28 lines
Quoted 28 lines
t/t0003-attributes.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+)t/t0003-attributes.sh +12
-0@@ -394,6 +394,18 @@ test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' ' test_cmp expect actual ' +test_expect_success '--attr-source that points at a non-treeish' ' + test_when_finished rm -rf empty && + git init empty && + ( + cd empty && + echo "$bad_attr_source_err" >expect_err && + H=$(git hash-object -t blob --stdin -w </dev/null) && + test_must_fail git --attr-source=$H check-attr test -- f/path 2>err && + test_cmp expect_err err + ) +' + test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then attr.tree' ' test_when_finished rm -rf empty && git init empty &&