[PATCH] attr: attr.allowInvalidSource config to allow invalid revision

[PATCH] attr: attr.allowInvalidSource config to allow invalid revision

[John Cai via GitGitGadget]

From: John Cai <johncai86@gmail.com>

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.

Signed-off-by: John Cai <johncai86@gmail.com>
---
    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

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 229b63a454c..b1891c2b5af 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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[]
diff --git a/Documentation/config/attr.txt b/Documentation/config/attr.txt
new file mode 100644
index 00000000000..2218f0c982a
--- /dev/null
+++ b/Documentation/config/attr.txt
@@ -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.
diff --git a/attr.c b/attr.c
index 71c84fbcf86..854a3720e3f 100644
--- a/attr.c
+++ b/attr.c
@@ -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)
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 26e082f05b4..3272237ee2b 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -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
-- 
gitgitgadget

Re: [PATCH] attr: attr.allowInvalidSource config to allow invalid revision

[Junio C Hamano]

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: John Cai <johncai86@gmail.com>
>
> 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.

> 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.

> 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.

Re: [PATCH] attr: attr.allowInvalidSource config to allow invalid revision

[Jeff King]

On Wed, Sep 20, 2023 at 09:06:46AM -0700, Junio C Hamano wrote:

> > 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.
> 
> > 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.

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

Re: [PATCH] attr: attr.allowInvalidSource config to allow invalid revision

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

> 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.

> 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.

> ... 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.

Re: [PATCH] attr: attr.allowInvalidSource config to allow invalid revision

[Jeff King]

On Thu, Sep 21, 2023 at 01:52:52AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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.

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.

> 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).

> > 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.

> 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.

> 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

Re: [PATCH] attr: attr.allowInvalidSource config to allow invalid revision

[John Cai]

Hi Peff,

On 21 Sep 2023, at 0:15, Jeff King wrote:

> On Wed, Sep 20, 2023 at 09:06:46AM -0700, Junio C Hamano wrote:
>
>>> 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.
>>
>>> 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.
>
> 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.

You captured this quite well!

>
> 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").

Yeah, I see the point about using a config to change default behavior of a
command leading to confusion.

>
> 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/pull.1470.git.git.1679109928556.gitgitgadget@gmail.com/

>
> 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

Re: [PATCH] attr: attr.allowInvalidSource config to allow invalid revision

[John Cai]

Hi Peff,	

On 21 Sep 2023, at 17:40, Jeff King wrote:

> On Thu, Sep 21, 2023 at 01:52:52AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>>> 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.
>
> 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.
>
>> 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).
>
>>> 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.
>
>> 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.

Yep, this is the primary use case for us.

>
>   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.
>
>> 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/ZBMn5T6zfKK+PYUe@coredump.intra.peff.net/

>
> -Peff

Re: [PATCH] attr: attr.allowInvalidSource config to allow invalid revision

[John Cai]

Hi Junio,

On 21 Sep 2023, at 4:52, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
>
>> 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.
>
>> 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.
>
>> 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.
>
>> ... 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.

>
> True, too.
>
> Thanks.

thanks
John

[PATCH v2 0/2] attr: add attr.tree and attr.allowInvalidSource configs

[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 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 <johncai86@gmail.com>
      
       ## 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 <johncai86@gmail.com>
      
     - ## 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' '
       	(

-- 
gitgitgadget

[PATCH v2 1/2] attr: add attr.tree for setting the treeish to read attributes from

[John Cai via GitGitGadget]

From: John Cai <johncai86@gmail.com>

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.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 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

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 229b63a454c..b1891c2b5af 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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[]
diff --git a/Documentation/config/attr.txt b/Documentation/config/attr.txt
new file mode 100644
index 00000000000..e4f2122b7ab
--- /dev/null
+++ b/Documentation/config/attr.txt
@@ -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.
diff --git a/attr.c b/attr.c
index 71c84fbcf86..bb0d54eb967 100644
--- a/attr.c
+++ b/attr.c
@@ -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;
 
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 26e082f05b4..6342187c751 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -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
-- 
gitgitgadget

[PATCH v2 2/2] attr: add attr.allowInvalidSource config to allow invalid revision

[John Cai via GitGitGadget]

From: John Cai <johncai86@gmail.com>

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.

Signed-off-by: John Cai <johncai86@gmail.com>
---
 Documentation/config/attr.txt |  7 ++++++
 attr.c                        | 16 ++++++++++---
 t/t0003-attributes.sh         | 45 +++++++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/Documentation/config/attr.txt b/Documentation/config/attr.txt
index e4f2122b7ab..00113a4950e 100644
--- a/Documentation/config/attr.txt
+++ b/Documentation/config/attr.txt
@@ -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.
diff --git a/attr.c b/attr.c
index bb0d54eb967..1a7ac39b9d1 100644
--- a/attr.c
+++ b/attr.c
@@ -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)
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 6342187c751..972b64496e7 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -346,6 +346,51 @@ 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 &&
+	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 &&
+	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 &&
-- 
gitgitgadget

Re: [PATCH v2 1/2] attr: add attr.tree for setting the treeish to read attributes from

[Junio C Hamano]

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: John Cai <johncai86@gmail.com>
>
> 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

[Junio C Hamano]

[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)]

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/attr.c b/attr.c
> index 71c84fbcf86..bb0d54eb967 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -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;

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

