diff mbox series

[5/6] object-name.c: make dependency on object_type order more obvious

Message ID patch-5.6-94e13611f0-20210409T082935Z-avarab@gmail.com (mailing list archive)
State Superseded
Headers show
Series {tag,object}*.c: refactorings + prep for a larger change | expand

Commit Message

Ævar Arnfjörð Bjarmason April 9, 2021, 8:32 a.m. UTC
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...).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 object-name.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Jeff King April 9, 2021, 6:36 p.m. UTC | #1
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
diff mbox series

Patch

diff --git a/object-name.c b/object-name.c
index 4d7f0c66cf..b6a7328b7a 100644
--- a/object-name.c
+++ b/object-name.c
@@ -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);
 
 	/*
 	 * Sorts by hash within the same object type, just as
@@ -425,8 +427,8 @@  static int sort_ambiguous(const void *a, const void *b, void *ctx)
 	 * cleverly) do that with modulus, since the enum assigns 1 to
 	 * commit, so tag becomes 0.
 	 */
-	a_type_sort = a_type % 4;
-	b_type_sort = b_type % 4;
+	a_type_sort = a_type % tag_type_offs;
+	b_type_sort = b_type % tag_type_offs;
 	return a_type_sort > b_type_sort ? 1 : -1;
 }