Re: [PATCH] ref-filter: sort numerically when ":size" is used

From

Jeff King <peff@peff.net>

Date

Sep 1, 2023, 18:32 UTC

Message-ID

<20230901183206.GA1952051@coredump.intra.peff.net>

In-Reply-To

<xmqqr0nhpyf3.fsf@gitster.g>

On Fri, Sep 01, 2023 at 10:59:28AM -0700, Junio C Hamano wrote:

> > Yeah, I had the same thought after reading the patch. Unfortunately the
> > "type" is used only for comparison, not formatting. So you are still
> > stuck setting both v->value and v->s in grab_sub_body_contents(). It
> > feels like we could hoist that xstrfmt("%"PRIuMAX) to a higher level as
> > a preparatory refactoring. But it's not that big a deal to work around
> > it if that turns out to be hard.
> 
> Setting of the .value member happens O(N) times for the number of
> refs involved, which does not bother me.  Do you mean "when we know
> we are not sorting with size we should omit parsing the string into
> the .value member"?  If so, I think that would be nice to have.

diff --git a/ref-filter.c b/ref-filter.c
index 88b021dd1d..02b02d6813 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1886,7 +1886,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp
 		} else if (atom->u.contents.option == C_BODY_DEP)
 			v->s = xmemdupz(bodypos, bodylen);
 		else if (atom->u.contents.option == C_LENGTH)
-			v->s = xstrfmt("%"PRIuMAX, (uintmax_t)strlen(subpos));
+			v->value = strlen(subpos);
 		else if (atom->u.contents.option == C_BODY)
 			v->s = xmemdupz(bodypos, nonsiglen);
 		else if (atom->u.contents.option == C_SIG)

rather than setting both "value" and "s", and that some higher level
code would recognize "oh, this is FIELD_ULONG, so I'll format it rather
than looking at v->s". But it seems that such code does not exist. :)
All of the other spots that set v->value (e.g., objectsize), just set
both.

The ref-filter code is a weird mix of almost-object-oriented bits, giant
switch statements, and assumptions about which fields are set when.

> > I think they are covered implicitly by the "else" block of the
> > conditional that checks for FIELD_STR.
> 
> Ah, OK.  That needs to be future-proofed to force future developers
> who want to add different FIELD_FOO type to look at the comparison
> logic.  If we want to do so, it should be done as a separate topic
> for cleaning-up the mess, not as part of this effort.

Yes, agreed.

-Peff