Subject: Re: [PATCH v5 08/18] blame: emit a better error on 'git blame directory'

From: Junio C Hamano <[email-masked]>
Date: Fri, 02 Apr 2021 14:08:22 -0700

Ævar Arnfjörð Bjarmason <[email-masked]> writes:

> On Thu, Apr 01 2021, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason  <[email-masked]> writes:
>>
>>> Change an early check for non-blobs in verify_working_tree_path() to
>>> let any such objects pass, and instead die shortly thereafter in the
>>> fake_working_tree_commit() caller's type check.
>>>
>>> Now e.g. doing "git blame t" in git.git emits:
>>>
>>>     fatal: unsupported file type t
>>>
>>> Instead of:
>>>
>>>     fatal: no such path 't' in HEAD
>>
>> Sorry, but I fail to see why "unsupported file type t" is quite an
>> improvement.  Is this one of these irrelevant clean-up while at it
>> whose benefit is unclear until much later, I have to wonder.
>
> Because "t" is directory we can stat() and which exists in the index, so
> it makes more sense to fall through to the stat() codepath.

't' is not in the index, even though many things with 't/' prefix
may.

But my understanding of the point of that loop is to catch cases
where one side may have 't' as a directory, the other side may have
it as a blob, and we have a 't' regular file during a process of
resolving a conflicted merge (perhaps the tentative resolution has
already been "git add"ed to the index).  So "git blame t" would want
to start from the "working tree state", which is made into a virtual
commit, with two "virtual" parents, one is HEAD that has 't' as a
directory but the other MERGE_HEAD may have 't' as a regular file,
so "blame" should be able to follow the history of that regular file
through the merge in progress.

If we change, like the proposed patch does, the loop to exit
immediately after we notice 't' exists in HEAD (as a tree), without
even looking at the second parent to notice that it is a regular
file there, wouldn't that change the behaviour?

> I think the "unsupported file type" message is a bit odd, but it's the
> existing one, perhaps changing it while we're at it to something like:
>
>     fatal: cannot 'blame' a directory
>
> Would be better,...

Sure.  As long as the change does not break the original use case
the loop intended to support, that is fine, and the message would be
a lot better than the "unsupported file type t".