Hi Peff, On 21 Sep 2023, at 0:15, Jeff King wrote: > 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. You captured this quite well! > > 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"). Yeah, I see the point about using a config to change default behavior of a command leading to confusion. > > 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). In retrospect perhaps a config would have been better here. I think this patch started with improving an existing command line flag [1] by making it global. So I think we were just thinking about command line flags and didn't consider configs. That being said, for GitLab at least there's not a lot of difference since we pass in configs through the commandline anyways rather than relying on the config state itself on disk for our bare server-side repositories. 1. https://lore.kernel.org/git/pull.1470.git.git.1679109928556.gitgitgadget@xxxxxxxxx/ > > 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 thanks John
