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

From: Jeff King <peff@peff.net>
Date: Sep 21, 2023, 21:40 UTC
Message-ID: <20230921214021.GB302338@coredump.intra.peff.net>
In-Reply-To: <xmqq1qer7vrv.fsf@gitster.g>

On Thu, Sep 21, 2023 at 01:52:52AM -0700, Junio C Hamano wrote:

Show 13 quoted lines

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

I think it's unlikely for a user to write that. But if you are running a server full of bare repositories that does diffs, you might end up with a script that sticks "--attr-source=HEAD" at the beginning of every command.

It is true that HEAD may not be related. But that is what we use if you are in a non-bare repository and run "git diff foo^ foo".

Arguably:

  git --attr-source=$to diff $from $to

is a better default for this command. But something like "git log -p" is trickier, as you have many commits to show. You can try to use the tip of the traversal, but there may be multiple. Using the attributes from the destination of each commit is the most likely to avoid divergence between the attributes and the code, but it may also not be what people want. Using the modern attributes from the working tree often makes showing historical commits much nicer.

So I dunno. I could see arguments in both directions, but I think in general people have been OK with pulling attributes from the working tree. And --attr-source=HEAD is the bare equivalent.

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

I don't know that it was my example. :) But yes, if you do "--attr-source=$to", you're overriding even the non-bare case. That may be what you want for some cases, but as above, I think it's hard to apply consistently (or even what you'd want for the general case).

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

True, yeah. We can't do that here because attributes are spread across the tree. So all my mentions of attr.blob would really be attr.tree.

Show 10 quoted lines

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

Yeah, that is basically what I am proposing. It sounds from the discussion here that there are two interesting cases:

  1. You want to use --attr-source=HEAD because you are trying to make a
     bare repo behave like a non-bare one. You probably want the "don't
     complain if it is missing" behavior.

  2. You are trying to use the attributes from one side of the diff to
     override the worktree ones (because the two trees are unrelated).
     In which case it does not really matter if --attr-source complains,
     because the diff will likewise complain if the tree cannot be
     resolved.

Just trying to play devil's advocate, though, I guess you could run a non-diff operation like say:

  git --attr-source=my-branch check-attr foo

and then you probably _do_ want to know if that source was ignored or typo'd.

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

I think folks mentioned mailmap.blob in the original discussion for --attr-source. I don't remember why the patch went with --attr-source there. Maybe John can speak to that.

-Peff