Hi Junio, On 21 Sep 2023, at 4:52, Junio C Hamano wrote: > Jeff King <peff@xxxxxxxx> 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