[Jeff King]

On Wed, Oct 04, 2023 at 12:58:43PM -0700, Junio C Hamano wrote:

> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > From: John Cai <johncai86@gmail.com>
> >
> > 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?

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

[John Cai]

Hi Peff,

On 5 Oct 2023, at 13:07, Jeff King wrote:

> On Wed, Oct 04, 2023 at 12:58:43PM -0700, Junio C Hamano wrote:
>
>> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> From: John Cai <johncai86@gmail.com>
>>>
>>> 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?
>
> 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.

> 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

>
> -Peff

thanks!
John

Re: [PATCH v2 1/2] attr: add attr.tree for setting the treeish to read attributes from

[Jonathan Tan]

Junio C Hamano <gitster@pobox.com> writes:
> 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.

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.

> 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).

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 v3 0/2] attr: add attr.tree config

[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

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 <johncai86@gmail.com>
      
       ## 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 <johncai86@gmail.com>
      
     - ## 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 &&

-- 
gitgitgadget

[PATCH v3 1/2] attr: read attributes from HEAD when bare repo

[John Cai via GitGitGadget]

From: John Cai <johncai86@gmail.com>

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 <johncai86@gmail.com>
---
 attr.c                  | 12 +++++++++++-
 t/t0003-attributes.sh   | 14 ++++++++++++++
 t/t5001-archive-attr.sh |  2 +-
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/attr.c b/attr.c
index 71c84fbcf86..bf2ea1626a6 100644
--- a/attr.c
+++ b/attr.c
@@ -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"));
 }
 
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 26e082f05b4..e6b1a117228 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -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 &&
diff --git a/t/t5001-archive-attr.sh b/t/t5001-archive-attr.sh
index 0ff47a239db..eaf959d8f63 100755
--- a/t/t5001-archive-attr.sh
+++ b/t/t5001-archive-attr.sh
@@ -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' '
-- 
gitgitgadget

[PATCH v3 2/2] attr: add attr.tree for setting the treeish to read attributes from

[John Cai via GitGitGadget]

From: John Cai <johncai86@gmail.com>

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 <johncai86@gmail.com>
---
 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

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 229b63a454c..b1891c2b5af 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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[]
diff --git a/Documentation/config/attr.txt b/Documentation/config/attr.txt
new file mode 100644
index 00000000000..be882523f8b
--- /dev/null
+++ b/Documentation/config/attr.txt
@@ -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.
diff --git a/attr.c b/attr.c
index bf2ea1626a6..0ae6852d12b 100644
--- a/attr.c
+++ b/attr.c
@@ -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()) {
diff --git a/attr.h b/attr.h
index 2b745df4054..127998ae013 100644
--- a/attr.h
+++ b/attr.h
@@ -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 */
diff --git a/config.c b/config.c
index 3846a37be97..21a1590b505 100644
--- a/config.c
+++ b/config.c
@@ -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);
 
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index e6b1a117228..b0949125f26 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -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
@@ -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"
+
+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 &&
@@ -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' '
+	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 &&
-- 
gitgitgadget

Re: [PATCH v3 1/2] attr: read attributes from HEAD when bare repo

[Eric Sunshine]

On Tue, Oct 10, 2023 at 3:49 PM John Cai via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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 <johncai86@gmail.com>
> ---
> diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
> @@ -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" &&

> +       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
> +'

Re: [PATCH v3 2/2] attr: add attr.tree for setting the treeish to read attributes from

[Junio C Hamano]

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: John Cai <johncai86@gmail.com>
>
> 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 <johncai86@gmail.com>
> ---
>  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
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 229b63a454c..b1891c2b5af 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -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[]
> diff --git a/Documentation/config/attr.txt b/Documentation/config/attr.txt
> new file mode 100644
> index 00000000000..be882523f8b
> --- /dev/null
> +++ b/Documentation/config/attr.txt
> @@ -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.

