[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH v5 08/18] blame: emit a better error on 'git blame directory'
From:       Junio C Hamano <gitster () pobox ! com>
Date:       2021-03-31 23:26:22
Message-ID: xmqqwntm52ht.fsf () gitster ! g
[Download RAW message or body]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> 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.

> The main point of this test is to assert that we're not doing
> something uniquely bad when in a conflicted merge. See

"this test" refers to the logic "it is OK to skip the check if one
of the parents does have it as a blob", introduced in 9aeaab68
(blame: allow "blame file" in the middle of a conflicted merge,
2012-09-11)?


> -		if (!get_tree_entry(r, commit_oid, path, &blob_oid, &mode) &&
> -		    oid_object_info(r, &blob_oid, NULL) == OBJ_BLOB)
> +		if (!get_tree_entry(r, commit_oid, path, &blob_oid, &mode))
>  			return;
>  	}

At least, the original logic makes sense to me in that if an early
parent has the path as a directory we do not declare it is OK but
keep going until we find a blob in a later parent before deciding to
short-cut.  I am not sure what the updated "in this case we can
bypass the real check" condition even means.  Mechanically, it says
"if any parent has the path as any filesystem entity, even if it
were a directory, then it is OK", but why?

Thanks.
[prev in list] [next in list] [prev in thread] [next in thread]