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

From: Jeff King <peff@peff.net>
Date: Sep 21, 2023, 04:15 UTC
Message-ID: <20230921041545.GA2338791@coredump.intra.peff.net>
In-Reply-To: <xmqqfs38akx5.fsf@gitster.g>

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

Show 19 quoted lines

> > With empty repositories however, HEAD does not point to a valid treeish,
> > causing Git to die. This means we would need to check for a valid
> > treeish each time.
> 
> Naturally.
> 
> > 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