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

> 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