> diff --git a/attr.c b/attr.c
> index bf2ea1626a6..0ae6852d12b 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -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;
> +	}

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. 

>  	if (!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.

> +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.
	 */

> @@ -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.


> +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?

> +		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.

> +		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
> +	)
> +'

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.

> @@ -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.

> +	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 &&

Other than that, looking great.  Thanks.

Re: [PATCH v3 2/2] attr: add attr.tree for setting the treeish to read attributes from

[John Cai]

Hi Junio,

On 10 Oct 2023, at 18:14, Junio C Hamano wrote:

> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: John Cai <johncai86@gmail.com>
>>
>> 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 <johncai86@gmail.com>
>> ---
>>  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
>>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 229b63a454c..b1891c2b5af 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -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[]
>> diff --git a/Documentation/config/attr.txt b/Documentation/config/attr.txt
>> new file mode 100644
>> index 00000000000..be882523f8b
>> --- /dev/null
>> +++ b/Documentation/config/attr.txt
>> @@ -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.

sounds good

>
> 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.

>
> 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.

sounds good to me, thanks

>
>> diff --git a/attr.c b/attr.c
>> index bf2ea1626a6..0ae6852d12b 100644
>> --- a/attr.c
>> +++ b/attr.c
>> @@ -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;
>> +	}
>
> 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.
>
>>  	if (!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.

good point--I think this logic is easier to follow.

>
>> +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.
> 	 */
>
>> @@ -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.
>
>
>> +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?
>
>> +		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.
>
>> +		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
>> +	)
>> +'
>
> 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.

>
>> @@ -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.

>
>> +	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 &&
>
> Other than that, looking great.  Thanks.

thanks!
John

[PATCH v4 0/2] attr: add attr.tree config

[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

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
      +	)
      +'

-- 
gitgitgadget

[PATCH v4 1/2] attr: read attributes from HEAD when bare repo

[John Cai via GitGitGadget]

From: John Cai <johncai86@gmail.com>

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 <johncai86@gmail.com>
---
 attr.c                  | 12 +++++++++++-
 t/t0003-attributes.sh   | 11 +++++++++++
 t/t5001-archive-attr.sh |  2 +-
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/attr.c b/attr.c
index 71c84fbcf86..bf2ea1626a6 100644
--- a/attr.c
+++ b/attr.c
@@ -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"));
 }
 
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 26e082f05b4..5665cdc079f 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -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 &&
diff --git a/t/t5001-archive-attr.sh b/t/t5001-archive-attr.sh
index 0ff47a239db..eaf959d8f63 100755
--- a/t/t5001-archive-attr.sh
+++ b/t/t5001-archive-attr.sh
@@ -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' '
-- 
gitgitgadget

[PATCH v4 2/2] attr: add attr.tree for setting the treeish to read attributes from

[John Cai via GitGitGadget]

From: John Cai <johncai86@gmail.com>

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 <johncai86@gmail.com>
---
 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

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 229b63a454c..b1891c2b5af 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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[]
diff --git a/Documentation/config/attr.txt b/Documentation/config/attr.txt
new file mode 100644
index 00000000000..1a482d6af2b
--- /dev/null
+++ b/Documentation/config/attr.txt
@@ -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.
diff --git a/attr.c b/attr.c
index bf2ea1626a6..e62876dfd3e 100644
--- a/attr.c
+++ b/attr.c
@@ -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()) {
diff --git a/attr.h b/attr.h
index 2b745df4054..127998ae013 100644
--- a/attr.h
+++ b/attr.h
@@ -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 */
diff --git a/config.c b/config.c
index 3846a37be97..fb6a2db1d9b 100644
--- a/config.c
+++ b/config.c
@@ -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);
 
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 5665cdc079f..e9953ce19c5 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -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
@@ -342,6 +346,55 @@ 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-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 "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 "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 &&
@@ -353,6 +406,26 @@ test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '
 	test_cmp expect actual
 '
 
+test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then 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_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 &&
+		test_cmp expect actual
+	)
+'
+
 test_expect_success 'bare repository: with --source' '
 	(
 		cd bare.git &&
-- 
gitgitgadget

Re: [PATCH v4 0/2] attr: add attr.tree config

[Junio C Hamano]

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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
>
> 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?

> 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" &&

OK.

>  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.

OK.

>      -+	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.

>      @@ 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
>      ++	)
>      ++'

OK.  We fail when explicitly given a bad 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 &&

But we silently ignore when given via a configuration variable.

