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

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