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.
No, I wasn't worried about code efficiency, but rather programmer
effort. IOW, I expected that the second hunk that I showed could look
like this:
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
