On Fri, Sep 01, 2023 at 02:32:06PM -0400, Jeff King wrote:
> 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)
This looks very tempting, although too good to be true with the current
ref-filter I guess, as you explain below.
> 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.
This was also one of the reasons why I decided to set both v->value
and v->s, that is because "objectsize" was implemented in a similar
fashion. Although I left "cmp_type" field untouched for the reasons
below.
> > > 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.
What I also find weird is the fact that we assign a "cmp_type" to the
whole atom. Like "contents" is FIELD_STR and "objectsize" is "FIELD_ULONG"
in "valid_atom". This seems wrong because the options of the atoms should be
the ones deciding the "cmp_type", no?
I wanted to leave the "cmp_type" field of the atom untouched because that
would mess up this "global" setting of "contents" to be a "FIELD_STR" (or
even "raw" for that matter). Although that seems like a bad idea, after
I've read Junio's and your comments.
Thanks
>
> Yes, agreed.
>
> -Peff
