On Fri, Apr 09, 2021 at 10:32:53AM +0200, Ævar Arnfjörð Bjarmason wrote:
> Add an assert to make it more obvious that we were effectively
> hardcoding OBJ_TAG in sort_ambiguous() as "4".
>
> I wrote this code in 5cc044e0257 (get_short_oid: sort ambiguous
> objects by type, then SHA-1, 2018-05-10), there was already a comment
> about this magic, but let's make sure that someone doing a potential
> reordering of "enum object_type" in the future would notice it
> breaking this function (and probably a bunch of other things...).
Yeah, those object type values are used for the on-disk formats, so
quite a lot would break.
> @@ -408,6 +408,8 @@ static int sort_ambiguous(const void *a, const void *b, void *ctx)
> enum object_type b_type = oid_object_info(sort_ambiguous_repo, b, NULL);
> enum object_type a_type_sort;
> enum object_type b_type_sort;
> + const enum object_type tag_type_offs = OBJ_TAG - OBJ_NONE;
> + assert(tag_type_offs == 4);
This protects us against shifting of the values or reordering within the
main 4 types, but it doesn't protect against new types, nor reordering
in which the main 4 types are no longer contiguous. E.g.:
enum object_type {
OBJ_NONE = 0,
OBJ_REF_DELTA = 1,
OBJ_OFS_DELTA = 2,
OBJ_COMMIT = 3,
OBJ_TAG = 4,
OBJ_BLOB = 5,
OBJ_TREE = 6,
};
would be wrong. I dunno. I guess in some sense I am glad to see an
attempt at automated enforcement of assumptions. But I think if we are
worried about the object_type enum changing, we'd do better to write
this function in a less-clever way.
/* sort tags before anything else */
if (a_type == OBJ_TAG)
a_type = 0;
if (b_type == OBJ_TAG)
b_type = 0;
Of course that is still assuming that normal values are all positive,
but that seems reasonable. If you really wanted to be agnostic, you
could assign the minimum value. But you can't easily know that for an
enum. So you'd want to store them as ints (reversing your previous
commit!) and using INT_MIN.
The conditional probably performs less well in a tight loop, but I doubt
that matters for the size of array we expect to sort (if we cared about
performance we would not call oid_object_info() twice inside the
comparator!).
-Peff
