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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> On Fri, Sep 01, 2023 at 09:43:07AM -0700, Junio C Hamano wrote:
>
>> IOW, when you notice that you need to set, say, u.contents.option of
>> an atom to C_LENGTH, shouldn't you set cmp_type of the atom to
>> FIELD_ULONG, somewhere in contents_atom_parser() and friends, and
>> everything should naturally follow, no?
>
> 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.

>> It seems that support for other cmp_types are incomplete in the
>> current code.  There are FIELD_ULONG and FIELD_TIME defined, but
>> they do not appear to be used in any way, so the cmp_ref_sorting()
>> would need to be updated to make it actually pay attention to the
>> cmp_type and perform numeric comparison.
>
> 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.

> So just this is sufficient to make contents:size work:
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 88b021dd1d..02e3b6ba82 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -583,9 +583,10 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato
>  		atom->u.contents.option = C_BARE;
>  	else if (!strcmp(arg, "body"))
>  		atom->u.contents.option = C_BODY;
> -	else if (!strcmp(arg, "size"))
> +	else if (!strcmp(arg, "size")) {
> +		atom->type = FIELD_ULONG;
>  		atom->u.contents.option = C_LENGTH;
> -	else if (!strcmp(arg, "signature"))
> +	} else if (!strcmp(arg, "signature"))
>  		atom->u.contents.option = C_SIG;
>  	else if (!strcmp(arg, "subject"))
>  		atom->u.contents.option = C_SUB;
> @@ -1885,9 +1886,10 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp
>  			v->s = strbuf_detach(&sb, NULL);
>  		} 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));
> -		else if (atom->u.contents.option == C_BODY)
> +		else if (atom->u.contents.option == C_LENGTH) {
> +			v->value = strlen(subpos);
> +			v->s = xstrfmt("%"PRIuMAX, v->value);
> +		} else if (atom->u.contents.option == C_BODY)
>  			v->s = xmemdupz(bodypos, nonsiglen);
>  		else if (atom->u.contents.option == C_SIG)
>  			v->s = xmemdupz(sigpos, siglen);

Yup, exactly.

Thanks.