Re: [[PATCH v2] 4/4] rebase: avoid computing unnecessary patch IDs

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 4, 2016, 03:00 UTC
Message-ID: <xmqqziot5jc3.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqk2fzf69w.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 17 quoted lines

> I do not think negative (or non-zero) return is an "abuse" at all.
> It is misleading in the context of the function whose name has "cmp"
> in it, but that is not the fault of this function, rather, the
> breakage is more in the API that calls a function that wants to know
> only equality a "cmp".  A in-code comment before the function name
> may be appropriate:
>
>         /*
>          * hashmap API calls hashmap_cmp_fn, but it only wants
>          * "does the key match the entry?" with 0 (matches) and
>          * non-zero (does not match).
>          */
>         static int patch_id_match(const struct patch_id *ent,
>                                   const struct patch_id *key,
>                                   const void *keydata)
>         {
>                 ...

How about this one instead (to be squashed into 4/4)?

The updated wording directly addresses the puzzlement I initially felt "This returns error() which is always negative, so comparing (A, B) would say A < B, while comparing (B, A) would say B < A. Would it cause a problem in the caller?" while reading the function by being explicit that the sign does not matter.

 patch-ids.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/patch-ids.c b/patch-ids.c
index 0a4828a..082412a 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -16,6 +16,16 @@ int commit_patch_id(struct commit *commit, struct diff_options *options,
 	return diff_flush_patch_id(options, sha1, diff_header_only);
 }
 
+/*
+ * When we cannot load the full patch-id for both commits for whatever
+ * reason, the function returns -1 (i.e. return error(...)). Despite
+ * the "cmp" in the name of this function, the caller only cares about
+ * the return value being zero (a and b are equivalent) or non-zero (a
+ * and b are different), and returning non-zero would keep both in the
+ * result, even if they actually were equivalent, in order to err on
+ * the side of safety.  The actual value being negative does not have
+ * any significance; only that it is non-zero matters.
+ */
 static int patch_id_cmp(struct patch_id *a,
 			struct patch_id *b,
 			struct diff_options *opt)