Re: [PATCH] bisect: don't use invalid oid as rev when starting

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 25, 2020, 16:54 UTC
Message-ID: <xmqqwo0hlrus.fsf@gitster.c.googlers.com>
In-Reply-To: <nycvar.QRO.7.76.6.2009250910200.5061@tvgsbejvaqbjf.bet>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 6 quoted lines

> A slightly bigger question is whether `git_oid_committish()` would be okay
> enough, or whether we do need `get_oidf(&oid, "%s^{commit}", arg)` (as
> your `SQUASH???` does).
>
> I'm not quite sure: aren't those two calls idempotent, with the latter
> going through some unnecessary string processing dances?

I tend to agree that get_oidf() is not quite satisfactory but it is a handy short-hand that is readily available to us to parse a string into the name of the object of type commit the string peels into.

While get_oid_X_ish() makes sure the result can be peeled to type X, it gives the outer object back to the caller to allow it to behave differently and it is up to the caller to peel the tag down to commit.

I audited all uses of get_oid_X_ish(), in the hope that perhaps we have enough callers that want to get peeled object back, and more importantly, no caller that wants the original. Then we could change these to peel for the callers, and we may be able to lose a few lines from the callers.

But unfortunately that was not the case.  A new helper

	int oid_of_type(struct object_id *result_oid,
			const char *name_text,
			enum object_type peel_to);

is a possibility, but I have a suspicion that the API in question encourages to manipulate and swap objects at their hash level for too long with little upside. If we were considering to clean the callers up, we would want to encourage them to work with in-core objects longer.

IOW, repo_peel_to_type() might be a better fit for some callers that use get_oid_X_ish() and then peel the result to obtain an object of desired type.

Thanks.