Re: [PATCH v2 06/11] object API: make check_object_signature() oideq()-like, move docs

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 4, 2022, 21:15 UTC
Message-ID: <xmqqczk2l4f1.fsf@gitster.g>
In-Reply-To: <patch-v2-06.11-636a647ac51-20220204T135005Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Make the return value of check_object_signature() behave like oideq()
> and memcmp() instead of returning negative values on failure.

First of all, oideq() and memcmp() behave completely oppositely to each other. When given two same things, eq() says "true" and cmp() says 0. They use the opposite polarity. Which one do you want?

I am not enthused to see a function that used to signal success with 0 and failure with a negative value to suddenly start returning 1 for "they are identical" and 0 for "they are not the same", without changing its name or parameters to help compilers catch new call sites that still expect the old behaviour (hence can use "ret < 0", and "ret" interchangeably to check for "failure - the given data does not hash to the object name").

Show 9 quoted lines

> This reduces the boilerplate required when calling the function, and
> makes the calling code behave the same is if though we'd called
> oideq(), which is basically what we're doing here. We already had some
> callers using "f() < 0", with others using "!f()". Instead of
> declaring the latter a bug let's convert all callers to it.
>
> It is unfortunate that there's also cases where we "return -1" on
> various errors, and we can't tell those apart from the expected OID
> being less than the real OID, but this was the case already.

Well, it is not checking "less than" to begin with, no? The point of calling this function is to see if the "object_signature" (aka "hash") is correct for the object data we have, and yield yes/no (or success or failure) answer. If we encountered an error while trying to compute that answer, well, we failed to ensure that the hash matches the contents, so it is reasonable to say "no, it does not match".

> This change is rather dangerous stand-alone as we're changing the
> return semantics, but not changing the prototype.

Yes, if you know it, why do it and have reviewers spend their cycles?

If the function is renamed so that its name ends with "eq" or "matches" (e.g. object_signature_matches()), such a change may be justified, though.