Junio C Hamano <gitster@xxxxxxxxx> writes:
> 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)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html