{"thread":{"id":"60181","subject":"[PATCH] ref-filter: sort numerically when \":size\" is used","startedAt":"2023-09-01T14:27:33Z","lastAt":"2023-09-02T22:19:49Z","messageCount":14,"participants":["Kousik Sanagavarapu","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"481290","messageId":"20230901142624.12063-1-five231003@gmail.com","threadId":"60181","inReplyTo":null,"subject":"[PATCH] ref-filter: sort numerically when \":size\" is used","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-09-01T14:24:54Z","receivedAt":"2023-09-01T14:27:33Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"Atoms like \"raw\" and \"contents\" have a \":size\" option which can be used\nto know the size of the data. Since these atoms have the cmp_type\nFIELD_STR, they are sorted alphabetically from 'a' to 'z' and '0' to\n'9'. Meaning, even when the \":size\" option is used and what we\nultimatlely have is numbers, we still sort alphabetically.\n\nFor example, consider the the following case in a repo\n\nrefname\t\t\tcontents:size\t\traw:size\n=======\t\t\t=============\t\t========\nrefs/heads/branch1\t1130\t\t\t1210\nrefs/heads/master\t300\t\t\t410\nrefs/tags/v1.0\t\t140\t\t\t260\n\nSorting with \"--format=\"%(refname) %(contents:size) --sort=contents:size\"\nwould give\n\nrefs/heads/branch1 1130\nrefs/tags/v1.0.0 140\nrefs/heads/master 300\n\nwhich is an alphabetic sort, while what one might really expect is\n\nrefs/tags/v1.0.0 140\nrefs/heads/master 300\nrefs/heads/branch1 1130\n\nwhich is a numeric sort (that is, a \"$ sort -n file\" as opposed to a\n\"$ sort file\", where \"file\" contains only the \"contents:size\" or\n\"raw:size\" info, each of which is on a newline).\n\nSame is the case with \"--sort=raw:size\".\n\nSo, sort numerically whenever the sort is done with \"contents:size\" or\n\"raw:size\" and do it the normal alphabetic way when \"contents\" or \"raw\"\nare used with some other option (they are FIELD_STR anyways).\n\nSigned-off-by: Kousik Sanagavarapu <five231003@gmail.com>\n---\n ref-filter.c            | 20 +++++++++++++++-----\n t/t6300-for-each-ref.sh | 15 +++++++++++++--\n 2 files changed, 28 insertions(+), 7 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 1bfaf20fbf..5d7bea5f23 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -932,7 +932,13 @@ struct atom_value {\n \tssize_t s_size;\n \tint (*handler)(struct atom_value *atomv, struct ref_formatting_state *state,\n \t\t       struct strbuf *err);\n-\tuintmax_t value; /* used for sorting when not FIELD_STR */\n+\n+\t/*\n+\t * Used for sorting when not FIELD_STR or when FIELD_STR but the\n+\t * sort should be numeric and not alphabetic.\n+\t */\n+\tuintmax_t value;\n+\n \tstruct used_atom *atom;\n };\n \n@@ -1857,7 +1863,8 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp\n \t\t\t\tv->s = xmemdupz(buf, buf_size);\n \t\t\t\tv->s_size = buf_size;\n \t\t\t} else if (atom->u.raw_data.option == RAW_LENGTH) {\n-\t\t\t\tv->s = xstrfmt(\"%\"PRIuMAX, (uintmax_t)buf_size);\n+\t\t\t\tv->value = (uintmax_t)buf_size;\n+\t\t\t\tv->s = xstrfmt(\"%\"PRIuMAX, v->value);\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\n@@ -1883,8 +1890,10 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp\n \t\t\tv->s = strbuf_detach(&sb, NULL);\n \t\t} else if (atom->u.contents.option == C_BODY_DEP)\n \t\t\tv->s = xmemdupz(bodypos, bodylen);\n-\t\telse if (atom->u.contents.option == C_LENGTH)\n-\t\t\tv->s = xstrfmt(\"%\"PRIuMAX, (uintmax_t)strlen(subpos));\n+\t\telse if (atom->u.contents.option == C_LENGTH) {\n+\t\t\tv->value = (uintmax_t)strlen(subpos);\n+\t\t\tv->s = xstrfmt(\"%\"PRIuMAX, v->value);\n+\t\t}\n \t\telse if (atom->u.contents.option == C_BODY)\n \t\t\tv->s = xmemdupz(bodypos, nonsiglen);\n \t\telse if (atom->u.contents.option == C_SIG)\n@@ -2265,6 +2274,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n \n \t\tv->s_size = ATOM_SIZE_UNSPECIFIED;\n \t\tv->handler = append_atom;\n+\t\tv->value = 0;\n \t\tv->atom = atom;\n \n \t\tif (*name == '*') {\n@@ -2986,7 +2996,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n \t\tcmp_detached_head = 1;\n \t} else if (s->sort_flags & REF_SORTING_VERSION) {\n \t\tcmp = versioncmp(va->s, vb->s);\n-\t} else if (cmp_type == FIELD_STR) {\n+\t} else if (cmp_type == FIELD_STR && !va->value && !vb->value) {\n \t\tif (va->s_size < 0 && vb->s_size < 0) {\n \t\t\tint (*cmp_fn)(const char *, const char *);\n \t\t\tcmp_fn = s->sort_flags & REF_SORTING_ICASE\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex aa3c7c03c4..7b943fd34c 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -1017,16 +1017,16 @@ test_expect_success 'Verify sorts with raw' '\n test_expect_success 'Verify sorts with raw:size' '\n \tcat >expected <<-EOF &&\n \trefs/myblobs/blob8\n-\trefs/myblobs/first\n \trefs/myblobs/blob7\n-\trefs/heads/main\n \trefs/myblobs/blob4\n \trefs/myblobs/blob1\n \trefs/myblobs/blob2\n \trefs/myblobs/blob3\n \trefs/myblobs/blob5\n \trefs/myblobs/blob6\n+\trefs/myblobs/first\n \trefs/mytrees/first\n+\trefs/heads/main\n \tEOF\n \tgit for-each-ref --format=\"%(refname)\" --sort=raw:size \\\n \t\trefs/heads/main refs/myblobs/ refs/mytrees/first >actual &&\n@@ -1138,6 +1138,17 @@ test_expect_success 'for-each-ref --format compare with cat-file --batch' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'verify sorts with contents:size' '\n+\tcat >expect <<-\\EOF &&\n+\trefs/heads/main\n+\trefs/heads/newtag\n+\trefs/heads/ambiguous\n+\tEOF\n+\tgit for-each-ref --format=\"%(refname)\" \\\n+\t\t--sort=contents:size refs/heads/ >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'set up multiple-sort tags' '\n \tfor when in 100000 200000\n \tdo\n-- \n2.42.0.51.g5dc72c0fbc.dirty\n\n"},{"id":"481298","messageId":"xmqqa5u5rgis.fsf@gitster.g","threadId":"60181","inReplyTo":"20230901142624.12063-1-five231003@gmail.com","subject":"Re: [PATCH] ref-filter: sort numerically when \":size\" is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-01T16:43:07Z","receivedAt":"2023-09-01T16:43:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kousik Sanagavarapu <five231003@gmail.com> writes:\n\n> Atoms like \"raw\" and \"contents\" have a \":size\" option which can be used\n> to know the size of the data. Since these atoms have the cmp_type\n> FIELD_STR, they are sorted alphabetically from 'a' to 'z' and '0' to\n> '9'. Meaning, even when the \":size\" option is used and what we\n> ultimatlely have is numbers, we still sort alphabetically.\n\nThere are other cmp_types already defined, like ULONG and TIME.  How\nare they used and affect the comparison?  Naively, :size sounds like\na good candidate to compare as ULONG, as it cannot be negative even\nthough 0 is a valid size.\n\nI understand and agree with the motivation, but the implementation\nlooks puzzling.\n\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 1bfaf20fbf..5d7bea5f23 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -932,7 +932,13 @@ struct atom_value {\n>  \tssize_t s_size;\n>  \tint (*handler)(struct atom_value *atomv, struct ref_formatting_state *state,\n>  \t\t       struct strbuf *err);\n> -\tuintmax_t value; /* used for sorting when not FIELD_STR */\n> +\n> +\t/*\n> +\t * Used for sorting when not FIELD_STR or when FIELD_STR but the\n> +\t * sort should be numeric and not alphabetic.\n> +\t */\n> +\tuintmax_t value;\n\nThis does not explain why we cannot make <anything>:size FIELD_ULONG\nfor <anything> that is of FIELD_STR type, though.  IOW, why such a\nstrange \"when FIELD_STR but the sort should be numeric\" is needed?\nIf you have a <size>, shouldn't it always be numeric?\n\nIOW, when you notice that you need to set, say, u.contents.option of\nan atom to C_LENGTH, shouldn't you set cmp_type of the atom to\nFIELD_ULONG, somewhere in contents_atom_parser() and friends, and\neverything should naturally follow, no?\n\nIt seems that support for other cmp_types are incomplete in the\ncurrent code.  There are FIELD_ULONG and FIELD_TIME defined, but\nthey do not appear to be used in any way, so the cmp_ref_sorting()\nwould need to be updated to make it actually pay attention to the\ncmp_type and perform numeric comparison.\n\n> @@ -1883,8 +1890,10 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp\n>  \t\t\tv->s = strbuf_detach(&sb, NULL);\n>  \t\t} else if (atom->u.contents.option == C_BODY_DEP)\n>  \t\t\tv->s = xmemdupz(bodypos, bodylen);\n> -\t\telse if (atom->u.contents.option == C_LENGTH)\n> -\t\t\tv->s = xstrfmt(\"%\"PRIuMAX, (uintmax_t)strlen(subpos));\n> +\t\telse if (atom->u.contents.option == C_LENGTH) {\n> +\t\t\tv->value = (uintmax_t)strlen(subpos);\n> +\t\t\tv->s = xstrfmt(\"%\"PRIuMAX, v->value);\n> +\t\t}\n\nWe should take a note that all of these v->value are *per* *item*\nthat will be sorted.\n\n> @@ -2986,7 +2996,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n>  \t\tcmp_detached_head = 1;\n>  \t} else if (s->sort_flags & REF_SORTING_VERSION) {\n>  \t\tcmp = versioncmp(va->s, vb->s);\n> -\t} else if (cmp_type == FIELD_STR) {\n> +\t} else if (cmp_type == FIELD_STR && !va->value && !vb->value) {\n\nTwo refs may point at an empty object with zero length, i.e.  for\nthem, !v->value is true, and another ref may point at a non-empty\nobject.  The two empty refs are compared with an algorithm different\nfrom the algorithm used to compare the empty ref and the non-empty\nref.  Isn't this broken as a comparison function to be given to\nQSORT(), which must be transitive (e.g. if A < B and B < C, then it\nshould be guaranteed that A < C and you do not have to compare A and\nC)?\n\nIOW, the choice of the comparison algorithm should not depend on an\nattribute (like value or s) that is specific to the item being\ncompared.  Things like cmp_type that is defined at the used_atom\nlefvel to make the sorting function stable, I would think.\n"},{"id":"481304","messageId":"20230901174540.GB1947546@coredump.intra.peff.net","threadId":"60181","inReplyTo":"xmqqa5u5rgis.fsf@gitster.g","subject":"Re: [PATCH] ref-filter: sort numerically when \":size\" is used","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-01T17:45:40Z","receivedAt":"2023-09-01T17:45:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 01, 2023 at 09:43:07AM -0700, Junio C Hamano wrote:\n\n> IOW, when you notice that you need to set, say, u.contents.option of\n> an atom to C_LENGTH, shouldn't you set cmp_type of the atom to\n> FIELD_ULONG, somewhere in contents_atom_parser() and friends, and\n> everything should naturally follow, no?\n\nYeah, I had the same thought after reading the patch. Unfortunately the\n\"type\" is used only for comparison, not formatting. So you are still\nstuck setting both v->value and v->s in grab_sub_body_contents(). It\nfeels like we could hoist that xstrfmt(\"%\"PRIuMAX) to a higher level as\na preparatory refactoring. But it's not that big a deal to work around\nit if that turns out to be hard.\n\n> It seems that support for other cmp_types are incomplete in the\n> current code.  There are FIELD_ULONG and FIELD_TIME defined, but\n> they do not appear to be used in any way, so the cmp_ref_sorting()\n> would need to be updated to make it actually pay attention to the\n> cmp_type and perform numeric comparison.\n\nI think they are covered implicitly by the \"else\" block of the\nconditional that checks for FIELD_STR.\n\nSo just this is sufficient to make contents:size work:\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 88b021dd1d..02e3b6ba82 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -583,9 +583,10 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato\n \t\tatom->u.contents.option = C_BARE;\n \telse if (!strcmp(arg, \"body\"))\n \t\tatom->u.contents.option = C_BODY;\n-\telse if (!strcmp(arg, \"size\"))\n+\telse if (!strcmp(arg, \"size\")) {\n+\t\tatom->type = FIELD_ULONG;\n \t\tatom->u.contents.option = C_LENGTH;\n-\telse if (!strcmp(arg, \"signature\"))\n+\t} else if (!strcmp(arg, \"signature\"))\n \t\tatom->u.contents.option = C_SIG;\n \telse if (!strcmp(arg, \"subject\"))\n \t\tatom->u.contents.option = C_SUB;\n@@ -1885,9 +1886,10 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp\n \t\t\tv->s = strbuf_detach(&sb, NULL);\n \t\t} else if (atom->u.contents.option == C_BODY_DEP)\n \t\t\tv->s = xmemdupz(bodypos, bodylen);\n-\t\telse if (atom->u.contents.option == C_LENGTH)\n-\t\t\tv->s = xstrfmt(\"%\"PRIuMAX, (uintmax_t)strlen(subpos));\n-\t\telse if (atom->u.contents.option == C_BODY)\n+\t\telse if (atom->u.contents.option == C_LENGTH) {\n+\t\t\tv->value = strlen(subpos);\n+\t\t\tv->s = xstrfmt(\"%\"PRIuMAX, v->value);\n+\t\t} else if (atom->u.contents.option == C_BODY)\n \t\t\tv->s = xmemdupz(bodypos, nonsiglen);\n \t\telse if (atom->u.contents.option == C_SIG)\n \t\t\tv->s = xmemdupz(sigpos, siglen);\n\n-Peff\n"},{"id":"481305","messageId":"xmqqr0nhpyf3.fsf@gitster.g","threadId":"60181","inReplyTo":"20230901174540.GB1947546@coredump.intra.peff.net","subject":"Re: [PATCH] ref-filter: sort numerically when \":size\" is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-01T17:59:28Z","receivedAt":"2023-09-01T17:59:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Sep 01, 2023 at 09:43:07AM -0700, Junio C Hamano wrote:\n>\n>> IOW, when you notice that you need to set, say, u.contents.option of\n>> an atom to C_LENGTH, shouldn't you set cmp_type of the atom to\n>> FIELD_ULONG, somewhere in contents_atom_parser() and friends, and\n>> everything should naturally follow, no?\n>\n> Yeah, I had the same thought after reading the patch. Unfortunately the\n> \"type\" is used only for comparison, not formatting. So you are still\n> stuck setting both v->value and v->s in grab_sub_body_contents(). It\n> feels like we could hoist that xstrfmt(\"%\"PRIuMAX) to a higher level as\n> a preparatory refactoring. But it's not that big a deal to work around\n> it if that turns out to be hard.\n\nSetting of the .value member happens O(N) times for the number of\nrefs involved, which does not bother me.  Do you mean \"when we know\nwe are not sorting with size we should omit parsing the string into\nthe .value member\"?  If so, I think that would be nice to have.\n\n>> It seems that support for other cmp_types are incomplete in the\n>> current code.  There are FIELD_ULONG and FIELD_TIME defined, but\n>> they do not appear to be used in any way, so the cmp_ref_sorting()\n>> would need to be updated to make it actually pay attention to the\n>> cmp_type and perform numeric comparison.\n>\n> I think they are covered implicitly by the \"else\" block of the\n> conditional that checks for FIELD_STR.\n\nAh, OK.  That needs to be future-proofed to force future developers\nwho want to add different FIELD_FOO type to look at the comparison\nlogic.  If we want to do so, it should be done as a separate topic\nfor cleaning-up the mess, not as part of this effort.\n\n> So just this is sufficient to make contents:size work:\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 88b021dd1d..02e3b6ba82 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -583,9 +583,10 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato\n>  \t\tatom->u.contents.option = C_BARE;\n>  \telse if (!strcmp(arg, \"body\"))\n>  \t\tatom->u.contents.option = C_BODY;\n> -\telse if (!strcmp(arg, \"size\"))\n> +\telse if (!strcmp(arg, \"size\")) {\n> +\t\tatom->type = FIELD_ULONG;\n>  \t\tatom->u.contents.option = C_LENGTH;\n> -\telse if (!strcmp(arg, \"signature\"))\n> +\t} else if (!strcmp(arg, \"signature\"))\n>  \t\tatom->u.contents.option = C_SIG;\n>  \telse if (!strcmp(arg, \"subject\"))\n>  \t\tatom->u.contents.option = C_SUB;\n> @@ -1885,9 +1886,10 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp\n>  \t\t\tv->s = strbuf_detach(&sb, NULL);\n>  \t\t} else if (atom->u.contents.option == C_BODY_DEP)\n>  \t\t\tv->s = xmemdupz(bodypos, bodylen);\n> -\t\telse if (atom->u.contents.option == C_LENGTH)\n> -\t\t\tv->s = xstrfmt(\"%\"PRIuMAX, (uintmax_t)strlen(subpos));\n> -\t\telse if (atom->u.contents.option == C_BODY)\n> +\t\telse if (atom->u.contents.option == C_LENGTH) {\n> +\t\t\tv->value = strlen(subpos);\n> +\t\t\tv->s = xstrfmt(\"%\"PRIuMAX, v->value);\n> +\t\t} else if (atom->u.contents.option == C_BODY)\n>  \t\t\tv->s = xmemdupz(bodypos, nonsiglen);\n>  \t\telse if (atom->u.contents.option == C_SIG)\n>  \t\t\tv->s = xmemdupz(sigpos, siglen);\n\nYup, exactly.\n\nThanks.\n"},{"id":"481308","messageId":"20230901183206.GA1952051@coredump.intra.peff.net","threadId":"60181","inReplyTo":"xmqqr0nhpyf3.fsf@gitster.g","subject":"Re: [PATCH] ref-filter: sort numerically when \":size\" is used","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-01T18:32:06Z","receivedAt":"2023-09-01T18:32:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 01, 2023 at 10:59:28AM -0700, Junio C Hamano wrote:\n\n> > Yeah, I had the same thought after reading the patch. Unfortunately the\n> > \"type\" is used only for comparison, not formatting. So you are still\n> > stuck setting both v->value and v->s in grab_sub_body_contents(). It\n> > feels like we could hoist that xstrfmt(\"%\"PRIuMAX) to a higher level as\n> > a preparatory refactoring. But it's not that big a deal to work around\n> > it if that turns out to be hard.\n> \n> Setting of the .value member happens O(N) times for the number of\n> refs involved, which does not bother me.  Do you mean \"when we know\n> we are not sorting with size we should omit parsing the string into\n> the .value member\"?  If so, I think that would be nice to have.\n\nNo, I wasn't worried about code efficiency, but rather programmer\neffort. IOW, I expected that the second hunk that I showed could look\nlike this:\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 88b021dd1d..02b02d6813 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1886,7 +1886,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp\n \t\t} else if (atom->u.contents.option == C_BODY_DEP)\n \t\t\tv->s = xmemdupz(bodypos, bodylen);\n \t\telse if (atom->u.contents.option == C_LENGTH)\n-\t\t\tv->s = xstrfmt(\"%\"PRIuMAX, (uintmax_t)strlen(subpos));\n+\t\t\tv->value = strlen(subpos);\n \t\telse if (atom->u.contents.option == C_BODY)\n \t\t\tv->s = xmemdupz(bodypos, nonsiglen);\n \t\telse if (atom->u.contents.option == C_SIG)\n\nrather than setting both \"value\" and \"s\", and that some higher level\ncode would recognize \"oh, this is FIELD_ULONG, so I'll format it rather\nthan looking at v->s\". But it seems that such code does not exist. :)\nAll of the other spots that set v->value (e.g., objectsize), just set\nboth.\n\nThe ref-filter code is a weird mix of almost-object-oriented bits, giant\nswitch statements, and assumptions about which fields are set when.\n\n> > I think they are covered implicitly by the \"else\" block of the\n> > conditional that checks for FIELD_STR.\n> \n> Ah, OK.  That needs to be future-proofed to force future developers\n> who want to add different FIELD_FOO type to look at the comparison\n> logic.  If we want to do so, it should be done as a separate topic\n> for cleaning-up the mess, not as part of this effort.\n\nYes, agreed.\n\n-Peff\n"},{"id":"481309","messageId":"ZPI0e1XzZrDV2fJk@five231003","threadId":"60181","inReplyTo":"20230901183206.GA1952051@coredump.intra.peff.net","subject":"Re: [PATCH] ref-filter: sort numerically when \":size\" is used","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-09-01T18:59:07Z","receivedAt":"2023-09-01T18:59:23Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"On Fri, Sep 01, 2023 at 02:32:06PM -0400, Jeff King wrote:\n> On Fri, Sep 01, 2023 at 10:59:28AM -0700, Junio C Hamano wrote:\n> \n> > > Yeah, I had the same thought after reading the patch. Unfortunately the\n> > > \"type\" is used only for comparison, not formatting. So you are still\n> > > stuck setting both v->value and v->s in grab_sub_body_contents(). It\n> > > feels like we could hoist that xstrfmt(\"%\"PRIuMAX) to a higher level as\n> > > a preparatory refactoring. But it's not that big a deal to work around\n> > > it if that turns out to be hard.\n> > \n> > Setting of the .value member happens O(N) times for the number of\n> > refs involved, which does not bother me.  Do you mean \"when we know\n> > we are not sorting with size we should omit parsing the string into\n> > the .value member\"?  If so, I think that would be nice to have.\n> \n> No, I wasn't worried about code efficiency, but rather programmer\n> effort. IOW, I expected that the second hunk that I showed could look\n> like this:\n> \n> diff --git a/ref-filter.c b/ref-filter.c\n> index 88b021dd1d..02b02d6813 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1886,7 +1886,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp\n>  \t\t} else if (atom->u.contents.option == C_BODY_DEP)\n>  \t\t\tv->s = xmemdupz(bodypos, bodylen);\n>  \t\telse if (atom->u.contents.option == C_LENGTH)\n> -\t\t\tv->s = xstrfmt(\"%\"PRIuMAX, (uintmax_t)strlen(subpos));\n> +\t\t\tv->value = strlen(subpos);\n>  \t\telse if (atom->u.contents.option == C_BODY)\n>  \t\t\tv->s = xmemdupz(bodypos, nonsiglen);\n>  \t\telse if (atom->u.contents.option == C_SIG)\n\nThis looks very tempting, although too good to be true with the current\nref-filter I guess, as you explain below.\n\n> rather than setting both \"value\" and \"s\", and that some higher level\n> code would recognize \"oh, this is FIELD_ULONG, so I'll format it rather\n> than looking at v->s\". But it seems that such code does not exist. :)\n> All of the other spots that set v->value (e.g., objectsize), just set\n> both.\n\nThis was also one of the reasons why I decided to set both v->value\nand v->s, that is because \"objectsize\" was implemented in a similar\nfashion. Although I left \"cmp_type\" field untouched for the reasons\nbelow.\n \n> > > I think they are covered implicitly by the \"else\" block of the\n> > > conditional that checks for FIELD_STR.\n> > \n> > Ah, OK.  That needs to be future-proofed to force future developers\n> > who want to add different FIELD_FOO type to look at the comparison\n> > logic.  If we want to do so, it should be done as a separate topic\n> > for cleaning-up the mess, not as part of this effort.\n\nWhat I also find weird is the fact that we assign a \"cmp_type\" to the\nwhole atom. Like \"contents\" is FIELD_STR and \"objectsize\" is \"FIELD_ULONG\"\nin \"valid_atom\". This seems wrong because the options of the atoms should be\nthe ones deciding the \"cmp_type\", no?\n\nI wanted to leave the \"cmp_type\" field of the atom untouched because that\nwould mess up this \"global\" setting of \"contents\" to be a \"FIELD_STR\" (or\neven \"raw\" for that matter). Although that seems like a bad idea, after\nI've read Junio's and your comments.\n\nThanks\n\n> \n> Yes, agreed.\n> \n> -Peff\n"},{"id":"481310","messageId":"20230901191639.GA1955435@coredump.intra.peff.net","threadId":"60181","inReplyTo":"ZPI0e1XzZrDV2fJk@five231003","subject":"Re: [PATCH] ref-filter: sort numerically when \":size\" is used","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-01T19:16:39Z","receivedAt":"2023-09-01T19:18:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 02, 2023 at 12:29:07AM +0530, Kousik Sanagavarapu wrote:\n\n> > > > I think they are covered implicitly by the \"else\" block of the\n> > > > conditional that checks for FIELD_STR.\n> > > \n> > > Ah, OK.  That needs to be future-proofed to force future developers\n> > > who want to add different FIELD_FOO type to look at the comparison\n> > > logic.  If we want to do so, it should be done as a separate topic\n> > > for cleaning-up the mess, not as part of this effort.\n> \n> What I also find weird is the fact that we assign a \"cmp_type\" to the\n> whole atom. Like \"contents\" is FIELD_STR and \"objectsize\" is \"FIELD_ULONG\"\n> in \"valid_atom\". This seems wrong because the options of the atoms should be\n> the ones deciding the \"cmp_type\", no?\n\nI think the data structure is a little confusing if you haven't worked\nwith it before. But basically each \"atom\" corresponds to a single \"%()\"\nblock in the format. So if you ran:\n\n  git for-each-ref --format=\"%(contents:size) %(contents:body)\"\n\nyou'd have two atoms in the used_atom struct: one for the size and one\nfor the body.\n\nIMHO the code would be a lot easier to work with if the atoms were\nstructured as a parse tree with child pointers (especially when you get\ninto things like \"if\" that have sub-expressions). I think one of the\nreasons that used_atom is an array is to de-duplicate repeated mentions\n(so if you formatted \"%(foo) %(foo)\" it would only have to store the\ncomputed value once).\n\nBut I think that is the wrong way to optimize it. We shouldn't be\nstoring any strings per-atom, but rather walking the parse tree to\nproduce a single output buffer. And the values should be cheap to fill\nin, because we should parse the object as necessary up front. This is\nmore or less the way the pretty.c parser does it.\n\nBut that is all quite a large tangent from what you're working on, and\nwould probably be a ground-up rewrite of the formatting code. You can\nsafely ignore my rant for the purposes of your patch. ;)\n\n> I wanted to leave the \"cmp_type\" field of the atom untouched because that\n> would mess up this \"global\" setting of \"contents\" to be a \"FIELD_STR\" (or\n> even \"raw\" for that matter). Although that seems like a bad idea, after\n> I've read Junio's and your comments.\n\nYeah, I agree that would be a problem if there were one global\n\"contents\". But we are allocating a new atom struct on the fly for the\ncontents:size directive that we parse, and so on.\n\n-Peff\n"},{"id":"481311","messageId":"xmqqcyz1psnb.fsf@gitster.g","threadId":"60181","inReplyTo":"ZPI0e1XzZrDV2fJk@five231003","subject":"Re: [PATCH] ref-filter: sort numerically when \":size\" is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-01T20:04:08Z","receivedAt":"2023-09-01T20:04:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kousik Sanagavarapu <five231003@gmail.com> writes:\n\n> What I also find weird is the fact that we assign a \"cmp_type\" to the\n> whole atom. Like \"contents\" is FIELD_STR and \"objectsize\" is \"FIELD_ULONG\"\n> in \"valid_atom\". This seems wrong because the options of the atoms should be\n> the ones deciding the \"cmp_type\", no?\n\nI do not quite get where your confusion comes from.\n\nThe use of valid_atom[] purely for catalogging things like\n\"contents\", \"refname\", etc., before specialization, as opposed to\nused_atom[] that lists the actual specialized form of the atoms used\nin the format string.  If you refer to \"contents:body\" and\n\"contents:size\" in your format string, they become two entries in\nused_atom[], both of which refer to the same atom_type obtained by\nconsulting the same entry in the valid_atom[] array.\n\nThe specialization between \"contents:body\" and \"contents:size\" must\nbe captured somewhere, and that happens by using two used_atom[]\nentries.  There will be one \"struct atom_value\" for each of these\nplaceholders, each of which refers to its own used_atom that knows\nfor which variant of \"contents\" it was created.  Of course, these\ntwo \"struct atom_value\" instances will have different content string\nfor the same ref (one stores the body part of the string, the other\nstores the size of the contents).\n\n> I wanted to leave the \"cmp_type\" field of the atom untouched because that\n> would mess up this \"global\" setting of \"contents\" to be a \"FIELD_STR\" (or\n> even \"raw\" for that matter).\n\nWe are not talking about futzing with valid_atom[] array.  \n\nBecause the used_atom[] array is designed to be used to capture the\ndifferences among \"contents\" vs \"contents:body\" vs \"contents:size\",\nwhat types of entities the values that uses an entry in used_atom[]\narray (i.e. an instance of \"struct atom_value\") should be decided\nusing the information stored there.\n\nI agree that Peff's \"the value for 'contents:size' we know is\nnumeric, so only store the numeric value in atom_value and let the\noutput logic handle that using cmp_type information\" sound very\ntempting.  If we were to tackle it, however, I think it should be a\nseparate topic.\n\nIn any case, it was very good that you noticed we do not sort\nnumerically when sorting by size (I guess our sort by timestamp\nweren't affected only because we have been lucky?).  Thanks for\nstarting this topic.\n\n\n"},{"id":"481312","messageId":"xmqq7cp9prux.fsf@gitster.g","threadId":"60181","inReplyTo":"20230901191639.GA1955435@coredump.intra.peff.net","subject":"Re: [PATCH] ref-filter: sort numerically when \":size\" is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-01T20:21:10Z","receivedAt":"2023-09-01T20:21:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> But I think that is the wrong way to optimize it. We shouldn't be\n> storing any strings per-atom, but rather walking the parse tree to\n> produce a single output buffer. And the values should be cheap to fill\n> in, because we should parse the object as necessary up front. This is\n> more or less the way the pretty.c parser does it.\n\nI thought \"as necessary\" may be a bit tricky as populate_value()\nwere taught to omit doing the whole get_object() thing when the\nvalues for used_atom[] are all computable without parsing the object\nat all, but it seems that over time the populate_value() callchain\nhas degraded sufficiently to unconditionally call get_object() these\ndays, so I agree that the arrangement does not have much optimization\nvalue, at least in the current code.\n\n"},{"id":"481313","messageId":"20230901204006.GA1960498@coredump.intra.peff.net","threadId":"60181","inReplyTo":"xmqqcyz1psnb.fsf@gitster.g","subject":"Re: [PATCH] ref-filter: sort numerically when \":size\" is used","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-01T20:40:06Z","receivedAt":"2023-09-01T20:40:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 01, 2023 at 01:04:08PM -0700, Junio C Hamano wrote:\n\n> In any case, it was very good that you noticed we do not sort\n> numerically when sorting by size (I guess our sort by timestamp\n> weren't affected only because we have been lucky?).  Thanks for\n> starting this topic.\n\nI think the date code works as expected, and by design. It's just that\nthe code is a little confusing. :) Something like %(committerdate) gets\na used_atom with FIELD_TIME, and the \"grab\" function sets v->value to\nthe unix-epoch timestamp. And then the comparison function uses\nv->value, since it's not FIELD_STR.\n\nIt's a little hard to follow the code that fills in v->value, but the\ncall stack is roughly grab_values() -> grab_person() -> grab_date().\n\n-Peff\n"},{"id":"481314","messageId":"20230901205145.GB1960498@coredump.intra.peff.net","threadId":"60181","inReplyTo":"xmqq7cp9prux.fsf@gitster.g","subject":"Re: [PATCH] ref-filter: sort numerically when \":size\" is used","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-01T20:51:45Z","receivedAt":"2023-09-01T20:51:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 01, 2023 at 01:21:10PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > But I think that is the wrong way to optimize it. We shouldn't be\n> > storing any strings per-atom, but rather walking the parse tree to\n> > produce a single output buffer. And the values should be cheap to fill\n> > in, because we should parse the object as necessary up front. This is\n> > more or less the way the pretty.c parser does it.\n> \n> I thought \"as necessary\" may be a bit tricky as populate_value()\n> were taught to omit doing the whole get_object() thing when the\n> values for used_atom[] are all computable without parsing the object\n> at all, but it seems that over time the populate_value() callchain\n> has degraded sufficiently to unconditionally call get_object() these\n> days, so I agree that the arrangement does not have much optimization\n> value, at least in the current code.\n\nNo, I think we still do that optimization. When parsing the format\nstring, the parser function for each atom sets fields in an object_info\nstruct to indicate what we're interested in. Then for each ref, we call\npopulate_value(). If that object_info doesn't need anything (we\nbyte-wise compare it to an empty dummy struct), then we return early,\nbefore calling get_object().\n\nAnd that optimization is very important to retain; it makes a format\nlike %(refname) an order of magnitude faster.\n\nThe optimization I was referring to is that if you have a format like:\n\n  %(contents:body) %(contents:body)\n\nthen we'll de-duplicate that to a single used_atom struct, and they'll\nshare the same v->s result string. That's much harder to do if you parse\ninto an abstract syntax tree, since the two occupy different parts of\nthe tree. But my contention is that it does not matter if you stop\nallocating v->s in the first place, and just walk the tree to directly\noutput the result (either to a strbuf or directly to stdout).\n\n-Peff\n"},{"id":"481324","messageId":"20230902090155.8978-1-five231003@gmail.com","threadId":"60181","inReplyTo":"20230901142624.12063-1-five231003@gmail.com","subject":"[PATCH v2] ref-filter: sort numerically when \":size\" is used","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-09-02T09:00:39Z","receivedAt":"2023-09-02T09:02:37Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"Atoms like \"raw\" and \"contents\" have a \":size\" option which can be used\nto know the size of the data. Since these atoms have the cmp_type\nFIELD_STR, they are sorted alphabetically from 'a' to 'z' and '0' to\n'9'. Meaning, even when the \":size\" option is used and what we\nultimatlely have is numbers, we still sort alphabetically.\n\nFor example, consider the the following case in a repo\n\nrefname\t\t\tcontents:size\t\traw:size\n=======\t\t\t=============\t\t========\nrefs/heads/branch1\t1130\t\t\t1210\nrefs/heads/master\t300\t\t\t410\nrefs/tags/v1.0\t\t140\t\t\t260\n\nSorting with \"--format=\"%(refname) %(contents:size) --sort=contents:size\"\nwould give\n\nrefs/heads/branch1 1130\nrefs/tags/v1.0.0 140\nrefs/heads/master 300\n\nwhich is an alphabetic sort, while what one might really expect is\n\nrefs/tags/v1.0.0 140\nrefs/heads/master 300\nrefs/heads/branch1 1130\n\nwhich is a numeric sort (that is, a \"$ sort -n file\" as opposed to a\n\"$ sort file\", where \"file\" contains only the \"contents:size\" or\n\"raw:size\" info, each of which is on a newline).\n\nSame is the case with \"--sort=raw:size\".\n\nSo, sort numerically whenever the sort is done with \"contents:size\" or\n\"raw:size\" and do it the normal alphabetic way when \"contents\" or \"raw\"\nare used with some other option (they are FIELD_STR anyways).\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Kousik Sanagavarapu <five231003@gmail.com>\n---\n ref-filter.c            | 21 +++++++++++++--------\n t/t6300-for-each-ref.sh | 15 +++++++++++++--\n 2 files changed, 26 insertions(+), 10 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 1bfaf20fbf..9dbc4f71bd 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -582,9 +582,10 @@ static int contents_atom_parser(struct ref_format *format, struct used_atom *ato\n \t\tatom->u.contents.option = C_BARE;\n \telse if (!strcmp(arg, \"body\"))\n \t\tatom->u.contents.option = C_BODY;\n-\telse if (!strcmp(arg, \"size\"))\n+\telse if (!strcmp(arg, \"size\")) {\n+\t\tatom->type = FIELD_ULONG;\n \t\tatom->u.contents.option = C_LENGTH;\n-\telse if (!strcmp(arg, \"signature\"))\n+\t} else if (!strcmp(arg, \"signature\"))\n \t\tatom->u.contents.option = C_SIG;\n \telse if (!strcmp(arg, \"subject\"))\n \t\tatom->u.contents.option = C_SUB;\n@@ -690,9 +691,10 @@ static int raw_atom_parser(struct ref_format *format UNUSED,\n {\n \tif (!arg)\n \t\tatom->u.raw_data.option = RAW_BARE;\n-\telse if (!strcmp(arg, \"size\"))\n+\telse if (!strcmp(arg, \"size\")) {\n+\t\tatom->type = FIELD_ULONG;\n \t\tatom->u.raw_data.option = RAW_LENGTH;\n-\telse\n+\t} else\n \t\treturn err_bad_arg(err, \"raw\", arg);\n \treturn 0;\n }\n@@ -1857,7 +1859,8 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp\n \t\t\t\tv->s = xmemdupz(buf, buf_size);\n \t\t\t\tv->s_size = buf_size;\n \t\t\t} else if (atom->u.raw_data.option == RAW_LENGTH) {\n-\t\t\t\tv->s = xstrfmt(\"%\"PRIuMAX, (uintmax_t)buf_size);\n+\t\t\t\tv->value = buf_size;\n+\t\t\t\tv->s = xstrfmt(\"%\"PRIuMAX, v->value);\n \t\t\t}\n \t\t\tcontinue;\n \t\t}\n@@ -1883,9 +1886,10 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp\n \t\t\tv->s = strbuf_detach(&sb, NULL);\n \t\t} else if (atom->u.contents.option == C_BODY_DEP)\n \t\t\tv->s = xmemdupz(bodypos, bodylen);\n-\t\telse if (atom->u.contents.option == C_LENGTH)\n-\t\t\tv->s = xstrfmt(\"%\"PRIuMAX, (uintmax_t)strlen(subpos));\n-\t\telse if (atom->u.contents.option == C_BODY)\n+\t\telse if (atom->u.contents.option == C_LENGTH) {\n+\t\t\tv->value = strlen(subpos);\n+\t\t\tv->s = xstrfmt(\"%\"PRIuMAX, v->value);\n+\t\t} else if (atom->u.contents.option == C_BODY)\n \t\t\tv->s = xmemdupz(bodypos, nonsiglen);\n \t\telse if (atom->u.contents.option == C_SIG)\n \t\t\tv->s = xmemdupz(sigpos, siglen);\n@@ -2265,6 +2269,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n \n \t\tv->s_size = ATOM_SIZE_UNSPECIFIED;\n \t\tv->handler = append_atom;\n+\t\tv->value = 0;\n \t\tv->atom = atom;\n \n \t\tif (*name == '*') {\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex aa3c7c03c4..7b943fd34c 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -1017,16 +1017,16 @@ test_expect_success 'Verify sorts with raw' '\n test_expect_success 'Verify sorts with raw:size' '\n \tcat >expected <<-EOF &&\n \trefs/myblobs/blob8\n-\trefs/myblobs/first\n \trefs/myblobs/blob7\n-\trefs/heads/main\n \trefs/myblobs/blob4\n \trefs/myblobs/blob1\n \trefs/myblobs/blob2\n \trefs/myblobs/blob3\n \trefs/myblobs/blob5\n \trefs/myblobs/blob6\n+\trefs/myblobs/first\n \trefs/mytrees/first\n+\trefs/heads/main\n \tEOF\n \tgit for-each-ref --format=\"%(refname)\" --sort=raw:size \\\n \t\trefs/heads/main refs/myblobs/ refs/mytrees/first >actual &&\n@@ -1138,6 +1138,17 @@ test_expect_success 'for-each-ref --format compare with cat-file --batch' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'verify sorts with contents:size' '\n+\tcat >expect <<-\\EOF &&\n+\trefs/heads/main\n+\trefs/heads/newtag\n+\trefs/heads/ambiguous\n+\tEOF\n+\tgit for-each-ref --format=\"%(refname)\" \\\n+\t\t--sort=contents:size refs/heads/ >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'set up multiple-sort tags' '\n \tfor when in 100000 200000\n \tdo\n-- \n2.42.0.101.g9b561e429b.dirty\n\n"},{"id":"481325","messageId":"ZPL8SQTxbqZ3LTCP@five231003","threadId":"60181","inReplyTo":"20230902090155.8978-1-five231003@gmail.com","subject":"Re: [PATCH v2] ref-filter: sort numerically when \":size\" is used","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2023-09-02T09:11:37Z","receivedAt":"2023-09-02T09:11:44Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"On Sat, Sep 02, 2023 at 02:30:39PM +0530, Kousik Sanagavarapu wrote:\n> Atoms like \"raw\" and \"contents\" have a \":size\" option which can be used\n> to know the size of the data. Since these atoms have the cmp_type\n> FIELD_STR, they are sorted alphabetically from 'a' to 'z' and '0' to\n> '9'. Meaning, even when the \":size\" option is used and what we\n> ultimatlely have is numbers, we still sort alphabetically.\n[...]\n> So, sort numerically whenever the sort is done with \"contents:size\" or\n> \"raw:size\" and do it the normal alphabetic way when \"contents\" or \"raw\"\n> are used with some other option (they are FIELD_STR anyways).\n> \n> Helped-by: Jeff King <peff@peff.net>\n> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>\n> ---\n\nOops, forgot the range-diff.\n\nRange-diff against v1:\n\n1:  9b561e429b ! 1:  194dcb0b0d ref-filter: sort numerically when \":size\" is used\n    @@ Commit message\n         \"raw:size\" and do it the normal alphabetic way when \"contents\" or \"raw\"\n         are used with some other option (they are FIELD_STR anyways).\n\n    +    Helped-by: Jeff King <peff@peff.net>\n         Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>\n\n      ## ref-filter.c ##\n    -@@ ref-filter.c: struct atom_value {\n    -   ssize_t s_size;\n    -   int (*handler)(struct atom_value *atomv, struct ref_formatting_state *state,\n    -                  struct strbuf *err);\n    --  uintmax_t value; /* used for sorting when not FIELD_STR */\n    -+\n    -+  /*\n    -+   * Used for sorting when not FIELD_STR or when FIELD_STR but the\n    -+   * sort should be numeric and not alphabetic.\n    -+   */\n    -+  uintmax_t value;\n    -+\n    -   struct used_atom *atom;\n    - };\n    -\n    +@@ ref-filter.c: static int contents_atom_parser(struct ref_format *format, struct used_atom *ato\n    +           atom->u.contents.option = C_BARE;\n    +   else if (!strcmp(arg, \"body\"))\n    +           atom->u.contents.option = C_BODY;\n    +-  else if (!strcmp(arg, \"size\"))\n    ++  else if (!strcmp(arg, \"size\")) {\n    ++          atom->type = FIELD_ULONG;\n    +           atom->u.contents.option = C_LENGTH;\n    +-  else if (!strcmp(arg, \"signature\"))\n    ++  } else if (!strcmp(arg, \"signature\"))\n    +           atom->u.contents.option = C_SIG;\n    +   else if (!strcmp(arg, \"subject\"))\n    +           atom->u.contents.option = C_SUB;\n    +@@ ref-filter.c: static int raw_atom_parser(struct ref_format *format UNUSED,\n    + {\n    +   if (!arg)\n    +           atom->u.raw_data.option = RAW_BARE;\n    +-  else if (!strcmp(arg, \"size\"))\n    ++  else if (!strcmp(arg, \"size\")) {\n    ++          atom->type = FIELD_ULONG;\n    +           atom->u.raw_data.option = RAW_LENGTH;\n    +-  else\n    ++  } else\n    +           return err_bad_arg(err, \"raw\", arg);\n    +   return 0;\n    + }\n     @@ ref-filter.c: static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp\n                                v->s = xmemdupz(buf, buf_size);\n                                v->s_size = buf_size;\n                                v->s = xmemdupz(buf, buf_size);\n                                v->s_size = buf_size;\n                                v->s_size = buf_size;\n                        } else if (atom->u.raw_data.option == RAW_LENGTH) {\n     -                          v->s = xstrfmt(\"%\"PRIuMAX, (uintmax_t)buf_size);\n    -+                          v->value = (uintmax_t)buf_size;\n    ++                          v->value = buf_size;\n     +                          v->s = xstrfmt(\"%\"PRIuMAX, v->value);\n                        }\n                        continue;\n    @@ ref-filter.c: static void grab_sub_body_contents(struct atom_value *val, int der\n                        v->s = xmemdupz(bodypos, bodylen);\n     -          else if (atom->u.contents.option == C_LENGTH)\n     -                  v->s = xstrfmt(\"%\"PRIuMAX, (uintmax_t)strlen(subpos));\n    +-          else if (atom->u.contents.option == C_BODY)\n     +          else if (atom->u.contents.option == C_LENGTH) {\n    -+                  v->value = (uintmax_t)strlen(subpos);\n    ++                  v->value = strlen(subpos);\n     +                  v->s = xstrfmt(\"%\"PRIuMAX, v->value);\n    -+          }\n    -           else if (atom->u.contents.option == C_BODY)\n    ++          } else if (atom->u.contents.option == C_BODY)\n                        v->s = xmemdupz(bodypos, nonsiglen);\n                else if (atom->u.contents.option == C_SIG)\n    +                   v->s = xmemdupz(sigpos, siglen);\n     @@ ref-filter.c: static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n\n                v->s_size = ATOM_SIZE_UNSPECIFIED;\n    @@ ref-filter.c: static int populate_value(struct ref_array_item *ref, struct strbu\n                v->atom = atom;\n\n                if (*name == '*') {\n    -@@ ref-filter.c: static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru\n    -           cmp_detached_head = 1;\n    -   } else if (s->sort_flags & REF_SORTING_VERSION) {\n    -           cmp = versioncmp(va->s, vb->s);\n    --  } else if (cmp_type == FIELD_STR) {\n    -+  } else if (cmp_type == FIELD_STR && !va->value && !vb->value) {\n    -           if (va->s_size < 0 && vb->s_size < 0) {\n    -                   int (*cmp_fn)(const char *, const char *);\n    -                   cmp_fn = s->sort_flags & REF_SORTING_ICASE\n\n      ## t/t6300-for-each-ref.sh ##\n     @@ t/t6300-for-each-ref.sh: test_expect_success 'Verify sorts with raw' '\n"},{"id":"481339","messageId":"xmqqy1hokykh.fsf@gitster.g","threadId":"60181","inReplyTo":"20230902090155.8978-1-five231003@gmail.com","subject":"Re: [PATCH v2] ref-filter: sort numerically when \":size\" is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-02T22:19:42Z","receivedAt":"2023-09-02T22:19:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kousik Sanagavarapu <five231003@gmail.com> writes:\n\n>  \t\t\t} else if (atom->u.raw_data.option == RAW_LENGTH) {\n> -\t\t\t\tv->s = xstrfmt(\"%\"PRIuMAX, (uintmax_t)buf_size);\n> +\t\t\t\tv->value = buf_size;\n> +\t\t\t\tv->s = xstrfmt(\"%\"PRIuMAX, v->value);\n>  \t\t\t}\n>  \t\t\tcontinue;\n\nInteresting that typeof(.value) happens to be uintmax_t, keeping\nthis a safe change.\n\n"}]}