Re: [PATCH 2/4] blame: validate and peel the object names on the ignore list
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 26, 2020, 23:58 UTC
- Message-ID
- <xmqqsgb4gkf5.fsf@gitster.c.googlers.com>
- In-Reply-To
- <xmqqtuvkii1j.fsf@gitster.c.googlers.com>
Junio C Hamano <gitster@pobox.com> writes:
Show 15 quoted lines
>>> @@ -66,7 +72,8 @@ void oidset_parse_file(struct oidset *set, const char *path) >>> if (!sb.len) >>> continue; >>> >>> - if (parse_oid_hex(sb.buf, &oid, &p) || *p != '\0') >>> + if (parse_oid_hex(sb.buf, &oid, &p) || *p != '\0' || >>> + (fn && fn(&oid, cbdata))) >> >> OK, so this turns the basic all-I-know-is-hashes oidset loader into a >> flexible higher-order map function. Fun, but wise? Can't make up my >> mind. > > Fun and probably useful. It is a different matter if it is wise to > use it to (1) peel tags to commits and (2) fail on an nonexistent > object. My take on them is (1) is probably true, and (2) is Meh ;-)
If we choose to do (2) differently, we only need the following one-liner patch. I might suggest tweaking the semantics of the callback function a bit to allow it to tell the caller (i.e. oidset_parse_file_carefully()) that it wants to go on as usual, it wants to omit the object from the hashtable, it replaced the given object to something else, or it detected an error and wants to abort, and if we were doing that, we'd be returning "do not add this object to the table" signal, instead of 0 that signals "we are good doing business as usual", from here.
builtin/blame.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/builtin/blame.c b/builtin/blame.c index baa5d979cc..8d7b66e970 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -817,8 +817,19 @@ static int peel_to_commit_oid(struct object_id *oid_ret, void *cbdata) oidcpy(oid_ret, &oid); return 0; } + + /* + * We can ignore a request to ignore any nonexistent + * objects, trees and blobs by not doing anything + * special, as the blame machinery works with commits, + * so entries in the hashtable from these objects will + * never be looked up. But we do allow dereferencing + * an annotated tag, as silently ignoring a request to + * ignore v1.0.0 because it is an annotated tag is a + * bit too unfriendly to end-users. + */ if (kind != OBJ_TAG) - return -1; + return 0; obj = deref_tag(r, parse_object(r, &oid), NULL, 0); oidcpy(&oid, &obj->oid); }