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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 21, 2023, 08:52 UTC
Message-ID: <xmqq1qer7vrv.fsf@gitster.g>
In-Reply-To: <20230921041545.GA2338791@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 5 quoted lines

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

Show 6 quoted lines

> And there you really are saying "if there are attributes in HEAD, use
> them; otherwise, don't worry about it". This is exactly what we do with
> mailmap.blob: in a bare repository it is set to HEAD by default, but if
> HEAD does not resolve, we just ignore it (just like a HEAD that does not
> contain a .mailmap file). And those match the non-bare cases, where we'd
> read those files from the working tree instead.

"HEAD" -> "HEAD:.mailmap" if I recall correctly.

And if HEAD does not resolve, we pretend as if HEAD is an empty tree-ish (hence HEAD:.mailmap is missing). It becomes very tempting to do the same for the attribute sources and treat unborn HEAD as if it specifies an empty tree-ish, without any configuration or an extra option.

Such a change would be an end-user observable behaviour change, but nobody sane would be running "git --attr-source=HEAD diff HEAD^ HEAD" to check and detect an unborn HEAD for its error exit code, so I do not think it is a horribly wrong thing to do.

But again, as you said, --attr-source=<tree-ish> does not sound like a good fit for bare-repository hosted environment and a tentative hack waiting for a proper attr.blob support, or something like that, to appear.

Show 11 quoted lines

> But what is weird about this patch is that we are using a config option
> to change how a command-line option is interpreted. If the idea is that
> some invocations care about the validity of the source and some do not,
> then the config option is much too blunt. It is set once long ago, but
> it can't distinguish between times you care about invalid sources and
> times you don't.
>
> It would make much more sense to me to have another command-line option,
> like:
>
>   git --attr-source=HEAD --allow-invalid-attr-source

Yeah, if we were to make it configurable without changing the default behaviour, I agree that would be more correct approach. A configuration does not sound like a good fit.

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