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

From: Kousik Sanagavarapu <five231003@gmail.com>
Date: Sep 1, 2023, 18:59 UTC
Message-ID: <ZPI0e1XzZrDV2fJk@five231003>
In-Reply-To: <20230901183206.GA1952051@coredump.intra.peff.net>

On Fri, Sep 01, 2023 at 02:32:06PM -0400, Jeff King wrote:

Show 31 quoted lines

> 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.

Show 5 quoted lines

> 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.

Show 7 quoted lines

> > > 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