>      @@ 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.

>      @@ 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
>       +	)
>       +'

Looking good.

Thanks.  Queued.

Re: [PATCH v4 0/2] attr: add attr.tree config

[John Cai]

Hi Junio,

On 11 Oct 2023, at 18:09, Junio C Hamano wrote:

> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> 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
>>
>> 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

>
>> 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" &&
>
> OK.
>
>>  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.
>
> OK.
>
>>      -+	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.
>
>>      @@ 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
>>      ++	)
>>      ++'
>
> OK.  We fail when explicitly given a bad 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 &&
>
> But we silently ignore when given via a configuration variable.
>
>>      @@ 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.

Yeah looking at it now, this test seems like it doesn't add much.

>
>>      @@ 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
>>       +	)
>>       +'
>
> Looking good.
>
> Thanks.  Queued.

thanks!
John

[PATCH v5 0/2] attr: add attr.tree config

[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 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 &&

-- 
gitgitgadget

[PATCH v5 1/2] attr: read attributes from HEAD when bare repo

[John Cai via GitGitGadget]

From: John Cai <johncai86@gmail.com>

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 <johncai86@gmail.com>
---
 attr.c                  | 12 +++++++++++-
 t/t0003-attributes.sh   | 11 +++++++++++
 t/t5001-archive-attr.sh |  2 +-
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/attr.c b/attr.c
index 71c84fbcf86..bf2ea1626a6 100644
--- a/attr.c
+++ b/attr.c
@@ -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"));
 }
 
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 26e082f05b4..5665cdc079f 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -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 &&
diff --git a/t/t5001-archive-attr.sh b/t/t5001-archive-attr.sh
index 0ff47a239db..eaf959d8f63 100755
--- a/t/t5001-archive-attr.sh
+++ b/t/t5001-archive-attr.sh
@@ -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' '
-- 
gitgitgadget

[PATCH v5 2/2] attr: add attr.tree for setting the treeish to read attributes from

[John Cai via GitGitGadget]

From: John Cai <johncai86@gmail.com>

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 <johncai86@gmail.com>
---
 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

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 229b63a454c..b1891c2b5af 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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[]
diff --git a/Documentation/config/attr.txt b/Documentation/config/attr.txt
new file mode 100644
index 00000000000..1a482d6af2b
--- /dev/null
+++ b/Documentation/config/attr.txt
@@ -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.
diff --git a/attr.c b/attr.c
index bf2ea1626a6..e62876dfd3e 100644
--- a/attr.c
+++ b/attr.c
@@ -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()) {
diff --git a/attr.h b/attr.h
index 2b745df4054..127998ae013 100644
--- a/attr.h
+++ b/attr.h
@@ -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 */
diff --git a/config.c b/config.c
index 3846a37be97..fb6a2db1d9b 100644
--- a/config.c
+++ b/config.c
@@ -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);
 
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 5665cdc079f..ecf43ab5454 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -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
@@ -342,6 +346,43 @@ 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-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 "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 '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 &&
@@ -353,6 +394,26 @@ test_expect_success 'bare repo defaults to reading .gitattributes from HEAD' '
 	test_cmp expect actual
 '
 
+test_expect_success 'precedence of --attr-source, GIT_ATTR_SOURCE, then 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_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 &&
+		test_cmp expect actual
+	)
+'
+
 test_expect_success 'bare repository: with --source' '
 	(
 		cd bare.git &&
-- 
gitgitgadget

Re: [PATCH v5 0/2] attr: add attr.tree config

[Junio C Hamano]

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 	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?

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(+)

diff --git c/t/t0003-attributes.sh w/t/t0003-attributes.sh
index ecf43ab545..0f02f22171 100755
--- c/t/t0003-attributes.sh
+++ w/t/t0003-attributes.sh
@@ -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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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.


>  t/t0003-attributes.sh | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git c/t/t0003-attributes.sh w/t/t0003-attributes.sh
> index ecf43ab545..0f02f22171 100755
> --- c/t/t0003-attributes.sh
> +++ w/t/t0003-attributes.sh
> @@ -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

[John Cai]

Hi Junio,

On 13 Oct 2023, at 16:47, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> 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.

That sounds good--let's fix in a separate series. Thanks for the careful review!

thanks
John

>
>
>>  t/t0003-attributes.sh | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git c/t/t0003-attributes.sh w/t/t0003-attributes.sh
>> index ecf43ab545..0f02f22171 100755
>> --- c/t/t0003-attributes.sh
>> +++ w/t/t0003-attributes.sh
>> @@ -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 &&