threads / patch / 48197

patchls-remote: create option to sort by versions

Subject: [PATCH] ls-remote: create option to sort by versions

## tl;dr

62 messages between Apr 2, 2018 and May 12, 2018. Diffs are folded; open one to read it.

replies: 61people: 9as markdown or json

Harald Nordgren· Apr 2, 2018, 00:52 UTC · lore

Create the options '-V ' and '--version-sort' to sort 'git ls-remote' output by version semantics. This is useful e.g. for the Go repository after the release of version 1.10, where otherwise v1.10 is sorted before v1.2. See:

	$ git ls-remote -t https://go.googlesource.com/go
	...
	205f850ceacfc39d1e9d76a9569416284594ce8c	refs/tags/go1.1
	d260448f6b6ac10efe4ae7f6dfe944e72bc2a676	refs/tags/go1.1.1
	1d6d8fca241bb611af51e265c1b5a2e9ae904702	refs/tags/go1.1.2
	bf86aec25972f3a100c3aa58a6abcbcc35bdea49	refs/tags/go1.10
	ac7c0ee26dda18076d5f6c151d8f920b43340ae3	refs/tags/go1.10.1
	9ce6b5c2ed5d3d5251b9a6a0c548d5fb2c8567e8	refs/tags/go1.10beta1
	594668a5a96267a46282ce3007a584ec07adf705	refs/tags/go1.10beta2
	5348aed83e39bd1d450d92d7f627e994c2db6ebf	refs/tags/go1.10rc1
	20e228f2fdb44350c858de941dff4aea9f3127b8	refs/tags/go1.10rc2
	1c5438aae896edcd1e9f9618f4776517f08053b3	refs/tags/go1.1rc2
	46a6097aa7943a490e9bd2e04274845d0e5e200f	refs/tags/go1.1rc3
	402d3590b54e4a0df9fb51ed14b2999e85ce0b76	refs/tags/go1.2
	9c9802fad57c1bcb72ea98c5c55ea2652efc5772	refs/tags/go1.2.1
	...
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
 builtin/ls-remote.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)
Show changes to builtin/ls-remote.c +25 −2
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 540d56429..740c6f117 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -1,6 +1,7 @@
 #include "builtin.h"
 #include "cache.h"
 #include "transport.h"
+#include "ref-filter.h"
 #include "remote.h"
 
 static const char * const ls_remote_usage[] = {
@@ -33,11 +34,20 @@ static int tail_match(const char **pattern, const char *path)
 	return 0;
 }
 
+static int cmp_ref_versions(const void *_a, const void *_b)
+{
+	const struct ref *a = *(const struct ref **)_a;
+	const struct ref *b = *(const struct ref **)_b;
+
+	return versioncmp(a->name, b->name);
+}
+
 int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 {
 	const char *dest = NULL;
 	unsigned flags = 0;
 	int get_url = 0;
+	int version_sort = 0;
 	int quiet = 0;
 	int status = 0;
 	int show_symref_target = 0;
@@ -47,6 +57,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	struct remote *remote;
 	struct transport *transport;
 	const struct ref *ref;
+	const struct ref **refs = NULL;
+	int nr = 0;
 
 	struct option options[] = {
 		OPT__QUIET(&quiet, N_("do not print remote URL")),
@@ -60,6 +72,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL),
 		OPT_BOOL(0, "get-url", &get_url,
 			 N_("take url.<base>.insteadOf into account")),
+		OPT_BOOL('V', "version-sort", &version_sort,
+			 N_("sort tags by version numbers")),
 		OPT_SET_INT_F(0, "exit-code", &status,
 			      N_("exit with exit code 2 if no matching refs are found"),
 			      2, PARSE_OPT_NOCOMPLETE),
@@ -101,13 +115,22 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	if (transport_disconnect(transport))
 		return 1;
 
-	if (!dest && !quiet)
-		fprintf(stderr, "From %s\n", *remote->url);
 	for ( ; ref; ref = ref->next) {
 		if (!check_ref_type(ref, flags))
 			continue;
 		if (!tail_match(pattern, ref->name))
 			continue;
+		REALLOC_ARRAY(refs, nr + 1);
+		refs[nr++] = ref;
+	}
+
+	if (version_sort)
+		QSORT(refs, nr, cmp_ref_versions);
+
+	if (!dest && !quiet)
+		fprintf(stderr, "From %s\n", *remote->url);
+	for (int i = 0; i < nr; i++) {
+		const struct ref *ref = refs[i];
 		if (show_symref_target && ref->symref)
 			printf("ref: %s\t%s\n", ref->symref, ref->name);
 		printf("%s\t%s\n", oid_to_hex(&ref->old_oid), ref->name);
-- 
2.14.3 (Apple Git-98)
Ævar Arnfjörð Bjarmason· Apr 2, 2018, 06:37 UTC · re: Harald Nordgren · lore

Re: [PATCH] ls-remote: create option to sort by versions

On Mon, Apr 02 2018, Harald Nordgren wrote:
Show 21 quoted lines
> Create the options '-V ' and '--version-sort' to sort
> 'git ls-remote' output by version semantics. This is useful e.g. for
> the Go repository after the release of version 1.10, where otherwise
> v1.10 is sorted before v1.2. See:
>
> 	$ git ls-remote -t https://go.googlesource.com/go
> 	...
> 	205f850ceacfc39d1e9d76a9569416284594ce8c	refs/tags/go1.1
> 	d260448f6b6ac10efe4ae7f6dfe944e72bc2a676	refs/tags/go1.1.1
> 	1d6d8fca241bb611af51e265c1b5a2e9ae904702	refs/tags/go1.1.2
> 	bf86aec25972f3a100c3aa58a6abcbcc35bdea49	refs/tags/go1.10
> 	ac7c0ee26dda18076d5f6c151d8f920b43340ae3	refs/tags/go1.10.1
> 	9ce6b5c2ed5d3d5251b9a6a0c548d5fb2c8567e8	refs/tags/go1.10beta1
> 	594668a5a96267a46282ce3007a584ec07adf705	refs/tags/go1.10beta2
> 	5348aed83e39bd1d450d92d7f627e994c2db6ebf	refs/tags/go1.10rc1
> 	20e228f2fdb44350c858de941dff4aea9f3127b8	refs/tags/go1.10rc2
> 	1c5438aae896edcd1e9f9618f4776517f08053b3	refs/tags/go1.1rc2
> 	46a6097aa7943a490e9bd2e04274845d0e5e200f	refs/tags/go1.1rc3
> 	402d3590b54e4a0df9fb51ed14b2999e85ce0b76	refs/tags/go1.2
> 	9c9802fad57c1bcb72ea98c5c55ea2652efc5772	refs/tags/go1.2.1
> 	...

This is a sensible thing to want, but why not follow the UI we have for this with git-tag? I.e. --sort=<key> & -i (or --ignore-case)? Of course ls-remote doesn't just show tags, so maybe we'd want --tag-sort=<key> and --ignore-tag-case or something, but the rest should be equivalent, no?

Show 21 quoted lines
> [...]
> @@ -101,13 +115,22 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>  	if (transport_disconnect(transport))
>  		return 1;
>
> -	if (!dest && !quiet)
> -		fprintf(stderr, "From %s\n", *remote->url);
>  	for ( ; ref; ref = ref->next) {
>  		if (!check_ref_type(ref, flags))
>  			continue;
>  		if (!tail_match(pattern, ref->name))
>  			continue;
> +		REALLOC_ARRAY(refs, nr + 1);
> +		refs[nr++] = ref;
> +	}
> +
> +	if (version_sort)
> +		QSORT(refs, nr, cmp_ref_versions);
> +
> +	if (!dest && !quiet)
> +		fprintf(stderr, "From %s\n", *remote->url);

Is there some subtlety here I'm missing which means that when sorting we'd now need to print this "From" line later (i.e. after sorting?

Harald Nordgren· Apr 2, 2018, 16:26 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] ls-remote: create option to sort by versions

Thanks for your comment Ævar!

In regards the the print statement, it was only moved down according to the diff because I added more logic above. Basically there is 1) the unrolling of the linked list to an array and 2) the printing logic. I could move it and make the diff smaller, but that probably makes the code a tiny bit more complicated.

It would be nice to have a uniform option like '--sort=version:refname'. But spending a few hours to look over the code, it seems that ls-remote.c would require a lot of rewrites if we wanted to start using `ref_array` and `ref_array_item` for storing the refs.

Which seems necessary in order to hook in to the sorting flow used in other subcommands. That, or reimplement `cmp_ref_sorting`. But maybe I'm missing something?

On Mon, Apr 2, 2018 at 8:37 AM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 55 quoted lines
>
> On Mon, Apr 02 2018, Harald Nordgren wrote:
>
>> Create the options '-V ' and '--version-sort' to sort
>> 'git ls-remote' output by version semantics. This is useful e.g. for
>> the Go repository after the release of version 1.10, where otherwise
>> v1.10 is sorted before v1.2. See:
>>
>>       $ git ls-remote -t https://go.googlesource.com/go
>>       ...
>>       205f850ceacfc39d1e9d76a9569416284594ce8c        refs/tags/go1.1
>>       d260448f6b6ac10efe4ae7f6dfe944e72bc2a676        refs/tags/go1.1.1
>>       1d6d8fca241bb611af51e265c1b5a2e9ae904702        refs/tags/go1.1.2
>>       bf86aec25972f3a100c3aa58a6abcbcc35bdea49        refs/tags/go1.10
>>       ac7c0ee26dda18076d5f6c151d8f920b43340ae3        refs/tags/go1.10.1
>>       9ce6b5c2ed5d3d5251b9a6a0c548d5fb2c8567e8        refs/tags/go1.10beta1
>>       594668a5a96267a46282ce3007a584ec07adf705        refs/tags/go1.10beta2
>>       5348aed83e39bd1d450d92d7f627e994c2db6ebf        refs/tags/go1.10rc1
>>       20e228f2fdb44350c858de941dff4aea9f3127b8        refs/tags/go1.10rc2
>>       1c5438aae896edcd1e9f9618f4776517f08053b3        refs/tags/go1.1rc2
>>       46a6097aa7943a490e9bd2e04274845d0e5e200f        refs/tags/go1.1rc3
>>       402d3590b54e4a0df9fb51ed14b2999e85ce0b76        refs/tags/go1.2
>>       9c9802fad57c1bcb72ea98c5c55ea2652efc5772        refs/tags/go1.2.1
>>       ...
>
> This is a sensible thing to want, but why not follow the UI we have for
> this with git-tag? I.e. --sort=<key> & -i (or --ignore-case)? Of course
> ls-remote doesn't just show tags, so maybe we'd want --tag-sort=<key>
> and --ignore-tag-case or something, but the rest should be equivalent,
> no?
>
>> [...]
>> @@ -101,13 +115,22 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>>       if (transport_disconnect(transport))
>>               return 1;
>>
>> -     if (!dest && !quiet)
>> -             fprintf(stderr, "From %s\n", *remote->url);
>>       for ( ; ref; ref = ref->next) {
>>               if (!check_ref_type(ref, flags))
>>                       continue;
>>               if (!tail_match(pattern, ref->name))
>>                       continue;
>> +             REALLOC_ARRAY(refs, nr + 1);
>> +             refs[nr++] = ref;
>> +     }
>> +
>> +     if (version_sort)
>> +             QSORT(refs, nr, cmp_ref_versions);
>> +
>> +     if (!dest && !quiet)
>> +             fprintf(stderr, "From %s\n", *remote->url);
>
> Is there some subtlety here I'm missing which means that when sorting
> we'd now need to print this "From" line later (i.e. after sorting?
Ævar Arnfjörð Bjarmason· Apr 2, 2018, 17:32 UTC · re: Harald Nordgren · lore

Re: [PATCH] ls-remote: create option to sort by versions

On Mon, Apr 02 2018, Harald Nordgren wrote:
Show 5 quoted lines
> In regards the the print statement, it was only moved down according
> to the diff because I added more logic above. Basically there is 1)
> the unrolling of the linked list to an array and 2) the printing
> logic. I could move it and make the diff smaller, but that probably
> makes the code a tiny bit more complicated.

I was just wondering since it wasn't explained in the commit message, makes sense to copy this explanation into v2, or lead with a purely code re-arrangement patch.

Show 9 quoted lines
> It would be nice to have a uniform option like
> '--sort=version:refname'. But spending a few hours to look over the
> code, it seems that ls-remote.c would require a lot of rewrites if we
> wanted to start using `ref_array` and `ref_array_item` for storing the
> refs.
>
> Which seems necessary in order to hook in to the sorting flow used in
> other subcommands. That, or reimplement `cmp_ref_sorting`. But maybe
> I'm missing something?

I'm thinking just in terms of UI. If it's the case that porting this to whatever guts git-tag uses for sorting would be hard, then we could still use the same command-line option convention (and perhaps just die if anything except --sort=version:refname is supplied). Changing the underlying implementation is easier than cleaning up UI-differences that (seemingly) only arose due to underlying implementation details at the time.

Show 57 quoted lines
> On Mon, Apr 2, 2018 at 8:37 AM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> On Mon, Apr 02 2018, Harald Nordgren wrote:
>>
>>> Create the options '-V ' and '--version-sort' to sort
>>> 'git ls-remote' output by version semantics. This is useful e.g. for
>>> the Go repository after the release of version 1.10, where otherwise
>>> v1.10 is sorted before v1.2. See:
>>>
>>>       $ git ls-remote -t https://go.googlesource.com/go
>>>       ...
>>>       205f850ceacfc39d1e9d76a9569416284594ce8c        refs/tags/go1.1
>>>       d260448f6b6ac10efe4ae7f6dfe944e72bc2a676        refs/tags/go1.1.1
>>>       1d6d8fca241bb611af51e265c1b5a2e9ae904702        refs/tags/go1.1.2
>>>       bf86aec25972f3a100c3aa58a6abcbcc35bdea49        refs/tags/go1.10
>>>       ac7c0ee26dda18076d5f6c151d8f920b43340ae3        refs/tags/go1.10.1
>>>       9ce6b5c2ed5d3d5251b9a6a0c548d5fb2c8567e8        refs/tags/go1.10beta1
>>>       594668a5a96267a46282ce3007a584ec07adf705        refs/tags/go1.10beta2
>>>       5348aed83e39bd1d450d92d7f627e994c2db6ebf        refs/tags/go1.10rc1
>>>       20e228f2fdb44350c858de941dff4aea9f3127b8        refs/tags/go1.10rc2
>>>       1c5438aae896edcd1e9f9618f4776517f08053b3        refs/tags/go1.1rc2
>>>       46a6097aa7943a490e9bd2e04274845d0e5e200f        refs/tags/go1.1rc3
>>>       402d3590b54e4a0df9fb51ed14b2999e85ce0b76        refs/tags/go1.2
>>>       9c9802fad57c1bcb72ea98c5c55ea2652efc5772        refs/tags/go1.2.1
>>>       ...
>>
>> This is a sensible thing to want, but why not follow the UI we have for
>> this with git-tag? I.e. --sort=<key> & -i (or --ignore-case)? Of course
>> ls-remote doesn't just show tags, so maybe we'd want --tag-sort=<key>
>> and --ignore-tag-case or something, but the rest should be equivalent,
>> no?
>>
>>> [...]
>>> @@ -101,13 +115,22 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>>>       if (transport_disconnect(transport))
>>>               return 1;
>>>
>>> -     if (!dest && !quiet)
>>> -             fprintf(stderr, "From %s\n", *remote->url);
>>>       for ( ; ref; ref = ref->next) {
>>>               if (!check_ref_type(ref, flags))
>>>                       continue;
>>>               if (!tail_match(pattern, ref->name))
>>>                       continue;
>>> +             REALLOC_ARRAY(refs, nr + 1);
>>> +             refs[nr++] = ref;
>>> +     }
>>> +
>>> +     if (version_sort)
>>> +             QSORT(refs, nr, cmp_ref_versions);
>>> +
>>> +     if (!dest && !quiet)
>>> +             fprintf(stderr, "From %s\n", *remote->url);
>>
>> Is there some subtlety here I'm missing which means that when sorting
>> we'd now need to print this "From" line later (i.e. after sorting?
Harald Nordgren· Apr 2, 2018, 17:42 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] ls-remote: create option to sort by versions

Both points make sense and it sounds like a very pragmatic approach. I'll look into it!

On Mon, Apr 2, 2018 at 7:32 PM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 88 quoted lines
>
> On Mon, Apr 02 2018, Harald Nordgren wrote:
>
>> In regards the the print statement, it was only moved down according
>> to the diff because I added more logic above. Basically there is 1)
>> the unrolling of the linked list to an array and 2) the printing
>> logic. I could move it and make the diff smaller, but that probably
>> makes the code a tiny bit more complicated.
>
> I was just wondering since it wasn't explained in the commit message,
> makes sense to copy this explanation into v2, or lead with a purely code
> re-arrangement patch.
>
>> It would be nice to have a uniform option like
>> '--sort=version:refname'. But spending a few hours to look over the
>> code, it seems that ls-remote.c would require a lot of rewrites if we
>> wanted to start using `ref_array` and `ref_array_item` for storing the
>> refs.
>>
>> Which seems necessary in order to hook in to the sorting flow used in
>> other subcommands. That, or reimplement `cmp_ref_sorting`. But maybe
>> I'm missing something?
>
> I'm thinking just in terms of UI. If it's the case that porting this to
> whatever guts git-tag uses for sorting would be hard, then we could
> still use the same command-line option convention (and perhaps just die
> if anything except --sort=version:refname is supplied). Changing the
> underlying implementation is easier than cleaning up UI-differences that
> (seemingly) only arose due to underlying implementation details at the
> time.
>
>> On Mon, Apr 2, 2018 at 8:37 AM, Ævar Arnfjörð Bjarmason
>> <avarab@gmail.com> wrote:
>>>
>>> On Mon, Apr 02 2018, Harald Nordgren wrote:
>>>
>>>> Create the options '-V ' and '--version-sort' to sort
>>>> 'git ls-remote' output by version semantics. This is useful e.g. for
>>>> the Go repository after the release of version 1.10, where otherwise
>>>> v1.10 is sorted before v1.2. See:
>>>>
>>>>       $ git ls-remote -t https://go.googlesource.com/go
>>>>       ...
>>>>       205f850ceacfc39d1e9d76a9569416284594ce8c        refs/tags/go1.1
>>>>       d260448f6b6ac10efe4ae7f6dfe944e72bc2a676        refs/tags/go1.1.1
>>>>       1d6d8fca241bb611af51e265c1b5a2e9ae904702        refs/tags/go1.1.2
>>>>       bf86aec25972f3a100c3aa58a6abcbcc35bdea49        refs/tags/go1.10
>>>>       ac7c0ee26dda18076d5f6c151d8f920b43340ae3        refs/tags/go1.10.1
>>>>       9ce6b5c2ed5d3d5251b9a6a0c548d5fb2c8567e8        refs/tags/go1.10beta1
>>>>       594668a5a96267a46282ce3007a584ec07adf705        refs/tags/go1.10beta2
>>>>       5348aed83e39bd1d450d92d7f627e994c2db6ebf        refs/tags/go1.10rc1
>>>>       20e228f2fdb44350c858de941dff4aea9f3127b8        refs/tags/go1.10rc2
>>>>       1c5438aae896edcd1e9f9618f4776517f08053b3        refs/tags/go1.1rc2
>>>>       46a6097aa7943a490e9bd2e04274845d0e5e200f        refs/tags/go1.1rc3
>>>>       402d3590b54e4a0df9fb51ed14b2999e85ce0b76        refs/tags/go1.2
>>>>       9c9802fad57c1bcb72ea98c5c55ea2652efc5772        refs/tags/go1.2.1
>>>>       ...
>>>
>>> This is a sensible thing to want, but why not follow the UI we have for
>>> this with git-tag? I.e. --sort=<key> & -i (or --ignore-case)? Of course
>>> ls-remote doesn't just show tags, so maybe we'd want --tag-sort=<key>
>>> and --ignore-tag-case or something, but the rest should be equivalent,
>>> no?
>>>
>>>> [...]
>>>> @@ -101,13 +115,22 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>>>>       if (transport_disconnect(transport))
>>>>               return 1;
>>>>
>>>> -     if (!dest && !quiet)
>>>> -             fprintf(stderr, "From %s\n", *remote->url);
>>>>       for ( ; ref; ref = ref->next) {
>>>>               if (!check_ref_type(ref, flags))
>>>>                       continue;
>>>>               if (!tail_match(pattern, ref->name))
>>>>                       continue;
>>>> +             REALLOC_ARRAY(refs, nr + 1);
>>>> +             refs[nr++] = ref;
>>>> +     }
>>>> +
>>>> +     if (version_sort)
>>>> +             QSORT(refs, nr, cmp_ref_versions);
>>>> +
>>>> +     if (!dest && !quiet)
>>>> +             fprintf(stderr, "From %s\n", *remote->url);
>>>
>>> Is there some subtlety here I'm missing which means that when sorting
>>> we'd now need to print this "From" line later (i.e. after sorting?
Jeff King· Apr 2, 2018, 17:46 UTC · re: Harald Nordgren · lore

Re: [PATCH] ls-remote: create option to sort by versions

On Mon, Apr 02, 2018 at 06:26:49PM +0200, Harald Nordgren wrote:
Show 9 quoted lines
> It would be nice to have a uniform option like
> '--sort=version:refname'. But spending a few hours to look over the
> code, it seems that ls-remote.c would require a lot of rewrites if we
> wanted to start using `ref_array` and `ref_array_item` for storing the
> refs.
> 
> Which seems necessary in order to hook in to the sorting flow used in
> other subcommands. That, or reimplement `cmp_ref_sorting`. But maybe
> I'm missing something?

I haven't looked at how painful it might be to use ref-filter.c, but it would buy us even more if we could. That would open up other options like --format, I think (OTOH there may be some funny corner cases; that code assumes we're talking about local refs, so if you were to ask for "%(committerdate)" or something, we might have to more cleanly handle the case where we don't actually have the object).

-Peff
Junio C Hamano· Apr 2, 2018, 18:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] ls-remote: create option to sort by versions

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 5 quoted lines
> This is a sensible thing to want, but why not follow the UI we have for
> this with git-tag? I.e. --sort=<key> & -i (or --ignore-case)? Of course
> ls-remote doesn't just show tags, so maybe we'd want --tag-sort=<key>
> and --ignore-tag-case or something, but the rest should be equivalent,
> no?

Yeah, and if we can reuse more of ref-filter.c machinery (which was factored out of for-each-ref and tag you suggested borrows from), that would be even better. In the context of ls-remote, however, we cannot inspect the object (we typically do not have them yet), so it may not be practical, but I agree with your suggestion---we should match the behaviour at the UI level at least when we can.

Harald Nordgren· Apr 2, 2018, 20:03 UTC · re: Junio C Hamano · lore

Re: [PATCH] ls-remote: create option to sort by versions

Thanks for all the discussion!

I think I figured out a way to reuse more ref-filter.c machinery. I will submit another patchset shortly.

On Mon, Apr 2, 2018 at 8:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> This is a sensible thing to want, but why not follow the UI we have for
>> this with git-tag? I.e. --sort=<key> & -i (or --ignore-case)? Of course
>> ls-remote doesn't just show tags, so maybe we'd want --tag-sort=<key>
>> and --ignore-tag-case or something, but the rest should be equivalent,
>> no?
>
> Yeah, and if we can reuse more of ref-filter.c machinery (which was
> factored out of for-each-ref and tag you suggested borrows from),
> that would be even better.  In the context of ls-remote, however, we
> cannot inspect the object (we typically do not have them yet), so it
> may not be practical, but I agree with your suggestion---we should
> match the behaviour at the UI level at least when we can.
>
Harald Nordgren· Apr 2, 2018, 22:11 UTC · re: Harald Nordgren · lore

[PATCH v5] ls-remote: create '--sort' option

Create a '--sort' option for ls-remote, based on the one from for-each-ref. This e.g. allows ref names to be sorted by version semantics, so that v1.2 is sorted before v1.10.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
 Documentation/git-ls-remote.txt | 12 +++++++++++-
 builtin/ls-remote.c             | 26 ++++++++++++++++++++++++--
 t/t5512-ls-remote.sh            | 41 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 75 insertions(+), 4 deletions(-)
Show changes to 3 files +75 −4

Documentation/git-ls-remote.txt, builtin/ls-remote.c, t/t5512-ls-remote.sh

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 5f2628c8f..17fae7218 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git ls-remote' [--heads] [--tags] [--refs] [--upload-pack=<exec>]
-	      [-q | --quiet] [--exit-code] [--get-url]
+	      [-q | --quiet] [--exit-code] [--get-url] [--sort=<key>]
 	      [--symref] [<repository> [<refs>...]]
 
 DESCRIPTION
@@ -60,6 +60,16 @@ OPTIONS
 	upload-pack only shows the symref HEAD, so it will be the only
 	one shown by ls-remote.
 
+--sort=<key>::
+	Sort based on the key given.  Prefix `-` to sort in
+	descending order of the value. You may use the --sort=<key> option
+	multiple times, in which case the last key becomes the primary
+	key. Also supports "version:refname" or "v:refname" (tag
+	names are treated as versions). The "version:refname" sort
+	order can also be affected by the "versionsort.suffix"
+	configuration variable.
+	The keys supported are the same as those in `git for-each-ref`.
+
 <repository>::
 	The "remote" repository to query.  This parameter can be
 	either a URL or the name of a remote (see the GIT URLS and
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 540d56429..7e2d820c4 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -1,6 +1,7 @@
 #include "builtin.h"
 #include "cache.h"
 #include "transport.h"
+#include "ref-filter.h"
 #include "remote.h"
 
 static const char * const ls_remote_usage[] = {
@@ -47,6 +48,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	struct remote *remote;
 	struct transport *transport;
 	const struct ref *ref;
+	struct ref_array array;
+	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
 
 	struct option options[] = {
 		OPT__QUIET(&quiet, N_("do not print remote URL")),
@@ -60,6 +63,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL),
 		OPT_BOOL(0, "get-url", &get_url,
 			 N_("take url.<base>.insteadOf into account")),
+		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
+			     N_("field name to sort on"), &parse_opt_ref_sorting),
 		OPT_SET_INT_F(0, "exit-code", &status,
 			      N_("exit with exit code 2 if no matching refs are found"),
 			      2, PARSE_OPT_NOCOMPLETE),
@@ -68,6 +73,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	memset(&array, 0, sizeof(array));
+
 	argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 	dest = argv[0];
@@ -104,13 +111,28 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	if (!dest && !quiet)
 		fprintf(stderr, "From %s\n", *remote->url);
 	for ( ; ref; ref = ref->next) {
+		struct ref_array_item *item;
 		if (!check_ref_type(ref, flags))
 			continue;
 		if (!tail_match(pattern, ref->name))
 			continue;
+
+		FLEX_ALLOC_MEM(item, refname, ref->name, strlen(ref->name));
+		item->symref = ref->symref;
+		item->objectname = ref->old_oid;
+
+		REALLOC_ARRAY(array.items, array.nr + 1);
+		array.items[array.nr++] = item;
+	}
+
+	if (sorting)
+		ref_array_sort(sorting, &array);
+
+	for (int i = 0; i < array.nr; i++) {
+		const struct ref_array_item *ref = array.items[i];
 		if (show_symref_target && ref->symref)
-			printf("ref: %s\t%s\n", ref->symref, ref->name);
-		printf("%s\t%s\n", oid_to_hex(&ref->old_oid), ref->name);
+			printf("ref: %s\t%s\n", ref->symref, ref->refname);
+		printf("%s\t%s\n", oid_to_hex(&ref->objectname), ref->refname);
 		status = 0; /* we found something */
 	}
 	return status;
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 02106c922..66370cd88 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -10,6 +10,9 @@ test_expect_success setup '
 	test_tick &&
 	git commit -m initial &&
 	git tag mark &&
+	git tag mark1.1 &&
+	git tag mark1.2 &&
+	git tag mark1.10 &&
 	git show-ref --tags -d | sed -e "s/ /	/" >expected.tag &&
 	(
 		echo "$(git rev-parse HEAD)	HEAD"
@@ -39,6 +42,39 @@ test_expect_success 'ls-remote self' '
 	test_cmp expected.all actual
 '
 
+test_expect_success 'ls-remote --sort="version:refname" --tags self' '
+	cat >expect <<-\EOF &&
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	EOF
+	git ls-remote --sort="version:refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-remote --sort="-version:refname" --tags self' '
+	cat >expect <<-\EOF &&
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	EOF
+	git ls-remote --sort="-version:refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-remote --sort="-refname" --tags self' '
+	cat >expect <<-\EOF &&
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	EOF
+	git ls-remote --sort="-refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'dies when no remote specified and no default remotes found' '
 	test_must_fail git ls-remote
 '
@@ -131,7 +167,7 @@ test_expect_success 'Report no-match with --exit-code' '
 
 test_expect_success 'Report match with --exit-code' '
 	git ls-remote --exit-code other.git "refs/tags/*" >actual &&
-	git ls-remote . tags/mark >expect &&
+	git ls-remote . tags/mark* >expect &&
 	test_cmp expect actual
 '
 
@@ -178,6 +214,9 @@ test_expect_success 'ls-remote --symref' '
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/HEAD
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/master
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
 	EOF
 	git ls-remote --symref >actual &&
 	test_cmp expect actual
-- 
2.14.3 (Apple Git-98)
Eric Sunshine· Apr 2, 2018, 22:53 UTC · re: Harald Nordgren · lore

Re: [PATCH v5] ls-remote: create '--sort' option

On Mon, Apr 2, 2018 at 6:11 PM, Harald Nordgren <haraldnordgren@gmail.com> wrote:

Show 6 quoted lines
> Create a '--sort' option for ls-remote, based on the one from
> for-each-ref. This e.g. allows ref names to be sorted by version
> semantics, so that v1.2 is sorted before v1.10.
>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> ---

Please take pity on reviewers and use this space below the "---" line following your sign-off to explain what changed since the previous attempt and to provide a link to the previous round(s), like this[1]. Thanks.

[1]: https://www.youtube.com/watch?v=r_8ydghbGSg
>  Documentation/git-ls-remote.txt | 12 +++++++++++-
>  builtin/ls-remote.c             | 26 ++++++++++++++++++++++++--
>  t/t5512-ls-remote.sh            | 41 ++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 75 insertions(+), 4 deletions(-)
Eric Sunshine· Apr 2, 2018, 22:54 UTC · re: Eric Sunshine · lore

Re: [PATCH v5] ls-remote: create '--sort' option

On Mon, Apr 2, 2018 at 6:53 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 15 quoted lines
> On Mon, Apr 2, 2018 at 6:11 PM, Harald Nordgren
> <haraldnordgren@gmail.com> wrote:
>> Create a '--sort' option for ls-remote, based on the one from
>> for-each-ref. This e.g. allows ref names to be sorted by version
>> semantics, so that v1.2 is sorted before v1.10.
>>
>> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
>> ---
>
> Please take pity on reviewers and use this space below the "---" line
> following your sign-off to explain what changed since the previous
> attempt and to provide a link to the previous round(s), like this[1].
> Thanks.
>
> [1]: https://www.youtube.com/watch?v=r_8ydghbGSg
Or, like this if we want to be really accurate[1].
[1]: https://public-inbox.org/git/20180402210523.83293-1-haraldnordgren@gmail.com/
Harald Nordgren· Apr 3, 2018, 00:48 UTC · re: Harald Nordgren · lore

[PATCH v6] ls-remote: create '--sort' option

Create a '--sort' option for ls-remote, based on the one from for-each-ref. This e.g. allows ref names to be sorted by version semantics, so that v1.2 is sorted before v1.10.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
Notes:
    Moving 'int i' declaration outside of 'for' loop init
 Documentation/git-ls-remote.txt | 12 +++++++++++-
 builtin/ls-remote.c             | 27 +++++++++++++++++++++++++--
 t/t5512-ls-remote.sh            | 41 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 76 insertions(+), 4 deletions(-)
Show changes to 3 files +76 −4

Documentation/git-ls-remote.txt, builtin/ls-remote.c, t/t5512-ls-remote.sh

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 5f2628c8f..17fae7218 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git ls-remote' [--heads] [--tags] [--refs] [--upload-pack=<exec>]
-	      [-q | --quiet] [--exit-code] [--get-url]
+	      [-q | --quiet] [--exit-code] [--get-url] [--sort=<key>]
 	      [--symref] [<repository> [<refs>...]]
 
 DESCRIPTION
@@ -60,6 +60,16 @@ OPTIONS
 	upload-pack only shows the symref HEAD, so it will be the only
 	one shown by ls-remote.
 
+--sort=<key>::
+	Sort based on the key given.  Prefix `-` to sort in
+	descending order of the value. You may use the --sort=<key> option
+	multiple times, in which case the last key becomes the primary
+	key. Also supports "version:refname" or "v:refname" (tag
+	names are treated as versions). The "version:refname" sort
+	order can also be affected by the "versionsort.suffix"
+	configuration variable.
+	The keys supported are the same as those in `git for-each-ref`.
+
 <repository>::
 	The "remote" repository to query.  This parameter can be
 	either a URL or the name of a remote (see the GIT URLS and
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 540d56429..113a2fd7d 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -1,6 +1,7 @@
 #include "builtin.h"
 #include "cache.h"
 #include "transport.h"
+#include "ref-filter.h"
 #include "remote.h"
 
 static const char * const ls_remote_usage[] = {
@@ -43,10 +44,13 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	int show_symref_target = 0;
 	const char *uploadpack = NULL;
 	const char **pattern = NULL;
+	int i;
 
 	struct remote *remote;
 	struct transport *transport;
 	const struct ref *ref;
+	struct ref_array array;
+	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
 
 	struct option options[] = {
 		OPT__QUIET(&quiet, N_("do not print remote URL")),
@@ -60,6 +64,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL),
 		OPT_BOOL(0, "get-url", &get_url,
 			 N_("take url.<base>.insteadOf into account")),
+		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
+			     N_("field name to sort on"), &parse_opt_ref_sorting),
 		OPT_SET_INT_F(0, "exit-code", &status,
 			      N_("exit with exit code 2 if no matching refs are found"),
 			      2, PARSE_OPT_NOCOMPLETE),
@@ -68,6 +74,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	memset(&array, 0, sizeof(array));
+
 	argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 	dest = argv[0];
@@ -104,13 +112,28 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	if (!dest && !quiet)
 		fprintf(stderr, "From %s\n", *remote->url);
 	for ( ; ref; ref = ref->next) {
+		struct ref_array_item *item;
 		if (!check_ref_type(ref, flags))
 			continue;
 		if (!tail_match(pattern, ref->name))
 			continue;
+
+		FLEX_ALLOC_MEM(item, refname, ref->name, strlen(ref->name));
+		item->symref = ref->symref;
+		item->objectname = ref->old_oid;
+
+		REALLOC_ARRAY(array.items, array.nr + 1);
+		array.items[array.nr++] = item;
+	}
+
+	if (sorting)
+		ref_array_sort(sorting, &array);
+
+	for (i = 0; i < array.nr; i++) {
+		const struct ref_array_item *ref = array.items[i];
 		if (show_symref_target && ref->symref)
-			printf("ref: %s\t%s\n", ref->symref, ref->name);
-		printf("%s\t%s\n", oid_to_hex(&ref->old_oid), ref->name);
+			printf("ref: %s\t%s\n", ref->symref, ref->refname);
+		printf("%s\t%s\n", oid_to_hex(&ref->objectname), ref->refname);
 		status = 0; /* we found something */
 	}
 	return status;
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 02106c922..66370cd88 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -10,6 +10,9 @@ test_expect_success setup '
 	test_tick &&
 	git commit -m initial &&
 	git tag mark &&
+	git tag mark1.1 &&
+	git tag mark1.2 &&
+	git tag mark1.10 &&
 	git show-ref --tags -d | sed -e "s/ /	/" >expected.tag &&
 	(
 		echo "$(git rev-parse HEAD)	HEAD"
@@ -39,6 +42,39 @@ test_expect_success 'ls-remote self' '
 	test_cmp expected.all actual
 '
 
+test_expect_success 'ls-remote --sort="version:refname" --tags self' '
+	cat >expect <<-\EOF &&
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	EOF
+	git ls-remote --sort="version:refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-remote --sort="-version:refname" --tags self' '
+	cat >expect <<-\EOF &&
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	EOF
+	git ls-remote --sort="-version:refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-remote --sort="-refname" --tags self' '
+	cat >expect <<-\EOF &&
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	EOF
+	git ls-remote --sort="-refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'dies when no remote specified and no default remotes found' '
 	test_must_fail git ls-remote
 '
@@ -131,7 +167,7 @@ test_expect_success 'Report no-match with --exit-code' '
 
 test_expect_success 'Report match with --exit-code' '
 	git ls-remote --exit-code other.git "refs/tags/*" >actual &&
-	git ls-remote . tags/mark >expect &&
+	git ls-remote . tags/mark* >expect &&
 	test_cmp expect actual
 '
 
@@ -178,6 +214,9 @@ test_expect_success 'ls-remote --symref' '
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/HEAD
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/master
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
 	EOF
 	git ls-remote --symref >actual &&
 	test_cmp expect actual
-- 
2.14.3 (Apple Git-98)
Harald Nordgren· Apr 2, 2018, 21:05 UTC · re: Harald Nordgren · lore

[PATCH v4] ls-remote: create '--sort' option

Create a '--sort' option for ls-remote, based on the one from for-each-ref. This e.g. allows ref names to be sorted by version semantics, so that v1.2 is sorted before v1.10.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
 Documentation/git-ls-remote.txt | 12 +++++++++++-
 builtin/ls-remote.c             | 27 +++++++++++++++++++++++++--
 t/t5512-ls-remote.sh            | 41 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 76 insertions(+), 4 deletions(-)
Show changes to 3 files +76 −4

Documentation/git-ls-remote.txt, builtin/ls-remote.c, t/t5512-ls-remote.sh

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 5f2628c8f..17fae7218 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git ls-remote' [--heads] [--tags] [--refs] [--upload-pack=<exec>]
-	      [-q | --quiet] [--exit-code] [--get-url]
+	      [-q | --quiet] [--exit-code] [--get-url] [--sort=<key>]
 	      [--symref] [<repository> [<refs>...]]
 
 DESCRIPTION
@@ -60,6 +60,16 @@ OPTIONS
 	upload-pack only shows the symref HEAD, so it will be the only
 	one shown by ls-remote.
 
+--sort=<key>::
+	Sort based on the key given.  Prefix `-` to sort in
+	descending order of the value. You may use the --sort=<key> option
+	multiple times, in which case the last key becomes the primary
+	key. Also supports "version:refname" or "v:refname" (tag
+	names are treated as versions). The "version:refname" sort
+	order can also be affected by the "versionsort.suffix"
+	configuration variable.
+	The keys supported are the same as those in `git for-each-ref`.
+
 <repository>::
 	The "remote" repository to query.  This parameter can be
 	either a URL or the name of a remote (see the GIT URLS and
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 540d56429..5521c72f4 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -1,6 +1,7 @@
 #include "builtin.h"
 #include "cache.h"
 #include "transport.h"
+#include "ref-filter.h"
 #include "remote.h"
 
 static const char * const ls_remote_usage[] = {
@@ -47,6 +48,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	struct remote *remote;
 	struct transport *transport;
 	const struct ref *ref;
+	struct ref_array array;
+	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
 
 	struct option options[] = {
 		OPT__QUIET(&quiet, N_("do not print remote URL")),
@@ -60,6 +63,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL),
 		OPT_BOOL(0, "get-url", &get_url,
 			 N_("take url.<base>.insteadOf into account")),
+		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
+			     N_("field name to sort on"), &parse_opt_ref_sorting),
 		OPT_SET_INT_F(0, "exit-code", &status,
 			      N_("exit with exit code 2 if no matching refs are found"),
 			      2, PARSE_OPT_NOCOMPLETE),
@@ -68,6 +73,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	memset(&array, 0, sizeof(array));
+
 	argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 	dest = argv[0];
@@ -108,9 +115,25 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 			continue;
 		if (!tail_match(pattern, ref->name))
 			continue;
+
+		struct ref_array_item *item;
+		FLEX_ALLOC_MEM(item, refname, ref->name, strlen(ref->name));
+		item->symref = ref->symref;
+		item->objectname = ref->old_oid;
+
+		REALLOC_ARRAY(array.items, array.nr + 1);
+		array.items[array.nr++] = item;
+	}
+
+	if (sorting) {
+		ref_array_sort(sorting, &array);
+	}
+
+	for (int i = 0; i < array.nr; i++) {
+		const struct ref_array_item *ref = array.items[i];
 		if (show_symref_target && ref->symref)
-			printf("ref: %s\t%s\n", ref->symref, ref->name);
-		printf("%s\t%s\n", oid_to_hex(&ref->old_oid), ref->name);
+			printf("ref: %s\t%s\n", ref->symref, ref->refname);
+		printf("%s\t%s\n", oid_to_hex(&ref->objectname), ref->refname);
 		status = 0; /* we found something */
 	}
 	return status;
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 02106c922..66370cd88 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -10,6 +10,9 @@ test_expect_success setup '
 	test_tick &&
 	git commit -m initial &&
 	git tag mark &&
+	git tag mark1.1 &&
+	git tag mark1.2 &&
+	git tag mark1.10 &&
 	git show-ref --tags -d | sed -e "s/ /	/" >expected.tag &&
 	(
 		echo "$(git rev-parse HEAD)	HEAD"
@@ -39,6 +42,39 @@ test_expect_success 'ls-remote self' '
 	test_cmp expected.all actual
 '
 
+test_expect_success 'ls-remote --sort="version:refname" --tags self' '
+	cat >expect <<-\EOF &&
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	EOF
+	git ls-remote --sort="version:refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-remote --sort="-version:refname" --tags self' '
+	cat >expect <<-\EOF &&
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	EOF
+	git ls-remote --sort="-version:refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-remote --sort="-refname" --tags self' '
+	cat >expect <<-\EOF &&
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	EOF
+	git ls-remote --sort="-refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'dies when no remote specified and no default remotes found' '
 	test_must_fail git ls-remote
 '
@@ -131,7 +167,7 @@ test_expect_success 'Report no-match with --exit-code' '
 
 test_expect_success 'Report match with --exit-code' '
 	git ls-remote --exit-code other.git "refs/tags/*" >actual &&
-	git ls-remote . tags/mark >expect &&
+	git ls-remote . tags/mark* >expect &&
 	test_cmp expect actual
 '
 
@@ -178,6 +214,9 @@ test_expect_success 'ls-remote --symref' '
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/HEAD
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/master
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
 	EOF
 	git ls-remote --symref >actual &&
 	test_cmp expect actual
-- 
2.14.3 (Apple Git-98)
Harald Nordgren· Apr 4, 2018, 17:11 UTC · re: Harald Nordgren · lore

[PATCH v7] ls-remote: create '--sort' option

Create a '--sort' option for ls-remote, based on the one from for-each-ref. This e.g. allows ref names to be sorted by version semantics, so that v1.2 is sorted before v1.10.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
Notes:
    Started using 'ALLOC_GROW'
 Documentation/git-ls-remote.txt | 12 +++++++++++-
 builtin/ls-remote.c             | 27 +++++++++++++++++++++++++--
 t/t5512-ls-remote.sh            | 41 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 76 insertions(+), 4 deletions(-)
Show changes to 3 files +76 −4

Documentation/git-ls-remote.txt, builtin/ls-remote.c, t/t5512-ls-remote.sh

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 5f2628c8f..17fae7218 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git ls-remote' [--heads] [--tags] [--refs] [--upload-pack=<exec>]
-	      [-q | --quiet] [--exit-code] [--get-url]
+	      [-q | --quiet] [--exit-code] [--get-url] [--sort=<key>]
 	      [--symref] [<repository> [<refs>...]]
 
 DESCRIPTION
@@ -60,6 +60,16 @@ OPTIONS
 	upload-pack only shows the symref HEAD, so it will be the only
 	one shown by ls-remote.
 
+--sort=<key>::
+	Sort based on the key given.  Prefix `-` to sort in
+	descending order of the value. You may use the --sort=<key> option
+	multiple times, in which case the last key becomes the primary
+	key. Also supports "version:refname" or "v:refname" (tag
+	names are treated as versions). The "version:refname" sort
+	order can also be affected by the "versionsort.suffix"
+	configuration variable.
+	The keys supported are the same as those in `git for-each-ref`.
+
 <repository>::
 	The "remote" repository to query.  This parameter can be
 	either a URL or the name of a remote (see the GIT URLS and
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 540d56429..5bb8ee68a 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -1,6 +1,7 @@
 #include "builtin.h"
 #include "cache.h"
 #include "transport.h"
+#include "ref-filter.h"
 #include "remote.h"
 
 static const char * const ls_remote_usage[] = {
@@ -43,10 +44,13 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	int show_symref_target = 0;
 	const char *uploadpack = NULL;
 	const char **pattern = NULL;
+	int i;
 
 	struct remote *remote;
 	struct transport *transport;
 	const struct ref *ref;
+	struct ref_array array;
+	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
 
 	struct option options[] = {
 		OPT__QUIET(&quiet, N_("do not print remote URL")),
@@ -60,6 +64,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL),
 		OPT_BOOL(0, "get-url", &get_url,
 			 N_("take url.<base>.insteadOf into account")),
+		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
+			     N_("field name to sort on"), &parse_opt_ref_sorting),
 		OPT_SET_INT_F(0, "exit-code", &status,
 			      N_("exit with exit code 2 if no matching refs are found"),
 			      2, PARSE_OPT_NOCOMPLETE),
@@ -68,6 +74,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	memset(&array, 0, sizeof(array));
+
 	argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 	dest = argv[0];
@@ -104,13 +112,28 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	if (!dest && !quiet)
 		fprintf(stderr, "From %s\n", *remote->url);
 	for ( ; ref; ref = ref->next) {
+		struct ref_array_item *item;
 		if (!check_ref_type(ref, flags))
 			continue;
 		if (!tail_match(pattern, ref->name))
 			continue;
+
+		FLEX_ALLOC_MEM(item, refname, ref->name, strlen(ref->name));
+		item->symref = ref->symref;
+		item->objectname = ref->old_oid;
+
+		ALLOC_GROW(array.items, array.nr + 1, array.alloc);
+		array.items[array.nr++] = item;
+	}
+
+	if (sorting)
+		ref_array_sort(sorting, &array);
+
+	for (i = 0; i < array.nr; i++) {
+		const struct ref_array_item *ref = array.items[i];
 		if (show_symref_target && ref->symref)
-			printf("ref: %s\t%s\n", ref->symref, ref->name);
-		printf("%s\t%s\n", oid_to_hex(&ref->old_oid), ref->name);
+			printf("ref: %s\t%s\n", ref->symref, ref->refname);
+		printf("%s\t%s\n", oid_to_hex(&ref->objectname), ref->refname);
 		status = 0; /* we found something */
 	}
 	return status;
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 02106c922..66370cd88 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -10,6 +10,9 @@ test_expect_success setup '
 	test_tick &&
 	git commit -m initial &&
 	git tag mark &&
+	git tag mark1.1 &&
+	git tag mark1.2 &&
+	git tag mark1.10 &&
 	git show-ref --tags -d | sed -e "s/ /	/" >expected.tag &&
 	(
 		echo "$(git rev-parse HEAD)	HEAD"
@@ -39,6 +42,39 @@ test_expect_success 'ls-remote self' '
 	test_cmp expected.all actual
 '
 
+test_expect_success 'ls-remote --sort="version:refname" --tags self' '
+	cat >expect <<-\EOF &&
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	EOF
+	git ls-remote --sort="version:refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-remote --sort="-version:refname" --tags self' '
+	cat >expect <<-\EOF &&
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	EOF
+	git ls-remote --sort="-version:refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-remote --sort="-refname" --tags self' '
+	cat >expect <<-\EOF &&
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	EOF
+	git ls-remote --sort="-refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'dies when no remote specified and no default remotes found' '
 	test_must_fail git ls-remote
 '
@@ -131,7 +167,7 @@ test_expect_success 'Report no-match with --exit-code' '
 
 test_expect_success 'Report match with --exit-code' '
 	git ls-remote --exit-code other.git "refs/tags/*" >actual &&
-	git ls-remote . tags/mark >expect &&
+	git ls-remote . tags/mark* >expect &&
 	test_cmp expect actual
 '
 
@@ -178,6 +214,9 @@ test_expect_success 'ls-remote --symref' '
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/HEAD
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/master
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
 	EOF
 	git ls-remote --symref >actual &&
 	test_cmp expect actual
-- 
2.14.3 (Apple Git-98)
Harald Nordgren· Apr 4, 2018, 17:18 UTC · re: Harald Nordgren · lore

Re: [PATCH v7] ls-remote: create '--sort' option

I updated the code to use 'ALLOC_GROW'. I makes sense, I now I realize why array.alloc is there ;)

Jeff, you are right that 'git ls-remote --sort=committerdate' will not work. Do you think we need to do something about this, or it's fine that it fails the way you showed?

On Wed, Apr 4, 2018 at 7:11 PM, Harald Nordgren <haraldnordgren@gmail.com> wrote:

Show 196 quoted lines
> Create a '--sort' option for ls-remote, based on the one from
> for-each-ref. This e.g. allows ref names to be sorted by version
> semantics, so that v1.2 is sorted before v1.10.
>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> ---
>
> Notes:
>     Started using 'ALLOC_GROW'
>
>  Documentation/git-ls-remote.txt | 12 +++++++++++-
>  builtin/ls-remote.c             | 27 +++++++++++++++++++++++++--
>  t/t5512-ls-remote.sh            | 41 ++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 76 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
> index 5f2628c8f..17fae7218 100644
> --- a/Documentation/git-ls-remote.txt
> +++ b/Documentation/git-ls-remote.txt
> @@ -10,7 +10,7 @@ SYNOPSIS
>  --------
>  [verse]
>  'git ls-remote' [--heads] [--tags] [--refs] [--upload-pack=<exec>]
> -             [-q | --quiet] [--exit-code] [--get-url]
> +             [-q | --quiet] [--exit-code] [--get-url] [--sort=<key>]
>               [--symref] [<repository> [<refs>...]]
>
>  DESCRIPTION
> @@ -60,6 +60,16 @@ OPTIONS
>         upload-pack only shows the symref HEAD, so it will be the only
>         one shown by ls-remote.
>
> +--sort=<key>::
> +       Sort based on the key given.  Prefix `-` to sort in
> +       descending order of the value. You may use the --sort=<key> option
> +       multiple times, in which case the last key becomes the primary
> +       key. Also supports "version:refname" or "v:refname" (tag
> +       names are treated as versions). The "version:refname" sort
> +       order can also be affected by the "versionsort.suffix"
> +       configuration variable.
> +       The keys supported are the same as those in `git for-each-ref`.
> +
>  <repository>::
>         The "remote" repository to query.  This parameter can be
>         either a URL or the name of a remote (see the GIT URLS and
> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
> index 540d56429..5bb8ee68a 100644
> --- a/builtin/ls-remote.c
> +++ b/builtin/ls-remote.c
> @@ -1,6 +1,7 @@
>  #include "builtin.h"
>  #include "cache.h"
>  #include "transport.h"
> +#include "ref-filter.h"
>  #include "remote.h"
>
>  static const char * const ls_remote_usage[] = {
> @@ -43,10 +44,13 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>         int show_symref_target = 0;
>         const char *uploadpack = NULL;
>         const char **pattern = NULL;
> +       int i;
>
>         struct remote *remote;
>         struct transport *transport;
>         const struct ref *ref;
> +       struct ref_array array;
> +       static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
>
>         struct option options[] = {
>                 OPT__QUIET(&quiet, N_("do not print remote URL")),
> @@ -60,6 +64,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>                 OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL),
>                 OPT_BOOL(0, "get-url", &get_url,
>                          N_("take url.<base>.insteadOf into account")),
> +               OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
> +                            N_("field name to sort on"), &parse_opt_ref_sorting),
>                 OPT_SET_INT_F(0, "exit-code", &status,
>                               N_("exit with exit code 2 if no matching refs are found"),
>                               2, PARSE_OPT_NOCOMPLETE),
> @@ -68,6 +74,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>                 OPT_END()
>         };
>
> +       memset(&array, 0, sizeof(array));
> +
>         argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
>                              PARSE_OPT_STOP_AT_NON_OPTION);
>         dest = argv[0];
> @@ -104,13 +112,28 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>         if (!dest && !quiet)
>                 fprintf(stderr, "From %s\n", *remote->url);
>         for ( ; ref; ref = ref->next) {
> +               struct ref_array_item *item;
>                 if (!check_ref_type(ref, flags))
>                         continue;
>                 if (!tail_match(pattern, ref->name))
>                         continue;
> +
> +               FLEX_ALLOC_MEM(item, refname, ref->name, strlen(ref->name));
> +               item->symref = ref->symref;
> +               item->objectname = ref->old_oid;
> +
> +               ALLOC_GROW(array.items, array.nr + 1, array.alloc);
> +               array.items[array.nr++] = item;
> +       }
> +
> +       if (sorting)
> +               ref_array_sort(sorting, &array);
> +
> +       for (i = 0; i < array.nr; i++) {
> +               const struct ref_array_item *ref = array.items[i];
>                 if (show_symref_target && ref->symref)
> -                       printf("ref: %s\t%s\n", ref->symref, ref->name);
> -               printf("%s\t%s\n", oid_to_hex(&ref->old_oid), ref->name);
> +                       printf("ref: %s\t%s\n", ref->symref, ref->refname);
> +               printf("%s\t%s\n", oid_to_hex(&ref->objectname), ref->refname);
>                 status = 0; /* we found something */
>         }
>         return status;
> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
> index 02106c922..66370cd88 100755
> --- a/t/t5512-ls-remote.sh
> +++ b/t/t5512-ls-remote.sh
> @@ -10,6 +10,9 @@ test_expect_success setup '
>         test_tick &&
>         git commit -m initial &&
>         git tag mark &&
> +       git tag mark1.1 &&
> +       git tag mark1.2 &&
> +       git tag mark1.10 &&
>         git show-ref --tags -d | sed -e "s/ /   /" >expected.tag &&
>         (
>                 echo "$(git rev-parse HEAD)     HEAD"
> @@ -39,6 +42,39 @@ test_expect_success 'ls-remote self' '
>         test_cmp expected.all actual
>  '
>
> +test_expect_success 'ls-remote --sort="version:refname" --tags self' '
> +       cat >expect <<-\EOF &&
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.1
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.2
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.10
> +       EOF
> +       git ls-remote --sort="version:refname" --tags self >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'ls-remote --sort="-version:refname" --tags self' '
> +       cat >expect <<-\EOF &&
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.10
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.2
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.1
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark
> +       EOF
> +       git ls-remote --sort="-version:refname" --tags self >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'ls-remote --sort="-refname" --tags self' '
> +       cat >expect <<-\EOF &&
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.2
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.10
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.1
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark
> +       EOF
> +       git ls-remote --sort="-refname" --tags self >actual &&
> +       test_cmp expect actual
> +'
> +
>  test_expect_success 'dies when no remote specified and no default remotes found' '
>         test_must_fail git ls-remote
>  '
> @@ -131,7 +167,7 @@ test_expect_success 'Report no-match with --exit-code' '
>
>  test_expect_success 'Report match with --exit-code' '
>         git ls-remote --exit-code other.git "refs/tags/*" >actual &&
> -       git ls-remote . tags/mark >expect &&
> +       git ls-remote . tags/mark* >expect &&
>         test_cmp expect actual
>  '
>
> @@ -178,6 +214,9 @@ test_expect_success 'ls-remote --symref' '
>         1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/remotes/origin/HEAD
>         1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/remotes/origin/master
>         1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.1
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.10
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.2
>         EOF
>         git ls-remote --symref >actual &&
>         test_cmp expect actual
> --
> 2.14.3 (Apple Git-98)
>
Harald Nordgren· Apr 4, 2018, 17:47 UTC · re: Harald Nordgren · lore

Re: [PATCH v7] ls-remote: create '--sort' option

Links to previous revisions:

[1] https://public-inbox.org/git/20180402174614.GA28566@sigill.intra.peff.net/T/#m108fe8c83f3558afaea8e317e680f7eaa136e9a9 [2] https://public-inbox.org/git/20180402211920.GA32099@sigill.intra.peff.net/T/#ma9ec4e0ce664160086e535c012e20d76822c60e5 ... [4] https://public-inbox.org/git/20180402174614.GA28566@sigill.intra.peff.net/T/#maa02c40c87b192e56c370c312098d469c9fce757 [5] https://public-inbox.org/git/20180402174614.GA28566@sigill.intra.peff.net/T/#m52cdbbbba3f359e1257e7bdfe19cd9a26f55fa20 [6] https://public-inbox.org/git/20180402174614.GA28566@sigill.intra.peff.net/T/#m6d3ce17f0f6dabeddaf03336c92512b7c413422b

On Wed, Apr 4, 2018 at 7:18 PM, Harald Nordgren <haraldnordgren@gmail.com> wrote:

Show 205 quoted lines
> I updated the code to use 'ALLOC_GROW'. I makes sense, I now I realize
> why array.alloc is there ;)
>
> Jeff, you are right that 'git ls-remote --sort=committerdate' will not
> work. Do you think we need to do something about this, or it's fine
> that it fails the way you showed?
>
> On Wed, Apr 4, 2018 at 7:11 PM, Harald Nordgren
> <haraldnordgren@gmail.com> wrote:
>> Create a '--sort' option for ls-remote, based on the one from
>> for-each-ref. This e.g. allows ref names to be sorted by version
>> semantics, so that v1.2 is sorted before v1.10.
>>
>> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
>> ---
>>
>> Notes:
>>     Started using 'ALLOC_GROW'
>>
>>  Documentation/git-ls-remote.txt | 12 +++++++++++-
>>  builtin/ls-remote.c             | 27 +++++++++++++++++++++++++--
>>  t/t5512-ls-remote.sh            | 41 ++++++++++++++++++++++++++++++++++++++++-
>>  3 files changed, 76 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
>> index 5f2628c8f..17fae7218 100644
>> --- a/Documentation/git-ls-remote.txt
>> +++ b/Documentation/git-ls-remote.txt
>> @@ -10,7 +10,7 @@ SYNOPSIS
>>  --------
>>  [verse]
>>  'git ls-remote' [--heads] [--tags] [--refs] [--upload-pack=<exec>]
>> -             [-q | --quiet] [--exit-code] [--get-url]
>> +             [-q | --quiet] [--exit-code] [--get-url] [--sort=<key>]
>>               [--symref] [<repository> [<refs>...]]
>>
>>  DESCRIPTION
>> @@ -60,6 +60,16 @@ OPTIONS
>>         upload-pack only shows the symref HEAD, so it will be the only
>>         one shown by ls-remote.
>>
>> +--sort=<key>::
>> +       Sort based on the key given.  Prefix `-` to sort in
>> +       descending order of the value. You may use the --sort=<key> option
>> +       multiple times, in which case the last key becomes the primary
>> +       key. Also supports "version:refname" or "v:refname" (tag
>> +       names are treated as versions). The "version:refname" sort
>> +       order can also be affected by the "versionsort.suffix"
>> +       configuration variable.
>> +       The keys supported are the same as those in `git for-each-ref`.
>> +
>>  <repository>::
>>         The "remote" repository to query.  This parameter can be
>>         either a URL or the name of a remote (see the GIT URLS and
>> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
>> index 540d56429..5bb8ee68a 100644
>> --- a/builtin/ls-remote.c
>> +++ b/builtin/ls-remote.c
>> @@ -1,6 +1,7 @@
>>  #include "builtin.h"
>>  #include "cache.h"
>>  #include "transport.h"
>> +#include "ref-filter.h"
>>  #include "remote.h"
>>
>>  static const char * const ls_remote_usage[] = {
>> @@ -43,10 +44,13 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>>         int show_symref_target = 0;
>>         const char *uploadpack = NULL;
>>         const char **pattern = NULL;
>> +       int i;
>>
>>         struct remote *remote;
>>         struct transport *transport;
>>         const struct ref *ref;
>> +       struct ref_array array;
>> +       static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
>>
>>         struct option options[] = {
>>                 OPT__QUIET(&quiet, N_("do not print remote URL")),
>> @@ -60,6 +64,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>>                 OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL),
>>                 OPT_BOOL(0, "get-url", &get_url,
>>                          N_("take url.<base>.insteadOf into account")),
>> +               OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
>> +                            N_("field name to sort on"), &parse_opt_ref_sorting),
>>                 OPT_SET_INT_F(0, "exit-code", &status,
>>                               N_("exit with exit code 2 if no matching refs are found"),
>>                               2, PARSE_OPT_NOCOMPLETE),
>> @@ -68,6 +74,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>>                 OPT_END()
>>         };
>>
>> +       memset(&array, 0, sizeof(array));
>> +
>>         argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
>>                              PARSE_OPT_STOP_AT_NON_OPTION);
>>         dest = argv[0];
>> @@ -104,13 +112,28 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>>         if (!dest && !quiet)
>>                 fprintf(stderr, "From %s\n", *remote->url);
>>         for ( ; ref; ref = ref->next) {
>> +               struct ref_array_item *item;
>>                 if (!check_ref_type(ref, flags))
>>                         continue;
>>                 if (!tail_match(pattern, ref->name))
>>                         continue;
>> +
>> +               FLEX_ALLOC_MEM(item, refname, ref->name, strlen(ref->name));
>> +               item->symref = ref->symref;
>> +               item->objectname = ref->old_oid;
>> +
>> +               ALLOC_GROW(array.items, array.nr + 1, array.alloc);
>> +               array.items[array.nr++] = item;
>> +       }
>> +
>> +       if (sorting)
>> +               ref_array_sort(sorting, &array);
>> +
>> +       for (i = 0; i < array.nr; i++) {
>> +               const struct ref_array_item *ref = array.items[i];
>>                 if (show_symref_target && ref->symref)
>> -                       printf("ref: %s\t%s\n", ref->symref, ref->name);
>> -               printf("%s\t%s\n", oid_to_hex(&ref->old_oid), ref->name);
>> +                       printf("ref: %s\t%s\n", ref->symref, ref->refname);
>> +               printf("%s\t%s\n", oid_to_hex(&ref->objectname), ref->refname);
>>                 status = 0; /* we found something */
>>         }
>>         return status;
>> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
>> index 02106c922..66370cd88 100755
>> --- a/t/t5512-ls-remote.sh
>> +++ b/t/t5512-ls-remote.sh
>> @@ -10,6 +10,9 @@ test_expect_success setup '
>>         test_tick &&
>>         git commit -m initial &&
>>         git tag mark &&
>> +       git tag mark1.1 &&
>> +       git tag mark1.2 &&
>> +       git tag mark1.10 &&
>>         git show-ref --tags -d | sed -e "s/ /   /" >expected.tag &&
>>         (
>>                 echo "$(git rev-parse HEAD)     HEAD"
>> @@ -39,6 +42,39 @@ test_expect_success 'ls-remote self' '
>>         test_cmp expected.all actual
>>  '
>>
>> +test_expect_success 'ls-remote --sort="version:refname" --tags self' '
>> +       cat >expect <<-\EOF &&
>> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark
>> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.1
>> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.2
>> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.10
>> +       EOF
>> +       git ls-remote --sort="version:refname" --tags self >actual &&
>> +       test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'ls-remote --sort="-version:refname" --tags self' '
>> +       cat >expect <<-\EOF &&
>> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.10
>> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.2
>> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.1
>> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark
>> +       EOF
>> +       git ls-remote --sort="-version:refname" --tags self >actual &&
>> +       test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'ls-remote --sort="-refname" --tags self' '
>> +       cat >expect <<-\EOF &&
>> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.2
>> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.10
>> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.1
>> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark
>> +       EOF
>> +       git ls-remote --sort="-refname" --tags self >actual &&
>> +       test_cmp expect actual
>> +'
>> +
>>  test_expect_success 'dies when no remote specified and no default remotes found' '
>>         test_must_fail git ls-remote
>>  '
>> @@ -131,7 +167,7 @@ test_expect_success 'Report no-match with --exit-code' '
>>
>>  test_expect_success 'Report match with --exit-code' '
>>         git ls-remote --exit-code other.git "refs/tags/*" >actual &&
>> -       git ls-remote . tags/mark >expect &&
>> +       git ls-remote . tags/mark* >expect &&
>>         test_cmp expect actual
>>  '
>>
>> @@ -178,6 +214,9 @@ test_expect_success 'ls-remote --symref' '
>>         1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/remotes/origin/HEAD
>>         1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/remotes/origin/master
>>         1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark
>> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.1
>> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.10
>> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.2
>>         EOF
>>         git ls-remote --symref >actual &&
>>         test_cmp expect actual
>> --
>> 2.14.3 (Apple Git-98)
>>
Jeff King· Apr 4, 2018, 18:56 UTC · re: Harald Nordgren · lore

Re: [PATCH v7] ls-remote: create '--sort' option

On Wed, Apr 04, 2018 at 07:18:42PM +0200, Harald Nordgren wrote:
> Jeff, you are right that 'git ls-remote --sort=committerdate' will not
> work. Do you think we need to do something about this, or it's fine
> that it fails the way you showed?

It's a reasonable-sized footgun, but one that I think most people would be unlikely to trigger. I think it's probably OK as long as we warn the user in the documentation (see my other response).

-Peff
Jeff King· Apr 4, 2018, 18:55 UTC · re: Harald Nordgren · lore

Re: [PATCH v7] ls-remote: create '--sort' option

On Wed, Apr 04, 2018 at 07:11:53PM +0200, Harald Nordgren wrote:
Show 13 quoted lines
> @@ -60,6 +60,16 @@ OPTIONS
>  	upload-pack only shows the symref HEAD, so it will be the only
>  	one shown by ls-remote.
>  
> +--sort=<key>::
> +	Sort based on the key given.  Prefix `-` to sort in
> +	descending order of the value. You may use the --sort=<key> option
> +	multiple times, in which case the last key becomes the primary
> +	key. Also supports "version:refname" or "v:refname" (tag
> +	names are treated as versions). The "version:refname" sort
> +	order can also be affected by the "versionsort.suffix"
> +	configuration variable.
> +	The keys supported are the same as those in `git for-each-ref`.

We probably ought to warn the user in that final sentence that keys which actually look at the objects may not work, since we don't necessarily have the objects.

There's one other subtlety, which is that things like %(HEAD) assume we're talking about local refs, not the remote HEAD. So that wouldn't work (of course it seems unlikely that anybody woudl _sort_ on that).

Show 11 quoted lines
> @@ -104,13 +112,28 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>  	if (!dest && !quiet)
>  		fprintf(stderr, "From %s\n", *remote->url);
>  	for ( ; ref; ref = ref->next) {
> +		struct ref_array_item *item;
>  		if (!check_ref_type(ref, flags))
>  			continue;
>  		if (!tail_match(pattern, ref->name))
>  			continue;
> +
> +		FLEX_ALLOC_MEM(item, refname, ref->name, strlen(ref->name));
I think this can use the slightly-simpler FLEX_ALLOC_STR().
> +		item->symref = ref->symref;

Normally a ref_array_item's symref is an allocated string owned by the item. I don't think it actually matters now, but in the spirit of least-surprise for the future, should this be xstrdup_or_null(ref->symref)?

> +		item->objectname = ref->old_oid;

This is actually a struct assignment. Which does work, but our usual mechanism would be to use "oidcpy(&item->objectname, &ref->old_oid)".

All of this might be a little nicer if ref-filter provided a function to allocate a new item. We're pushing the boundaries of ref-filter was meant to be used here, as it was assumed you'd always start with a call to filter_refs().

> +		ALLOC_GROW(array.items, array.nr + 1, array.alloc);
> +		array.items[array.nr++] = item;

The existing ref-filter code fails to use ALLOC_GROW() correctly. I don't think it actually matters, since we don't intermingle this with allocations done there. But perhaps we should be fixing that one while we're looking at it. Or again, maybe it would be nicer still if there were a ref-filter function to do this, and the whole call here could just be:

  ref_array_push(&array, ref->name, &ref->old_oid);

One more drastic alternative is to actually use the existing filter_refs(), and just teach it to populate the array from a list of refs. As you can see from its implementation, it does a few other setup steps. I don't think they matter now, but if you eventually wanted to be able to do "git ls-remote --contains HEAD", you'd need that setup.

-Peff
Harald Nordgren· Apr 4, 2018, 23:01 UTC · re: Harald Nordgren · lore

[PATCH v8] ls-remote: create '--sort' option

Create a '--sort' option for ls-remote, based on the one from for-each-ref. This e.g. allows ref names to be sorted by version semantics, so that v1.2 is sorted before v1.10.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
Notes:
    Partial fixes from Jeff King's comments
 Documentation/git-ls-remote.txt | 15 ++++++++++++++-
 builtin/ls-remote.c             | 27 +++++++++++++++++++++++++--
 t/t5512-ls-remote.sh            | 41 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 79 insertions(+), 4 deletions(-)
Show changes to 3 files +79 −4

Documentation/git-ls-remote.txt, builtin/ls-remote.c, t/t5512-ls-remote.sh

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 5f2628c8f..fa4505fd7 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git ls-remote' [--heads] [--tags] [--refs] [--upload-pack=<exec>]
-	      [-q | --quiet] [--exit-code] [--get-url]
+	      [-q | --quiet] [--exit-code] [--get-url] [--sort=<key>]
 	      [--symref] [<repository> [<refs>...]]
 
 DESCRIPTION
@@ -60,6 +60,19 @@ OPTIONS
 	upload-pack only shows the symref HEAD, so it will be the only
 	one shown by ls-remote.
 
+--sort=<key>::
+	Sort based on the key given.  Prefix `-` to sort in
+	descending order of the value. You may use the --sort=<key> option
+	multiple times, in which case the last key becomes the primary
+	key. Also supports "version:refname" or "v:refname" (tag
+	names are treated as versions). The "version:refname" sort
+	order can also be affected by the "versionsort.suffix"
+	configuration variable.
+	The keys supported are the same as those in `git for-each-ref`,
+	except that because `ls-remote` deals only with remotes, keys like
+	`committerdate` that require access to the objects themselves will
+	not work.
+
 <repository>::
 	The "remote" repository to query.  This parameter can be
 	either a URL or the name of a remote (see the GIT URLS and
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 540d56429..fbec2bc95 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -1,6 +1,7 @@
 #include "builtin.h"
 #include "cache.h"
 #include "transport.h"
+#include "ref-filter.h"
 #include "remote.h"
 
 static const char * const ls_remote_usage[] = {
@@ -43,10 +44,13 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	int show_symref_target = 0;
 	const char *uploadpack = NULL;
 	const char **pattern = NULL;
+	int i;
 
 	struct remote *remote;
 	struct transport *transport;
 	const struct ref *ref;
+	struct ref_array array;
+	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
 
 	struct option options[] = {
 		OPT__QUIET(&quiet, N_("do not print remote URL")),
@@ -60,6 +64,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL),
 		OPT_BOOL(0, "get-url", &get_url,
 			 N_("take url.<base>.insteadOf into account")),
+		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
+			     N_("field name to sort on"), &parse_opt_ref_sorting),
 		OPT_SET_INT_F(0, "exit-code", &status,
 			      N_("exit with exit code 2 if no matching refs are found"),
 			      2, PARSE_OPT_NOCOMPLETE),
@@ -68,6 +74,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	memset(&array, 0, sizeof(array));
+
 	argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 	dest = argv[0];
@@ -104,13 +112,28 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	if (!dest && !quiet)
 		fprintf(stderr, "From %s\n", *remote->url);
 	for ( ; ref; ref = ref->next) {
+		struct ref_array_item *item;
 		if (!check_ref_type(ref, flags))
 			continue;
 		if (!tail_match(pattern, ref->name))
 			continue;
+
+		FLEX_ALLOC_STR(item, refname, ref->name);
+		item->symref = xstrdup_or_null(ref->symref);
+		oidcpy(&item->objectname, &ref->old_oid);
+
+		ALLOC_GROW(array.items, array.nr + 1, array.alloc);
+		array.items[array.nr++] = item;
+	}
+
+	if (sorting)
+		ref_array_sort(sorting, &array);
+
+	for (i = 0; i < array.nr; i++) {
+		const struct ref_array_item *ref = array.items[i];
 		if (show_symref_target && ref->symref)
-			printf("ref: %s\t%s\n", ref->symref, ref->name);
-		printf("%s\t%s\n", oid_to_hex(&ref->old_oid), ref->name);
+			printf("ref: %s\t%s\n", ref->symref, ref->refname);
+		printf("%s\t%s\n", oid_to_hex(&ref->objectname), ref->refname);
 		status = 0; /* we found something */
 	}
 	return status;
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 02106c922..66370cd88 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -10,6 +10,9 @@ test_expect_success setup '
 	test_tick &&
 	git commit -m initial &&
 	git tag mark &&
+	git tag mark1.1 &&
+	git tag mark1.2 &&
+	git tag mark1.10 &&
 	git show-ref --tags -d | sed -e "s/ /	/" >expected.tag &&
 	(
 		echo "$(git rev-parse HEAD)	HEAD"
@@ -39,6 +42,39 @@ test_expect_success 'ls-remote self' '
 	test_cmp expected.all actual
 '
 
+test_expect_success 'ls-remote --sort="version:refname" --tags self' '
+	cat >expect <<-\EOF &&
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	EOF
+	git ls-remote --sort="version:refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-remote --sort="-version:refname" --tags self' '
+	cat >expect <<-\EOF &&
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	EOF
+	git ls-remote --sort="-version:refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-remote --sort="-refname" --tags self' '
+	cat >expect <<-\EOF &&
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	EOF
+	git ls-remote --sort="-refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'dies when no remote specified and no default remotes found' '
 	test_must_fail git ls-remote
 '
@@ -131,7 +167,7 @@ test_expect_success 'Report no-match with --exit-code' '
 
 test_expect_success 'Report match with --exit-code' '
 	git ls-remote --exit-code other.git "refs/tags/*" >actual &&
-	git ls-remote . tags/mark >expect &&
+	git ls-remote . tags/mark* >expect &&
 	test_cmp expect actual
 '
 
@@ -178,6 +214,9 @@ test_expect_success 'ls-remote --symref' '
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/HEAD
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/master
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
 	EOF
 	git ls-remote --symref >actual &&
 	test_cmp expect actual
-- 
2.14.3 (Apple Git-98)
Harald Nordgren· Apr 4, 2018, 23:11 UTC · re: Harald Nordgren · lore

Re: [PATCH v8] ls-remote: create '--sort' option

Without digging to much into the `ref-filter` code itself, it seems like there is an opportunity to generalize and unfify the logic between these two cases. As well as using `ALLOC_GROW`. But maybe that is best left as a follow-up task? Especially since this patch focuses on `ls-remote`. Seems possibly like too big of a change to start changing a different sub-command.

Wouldn't a `ref_array_push()` also require `ref->symref`, maybe then we could pass the whole ref? It needs to be very clear that it's a `ref` and not a `ref_array_item` that is being pushed. Much of my logic here deals specifically with trying to treat a ref as ref_array_item.

From my viewpoint as implementer, I was very happy that I could implement the feature *without* invoking `filter_refs` since that `filter->kind` switching looks a pretty daunting. I'm not exactly sure what a `git ls-remote --contains HEAD` would do, maybe you could explain a bit more?

On Thu, Apr 5, 2018 at 1:01 AM, Harald Nordgren <haraldnordgren@gmail.com> wrote:

Show 199 quoted lines
> Create a '--sort' option for ls-remote, based on the one from
> for-each-ref. This e.g. allows ref names to be sorted by version
> semantics, so that v1.2 is sorted before v1.10.
>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> ---
>
> Notes:
>     Partial fixes from Jeff King's comments
>
>  Documentation/git-ls-remote.txt | 15 ++++++++++++++-
>  builtin/ls-remote.c             | 27 +++++++++++++++++++++++++--
>  t/t5512-ls-remote.sh            | 41 ++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 79 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
> index 5f2628c8f..fa4505fd7 100644
> --- a/Documentation/git-ls-remote.txt
> +++ b/Documentation/git-ls-remote.txt
> @@ -10,7 +10,7 @@ SYNOPSIS
>  --------
>  [verse]
>  'git ls-remote' [--heads] [--tags] [--refs] [--upload-pack=<exec>]
> -             [-q | --quiet] [--exit-code] [--get-url]
> +             [-q | --quiet] [--exit-code] [--get-url] [--sort=<key>]
>               [--symref] [<repository> [<refs>...]]
>
>  DESCRIPTION
> @@ -60,6 +60,19 @@ OPTIONS
>         upload-pack only shows the symref HEAD, so it will be the only
>         one shown by ls-remote.
>
> +--sort=<key>::
> +       Sort based on the key given.  Prefix `-` to sort in
> +       descending order of the value. You may use the --sort=<key> option
> +       multiple times, in which case the last key becomes the primary
> +       key. Also supports "version:refname" or "v:refname" (tag
> +       names are treated as versions). The "version:refname" sort
> +       order can also be affected by the "versionsort.suffix"
> +       configuration variable.
> +       The keys supported are the same as those in `git for-each-ref`,
> +       except that because `ls-remote` deals only with remotes, keys like
> +       `committerdate` that require access to the objects themselves will
> +       not work.
> +
>  <repository>::
>         The "remote" repository to query.  This parameter can be
>         either a URL or the name of a remote (see the GIT URLS and
> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
> index 540d56429..fbec2bc95 100644
> --- a/builtin/ls-remote.c
> +++ b/builtin/ls-remote.c
> @@ -1,6 +1,7 @@
>  #include "builtin.h"
>  #include "cache.h"
>  #include "transport.h"
> +#include "ref-filter.h"
>  #include "remote.h"
>
>  static const char * const ls_remote_usage[] = {
> @@ -43,10 +44,13 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>         int show_symref_target = 0;
>         const char *uploadpack = NULL;
>         const char **pattern = NULL;
> +       int i;
>
>         struct remote *remote;
>         struct transport *transport;
>         const struct ref *ref;
> +       struct ref_array array;
> +       static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
>
>         struct option options[] = {
>                 OPT__QUIET(&quiet, N_("do not print remote URL")),
> @@ -60,6 +64,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>                 OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL),
>                 OPT_BOOL(0, "get-url", &get_url,
>                          N_("take url.<base>.insteadOf into account")),
> +               OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
> +                            N_("field name to sort on"), &parse_opt_ref_sorting),
>                 OPT_SET_INT_F(0, "exit-code", &status,
>                               N_("exit with exit code 2 if no matching refs are found"),
>                               2, PARSE_OPT_NOCOMPLETE),
> @@ -68,6 +74,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>                 OPT_END()
>         };
>
> +       memset(&array, 0, sizeof(array));
> +
>         argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
>                              PARSE_OPT_STOP_AT_NON_OPTION);
>         dest = argv[0];
> @@ -104,13 +112,28 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>         if (!dest && !quiet)
>                 fprintf(stderr, "From %s\n", *remote->url);
>         for ( ; ref; ref = ref->next) {
> +               struct ref_array_item *item;
>                 if (!check_ref_type(ref, flags))
>                         continue;
>                 if (!tail_match(pattern, ref->name))
>                         continue;
> +
> +               FLEX_ALLOC_STR(item, refname, ref->name);
> +               item->symref = xstrdup_or_null(ref->symref);
> +               oidcpy(&item->objectname, &ref->old_oid);
> +
> +               ALLOC_GROW(array.items, array.nr + 1, array.alloc);
> +               array.items[array.nr++] = item;
> +       }
> +
> +       if (sorting)
> +               ref_array_sort(sorting, &array);
> +
> +       for (i = 0; i < array.nr; i++) {
> +               const struct ref_array_item *ref = array.items[i];
>                 if (show_symref_target && ref->symref)
> -                       printf("ref: %s\t%s\n", ref->symref, ref->name);
> -               printf("%s\t%s\n", oid_to_hex(&ref->old_oid), ref->name);
> +                       printf("ref: %s\t%s\n", ref->symref, ref->refname);
> +               printf("%s\t%s\n", oid_to_hex(&ref->objectname), ref->refname);
>                 status = 0; /* we found something */
>         }
>         return status;
> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
> index 02106c922..66370cd88 100755
> --- a/t/t5512-ls-remote.sh
> +++ b/t/t5512-ls-remote.sh
> @@ -10,6 +10,9 @@ test_expect_success setup '
>         test_tick &&
>         git commit -m initial &&
>         git tag mark &&
> +       git tag mark1.1 &&
> +       git tag mark1.2 &&
> +       git tag mark1.10 &&
>         git show-ref --tags -d | sed -e "s/ /   /" >expected.tag &&
>         (
>                 echo "$(git rev-parse HEAD)     HEAD"
> @@ -39,6 +42,39 @@ test_expect_success 'ls-remote self' '
>         test_cmp expected.all actual
>  '
>
> +test_expect_success 'ls-remote --sort="version:refname" --tags self' '
> +       cat >expect <<-\EOF &&
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.1
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.2
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.10
> +       EOF
> +       git ls-remote --sort="version:refname" --tags self >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'ls-remote --sort="-version:refname" --tags self' '
> +       cat >expect <<-\EOF &&
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.10
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.2
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.1
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark
> +       EOF
> +       git ls-remote --sort="-version:refname" --tags self >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'ls-remote --sort="-refname" --tags self' '
> +       cat >expect <<-\EOF &&
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.2
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.10
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.1
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark
> +       EOF
> +       git ls-remote --sort="-refname" --tags self >actual &&
> +       test_cmp expect actual
> +'
> +
>  test_expect_success 'dies when no remote specified and no default remotes found' '
>         test_must_fail git ls-remote
>  '
> @@ -131,7 +167,7 @@ test_expect_success 'Report no-match with --exit-code' '
>
>  test_expect_success 'Report match with --exit-code' '
>         git ls-remote --exit-code other.git "refs/tags/*" >actual &&
> -       git ls-remote . tags/mark >expect &&
> +       git ls-remote . tags/mark* >expect &&
>         test_cmp expect actual
>  '
>
> @@ -178,6 +214,9 @@ test_expect_success 'ls-remote --symref' '
>         1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/remotes/origin/HEAD
>         1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/remotes/origin/master
>         1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.1
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.10
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.2
>         EOF
>         git ls-remote --symref >actual &&
>         test_cmp expect actual
> --
> 2.14.3 (Apple Git-98)
>
Jeff King· Apr 6, 2018, 18:58 UTC · re: Harald Nordgren · lore

Re: [PATCH v8] ls-remote: create '--sort' option

On Thu, Apr 05, 2018 at 01:11:34AM +0200, Harald Nordgren wrote:
Show 6 quoted lines
> Without digging to much into the `ref-filter` code itself, it seems
> like there is an opportunity to generalize and unfify the logic
> between these two cases. As well as using `ALLOC_GROW`. But maybe that
> is best left as a follow-up task? Especially since this patch focuses
> on `ls-remote`. Seems possibly like too big of a change to start
> changing a different sub-command.

Yes and no. By refactoring we have the potential to regress another unrelated command. But by not refactoring, we set up a potential misuse of the other interface which may lead to more bugs down the road.

I think an important thing is to split out the refactoring steps into their own patches, so it's easy to see that they're not hurting the existing uses. I have some sample patches, which I'll send in a moment.

Show 5 quoted lines
> Wouldn't a `ref_array_push()` also require `ref->symref`, maybe then
> we could pass the whole ref? It needs to be very clear that it's a
> `ref` and not a `ref_array_item` that is being pushed. Much of my
> logic here deals specifically with trying to treat a ref as
> ref_array_item.

I think it would be OK to have the push operation return the actual item, which can then be filled in more. That's how the internal ref-filter code does it already.

Show 5 quoted lines
> From my viewpoint as implementer, I was very happy that I could
> implement the feature *without* invoking `filter_refs` since that
> `filter->kind` switching looks a pretty daunting. I'm not exactly sure
> what a `git ls-remote --contains HEAD` would do, maybe you could
> explain a bit more?

I agree it would be a bit larger of a retro-fit to adapt filter_refs() for our purpose here. And since we're not doing any filtering at this point, there's not much benefit.

Here are the refactoring patches I came up with. I think they're a benefit to ref-filter on their own, and then it should be pretty easy for you to build your patch on top.

  [1/3]: ref-filter: use "struct object_id" consistently
  [2/3]: ref-filter: make ref_array_item allocation more consistent
  [3/3]: ref-filter: factor ref_array pushing into its own function
 builtin/tag.c        |  2 +-
 builtin/verify-tag.c |  2 +-
 ref-filter.c         | 36 +++++++++++++++++++++++++-----------
 ref-filter.h         | 10 +++++++++-
 4 files changed, 36 insertions(+), 14 deletions(-)
-Peff
PS I'm going to be offline for a few weeks starting tomorrow, so I won't
   be responsive to this thread. :) If you like the direction, please
   feel free to pick up those patches, rebase yours on top, and then
   send them all out as a series.
   In case it's easier than applying from the list, you can fetch them
   from the "jk/ref-array-push" branch of https://github.com/peff/git.
Jeff King· Apr 6, 2018, 18:58 UTC · re: Jeff King · lore

[PATCH 1/3] ref-filter: use "struct object_id" consistently

Internally we store a "struct object_id", and all of our callers have one to pass us. But we insist that they peel it to its bare-sha1 hash, which we then hashcpy() into place. Let's pass it around as an object_id, which future-proofs us for a post-sha1 world.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/tag.c        |  2 +-
 builtin/verify-tag.c |  2 +-
 ref-filter.c         | 10 +++++-----
 ref-filter.h         |  2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)
Show changes to 4 files +8 −8

builtin/tag.c, builtin/verify-tag.c, ref-filter.c, ref-filter.h

diff --git a/builtin/tag.c b/builtin/tag.c
index da186691ed..42278f5167 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -117,7 +117,7 @@ static int verify_tag(const char *name, const char *ref,
 		return -1;
 
 	if (format->format)
-		pretty_print_ref(name, oid->hash, format);
+		pretty_print_ref(name, oid, format);
 
 	return 0;
 }
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index ad7b79fa5c..6fa04b751a 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -72,7 +72,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 		}
 
 		if (format.format)
-			pretty_print_ref(name, oid.hash, &format);
+			pretty_print_ref(name, &oid, &format);
 	}
 	return had_error;
 }
diff --git a/ref-filter.c b/ref-filter.c
index 45fc56216a..ade97a8486 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1826,12 +1826,12 @@ static const struct object_id *match_points_at(struct oid_array *points_at,
 
 /* Allocate space for a new ref_array_item and copy the objectname and flag to it */
 static struct ref_array_item *new_ref_array_item(const char *refname,
-						 const unsigned char *objectname,
+						 const struct object_id *oid,
 						 int flag)
 {
 	struct ref_array_item *ref;
 	FLEX_ALLOC_STR(ref, refname, refname);
-	hashcpy(ref->objectname.hash, objectname);
+	oidcpy(&ref->objectname, oid);
 	ref->flag = flag;
 
 	return ref;
@@ -1927,7 +1927,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	 * to do its job and the resulting list may yet to be pruned
 	 * by maxcount logic.
 	 */
-	ref = new_ref_array_item(refname, oid->hash, flag);
+	ref = new_ref_array_item(refname, oid, flag);
 	ref->commit = commit;
 
 	REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
@@ -2165,11 +2165,11 @@ void show_ref_array_item(struct ref_array_item *info,
 	putchar('\n');
 }
 
-void pretty_print_ref(const char *name, const unsigned char *sha1,
+void pretty_print_ref(const char *name, const struct object_id *oid,
 		      const struct ref_format *format)
 {
 	struct ref_array_item *ref_item;
-	ref_item = new_ref_array_item(name, sha1, 0);
+	ref_item = new_ref_array_item(name, oid, 0);
 	ref_item->kind = ref_kind_from_refname(name);
 	show_ref_array_item(ref_item, format);
 	free_array_item(ref_item);
diff --git a/ref-filter.h b/ref-filter.h
index 0d98342b34..68268f9ebc 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -132,7 +132,7 @@ void setup_ref_filter_porcelain_msg(void);
  * Print a single ref, outside of any ref-filter. Note that the
  * name must be a fully qualified refname.
  */
-void pretty_print_ref(const char *name, const unsigned char *sha1,
+void pretty_print_ref(const char *name, const struct object_id *oid,
 		      const struct ref_format *format);
 
 #endif /*  REF_FILTER_H  */
-- 
2.17.0.686.g08b0810b04
Jeff King· Apr 6, 2018, 18:59 UTC · re: Jeff King · lore

[PATCH 2/3] ref-filter: make ref_array_item allocation more consistent

We have a helper function to allocate ref_array_item structs, but it only takes a subset of the possible fields in the struct as initializers. We could have it accept an argument for _every_ field, but that becomes a pain for the fields which some callers don't want to set initially.

Instead, let's be explicit that it takes only the minimum required to create the ref, and that callers should then fill in the rest themselves.

Signed-off-by: Jeff King <peff@peff.net>
---
Technically we don't need the oid to create the struct, just the name,
but I figured that every caller is going to have to have some oid.
 ref-filter.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)
Show changes to ref-filter.c +11 −7
diff --git a/ref-filter.c b/ref-filter.c
index ade97a8486..c1c3cc9480 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1824,15 +1824,18 @@ static const struct object_id *match_points_at(struct oid_array *points_at,
 	return NULL;
 }
 
-/* Allocate space for a new ref_array_item and copy the objectname and flag to it */
+/*
+ * Allocate space for a new ref_array_item and copy the name and oid to it.
+ *
+ * Callers can then fill in other struct members at their leisure.
+ */
 static struct ref_array_item *new_ref_array_item(const char *refname,
-						 const struct object_id *oid,
-						 int flag)
+						 const struct object_id *oid)
 {
 	struct ref_array_item *ref;
+
 	FLEX_ALLOC_STR(ref, refname, refname);
 	oidcpy(&ref->objectname, oid);
-	ref->flag = flag;
 
 	return ref;
 }
@@ -1927,12 +1930,13 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	 * to do its job and the resulting list may yet to be pruned
 	 * by maxcount logic.
 	 */
-	ref = new_ref_array_item(refname, oid, flag);
+	ref = new_ref_array_item(refname, oid);
 	ref->commit = commit;
+	ref->flag = flag;
+	ref->kind = kind;
 
 	REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
 	ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
-	ref->kind = kind;
 	return 0;
 }
 
@@ -2169,7 +2173,7 @@ void pretty_print_ref(const char *name, const struct object_id *oid,
 		      const struct ref_format *format)
 {
 	struct ref_array_item *ref_item;
-	ref_item = new_ref_array_item(name, oid, 0);
+	ref_item = new_ref_array_item(name, oid);
 	ref_item->kind = ref_kind_from_refname(name);
 	show_ref_array_item(ref_item, format);
 	free_array_item(ref_item);
-- 
2.17.0.686.g08b0810b04
Jeff King· Apr 6, 2018, 18:59 UTC · re: Jeff King · lore

[PATCH 3/3] ref-filter: factor ref_array pushing into its own function

In preparation for callers constructing their own ref_array structs, let's move our own internal push operation into its own function.

While we're at it, we can replace REALLOC_ARRAY() with ALLOC_GROW(), which should give the growth operation amortized linear complexity (as opposed to growing by one, which is potentially quadratic, though in-place realloc growth often makes this faster in practice).

Signed-off-by: Jeff King <peff@peff.net>
---
 ref-filter.c | 16 +++++++++++++---
 ref-filter.h |  8 ++++++++
 2 files changed, 21 insertions(+), 3 deletions(-)
Show changes to 2 files +21 −3

ref-filter.c, ref-filter.h

diff --git a/ref-filter.c b/ref-filter.c
index c1c3cc9480..6e9328b274 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1840,6 +1840,18 @@ static struct ref_array_item *new_ref_array_item(const char *refname,
 	return ref;
 }
 
+struct ref_array_item *ref_array_push(struct ref_array *array,
+				      const char *refname,
+				      const struct object_id *oid)
+{
+	struct ref_array_item *ref = new_ref_array_item(refname, oid);
+
+	ALLOC_GROW(array->items, array->nr + 1, array->alloc);
+	array->items[array->nr++] = ref;
+
+	return ref;
+}
+
 static int ref_kind_from_refname(const char *refname)
 {
 	unsigned int i;
@@ -1930,13 +1942,11 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	 * to do its job and the resulting list may yet to be pruned
 	 * by maxcount logic.
 	 */
-	ref = new_ref_array_item(refname, oid);
+	ref = ref_array_push(ref_cbdata->array, refname, oid);
 	ref->commit = commit;
 	ref->flag = flag;
 	ref->kind = kind;
 
-	REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
-	ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
 	return 0;
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 68268f9ebc..76cf87cb6c 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -135,4 +135,12 @@ void setup_ref_filter_porcelain_msg(void);
 void pretty_print_ref(const char *name, const struct object_id *oid,
 		      const struct ref_format *format);
 
+/*
+ * Push a single ref onto the array; this can be used to construct your own
+ * ref_array without using filter_refs().
+ */
+struct ref_array_item *ref_array_push(struct ref_array *array,
+				      const char *refname,
+				      const struct object_id *oid);
+
 #endif /*  REF_FILTER_H  */
-- 
2.17.0.686.g08b0810b04
Derrick Stolee· Apr 6, 2018, 19:27 UTC · re: Jeff King · lore

Re: [PATCH 3/3] ref-filter: factor ref_array pushing into its own function

On 4/6/2018 2:59 PM, Jeff King wrote:
Show 71 quoted lines
> In preparation for callers constructing their own ref_array
> structs, let's move our own internal push operation into its
> own function.
>
> While we're at it, we can replace REALLOC_ARRAY() with
> ALLOC_GROW(), which should give the growth operation
> amortized linear complexity (as opposed to growing by one,
> which is potentially quadratic, though in-place realloc
> growth often makes this faster in practice).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>   ref-filter.c | 16 +++++++++++++---
>   ref-filter.h |  8 ++++++++
>   2 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index c1c3cc9480..6e9328b274 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1840,6 +1840,18 @@ static struct ref_array_item *new_ref_array_item(const char *refname,
>   	return ref;
>   }
>   
> +struct ref_array_item *ref_array_push(struct ref_array *array,
> +				      const char *refname,
> +				      const struct object_id *oid)
> +{
> +	struct ref_array_item *ref = new_ref_array_item(refname, oid);
> +
> +	ALLOC_GROW(array->items, array->nr + 1, array->alloc);
> +	array->items[array->nr++] = ref;
> +
> +	return ref;
> +}
> +
>   static int ref_kind_from_refname(const char *refname)
>   {
>   	unsigned int i;
> @@ -1930,13 +1942,11 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
>   	 * to do its job and the resulting list may yet to be pruned
>   	 * by maxcount logic.
>   	 */
> -	ref = new_ref_array_item(refname, oid);
> +	ref = ref_array_push(ref_cbdata->array, refname, oid);
>   	ref->commit = commit;
>   	ref->flag = flag;
>   	ref->kind = kind;
>   
> -	REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
> -	ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
>   	return 0;
>   }
>   
> diff --git a/ref-filter.h b/ref-filter.h
> index 68268f9ebc..76cf87cb6c 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -135,4 +135,12 @@ void setup_ref_filter_porcelain_msg(void);
>   void pretty_print_ref(const char *name, const struct object_id *oid,
>   		      const struct ref_format *format);
>   
> +/*
> + * Push a single ref onto the array; this can be used to construct your own
> + * ref_array without using filter_refs().
> + */
> +struct ref_array_item *ref_array_push(struct ref_array *array,
> +				      const char *refname,
> +				      const struct object_id *oid);
> +
>   #endif /*  REF_FILTER_H  */
The three patches in this series look good to me.
Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Harald Nordgren· Apr 7, 2018, 15:22 UTC · re: Derrick Stolee · lore

Re: [PATCH 3/3] ref-filter: factor ref_array pushing into its own function

Looks good to me.
Reviewed-by: Harald Nordgren <haraldnordgren@gmail.com>
On Fri, Apr 6, 2018 at 9:27 PM, Derrick Stolee <stolee@gmail.com> wrote:
Show 78 quoted lines
> On 4/6/2018 2:59 PM, Jeff King wrote:
>>
>> In preparation for callers constructing their own ref_array
>> structs, let's move our own internal push operation into its
>> own function.
>>
>> While we're at it, we can replace REALLOC_ARRAY() with
>> ALLOC_GROW(), which should give the growth operation
>> amortized linear complexity (as opposed to growing by one,
>> which is potentially quadratic, though in-place realloc
>> growth often makes this faster in practice).
>>
>> Signed-off-by: Jeff King <peff@peff.net>
>> ---
>>   ref-filter.c | 16 +++++++++++++---
>>   ref-filter.h |  8 ++++++++
>>   2 files changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index c1c3cc9480..6e9328b274 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1840,6 +1840,18 @@ static struct ref_array_item
>> *new_ref_array_item(const char *refname,
>>         return ref;
>>   }
>>   +struct ref_array_item *ref_array_push(struct ref_array *array,
>> +                                     const char *refname,
>> +                                     const struct object_id *oid)
>> +{
>> +       struct ref_array_item *ref = new_ref_array_item(refname, oid);
>> +
>> +       ALLOC_GROW(array->items, array->nr + 1, array->alloc);
>> +       array->items[array->nr++] = ref;
>> +
>> +       return ref;
>> +}
>> +
>>   static int ref_kind_from_refname(const char *refname)
>>   {
>>         unsigned int i;
>> @@ -1930,13 +1942,11 @@ static int ref_filter_handler(const char *refname,
>> const struct object_id *oid,
>>          * to do its job and the resulting list may yet to be pruned
>>          * by maxcount logic.
>>          */
>> -       ref = new_ref_array_item(refname, oid);
>> +       ref = ref_array_push(ref_cbdata->array, refname, oid);
>>         ref->commit = commit;
>>         ref->flag = flag;
>>         ref->kind = kind;
>>   -     REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr +
>> 1);
>> -       ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
>>         return 0;
>>   }
>>   diff --git a/ref-filter.h b/ref-filter.h
>> index 68268f9ebc..76cf87cb6c 100644
>> --- a/ref-filter.h
>> +++ b/ref-filter.h
>> @@ -135,4 +135,12 @@ void setup_ref_filter_porcelain_msg(void);
>>   void pretty_print_ref(const char *name, const struct object_id *oid,
>>                       const struct ref_format *format);
>>   +/*
>> + * Push a single ref onto the array; this can be used to construct your
>> own
>> + * ref_array without using filter_refs().
>> + */
>> +struct ref_array_item *ref_array_push(struct ref_array *array,
>> +                                     const char *refname,
>> +                                     const struct object_id *oid);
>> +
>>   #endif /*  REF_FILTER_H  */
>
>
> The three patches in this series look good to me.
>
> Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Junio C Hamano· Apr 8, 2018, 23:18 UTC · re: Jeff King · lore

Re: [PATCH 3/3] ref-filter: factor ref_array pushing into its own function

Jeff King <peff@peff.net> writes:
Show 9 quoted lines
> In preparation for callers constructing their own ref_array
> structs, let's move our own internal push operation into its
> own function.
>
> While we're at it, we can replace REALLOC_ARRAY() with
> ALLOC_GROW(), which should give the growth operation
> amortized linear complexity (as opposed to growing by one,
> which is potentially quadratic, though in-place realloc
> growth often makes this faster in practice).

Sorry, but I do not quite get the last part this paragraph. Up to but not including ", though in-place..." I would understand as:

     - With ALLOC_GROW(), we are explicit in that we are amortizing
       the allocation cost by growing in exponential batches.
     - With REALLOC_ARRAY(), we are telling the underlying
       realloc(3) that it is OK to pretend that we grow by one, but
       if that permission is literally followed, it could end up
       quadratic.
    So if you have really a bad realloc(3) implementation, switching
    to use ALLOC_GROW() is an improvement.

But after that I am not sure what you are getting at. Do you mean "In practice, a well-done realloc(3) does the amortizing internally anyway, which makes it faster than the grow-by-one quadratic, so this change may not make that much of a difference"? Or do you mean "In practice, a well-done realloc(3) internally amortizes better than we do manually, so this change may hurt performance"?

In either case, I think this splitting into a helper is obviously a good move, and using ALLOC_GROW() is conceptually cleaner, as we use it everywhere and tend to avoid realloc(3) just to add one item.

Other than that small confusion (not even a nit), three patches were pleasant read. Thanks.

Jeff King· Apr 9, 2018, 03:57 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] ref-filter: factor ref_array pushing into its own function

On Mon, Apr 09, 2018 at 08:18:35AM +0900, Junio C Hamano wrote:
Show 25 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > In preparation for callers constructing their own ref_array
> > structs, let's move our own internal push operation into its
> > own function.
> >
> > While we're at it, we can replace REALLOC_ARRAY() with
> > ALLOC_GROW(), which should give the growth operation
> > amortized linear complexity (as opposed to growing by one,
> > which is potentially quadratic, though in-place realloc
> > growth often makes this faster in practice).
> 
> Sorry, but I do not quite get the last part this paragraph.  Up to
> but not including ", though in-place..." I would understand as:
> 
>      - With ALLOC_GROW(), we are explicit in that we are amortizing
>        the allocation cost by growing in exponential batches.
> 
>      - With REALLOC_ARRAY(), we are telling the underlying
>        realloc(3) that it is OK to pretend that we grow by one, but
>        if that permission is literally followed, it could end up
>        quadratic.
> 
>     So if you have really a bad realloc(3) implementation, switching
>     to use ALLOC_GROW() is an improvement.

Yes, that's the gist of what I was saying. Though it is not even necessarily "a bad realloc". At some point you may hit heap segmentation and need to copy (though I guess if that happens repeatedly then maybe your realloc really _is_ bad ;) ).

Show 6 quoted lines
> But after that I am not sure what you are getting at.  Do you mean
> "In practice, a well-done realloc(3) does the amortizing internally
> anyway, which makes it faster than the grow-by-one quadratic, so
> this change may not make that much of a difference"?  Or do you mean
> "In practice, a well-done realloc(3) internally amortizes better
> than we do manually, so this change may hurt performance"?

The first. I couldn't measure any speedup on glibc, which makes me think it's mostly doing in-place expansion.

Show 6 quoted lines
> In either case, I think this splitting into a helper is obviously a
> good move, and using ALLOC_GROW() is conceptually cleaner, as we use
> it everywhere and tend to avoid realloc(3) just to add one item.
> 
> Other than that small confusion (not even a nit), three patches were
> pleasant read.  Thanks.

Thanks. Please feel free to expand or clarify the commit message if that helps.

-Peff
Harald Nordgren· Apr 4, 2018, 23:32 UTC · re: Harald Nordgren · lore

[PATCH v9] ls-remote: create '--sort' option

Create a '--sort' option for ls-remote, based on the one from for-each-ref. This e.g. allows ref names to be sorted by version semantics, so that v1.2 is sorted before v1.10.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
Notes:
    Create 'ref_array_push' function in ref-filter
 Documentation/git-ls-remote.txt | 15 ++++++++++++++-
 builtin/ls-remote.c             | 20 ++++++++++++++++++--
 ref-filter.c                    | 12 ++++++++++++
 ref-filter.h                    |  2 ++
 t/t5512-ls-remote.sh            | 41 ++++++++++++++++++++++++++++++++++++++++-
 5 files changed, 86 insertions(+), 4 deletions(-)
Show changes to 5 files +86 −4

Documentation/git-ls-remote.txt, builtin/ls-remote.c, ref-filter.c, ref-filter.h, t/t5512-ls-remote.sh

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 5f2628c8f..fa4505fd7 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git ls-remote' [--heads] [--tags] [--refs] [--upload-pack=<exec>]
-	      [-q | --quiet] [--exit-code] [--get-url]
+	      [-q | --quiet] [--exit-code] [--get-url] [--sort=<key>]
 	      [--symref] [<repository> [<refs>...]]
 
 DESCRIPTION
@@ -60,6 +60,19 @@ OPTIONS
 	upload-pack only shows the symref HEAD, so it will be the only
 	one shown by ls-remote.
 
+--sort=<key>::
+	Sort based on the key given.  Prefix `-` to sort in
+	descending order of the value. You may use the --sort=<key> option
+	multiple times, in which case the last key becomes the primary
+	key. Also supports "version:refname" or "v:refname" (tag
+	names are treated as versions). The "version:refname" sort
+	order can also be affected by the "versionsort.suffix"
+	configuration variable.
+	The keys supported are the same as those in `git for-each-ref`,
+	except that because `ls-remote` deals only with remotes, keys like
+	`committerdate` that require access to the objects themselves will
+	not work.
+
 <repository>::
 	The "remote" repository to query.  This parameter can be
 	either a URL or the name of a remote (see the GIT URLS and
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 540d56429..f87b2657c 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -1,6 +1,7 @@
 #include "builtin.h"
 #include "cache.h"
 #include "transport.h"
+#include "ref-filter.h"
 #include "remote.h"
 
 static const char * const ls_remote_usage[] = {
@@ -43,10 +44,13 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	int show_symref_target = 0;
 	const char *uploadpack = NULL;
 	const char **pattern = NULL;
+	int i;
 
 	struct remote *remote;
 	struct transport *transport;
 	const struct ref *ref;
+	struct ref_array array;
+	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
 
 	struct option options[] = {
 		OPT__QUIET(&quiet, N_("do not print remote URL")),
@@ -60,6 +64,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL),
 		OPT_BOOL(0, "get-url", &get_url,
 			 N_("take url.<base>.insteadOf into account")),
+		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
+			     N_("field name to sort on"), &parse_opt_ref_sorting),
 		OPT_SET_INT_F(0, "exit-code", &status,
 			      N_("exit with exit code 2 if no matching refs are found"),
 			      2, PARSE_OPT_NOCOMPLETE),
@@ -68,6 +74,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	memset(&array, 0, sizeof(array));
+
 	argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 	dest = argv[0];
@@ -108,9 +116,17 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 			continue;
 		if (!tail_match(pattern, ref->name))
 			continue;
+		ref_array_push(&array, ref);
+	}
+
+	if (sorting)
+		ref_array_sort(sorting, &array);
+
+	for (i = 0; i < array.nr; i++) {
+		const struct ref_array_item *ref = array.items[i];
 		if (show_symref_target && ref->symref)
-			printf("ref: %s\t%s\n", ref->symref, ref->name);
-		printf("%s\t%s\n", oid_to_hex(&ref->old_oid), ref->name);
+			printf("ref: %s\t%s\n", ref->symref, ref->refname);
+		printf("%s\t%s\n", oid_to_hex(&ref->objectname), ref->refname);
 		status = 0; /* we found something */
 	}
 	return status;
diff --git a/ref-filter.c b/ref-filter.c
index 45fc56216..a5686dacd 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1861,6 +1861,18 @@ static int ref_kind_from_refname(const char *refname)
 	return FILTER_REFS_OTHERS;
 }
 
+void ref_array_push(struct ref_array *array, const struct ref *ref)
+{
+	struct ref_array_item *item;
+
+	FLEX_ALLOC_STR(item, refname, ref->name);
+	item->symref = xstrdup_or_null(ref->symref);
+	oidcpy(&item->objectname, &ref->old_oid);
+
+	ALLOC_GROW(array->items, array->nr + 1, array->alloc);
+	array->items[array->nr++] = item;
+}
+
 static int filter_ref_kind(struct ref_filter *filter, const char *refname)
 {
 	if (filter->kind == FILTER_REFS_BRANCHES ||
diff --git a/ref-filter.h b/ref-filter.h
index 0d98342b3..43e713a72 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -127,6 +127,8 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)
 char *get_head_description(void);
 /*  Set up translated strings in the output. */
 void setup_ref_filter_porcelain_msg(void);
+/*  Create a ref_array_item from a ref and push it to the ref_array */
+void ref_array_push(struct ref_array *array, const struct ref *ref);
 
 /*
  * Print a single ref, outside of any ref-filter. Note that the
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 02106c922..66370cd88 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -10,6 +10,9 @@ test_expect_success setup '
 	test_tick &&
 	git commit -m initial &&
 	git tag mark &&
+	git tag mark1.1 &&
+	git tag mark1.2 &&
+	git tag mark1.10 &&
 	git show-ref --tags -d | sed -e "s/ /	/" >expected.tag &&
 	(
 		echo "$(git rev-parse HEAD)	HEAD"
@@ -39,6 +42,39 @@ test_expect_success 'ls-remote self' '
 	test_cmp expected.all actual
 '
 
+test_expect_success 'ls-remote --sort="version:refname" --tags self' '
+	cat >expect <<-\EOF &&
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	EOF
+	git ls-remote --sort="version:refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-remote --sort="-version:refname" --tags self' '
+	cat >expect <<-\EOF &&
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	EOF
+	git ls-remote --sort="-version:refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-remote --sort="-refname" --tags self' '
+	cat >expect <<-\EOF &&
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	EOF
+	git ls-remote --sort="-refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'dies when no remote specified and no default remotes found' '
 	test_must_fail git ls-remote
 '
@@ -131,7 +167,7 @@ test_expect_success 'Report no-match with --exit-code' '
 
 test_expect_success 'Report match with --exit-code' '
 	git ls-remote --exit-code other.git "refs/tags/*" >actual &&
-	git ls-remote . tags/mark >expect &&
+	git ls-remote . tags/mark* >expect &&
 	test_cmp expect actual
 '
 
@@ -178,6 +214,9 @@ test_expect_success 'ls-remote --symref' '
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/HEAD
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/master
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
 	EOF
 	git ls-remote --symref >actual &&
 	test_cmp expect actual
-- 
2.14.3 (Apple Git-98)
Harald Nordgren· Apr 5, 2018, 00:04 UTC · re: Harald Nordgren · lore

[PATCH v10] ls-remote: create '--sort' option

Create a '--sort' option for ls-remote, based on the one from for-each-ref. This e.g. allows ref names to be sorted by version semantics, so that v1.2 is sorted before v1.10.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
Notes:
    Use 'ref_array_item_push' to fix 'REALLOC_ARRAY' call in ref-filter
 Documentation/git-ls-remote.txt | 15 ++++++++++++++-
 builtin/ls-remote.c             | 20 ++++++++++++++++++--
 ref-filter.c                    | 20 ++++++++++++++++++--
 ref-filter.h                    |  2 ++
 t/t5512-ls-remote.sh            | 41 ++++++++++++++++++++++++++++++++++++++++-
 5 files changed, 92 insertions(+), 6 deletions(-)
Show changes to 5 files +92 −6

Documentation/git-ls-remote.txt, builtin/ls-remote.c, ref-filter.c, ref-filter.h, t/t5512-ls-remote.sh

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 5f2628c8f..fa4505fd7 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git ls-remote' [--heads] [--tags] [--refs] [--upload-pack=<exec>]
-	      [-q | --quiet] [--exit-code] [--get-url]
+	      [-q | --quiet] [--exit-code] [--get-url] [--sort=<key>]
 	      [--symref] [<repository> [<refs>...]]
 
 DESCRIPTION
@@ -60,6 +60,19 @@ OPTIONS
 	upload-pack only shows the symref HEAD, so it will be the only
 	one shown by ls-remote.
 
+--sort=<key>::
+	Sort based on the key given.  Prefix `-` to sort in
+	descending order of the value. You may use the --sort=<key> option
+	multiple times, in which case the last key becomes the primary
+	key. Also supports "version:refname" or "v:refname" (tag
+	names are treated as versions). The "version:refname" sort
+	order can also be affected by the "versionsort.suffix"
+	configuration variable.
+	The keys supported are the same as those in `git for-each-ref`,
+	except that because `ls-remote` deals only with remotes, keys like
+	`committerdate` that require access to the objects themselves will
+	not work.
+
 <repository>::
 	The "remote" repository to query.  This parameter can be
 	either a URL or the name of a remote (see the GIT URLS and
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 540d56429..f87b2657c 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -1,6 +1,7 @@
 #include "builtin.h"
 #include "cache.h"
 #include "transport.h"
+#include "ref-filter.h"
 #include "remote.h"
 
 static const char * const ls_remote_usage[] = {
@@ -43,10 +44,13 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	int show_symref_target = 0;
 	const char *uploadpack = NULL;
 	const char **pattern = NULL;
+	int i;
 
 	struct remote *remote;
 	struct transport *transport;
 	const struct ref *ref;
+	struct ref_array array;
+	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
 
 	struct option options[] = {
 		OPT__QUIET(&quiet, N_("do not print remote URL")),
@@ -60,6 +64,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL),
 		OPT_BOOL(0, "get-url", &get_url,
 			 N_("take url.<base>.insteadOf into account")),
+		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
+			     N_("field name to sort on"), &parse_opt_ref_sorting),
 		OPT_SET_INT_F(0, "exit-code", &status,
 			      N_("exit with exit code 2 if no matching refs are found"),
 			      2, PARSE_OPT_NOCOMPLETE),
@@ -68,6 +74,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	memset(&array, 0, sizeof(array));
+
 	argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 	dest = argv[0];
@@ -108,9 +116,17 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 			continue;
 		if (!tail_match(pattern, ref->name))
 			continue;
+		ref_array_push(&array, ref);
+	}
+
+	if (sorting)
+		ref_array_sort(sorting, &array);
+
+	for (i = 0; i < array.nr; i++) {
+		const struct ref_array_item *ref = array.items[i];
 		if (show_symref_target && ref->symref)
-			printf("ref: %s\t%s\n", ref->symref, ref->name);
-		printf("%s\t%s\n", oid_to_hex(&ref->old_oid), ref->name);
+			printf("ref: %s\t%s\n", ref->symref, ref->refname);
+		printf("%s\t%s\n", oid_to_hex(&ref->objectname), ref->refname);
 		status = 0; /* we found something */
 	}
 	return status;
diff --git a/ref-filter.c b/ref-filter.c
index 45fc56216..6dbafba07 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1861,6 +1861,23 @@ static int ref_kind_from_refname(const char *refname)
 	return FILTER_REFS_OTHERS;
 }
 
+void ref_array_item_push(struct ref_array *array, struct ref_array_item *item)
+{
+	ALLOC_GROW(array->items, array->nr + 1, array->alloc);
+	array->items[array->nr++] = item;
+}
+
+void ref_array_push(struct ref_array *array, const struct ref *ref)
+{
+	struct ref_array_item *item;
+
+	FLEX_ALLOC_STR(item, refname, ref->name);
+	item->symref = xstrdup_or_null(ref->symref);
+	oidcpy(&item->objectname, &ref->old_oid);
+
+	ref_array_item_push(array, item);
+}
+
 static int filter_ref_kind(struct ref_filter *filter, const char *refname)
 {
 	if (filter->kind == FILTER_REFS_BRANCHES ||
@@ -1930,8 +1947,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	ref = new_ref_array_item(refname, oid->hash, flag);
 	ref->commit = commit;
 
-	REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
-	ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
+	ref_array_item_push(ref_cbdata->array, ref);
 	ref->kind = kind;
 	return 0;
 }
diff --git a/ref-filter.h b/ref-filter.h
index 0d98342b3..43e713a72 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -127,6 +127,8 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)
 char *get_head_description(void);
 /*  Set up translated strings in the output. */
 void setup_ref_filter_porcelain_msg(void);
+/*  Create a ref_array_item from a ref and push it to the ref_array */
+void ref_array_push(struct ref_array *array, const struct ref *ref);
 
 /*
  * Print a single ref, outside of any ref-filter. Note that the
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 02106c922..66370cd88 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -10,6 +10,9 @@ test_expect_success setup '
 	test_tick &&
 	git commit -m initial &&
 	git tag mark &&
+	git tag mark1.1 &&
+	git tag mark1.2 &&
+	git tag mark1.10 &&
 	git show-ref --tags -d | sed -e "s/ /	/" >expected.tag &&
 	(
 		echo "$(git rev-parse HEAD)	HEAD"
@@ -39,6 +42,39 @@ test_expect_success 'ls-remote self' '
 	test_cmp expected.all actual
 '
 
+test_expect_success 'ls-remote --sort="version:refname" --tags self' '
+	cat >expect <<-\EOF &&
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	EOF
+	git ls-remote --sort="version:refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-remote --sort="-version:refname" --tags self' '
+	cat >expect <<-\EOF &&
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	EOF
+	git ls-remote --sort="-version:refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-remote --sort="-refname" --tags self' '
+	cat >expect <<-\EOF &&
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	EOF
+	git ls-remote --sort="-refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'dies when no remote specified and no default remotes found' '
 	test_must_fail git ls-remote
 '
@@ -131,7 +167,7 @@ test_expect_success 'Report no-match with --exit-code' '
 
 test_expect_success 'Report match with --exit-code' '
 	git ls-remote --exit-code other.git "refs/tags/*" >actual &&
-	git ls-remote . tags/mark >expect &&
+	git ls-remote . tags/mark* >expect &&
 	test_cmp expect actual
 '
 
@@ -178,6 +214,9 @@ test_expect_success 'ls-remote --symref' '
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/HEAD
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/master
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
 	EOF
 	git ls-remote --symref >actual &&
 	test_cmp expect actual
-- 
2.14.3 (Apple Git-98)
Harald Nordgren· Apr 7, 2018, 16:42 UTC · re: Harald Nordgren · lore

[PATCH v11 1/4] ref-filter: use "struct object_id" consistently

From: Jeff King <peff@peff.net>
Internally we store a "struct object_id", and all of our
callers have one to pass us. But we insist that they peel it
to its bare-sha1 hash, which we then hashcpy() into place.
Let's pass it around as an object_id, which future-proofs us
for a post-sha1 world.
---
 builtin/tag.c        |  2 +-
 builtin/verify-tag.c |  2 +-
 ref-filter.c         | 10 +++++-----
 ref-filter.h         |  2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)
Show changes to 4 files +8 −8

builtin/tag.c, builtin/verify-tag.c, ref-filter.c, ref-filter.h

diff --git a/builtin/tag.c b/builtin/tag.c
index da186691e..42278f516 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -117,7 +117,7 @@ static int verify_tag(const char *name, const char *ref,
 		return -1;
 
 	if (format->format)
-		pretty_print_ref(name, oid->hash, format);
+		pretty_print_ref(name, oid, format);
 
 	return 0;
 }
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index ad7b79fa5..6fa04b751 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -72,7 +72,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 		}
 
 		if (format.format)
-			pretty_print_ref(name, oid.hash, &format);
+			pretty_print_ref(name, &oid, &format);
 	}
 	return had_error;
 }
diff --git a/ref-filter.c b/ref-filter.c
index 45fc56216..ade97a848 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1826,12 +1826,12 @@ static const struct object_id *match_points_at(struct oid_array *points_at,
 
 /* Allocate space for a new ref_array_item and copy the objectname and flag to it */
 static struct ref_array_item *new_ref_array_item(const char *refname,
-						 const unsigned char *objectname,
+						 const struct object_id *oid,
 						 int flag)
 {
 	struct ref_array_item *ref;
 	FLEX_ALLOC_STR(ref, refname, refname);
-	hashcpy(ref->objectname.hash, objectname);
+	oidcpy(&ref->objectname, oid);
 	ref->flag = flag;
 
 	return ref;
@@ -1927,7 +1927,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	 * to do its job and the resulting list may yet to be pruned
 	 * by maxcount logic.
 	 */
-	ref = new_ref_array_item(refname, oid->hash, flag);
+	ref = new_ref_array_item(refname, oid, flag);
 	ref->commit = commit;
 
 	REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
@@ -2165,11 +2165,11 @@ void show_ref_array_item(struct ref_array_item *info,
 	putchar('\n');
 }
 
-void pretty_print_ref(const char *name, const unsigned char *sha1,
+void pretty_print_ref(const char *name, const struct object_id *oid,
 		      const struct ref_format *format)
 {
 	struct ref_array_item *ref_item;
-	ref_item = new_ref_array_item(name, sha1, 0);
+	ref_item = new_ref_array_item(name, oid, 0);
 	ref_item->kind = ref_kind_from_refname(name);
 	show_ref_array_item(ref_item, format);
 	free_array_item(ref_item);
diff --git a/ref-filter.h b/ref-filter.h
index 0d98342b3..68268f9eb 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -132,7 +132,7 @@ void setup_ref_filter_porcelain_msg(void);
  * Print a single ref, outside of any ref-filter. Note that the
  * name must be a fully qualified refname.
  */
-void pretty_print_ref(const char *name, const unsigned char *sha1,
+void pretty_print_ref(const char *name, const struct object_id *oid,
 		      const struct ref_format *format);
 
 #endif /*  REF_FILTER_H  */
-- 
2.14.3 (Apple Git-98)
Harald Nordgren· Apr 7, 2018, 16:42 UTC · re: Harald Nordgren · lore

[PATCH v11 2/4] ref-filter: make ref_array_item allocation more consistent

From: Jeff King <peff@peff.net>

We have a helper function to allocate ref_array_item structs, but it only takes a subset of the possible fields in the struct as initializers. We could have it accept an argument for _every_ field, but that becomes a pain for the fields which some callers don't want to set initially.

Instead, let's be explicit that it takes only the minimum
required to create the ref, and that callers should then
fill in the rest themselves.
---
 ref-filter.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)
Show changes to ref-filter.c +11 −7
diff --git a/ref-filter.c b/ref-filter.c
index ade97a848..c1c3cc948 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1824,15 +1824,18 @@ static const struct object_id *match_points_at(struct oid_array *points_at,
 	return NULL;
 }
 
-/* Allocate space for a new ref_array_item and copy the objectname and flag to it */
+/*
+ * Allocate space for a new ref_array_item and copy the name and oid to it.
+ *
+ * Callers can then fill in other struct members at their leisure.
+ */
 static struct ref_array_item *new_ref_array_item(const char *refname,
-						 const struct object_id *oid,
-						 int flag)
+						 const struct object_id *oid)
 {
 	struct ref_array_item *ref;
+
 	FLEX_ALLOC_STR(ref, refname, refname);
 	oidcpy(&ref->objectname, oid);
-	ref->flag = flag;
 
 	return ref;
 }
@@ -1927,12 +1930,13 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	 * to do its job and the resulting list may yet to be pruned
 	 * by maxcount logic.
 	 */
-	ref = new_ref_array_item(refname, oid, flag);
+	ref = new_ref_array_item(refname, oid);
 	ref->commit = commit;
+	ref->flag = flag;
+	ref->kind = kind;
 
 	REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
 	ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
-	ref->kind = kind;
 	return 0;
 }
 
@@ -2169,7 +2173,7 @@ void pretty_print_ref(const char *name, const struct object_id *oid,
 		      const struct ref_format *format)
 {
 	struct ref_array_item *ref_item;
-	ref_item = new_ref_array_item(name, oid, 0);
+	ref_item = new_ref_array_item(name, oid);
 	ref_item->kind = ref_kind_from_refname(name);
 	show_ref_array_item(ref_item, format);
 	free_array_item(ref_item);
-- 
2.14.3 (Apple Git-98)
Harald Nordgren· Apr 7, 2018, 16:42 UTC · re: Harald Nordgren · lore

[PATCH v11 3/4] ref-filter: factor ref_array pushing into its own function

From: Jeff King <peff@peff.net>

In preparation for callers constructing their own ref_array structs, let's move our own internal push operation into its own function.

While we're at it, we can replace REALLOC_ARRAY() with
ALLOC_GROW(), which should give the growth operation
amortized linear complexity (as opposed to growing by one,
which is potentially quadratic, though in-place realloc
growth often makes this faster in practice).
---
 ref-filter.c | 16 +++++++++++++---
 ref-filter.h |  8 ++++++++
 2 files changed, 21 insertions(+), 3 deletions(-)
Show changes to 2 files +21 −3

ref-filter.c, ref-filter.h

diff --git a/ref-filter.c b/ref-filter.c
index c1c3cc948..6e9328b27 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1840,6 +1840,18 @@ static struct ref_array_item *new_ref_array_item(const char *refname,
 	return ref;
 }
 
+struct ref_array_item *ref_array_push(struct ref_array *array,
+				      const char *refname,
+				      const struct object_id *oid)
+{
+	struct ref_array_item *ref = new_ref_array_item(refname, oid);
+
+	ALLOC_GROW(array->items, array->nr + 1, array->alloc);
+	array->items[array->nr++] = ref;
+
+	return ref;
+}
+
 static int ref_kind_from_refname(const char *refname)
 {
 	unsigned int i;
@@ -1930,13 +1942,11 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	 * to do its job and the resulting list may yet to be pruned
 	 * by maxcount logic.
 	 */
-	ref = new_ref_array_item(refname, oid);
+	ref = ref_array_push(ref_cbdata->array, refname, oid);
 	ref->commit = commit;
 	ref->flag = flag;
 	ref->kind = kind;
 
-	REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
-	ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
 	return 0;
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 68268f9eb..76cf87cb6 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -135,4 +135,12 @@ void setup_ref_filter_porcelain_msg(void);
 void pretty_print_ref(const char *name, const struct object_id *oid,
 		      const struct ref_format *format);
 
+/*
+ * Push a single ref onto the array; this can be used to construct your own
+ * ref_array without using filter_refs().
+ */
+struct ref_array_item *ref_array_push(struct ref_array *array,
+				      const char *refname,
+				      const struct object_id *oid);
+
 #endif /*  REF_FILTER_H  */
-- 
2.14.3 (Apple Git-98)
Harald Nordgren· Apr 7, 2018, 16:42 UTC · re: Harald Nordgren · lore

[PATCH v11 4/4] ls-remote: create '--sort' option

Create a '--sort' option for ls-remote, based on the one from for-each-ref. This e.g. allows ref names to be sorted by version semantics, so that v1.2 is sorted before v1.10.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
Notes:
    Rebasing my patch on Jeff King's refatoring patches
 Documentation/git-ls-remote.txt | 15 ++++++++++++++-
 builtin/ls-remote.c             | 22 ++++++++++++++++++++--
 t/t5512-ls-remote.sh            | 41 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 74 insertions(+), 4 deletions(-)
Show changes to 3 files +74 −4

Documentation/git-ls-remote.txt, builtin/ls-remote.c, t/t5512-ls-remote.sh

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 5f2628c8f..fa4505fd7 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git ls-remote' [--heads] [--tags] [--refs] [--upload-pack=<exec>]
-	      [-q | --quiet] [--exit-code] [--get-url]
+	      [-q | --quiet] [--exit-code] [--get-url] [--sort=<key>]
 	      [--symref] [<repository> [<refs>...]]
 
 DESCRIPTION
@@ -60,6 +60,19 @@ OPTIONS
 	upload-pack only shows the symref HEAD, so it will be the only
 	one shown by ls-remote.
 
+--sort=<key>::
+	Sort based on the key given.  Prefix `-` to sort in
+	descending order of the value. You may use the --sort=<key> option
+	multiple times, in which case the last key becomes the primary
+	key. Also supports "version:refname" or "v:refname" (tag
+	names are treated as versions). The "version:refname" sort
+	order can also be affected by the "versionsort.suffix"
+	configuration variable.
+	The keys supported are the same as those in `git for-each-ref`,
+	except that because `ls-remote` deals only with remotes, keys like
+	`committerdate` that require access to the objects themselves will
+	not work.
+
 <repository>::
 	The "remote" repository to query.  This parameter can be
 	either a URL or the name of a remote (see the GIT URLS and
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 540d56429..4375e8322 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -1,6 +1,7 @@
 #include "builtin.h"
 #include "cache.h"
 #include "transport.h"
+#include "ref-filter.h"
 #include "remote.h"
 
 static const char * const ls_remote_usage[] = {
@@ -43,10 +44,13 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	int show_symref_target = 0;
 	const char *uploadpack = NULL;
 	const char **pattern = NULL;
+	int i;
 
 	struct remote *remote;
 	struct transport *transport;
 	const struct ref *ref;
+	struct ref_array array;
+	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
 
 	struct option options[] = {
 		OPT__QUIET(&quiet, N_("do not print remote URL")),
@@ -60,6 +64,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL),
 		OPT_BOOL(0, "get-url", &get_url,
 			 N_("take url.<base>.insteadOf into account")),
+		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
+			     N_("field name to sort on"), &parse_opt_ref_sorting),
 		OPT_SET_INT_F(0, "exit-code", &status,
 			      N_("exit with exit code 2 if no matching refs are found"),
 			      2, PARSE_OPT_NOCOMPLETE),
@@ -68,6 +74,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	memset(&array, 0, sizeof(array));
+
 	argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 	dest = argv[0];
@@ -104,13 +112,23 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	if (!dest && !quiet)
 		fprintf(stderr, "From %s\n", *remote->url);
 	for ( ; ref; ref = ref->next) {
+		struct ref_array_item *item;
 		if (!check_ref_type(ref, flags))
 			continue;
 		if (!tail_match(pattern, ref->name))
 			continue;
+		item = ref_array_push(&array, ref->name, &ref->old_oid);
+		item->symref = xstrdup_or_null(ref->symref);
+	}
+
+	if (sorting)
+		ref_array_sort(sorting, &array);
+
+	for (i = 0; i < array.nr; i++) {
+		const struct ref_array_item *ref = array.items[i];
 		if (show_symref_target && ref->symref)
-			printf("ref: %s\t%s\n", ref->symref, ref->name);
-		printf("%s\t%s\n", oid_to_hex(&ref->old_oid), ref->name);
+			printf("ref: %s\t%s\n", ref->symref, ref->refname);
+		printf("%s\t%s\n", oid_to_hex(&ref->objectname), ref->refname);
 		status = 0; /* we found something */
 	}
 	return status;
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 02106c922..66370cd88 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -10,6 +10,9 @@ test_expect_success setup '
 	test_tick &&
 	git commit -m initial &&
 	git tag mark &&
+	git tag mark1.1 &&
+	git tag mark1.2 &&
+	git tag mark1.10 &&
 	git show-ref --tags -d | sed -e "s/ /	/" >expected.tag &&
 	(
 		echo "$(git rev-parse HEAD)	HEAD"
@@ -39,6 +42,39 @@ test_expect_success 'ls-remote self' '
 	test_cmp expected.all actual
 '
 
+test_expect_success 'ls-remote --sort="version:refname" --tags self' '
+	cat >expect <<-\EOF &&
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	EOF
+	git ls-remote --sort="version:refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-remote --sort="-version:refname" --tags self' '
+	cat >expect <<-\EOF &&
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	EOF
+	git ls-remote --sort="-version:refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-remote --sort="-refname" --tags self' '
+	cat >expect <<-\EOF &&
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	EOF
+	git ls-remote --sort="-refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'dies when no remote specified and no default remotes found' '
 	test_must_fail git ls-remote
 '
@@ -131,7 +167,7 @@ test_expect_success 'Report no-match with --exit-code' '
 
 test_expect_success 'Report match with --exit-code' '
 	git ls-remote --exit-code other.git "refs/tags/*" >actual &&
-	git ls-remote . tags/mark >expect &&
+	git ls-remote . tags/mark* >expect &&
 	test_cmp expect actual
 '
 
@@ -178,6 +214,9 @@ test_expect_success 'ls-remote --symref' '
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/HEAD
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/master
 	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.1
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.10
+	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark1.2
 	EOF
 	git ls-remote --symref >actual &&
 	test_cmp expect actual
-- 
2.14.3 (Apple Git-98)
Eric Sunshine· Apr 8, 2018, 01:48 UTC · re: Harald Nordgren · lore

Re: [PATCH v11 4/4] ls-remote: create '--sort' option

On Sat, Apr 7, 2018 at 12:42 PM, Harald Nordgren <haraldnordgren@gmail.com> wrote:

Show 12 quoted lines
> Create a '--sort' option for ls-remote, based on the one from
> for-each-ref. This e.g. allows ref names to be sorted by version
> semantics, so that v1.2 is sorted before v1.10.
>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> ---
> diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
> @@ -60,6 +60,19 @@ OPTIONS
> +--sort=<key>::
> +       Sort based on the key given.  Prefix `-` to sort in
> +       descending order of the value. You may use the --sort=<key> option
> +       multiple times, in which case the last key becomes the primary

This "last becomes primary key" feels counterintuitive to me, however, I see it mirrors precedence of other Git commands.

In what situations would it make sense to specify --sort= multiple times in the context of ls-remote? If there are none, then I wonder if this should instead be documented as "last wins"...

> +       key. Also supports "version:refname" or "v:refname" (tag
To what does "Also" refer?
Show 7 quoted lines
> +       names are treated as versions). The "version:refname" sort
> +       order can also be affected by the "versionsort.suffix"
> +       configuration variable.
> +       The keys supported are the same as those in `git for-each-ref`,
> +       except that because `ls-remote` deals only with remotes, keys like
> +       `committerdate` that require access to the objects themselves will
> +       not work.

What does "not work" mean in this context? Will the command crash outright? Will it emit a suitable error message or warning? Will the sorting be somehow dysfunctional?

It seems like the sentence "The keys supported..." should go above the "Also supports..." sentence for this explanation to be more cohesive.

Finally, how about adding a linkgit:git-for-each-ref[1] to give the user an easy way to access the referenced documentation. For instance:

    The keys supported are the same as those accepted by the
    `--sort=` option of linkgit:git-for-each-ref[1], except...
Show 6 quoted lines
> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
> @@ -43,10 +44,13 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
> +       struct ref_array array;
> +       static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
> @@ -68,6 +74,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
> +       memset(&array, 0, sizeof(array));

Can we have a more meaningful name than 'array'? Even a name a simple as 'refs' would convey more information.

Show 9 quoted lines
> @@ -104,13 +112,23 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>         for ( ; ref; ref = ref->next) {
> +               struct ref_array_item *item;
>                 if (!check_ref_type(ref, flags))
>                         continue;
>                 if (!tail_match(pattern, ref->name))
>                         continue;
> +               item = ref_array_push(&array, ref->name, &ref->old_oid);
> +               item->symref = xstrdup_or_null(ref->symref);
Do we need to worry about freeing memory allocated by these two lines?

More generally, do we care that 'array' and 'sorting' are being leaked? If not, perhaps they should be annotated by UNLEAK.

Show 13 quoted lines
> +       }
> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
> @@ -39,6 +42,39 @@ test_expect_success 'ls-remote self' '
> +test_expect_success 'ls-remote --sort="version:refname" --tags self' '
> +       cat >expect <<-\EOF &&
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.1
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.2
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.10
> +       EOF
> +       git ls-remote --sort="version:refname" --tags self >actual &&
> +       test_cmp expect actual
> +'

This test script is already filled with these hardcoded SHA-1's, so this is not a new problem, but work is under way to make it possible to replace SHA-1 with some other hashing algorithm. Test scripts are being retrofitted to avoid hard-coding them; instead determining the hash value dynamically (for example, [1]). It would be nice if the new tests followed suit, thus saving someone else extra work down the road. (This, itself, is not worth a re-roll, but something to consider if you do re-roll.)

[1]: https://public-inbox.org/git/20180325192055.841459-10-sandals@crustytoothpaste.net/
Show 21 quoted lines
> +test_expect_success 'ls-remote --sort="-version:refname" --tags self' '
> +       cat >expect <<-\EOF &&
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.10
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.2
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.1
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark
> +       EOF
> +       git ls-remote --sort="-version:refname" --tags self >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'ls-remote --sort="-refname" --tags self' '
> +       cat >expect <<-\EOF &&
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.2
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.10
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark1.1
> +       1bd44cb9d13204b0fe1958db0082f5028a16eb3a        refs/tags/mark
> +       EOF
> +       git ls-remote --sort="-refname" --tags self >actual &&
> +       test_cmp expect actual
> +'

Do we want a test verifying that multiple sort keys are respected? (Is it even possible to construct such a test?)

Show 7 quoted lines
> @@ -131,7 +167,7 @@ test_expect_success 'Report no-match with --exit-code' '
>  test_expect_success 'Report match with --exit-code' '
>         git ls-remote --exit-code other.git "refs/tags/*" >actual &&
> -       git ls-remote . tags/mark >expect &&
> +       git ls-remote . tags/mark* >expect &&
>         test_cmp expect actual
>  '
Why this change?
Eric Sunshine· Apr 8, 2018, 01:06 UTC · re: Harald Nordgren · lore

Re: [PATCH v11 1/4] ref-filter: use "struct object_id" consistently

On Sat, Apr 7, 2018 at 12:42 PM, Harald Nordgren <haraldnordgren@gmail.com> wrote:

Show 8 quoted lines
> From: Jeff King <peff@peff.net>
>
> Internally we store a "struct object_id", and all of our
> callers have one to pass us. But we insist that they peel it
> to its bare-sha1 hash, which we then hashcpy() into place.
> Let's pass it around as an object_id, which future-proofs us
> for a post-sha1 world.
> ---

You incorrectly dropped Peff's sign-off[1] when re-sending the patches he authored in the series. And, your sign-off should follow his.

Also, if you made any changes to Peff's patch, it's a good idea to state so with a bracketed comment at the end of the commit message (before the sign-offs). For instance:

    [hn: tweaked doodle blap]
or such.
[1]: https://public-inbox.org/git/20180406185831.GA11108@sigill.intra.peff.net/
Harald Nordgren· Apr 8, 2018, 12:27 UTC · re: Eric Sunshine · lore

Re: [PATCH v11 1/4] ref-filter: use "struct object_id" consistently

Thanks for your very thorough review Eric! I think I address all the points, but if I missed something please let me know.

On Sun, Apr 8, 2018 at 3:06 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> You incorrectly dropped Peff's sign-off[1] when re-sending the patches
> he authored in the series. And, your sign-off should follow his.

I just rebased on https://github.com/peff/git/tree/jk/ref-array-push where there were no sign-off tags.

But I amended the commit messages now to add the sign-off. And also added my own sign-off.

Show 9 quoted lines
> Also, if you made any changes to Peff's patch, it's a good idea to
> state so with a bracketed comment at the end of the commit message
> (before the sign-offs). For instance:
>
>     [hn: tweaked doodle blap]
>
> or such.
>
> [1]: https://public-inbox.org/git/20180406185831.GA11108@sigill.intra.peff.net/
As a sign of respect I probably wouldn't want to tweak the patches.
They currently work well together with my implementation, so there is no need.
Show 8 quoted lines
> This "last becomes primary key" feels counterintuitive to me, however,
> I see it mirrors precedence of other Git commands.
>
> In what situations would it make sense to specify --sort= multiple
> times in the context of ls-remote? If there are none, then I wonder if
> this should instead be documented as "last wins"...
>
> To what does "Also" refer?
This was copied wholesale from Documentation/git-tag.txt.

I minimized the text now and removed all the references to using 'sort' multiple times. Although you technically could use multiple keys, since 'ls-remote' only outputs two columns, I guess it's never needed.

> What does "not work" mean in this context? Will the command crash
> outright? Will it emit a suitable error message or warning? Will the
> sorting be somehow dysfunctional?
This will be the output when trying to sort by commiterdate:
    From git@github.com:git/git.git
    fatal: missing object f0d0fd3a5985d5e588da1e1d11c85fba0ae132f8 for
refs/pull/10/head

I added a a note in the documentation saying that it will "cause a failure". Should we be even more explicit than that?

This is one side-effect of borrowing the ref-filter logic in this patch. We inherit things that won't work on remotes.

Show 9 quoted lines
> It seems like the sentence "The keys supported..." should go above the
> "Also supports..." sentence for this explanation to be more cohesive.
>
> Finally, how about adding a linkgit:git-for-each-ref[1] to give the
> user an easy way to access the referenced documentation. For instance:
>
>     The keys supported are the same as those accepted by the
>     `--sort=` option of linkgit:git-for-each-ref[1], except...
>
Added a "linkgit" to git-for-each-ref. And a "See also" section at the bottom.
> Can we have a more meaningful name than 'array'? Even a name a simple
> as 'refs' would convey more information.

I think 'refs' would be confusing considering that the return value of 'transport_get_remote_refs' is stored as 'ref'. I renamed it to 'ref_array'. I hope it's not a problem that is shadows its struct name. But I've seen us this naming paradigm in other places -- and in this file.

> Do we need to worry about freeing memory allocated by these two lines?
>
> More generally, do we care that 'array' and 'sorting' are being
> leaked? If not, perhaps they should be annotated by UNLEAK.

Since 'cmd_ls_remote' is always (?) called from another process I don't think we need to worry explicitly freeing the memory. I added UNLEAK annotations to my code.

Show 10 quoted lines
> This test script is already filled with these hardcoded SHA-1's, so
> this is not a new problem, but work is under way to make it possible
> to replace SHA-1 with some other hashing algorithm. Test scripts are
> being retrofitted to avoid hard-coding them; instead determining the
> hash value dynamically (for example, [1]). It would be nice if the new
> tests followed suit, thus saving someone else extra work down the
> road. (This, itself, is not worth a re-roll, but something to consider
> if you do re-roll.)
>
> [1]: https://public-inbox.org/git/20180325192055.841459-10-sandals@crustytoothpaste.net/

Added 'git rev-parse' calls to my tests do decrease the reliance on SHA-1's. For the test

    ls-remote --symref

I couldn't figure out a way to extract the hash for 'refs/remotes/origin/HEAD' so I re-used HEAD. I tried different ways of fetching origin, making another commit and pushing, but origin/HEAD is still unavailable.

By calling 'git fetch origin' before the test, at least I am able to extract 'git rev-parse refs/remotes/origin/master'.

This all makes the tests a bit more complicated, so I hope it helps us in the future! :)

> Do we want a test verifying that multiple sort keys are respected? (Is
> it even possible to construct such a test?)

To add to my previous point: Since 'ls-remote' outputs only two columns, I don't see a use case for multiple keys.

And I don't know what the test would look like either.
Show 9 quoted lines
>> @@ -131,7 +167,7 @@ test_expect_success 'Report no-match with --exit-code' '
>>  test_expect_success 'Report match with --exit-code' '
>>         git ls-remote --exit-code other.git "refs/tags/*" >actual &&
>> -       git ls-remote . tags/mark >expect &&
>> +       git ls-remote . tags/mark* >expect &&
>>         test_cmp expect actual
>>  '
>
> Why this change?
I added the asterisk to 'mark*' because without it, the test
    Report match with --exit-code

will fail because the input 'git ls-remote refs/tags/*' now gives gives back more tags (mark, mark1.1, mark1.2, mark1.10). So I think the asterisk is the cleanest solution to not have to create e.g. two separate setup functions.

Harald Nordgren· Apr 8, 2018, 12:28 UTC · re: Harald Nordgren · lore

[PATCH v12 1/4] ref-filter: use "struct object_id" consistently

From: Jeff King <peff@peff.net>

Internally we store a "struct object_id", and all of our callers have one to pass us. But we insist that they peel it to its bare-sha1 hash, which we then hashcpy() into place. Let's pass it around as an object_id, which future-proofs us for a post-sha1 world.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
 builtin/tag.c        |  2 +-
 builtin/verify-tag.c |  2 +-
 ref-filter.c         | 10 +++++-----
 ref-filter.h         |  2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)
Show changes to 4 files +8 −8

builtin/tag.c, builtin/verify-tag.c, ref-filter.c, ref-filter.h

diff --git a/builtin/tag.c b/builtin/tag.c
index da186691e..42278f516 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -117,7 +117,7 @@ static int verify_tag(const char *name, const char *ref,
 		return -1;
 
 	if (format->format)
-		pretty_print_ref(name, oid->hash, format);
+		pretty_print_ref(name, oid, format);
 
 	return 0;
 }
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index ad7b79fa5..6fa04b751 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -72,7 +72,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 		}
 
 		if (format.format)
-			pretty_print_ref(name, oid.hash, &format);
+			pretty_print_ref(name, &oid, &format);
 	}
 	return had_error;
 }
diff --git a/ref-filter.c b/ref-filter.c
index 45fc56216..ade97a848 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1826,12 +1826,12 @@ static const struct object_id *match_points_at(struct oid_array *points_at,
 
 /* Allocate space for a new ref_array_item and copy the objectname and flag to it */
 static struct ref_array_item *new_ref_array_item(const char *refname,
-						 const unsigned char *objectname,
+						 const struct object_id *oid,
 						 int flag)
 {
 	struct ref_array_item *ref;
 	FLEX_ALLOC_STR(ref, refname, refname);
-	hashcpy(ref->objectname.hash, objectname);
+	oidcpy(&ref->objectname, oid);
 	ref->flag = flag;
 
 	return ref;
@@ -1927,7 +1927,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	 * to do its job and the resulting list may yet to be pruned
 	 * by maxcount logic.
 	 */
-	ref = new_ref_array_item(refname, oid->hash, flag);
+	ref = new_ref_array_item(refname, oid, flag);
 	ref->commit = commit;
 
 	REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
@@ -2165,11 +2165,11 @@ void show_ref_array_item(struct ref_array_item *info,
 	putchar('\n');
 }
 
-void pretty_print_ref(const char *name, const unsigned char *sha1,
+void pretty_print_ref(const char *name, const struct object_id *oid,
 		      const struct ref_format *format)
 {
 	struct ref_array_item *ref_item;
-	ref_item = new_ref_array_item(name, sha1, 0);
+	ref_item = new_ref_array_item(name, oid, 0);
 	ref_item->kind = ref_kind_from_refname(name);
 	show_ref_array_item(ref_item, format);
 	free_array_item(ref_item);
diff --git a/ref-filter.h b/ref-filter.h
index 0d98342b3..68268f9eb 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -132,7 +132,7 @@ void setup_ref_filter_porcelain_msg(void);
  * Print a single ref, outside of any ref-filter. Note that the
  * name must be a fully qualified refname.
  */
-void pretty_print_ref(const char *name, const unsigned char *sha1,
+void pretty_print_ref(const char *name, const struct object_id *oid,
 		      const struct ref_format *format);
 
 #endif /*  REF_FILTER_H  */
-- 
2.14.3 (Apple Git-98)
Harald Nordgren· Apr 8, 2018, 12:28 UTC · re: Harald Nordgren · lore

[PATCH v12 3/4] ref-filter: factor ref_array pushing into its own function

From: Jeff King <peff@peff.net>

In preparation for callers constructing their own ref_array structs, let's move our own internal push operation into its own function.

While we're at it, we can replace REALLOC_ARRAY() with ALLOC_GROW(), which should give the growth operation amortized linear complexity (as opposed to growing by one, which is potentially quadratic, though in-place realloc growth often makes this faster in practice).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
 ref-filter.c | 16 +++++++++++++---
 ref-filter.h |  8 ++++++++
 2 files changed, 21 insertions(+), 3 deletions(-)
Show changes to 2 files +21 −3

ref-filter.c, ref-filter.h

diff --git a/ref-filter.c b/ref-filter.c
index c1c3cc948..6e9328b27 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1840,6 +1840,18 @@ static struct ref_array_item *new_ref_array_item(const char *refname,
 	return ref;
 }
 
+struct ref_array_item *ref_array_push(struct ref_array *array,
+				      const char *refname,
+				      const struct object_id *oid)
+{
+	struct ref_array_item *ref = new_ref_array_item(refname, oid);
+
+	ALLOC_GROW(array->items, array->nr + 1, array->alloc);
+	array->items[array->nr++] = ref;
+
+	return ref;
+}
+
 static int ref_kind_from_refname(const char *refname)
 {
 	unsigned int i;
@@ -1930,13 +1942,11 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	 * to do its job and the resulting list may yet to be pruned
 	 * by maxcount logic.
 	 */
-	ref = new_ref_array_item(refname, oid);
+	ref = ref_array_push(ref_cbdata->array, refname, oid);
 	ref->commit = commit;
 	ref->flag = flag;
 	ref->kind = kind;
 
-	REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
-	ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
 	return 0;
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 68268f9eb..76cf87cb6 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -135,4 +135,12 @@ void setup_ref_filter_porcelain_msg(void);
 void pretty_print_ref(const char *name, const struct object_id *oid,
 		      const struct ref_format *format);
 
+/*
+ * Push a single ref onto the array; this can be used to construct your own
+ * ref_array without using filter_refs().
+ */
+struct ref_array_item *ref_array_push(struct ref_array *array,
+				      const char *refname,
+				      const struct object_id *oid);
+
 #endif /*  REF_FILTER_H  */
-- 
2.14.3 (Apple Git-98)
Harald Nordgren· Apr 8, 2018, 12:28 UTC · re: Harald Nordgren · lore

[PATCH v12 4/4] ls-remote: create '--sort' option

Create a '--sort' option for ls-remote, based on the one from for-each-ref. This e.g. allows ref names to be sorted by version semantics, so that v1.2 is sorted before v1.10.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
Notes:
    Changes according to Eric Sunshine's code review
 Documentation/git-ls-remote.txt | 16 ++++++++++++-
 builtin/ls-remote.c             | 30 +++++++++++++++++++++---
 t/t5512-ls-remote.sh            | 52 ++++++++++++++++++++++++++++++++++++-----
 3 files changed, 88 insertions(+), 10 deletions(-)
Show changes to 3 files +88 −10

Documentation/git-ls-remote.txt, builtin/ls-remote.c, t/t5512-ls-remote.sh

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 5f2628c8f..80a09b518 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git ls-remote' [--heads] [--tags] [--refs] [--upload-pack=<exec>]
-	      [-q | --quiet] [--exit-code] [--get-url]
+	      [-q | --quiet] [--exit-code] [--get-url] [--sort=<key>]
 	      [--symref] [<repository> [<refs>...]]
 
 DESCRIPTION
@@ -60,6 +60,16 @@ OPTIONS
 	upload-pack only shows the symref HEAD, so it will be the only
 	one shown by ls-remote.
 
+--sort=<key>::
+	Sort based on the key given. Prefix `-` to sort in descending order
+	of the value. Supports "version:refname" or "v:refname" (tag names
+	are treated as versions). The "version:refname" sort order can also
+	be affected by the "versionsort.suffix" configuration variable.
+	See linkgit:git-for-each-ref[1] for more sort options, but be aware
+	that because `ls-remote` deals only with remotes, any key like
+	`committerdate` that requires access to the object itself will
+	cause a failure.
+
 <repository>::
 	The "remote" repository to query.  This parameter can be
 	either a URL or the name of a remote (see the GIT URLS and
@@ -90,6 +100,10 @@ EXAMPLES
 	c5db5456ae3b0873fc659c19fafdde22313cc441	refs/tags/v0.99.2
 	7ceca275d047c90c0c7d5afb13ab97efdf51bd6e	refs/tags/v0.99.3
 
+SEE ALSO
+--------
+linkgit:git-check-ref-format[1].
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 540d56429..d3851074c 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -1,6 +1,7 @@
 #include "builtin.h"
 #include "cache.h"
 #include "transport.h"
+#include "ref-filter.h"
 #include "remote.h"
 
 static const char * const ls_remote_usage[] = {
@@ -43,10 +44,13 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	int show_symref_target = 0;
 	const char *uploadpack = NULL;
 	const char **pattern = NULL;
+	int i;
 
 	struct remote *remote;
 	struct transport *transport;
 	const struct ref *ref;
+	struct ref_array ref_array;
+	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
 
 	struct option options[] = {
 		OPT__QUIET(&quiet, N_("do not print remote URL")),
@@ -60,6 +64,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL),
 		OPT_BOOL(0, "get-url", &get_url,
 			 N_("take url.<base>.insteadOf into account")),
+		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
+			     N_("field name to sort on"), &parse_opt_ref_sorting),
 		OPT_SET_INT_F(0, "exit-code", &status,
 			      N_("exit with exit code 2 if no matching refs are found"),
 			      2, PARSE_OPT_NOCOMPLETE),
@@ -68,6 +74,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	memset(&ref_array, 0, sizeof(ref_array));
+
 	argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 	dest = argv[0];
@@ -90,6 +98,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 
 	if (get_url) {
 		printf("%s\n", *remote->url);
+		UNLEAK(sorting);
 		return 0;
 	}
 
@@ -98,20 +107,35 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		transport_set_option(transport, TRANS_OPT_UPLOADPACK, uploadpack);
 
 	ref = transport_get_remote_refs(transport);
-	if (transport_disconnect(transport))
+	if (transport_disconnect(transport)) {
+		UNLEAK(sorting);
 		return 1;
+	}
 
 	if (!dest && !quiet)
 		fprintf(stderr, "From %s\n", *remote->url);
 	for ( ; ref; ref = ref->next) {
+		struct ref_array_item *item;
 		if (!check_ref_type(ref, flags))
 			continue;
 		if (!tail_match(pattern, ref->name))
 			continue;
+		item = ref_array_push(&ref_array, ref->name, &ref->old_oid);
+		item->symref = xstrdup_or_null(ref->symref);
+	}
+
+	if (sorting)
+		ref_array_sort(sorting, &ref_array);
+
+	for (i = 0; i < ref_array.nr; i++) {
+		const struct ref_array_item *ref = ref_array.items[i];
 		if (show_symref_target && ref->symref)
-			printf("ref: %s\t%s\n", ref->symref, ref->name);
-		printf("%s\t%s\n", oid_to_hex(&ref->old_oid), ref->name);
+			printf("ref: %s\t%s\n", ref->symref, ref->refname);
+		printf("%s\t%s\n", oid_to_hex(&ref->objectname), ref->refname);
 		status = 0; /* we found something */
 	}
+
+	UNLEAK(sorting);
+	UNLEAK(ref_array);
 	return status;
 }
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 02106c922..e8df4c7e0 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -10,6 +10,9 @@ test_expect_success setup '
 	test_tick &&
 	git commit -m initial &&
 	git tag mark &&
+	git tag mark1.1 &&
+	git tag mark1.2 &&
+	git tag mark1.10 &&
 	git show-ref --tags -d | sed -e "s/ /	/" >expected.tag &&
 	(
 		echo "$(git rev-parse HEAD)	HEAD"
@@ -39,6 +42,39 @@ test_expect_success 'ls-remote self' '
 	test_cmp expected.all actual
 '
 
+test_expect_success 'ls-remote --sort="version:refname" --tags self' '
+	cat >expect <<-\EOF &&
+	'$(git rev-parse mark)'	refs/tags/mark
+	'$(git rev-parse mark1.1)'	refs/tags/mark1.1
+	'$(git rev-parse mark1.2)'	refs/tags/mark1.2
+	'$(git rev-parse mark1.10)'	refs/tags/mark1.10
+	EOF
+	git ls-remote --sort="version:refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-remote --sort="-version:refname" --tags self' '
+	cat >expect <<-\EOF &&
+	'$(git rev-parse mark1.10)'	refs/tags/mark1.10
+	'$(git rev-parse mark1.2)'	refs/tags/mark1.2
+	'$(git rev-parse mark1.1)'	refs/tags/mark1.1
+	'$(git rev-parse mark)'	refs/tags/mark
+	EOF
+	git ls-remote --sort="-version:refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-remote --sort="-refname" --tags self' '
+	cat >expect <<-\EOF &&
+	'$(git rev-parse mark1.2)'	refs/tags/mark1.2
+	'$(git rev-parse mark1.10)'	refs/tags/mark1.10
+	'$(git rev-parse mark1.1)'	refs/tags/mark1.1
+	'$(git rev-parse mark)'	refs/tags/mark
+	EOF
+	git ls-remote --sort="-refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'dies when no remote specified and no default remotes found' '
 	test_must_fail git ls-remote
 '
@@ -131,7 +167,7 @@ test_expect_success 'Report no-match with --exit-code' '
 
 test_expect_success 'Report match with --exit-code' '
 	git ls-remote --exit-code other.git "refs/tags/*" >actual &&
-	git ls-remote . tags/mark >expect &&
+	git ls-remote . tags/mark* >expect &&
 	test_cmp expect actual
 '
 
@@ -170,14 +206,18 @@ test_expect_success 'overrides work between mixed transfer/upload-pack hideRefs'
 	grep refs/tags/magic actual
 '
 
+git fetch origin
 test_expect_success 'ls-remote --symref' '
 	cat >expect <<-\EOF &&
 	ref: refs/heads/master	HEAD
-	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	HEAD
-	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/heads/master
-	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/HEAD
-	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/master
-	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	'$(git rev-parse HEAD)'	HEAD
+	'$(git rev-parse refs/heads/master)'	refs/heads/master
+	'$(git rev-parse HEAD)'	refs/remotes/origin/HEAD
+	'$(git rev-parse refs/remotes/origin/master)'	refs/remotes/origin/master
+	'$(git rev-parse refs/tags/mark)'	refs/tags/mark
+	'$(git rev-parse refs/tags/mark1.1)'	refs/tags/mark1.1
+	'$(git rev-parse refs/tags/mark1.10)'	refs/tags/mark1.10
+	'$(git rev-parse refs/tags/mark1.2)'	refs/tags/mark1.2
 	EOF
 	git ls-remote --symref >actual &&
 	test_cmp expect actual
-- 
2.14.3 (Apple Git-98)
Junio C Hamano· Apr 8, 2018, 22:16 UTC · re: Harald Nordgren · lore

Re: [PATCH v12 4/4] ls-remote: create '--sort' option

Harald Nordgren <haraldnordgren@gmail.com> writes:
Show 9 quoted lines
> +--sort=<key>::
> +	Sort based on the key given. Prefix `-` to sort in descending order
> +	of the value. Supports "version:refname" or "v:refname" (tag names
> +	are treated as versions). The "version:refname" sort order can also
> +	be affected by the "versionsort.suffix" configuration variable.
> +	See linkgit:git-for-each-ref[1] for more sort options, but be aware
> +	that because `ls-remote` deals only with remotes, any key like
> +	`committerdate` that requires access to the object itself will
> +	cause a failure.

I can connect "because it deals only with remotes" and "access to the object may fail", but I wonder if we can clarify the former a bit better to make it easier to understand for those who are not yet familiar with Git.

    Keys like `committerdate` that require access to the object will
    not work [***HOW??? Do we get a segfault or something???***] for
    refs whose objects have not yet been fetched from the remote.

I added "for refs whose objects have not yet been fetched", whose explicit-ness made "will not work" to be an OK expression, but without it I would have suggested to rephrase it to "may not work".

This is a tangent but I suspect that the description of --sort in git-for-each-ref documentation is wrong on the use of multiple options, or I am misreading parse_opt_ref_sorting(), which I think appends later keys to the end of the list, and compare_refs() which I think yields results when an earlier key in the last decides two are not equal and ignores the later keys. The commit that added the description was c0f6dc9b ("git-for-each-ref.txt: minor improvements", 2008-06-05), and I think even back then the code in builtin-for-each-ref.c appended later keys at the end, and it seems nobody complained, so it is possible I am not reading the code right before fully adjusting to timezone change ;-)

Show 14 quoted lines
> +	for (i = 0; i < ref_array.nr; i++) {
> +		const struct ref_array_item *ref = ref_array.items[i];
>  		if (show_symref_target && ref->symref)
> -			printf("ref: %s\t%s\n", ref->symref, ref->name);
> -		printf("%s\t%s\n", oid_to_hex(&ref->old_oid), ref->name);
> +			printf("ref: %s\t%s\n", ref->symref, ref->refname);
> +		printf("%s\t%s\n", oid_to_hex(&ref->objectname), ref->refname);
>  		status = 0; /* we found something */
>  	}
> +
> +	UNLEAK(sorting);
> +	UNLEAK(ref_array);
>  	return status;
>  }

It is not too big a deal, but I find that liberal sprinkling of UNLEAK() is somewhat unsightly. From the code we leave with this change, it is clear what we'd need to do when we make the code fully leak-proof (i.e. we'd have a several lines to free resources held by these two variables here, and then UNLEAK() that appear earlier in the function will become a "goto cleanup;" after setting appropriate value to "status"), so let's not worry about it.

Show 7 quoted lines
> +test_expect_success 'ls-remote --sort="version:refname" --tags self' '
> +	cat >expect <<-\EOF &&
> +	'$(git rev-parse mark)'	refs/tags/mark
> +	'$(git rev-parse mark1.1)'	refs/tags/mark1.1
> +	'$(git rev-parse mark1.2)'	refs/tags/mark1.2
> +	'$(git rev-parse mark1.10)'	refs/tags/mark1.10
> +	EOF

Please do *NOT* step outside the pair of single quotes that protects the body of the test scriptlet and execute commands like "git rev-parse". These execute in order to prepare the command line argument strings BEFORE test_expect_success can run (or the test framework decides if the test will be skipped via GIT_TEST_SKIP).

Instead do it like so:
	cat >expect <<-EOF &&
	$(git rev-parse mark)	refs/tags/mark
	$(git rev-parse mark1.1)	refs/tags/mark1.1
	$(git rev-parse mark1.2)	refs/tags/mark1.2
	$(git rev-parse mark1.10)	refs/tags/mark1.10
	EOF

I.e. the end token for << that is not quoted allows $var and $(cmd) to be substituted.

Same comment applies throughout the remainder of this file.
Other than that, this patch was a very pleasant read.  Thanks.
Harald Nordgren· Apr 9, 2018, 00:09 UTC · re: Junio C Hamano · lore

Re: [PATCH v12 4/4] ls-remote: create '--sort' option

On Mon, Apr 9, 2018 at 12:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 25 quoted lines
>
> I can connect "because it deals only with remotes" and "access to
> the object may fail", but I wonder if we can clarify the former a
> bit better to make it easier to understand for those who are not yet
> familiar with Git.
>
>     Keys like `committerdate` that require access to the object will
>     not work [***HOW??? Do we get a segfault or something???***] for
>     refs whose objects have not yet been fetched from the remote.
>
> I added "for refs whose objects have not yet been fetched", whose
> explicit-ness made "will not work" to be an OK expression, but
> without it I would have suggested to rephrase it to "may not work".
>
> This is a tangent but I suspect that the description of --sort in
> git-for-each-ref documentation is wrong on the use of multiple
> options, or I am misreading parse_opt_ref_sorting(), which I think
> appends later keys to the end of the list, and compare_refs() which
> I think yields results when an earlier key in the last decides two
> are not equal and ignores the later keys.  The commit that added the
> description was c0f6dc9b ("git-for-each-ref.txt: minor
> improvements", 2008-06-05), and I think even back then the code in
> builtin-for-each-ref.c appended later keys at the end, and it seems
> nobody complained, so it is possible I am not reading the code right
> before fully adjusting to timezone change ;-)
Updated the docs.
Show 8 quoted lines
>
> It is not too big a deal, but I find that liberal sprinkling of
> UNLEAK() is somewhat unsightly.  From the code we leave with this
> change, it is clear what we'd need to do when we make the code fully
> leak-proof (i.e. we'd have a several lines to free resources held by
> these two variables here, and then UNLEAK() that appear earlier in
> the function will become a "goto cleanup;" after setting appropriate
> value to "status"), so let's not worry about it.

Removed the "extra" unleak annotations within the if-return blocks, but kept them at the end.

Show 19 quoted lines
> Please do *NOT* step outside the pair of single quotes that protects
> the body of the test scriptlet and execute commands like "git
> rev-parse".  These execute in order to prepare the command line
> argument strings BEFORE test_expect_success can run (or the test
> framework decides if the test will be skipped via GIT_TEST_SKIP).
>
> Instead do it like so:
>
>         cat >expect <<-EOF &&
>         $(git rev-parse mark)   refs/tags/mark
>         $(git rev-parse mark1.1)        refs/tags/mark1.1
>         $(git rev-parse mark1.2)        refs/tags/mark1.2
>         $(git rev-parse mark1.10)       refs/tags/mark1.10
>         EOF
>
> I.e. the end token for << that is not quoted allows $var and $(cmd)
> to be substituted.
>
> Same comment applies throughout the remainder of this file.

Ah, thanks! I was looking for some way to allow the values to expand within the proper test context.

So turns out '<<-\EOF' vs '<<-EOF' makes all the difference :)
> Other than that, this patch was a very pleasant read.  Thanks.
Thank you!
Junio C Hamano· Apr 9, 2018, 00:48 UTC · re: Harald Nordgren · lore

Re: [PATCH v12 4/4] ls-remote: create '--sort' option

Harald Nordgren <haraldnordgren@gmail.com> writes:
Show 10 quoted lines
>> It is not too big a deal, but I find that liberal sprinkling of
>> UNLEAK() is somewhat unsightly.  From the code we leave with this
>> change, it is clear what we'd need to do when we make the code fully
>> leak-proof (i.e. we'd have a several lines to free resources held by
>> these two variables here, and then UNLEAK() that appear earlier in
>> the function will become a "goto cleanup;" after setting appropriate
>> value to "status"), so let's not worry about it.
>
> Removed the "extra" unleak annotations within the if-return blocks,
> but kept them at the end.

I actually think that is worse. The UNLEAK()s near early-return serve to remind us: "here we are making early return and when we properly plug the leak, here is one of the places we need to fix". Please keep them, unless (and I do not recommend to do this) you are plugging the leaks for real.

Eric Sunshine· Apr 9, 2018, 02:31 UTC · re: Junio C Hamano · lore

Re: [PATCH v12 4/4] ls-remote: create '--sort' option

On Sun, Apr 8, 2018 at 6:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
> Harald Nordgren <haraldnordgren@gmail.com> writes:
>> +--sort=<key>::
>> +     Sort based on the key given. [...]
>
> This is a tangent but I suspect that the description of --sort in
> git-for-each-ref documentation is wrong on the use of multiple
> options, or I am misreading parse_opt_ref_sorting(), which I think
> appends later keys to the end of the list, and compare_refs() which
> I think yields results when an earlier key in the last decides two
> are not equal and ignores the later keys.  The commit that added the
> description was c0f6dc9b ("git-for-each-ref.txt: minor
> improvements", 2008-06-05), and I think even back then the code in
> builtin-for-each-ref.c appended later keys at the end, and it seems
> nobody complained, so it is possible I am not reading the code right
> before fully adjusting to timezone change ;-)

I just re-read parse_opt_ref_sorting(), and I'm pretty sure that it is _prepending_ keys to the list, despite the confusingly named variable "sorting_tail", thus it agrees with the documentation that --sort=<key> specified last becomes primary sort key.

For me, at least, that behavior (last --sort= becomes primary key) feels counterintuitive, but it's too late to change it now.

Harald Nordgren· Apr 8, 2018, 12:28 UTC · re: Harald Nordgren · lore

[PATCH v12 2/4] ref-filter: make ref_array_item allocation more consistent

From: Jeff King <peff@peff.net>

We have a helper function to allocate ref_array_item structs, but it only takes a subset of the possible fields in the struct as initializers. We could have it accept an argument for _every_ field, but that becomes a pain for the fields which some callers don't want to set initially.

Instead, let's be explicit that it takes only the minimum required to create the ref, and that callers should then fill in the rest themselves.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
 ref-filter.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)
Show changes to ref-filter.c +11 −7
diff --git a/ref-filter.c b/ref-filter.c
index ade97a848..c1c3cc948 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1824,15 +1824,18 @@ static const struct object_id *match_points_at(struct oid_array *points_at,
 	return NULL;
 }
 
-/* Allocate space for a new ref_array_item and copy the objectname and flag to it */
+/*
+ * Allocate space for a new ref_array_item and copy the name and oid to it.
+ *
+ * Callers can then fill in other struct members at their leisure.
+ */
 static struct ref_array_item *new_ref_array_item(const char *refname,
-						 const struct object_id *oid,
-						 int flag)
+						 const struct object_id *oid)
 {
 	struct ref_array_item *ref;
+
 	FLEX_ALLOC_STR(ref, refname, refname);
 	oidcpy(&ref->objectname, oid);
-	ref->flag = flag;
 
 	return ref;
 }
@@ -1927,12 +1930,13 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	 * to do its job and the resulting list may yet to be pruned
 	 * by maxcount logic.
 	 */
-	ref = new_ref_array_item(refname, oid, flag);
+	ref = new_ref_array_item(refname, oid);
 	ref->commit = commit;
+	ref->flag = flag;
+	ref->kind = kind;
 
 	REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
 	ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
-	ref->kind = kind;
 	return 0;
 }
 
@@ -2169,7 +2173,7 @@ void pretty_print_ref(const char *name, const struct object_id *oid,
 		      const struct ref_format *format)
 {
 	struct ref_array_item *ref_item;
-	ref_item = new_ref_array_item(name, oid, 0);
+	ref_item = new_ref_array_item(name, oid);
 	ref_item->kind = ref_kind_from_refname(name);
 	show_ref_array_item(ref_item, format);
 	free_array_item(ref_item);
-- 
2.14.3 (Apple Git-98)
Harald Nordgren· Apr 8, 2018, 23:58 UTC · re: Harald Nordgren · lore

[PATCH v13 1/4] ref-filter: use "struct object_id" consistently

From: Jeff King <peff@peff.net>

Internally we store a "struct object_id", and all of our callers have one to pass us. But we insist that they peel it to its bare-sha1 hash, which we then hashcpy() into place. Let's pass it around as an object_id, which future-proofs us for a post-sha1 world.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
 builtin/tag.c        |  2 +-
 builtin/verify-tag.c |  2 +-
 ref-filter.c         | 10 +++++-----
 ref-filter.h         |  2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)
Show changes to 4 files +8 −8

builtin/tag.c, builtin/verify-tag.c, ref-filter.c, ref-filter.h

diff --git a/builtin/tag.c b/builtin/tag.c
index da186691e..42278f516 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -117,7 +117,7 @@ static int verify_tag(const char *name, const char *ref,
 		return -1;
 
 	if (format->format)
-		pretty_print_ref(name, oid->hash, format);
+		pretty_print_ref(name, oid, format);
 
 	return 0;
 }
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index ad7b79fa5..6fa04b751 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -72,7 +72,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 		}
 
 		if (format.format)
-			pretty_print_ref(name, oid.hash, &format);
+			pretty_print_ref(name, &oid, &format);
 	}
 	return had_error;
 }
diff --git a/ref-filter.c b/ref-filter.c
index 45fc56216..ade97a848 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1826,12 +1826,12 @@ static const struct object_id *match_points_at(struct oid_array *points_at,
 
 /* Allocate space for a new ref_array_item and copy the objectname and flag to it */
 static struct ref_array_item *new_ref_array_item(const char *refname,
-						 const unsigned char *objectname,
+						 const struct object_id *oid,
 						 int flag)
 {
 	struct ref_array_item *ref;
 	FLEX_ALLOC_STR(ref, refname, refname);
-	hashcpy(ref->objectname.hash, objectname);
+	oidcpy(&ref->objectname, oid);
 	ref->flag = flag;
 
 	return ref;
@@ -1927,7 +1927,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	 * to do its job and the resulting list may yet to be pruned
 	 * by maxcount logic.
 	 */
-	ref = new_ref_array_item(refname, oid->hash, flag);
+	ref = new_ref_array_item(refname, oid, flag);
 	ref->commit = commit;
 
 	REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
@@ -2165,11 +2165,11 @@ void show_ref_array_item(struct ref_array_item *info,
 	putchar('\n');
 }
 
-void pretty_print_ref(const char *name, const unsigned char *sha1,
+void pretty_print_ref(const char *name, const struct object_id *oid,
 		      const struct ref_format *format)
 {
 	struct ref_array_item *ref_item;
-	ref_item = new_ref_array_item(name, sha1, 0);
+	ref_item = new_ref_array_item(name, oid, 0);
 	ref_item->kind = ref_kind_from_refname(name);
 	show_ref_array_item(ref_item, format);
 	free_array_item(ref_item);
diff --git a/ref-filter.h b/ref-filter.h
index 0d98342b3..68268f9eb 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -132,7 +132,7 @@ void setup_ref_filter_porcelain_msg(void);
  * Print a single ref, outside of any ref-filter. Note that the
  * name must be a fully qualified refname.
  */
-void pretty_print_ref(const char *name, const unsigned char *sha1,
+void pretty_print_ref(const char *name, const struct object_id *oid,
 		      const struct ref_format *format);
 
 #endif /*  REF_FILTER_H  */
-- 
2.14.3 (Apple Git-98)
Harald Nordgren· Apr 8, 2018, 23:58 UTC · re: Harald Nordgren · lore

[PATCH v13 2/4] ref-filter: make ref_array_item allocation more consistent

From: Jeff King <peff@peff.net>

We have a helper function to allocate ref_array_item structs, but it only takes a subset of the possible fields in the struct as initializers. We could have it accept an argument for _every_ field, but that becomes a pain for the fields which some callers don't want to set initially.

Instead, let's be explicit that it takes only the minimum required to create the ref, and that callers should then fill in the rest themselves.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
 ref-filter.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)
Show changes to ref-filter.c +11 −7
diff --git a/ref-filter.c b/ref-filter.c
index ade97a848..c1c3cc948 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1824,15 +1824,18 @@ static const struct object_id *match_points_at(struct oid_array *points_at,
 	return NULL;
 }
 
-/* Allocate space for a new ref_array_item and copy the objectname and flag to it */
+/*
+ * Allocate space for a new ref_array_item and copy the name and oid to it.
+ *
+ * Callers can then fill in other struct members at their leisure.
+ */
 static struct ref_array_item *new_ref_array_item(const char *refname,
-						 const struct object_id *oid,
-						 int flag)
+						 const struct object_id *oid)
 {
 	struct ref_array_item *ref;
+
 	FLEX_ALLOC_STR(ref, refname, refname);
 	oidcpy(&ref->objectname, oid);
-	ref->flag = flag;
 
 	return ref;
 }
@@ -1927,12 +1930,13 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	 * to do its job and the resulting list may yet to be pruned
 	 * by maxcount logic.
 	 */
-	ref = new_ref_array_item(refname, oid, flag);
+	ref = new_ref_array_item(refname, oid);
 	ref->commit = commit;
+	ref->flag = flag;
+	ref->kind = kind;
 
 	REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
 	ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
-	ref->kind = kind;
 	return 0;
 }
 
@@ -2169,7 +2173,7 @@ void pretty_print_ref(const char *name, const struct object_id *oid,
 		      const struct ref_format *format)
 {
 	struct ref_array_item *ref_item;
-	ref_item = new_ref_array_item(name, oid, 0);
+	ref_item = new_ref_array_item(name, oid);
 	ref_item->kind = ref_kind_from_refname(name);
 	show_ref_array_item(ref_item, format);
 	free_array_item(ref_item);
-- 
2.14.3 (Apple Git-98)
Harald Nordgren· Apr 8, 2018, 23:58 UTC · re: Harald Nordgren · lore

[PATCH v13 4/4] ls-remote: create '--sort' option

Create a '--sort' option for ls-remote, based on the one from for-each-ref. This e.g. allows ref names to be sorted by version semantics, so that v1.2 is sorted before v1.10.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
Notes:
    Changes according to Junio Hamano's code review
 Documentation/git-ls-remote.txt | 17 ++++++++++++-
 builtin/ls-remote.c             | 25 +++++++++++++++++--
 t/t5512-ls-remote.sh            | 54 +++++++++++++++++++++++++++++++++++------
 3 files changed, 86 insertions(+), 10 deletions(-)
Show changes to 3 files +86 −10

Documentation/git-ls-remote.txt, builtin/ls-remote.c, t/t5512-ls-remote.sh

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 5f2628c8f..0e26a67a1 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git ls-remote' [--heads] [--tags] [--refs] [--upload-pack=<exec>]
-	      [-q | --quiet] [--exit-code] [--get-url]
+	      [-q | --quiet] [--exit-code] [--get-url] [--sort=<key>]
 	      [--symref] [<repository> [<refs>...]]
 
 DESCRIPTION
@@ -60,6 +60,17 @@ OPTIONS
 	upload-pack only shows the symref HEAD, so it will be the only
 	one shown by ls-remote.
 
+--sort=<key>::
+	Sort based on the key given. Prefix `-` to sort in descending order
+	of the value. Supports "version:refname" or "v:refname" (tag names
+	are treated as versions). The "version:refname" sort order can also
+	be affected by the "versionsort.suffix" configuration variable.
+	See linkgit:git-for-each-ref[1] for more sort options, but be aware
+	that because `ls-remote` deals only with remotes, keys like
+	`committerdate` that requires access to the objects themselves will
+	not work for refs whose objects have not yet been fetched from the
+	remote.
+
 <repository>::
 	The "remote" repository to query.  This parameter can be
 	either a URL or the name of a remote (see the GIT URLS and
@@ -90,6 +101,10 @@ EXAMPLES
 	c5db5456ae3b0873fc659c19fafdde22313cc441	refs/tags/v0.99.2
 	7ceca275d047c90c0c7d5afb13ab97efdf51bd6e	refs/tags/v0.99.3
 
+SEE ALSO
+--------
+linkgit:git-check-ref-format[1].
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 540d56429..b5ca67167 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -1,6 +1,7 @@
 #include "builtin.h"
 #include "cache.h"
 #include "transport.h"
+#include "ref-filter.h"
 #include "remote.h"
 
 static const char * const ls_remote_usage[] = {
@@ -43,10 +44,13 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	int show_symref_target = 0;
 	const char *uploadpack = NULL;
 	const char **pattern = NULL;
+	int i;
 
 	struct remote *remote;
 	struct transport *transport;
 	const struct ref *ref;
+	struct ref_array ref_array;
+	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
 
 	struct option options[] = {
 		OPT__QUIET(&quiet, N_("do not print remote URL")),
@@ -60,6 +64,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL),
 		OPT_BOOL(0, "get-url", &get_url,
 			 N_("take url.<base>.insteadOf into account")),
+		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
+			     N_("field name to sort on"), &parse_opt_ref_sorting),
 		OPT_SET_INT_F(0, "exit-code", &status,
 			      N_("exit with exit code 2 if no matching refs are found"),
 			      2, PARSE_OPT_NOCOMPLETE),
@@ -68,6 +74,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	memset(&ref_array, 0, sizeof(ref_array));
+
 	argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 	dest = argv[0];
@@ -104,14 +112,27 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	if (!dest && !quiet)
 		fprintf(stderr, "From %s\n", *remote->url);
 	for ( ; ref; ref = ref->next) {
+		struct ref_array_item *item;
 		if (!check_ref_type(ref, flags))
 			continue;
 		if (!tail_match(pattern, ref->name))
 			continue;
+		item = ref_array_push(&ref_array, ref->name, &ref->old_oid);
+		item->symref = xstrdup_or_null(ref->symref);
+	}
+
+	if (sorting)
+		ref_array_sort(sorting, &ref_array);
+
+	for (i = 0; i < ref_array.nr; i++) {
+		const struct ref_array_item *ref = ref_array.items[i];
 		if (show_symref_target && ref->symref)
-			printf("ref: %s\t%s\n", ref->symref, ref->name);
-		printf("%s\t%s\n", oid_to_hex(&ref->old_oid), ref->name);
+			printf("ref: %s\t%s\n", ref->symref, ref->refname);
+		printf("%s\t%s\n", oid_to_hex(&ref->objectname), ref->refname);
 		status = 0; /* we found something */
 	}
+
+	UNLEAK(sorting);
+	UNLEAK(ref_array);
 	return status;
 }
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 02106c922..83cd35c39 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -10,6 +10,9 @@ test_expect_success setup '
 	test_tick &&
 	git commit -m initial &&
 	git tag mark &&
+	git tag mark1.1 &&
+	git tag mark1.2 &&
+	git tag mark1.10 &&
 	git show-ref --tags -d | sed -e "s/ /	/" >expected.tag &&
 	(
 		echo "$(git rev-parse HEAD)	HEAD"
@@ -39,6 +42,39 @@ test_expect_success 'ls-remote self' '
 	test_cmp expected.all actual
 '
 
+test_expect_success 'ls-remote --sort="version:refname" --tags self' '
+	cat >expect <<-EOF &&
+	$(git rev-parse mark)	refs/tags/mark
+	$(git rev-parse mark1.1)	refs/tags/mark1.1
+	$(git rev-parse mark1.2)	refs/tags/mark1.2
+	$(git rev-parse mark1.10)	refs/tags/mark1.10
+	EOF
+	git ls-remote --sort="version:refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-remote --sort="-version:refname" --tags self' '
+	cat >expect <<-EOF &&
+	$(git rev-parse mark1.10)	refs/tags/mark1.10
+	$(git rev-parse mark1.2)	refs/tags/mark1.2
+	$(git rev-parse mark1.1)	refs/tags/mark1.1
+	$(git rev-parse mark)	refs/tags/mark
+	EOF
+	git ls-remote --sort="-version:refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-remote --sort="-refname" --tags self' '
+	cat >expect <<-EOF &&
+	$(git rev-parse mark1.2)	refs/tags/mark1.2
+	$(git rev-parse mark1.10)	refs/tags/mark1.10
+	$(git rev-parse mark1.1)	refs/tags/mark1.1
+	$(git rev-parse mark)	refs/tags/mark
+	EOF
+	git ls-remote --sort="-refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'dies when no remote specified and no default remotes found' '
 	test_must_fail git ls-remote
 '
@@ -131,7 +167,7 @@ test_expect_success 'Report no-match with --exit-code' '
 
 test_expect_success 'Report match with --exit-code' '
 	git ls-remote --exit-code other.git "refs/tags/*" >actual &&
-	git ls-remote . tags/mark >expect &&
+	git ls-remote . tags/mark* >expect &&
 	test_cmp expect actual
 '
 
@@ -170,14 +206,18 @@ test_expect_success 'overrides work between mixed transfer/upload-pack hideRefs'
 	grep refs/tags/magic actual
 '
 
+git fetch origin
 test_expect_success 'ls-remote --symref' '
-	cat >expect <<-\EOF &&
+	cat >expect <<-EOF &&
 	ref: refs/heads/master	HEAD
-	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	HEAD
-	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/heads/master
-	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/HEAD
-	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/master
-	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	$(git rev-parse HEAD)	HEAD
+	$(git rev-parse refs/heads/master)	refs/heads/master
+	$(git rev-parse HEAD)	refs/remotes/origin/HEAD
+	$(git rev-parse refs/remotes/origin/master)	refs/remotes/origin/master
+	$(git rev-parse refs/tags/mark)	refs/tags/mark
+	$(git rev-parse refs/tags/mark1.1)	refs/tags/mark1.1
+	$(git rev-parse refs/tags/mark1.10)	refs/tags/mark1.10
+	$(git rev-parse refs/tags/mark1.2)	refs/tags/mark1.2
 	EOF
 	git ls-remote --symref >actual &&
 	test_cmp expect actual
-- 
2.14.3 (Apple Git-98)
Junio C Hamano· Apr 9, 2018, 00:56 UTC · re: Harald Nordgren · lore

Re: [PATCH v13 4/4] ls-remote: create '--sort' option

Harald Nordgren <haraldnordgren@gmail.com> writes:
Show 6 quoted lines
> Create a '--sort' option for ls-remote, based on the one from
> for-each-ref. This e.g. allows ref names to be sorted by version
> semantics, so that v1.2 is sorted before v1.10.
>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> ---
Thanks.
Show 10 quoted lines
> +--sort=<key>::
> +	Sort based on the key given. Prefix `-` to sort in descending order
> +	of the value. Supports "version:refname" or "v:refname" (tag names
> +	are treated as versions). The "version:refname" sort order can also
> +	be affected by the "versionsort.suffix" configuration variable.
> +	See linkgit:git-for-each-ref[1] for more sort options, but be aware
> +	that because `ls-remote` deals only with remotes, keys like
> +	`committerdate` that requires access to the objects themselves will
> +	not work for refs whose objects have not yet been fetched from the
> +	remote.

With the update since v12, I think "because `ls-remote` deals only with remotes," can be dropped entirely, and still convey what needs to be told: "Be aware some keys that needs access to objects that are not here won't work".

Instead, it is probably a better idea to spend that half-line worth of characters to describe in what way they do not work (do they try to deref a NULL pointer and dump core? do they notice we need missing objects and give an error?).

> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
> index 540d56429..b5ca67167 100644
> --- a/builtin/ls-remote.c
> +++ b/builtin/ls-remote.c

As I said earlier, let's keep these extra UNLEAK() near early return; they point developers at the places that needs future work.

Thanks.
Show changes to builtin/ls-remote.c +4 −1
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index b5ca67167d..d3851074c2 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -98,6 +98,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 
 	if (get_url) {
 		printf("%s\n", *remote->url);
+		UNLEAK(sorting);
 		return 0;
 	}
 
@@ -106,8 +107,10 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		transport_set_option(transport, TRANS_OPT_UPLOADPACK, uploadpack);
 
 	ref = transport_get_remote_refs(transport);
-	if (transport_disconnect(transport))
+	if (transport_disconnect(transport)) {
+		UNLEAK(sorting);
 		return 1;
+	}
 
 	if (!dest && !quiet)
 		fprintf(stderr, "From %s\n", *remote->url);
Harald Nordgren· Apr 9, 2018, 01:45 UTC · re: Junio C Hamano · lore

Re: [PATCH v13 4/4] ls-remote: create '--sort' option

On Mon, Apr 9, 2018 at 2:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 10 quoted lines
>
> With the update since v12, I think "because `ls-remote` deals only
> with remotes," can be dropped entirely, and still convey what needs
> to be told: "Be aware some keys that needs access to objects that
> are not here won't work".
>
> Instead, it is probably a better idea to spend that half-line worth
> of characters to describe in what way they do not work (do they try
> to deref a NULL pointer and dump core?  do they notice we need
> missing objects and give an error?).
Updated the docs again. The specific error we get on commiterdate is
    fatal: missing object f0d0fd3a5985d5e588da1e1d11c85fba0ae132f8 for
refs/pull/10/head
So no core dump. But a fatal error with a possibly confusing message.
Harald Nordgren· Apr 8, 2018, 23:58 UTC · re: Harald Nordgren · lore

[PATCH v13 3/4] ref-filter: factor ref_array pushing into its own function

From: Jeff King <peff@peff.net>

In preparation for callers constructing their own ref_array structs, let's move our own internal push operation into its own function.

While we're at it, we can replace REALLOC_ARRAY() with ALLOC_GROW(), which should give the growth operation amortized linear complexity (as opposed to growing by one, which is potentially quadratic, though in-place realloc growth often makes this faster in practice).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
 ref-filter.c | 16 +++++++++++++---
 ref-filter.h |  8 ++++++++
 2 files changed, 21 insertions(+), 3 deletions(-)
Show changes to 2 files +21 −3

ref-filter.c, ref-filter.h

diff --git a/ref-filter.c b/ref-filter.c
index c1c3cc948..6e9328b27 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1840,6 +1840,18 @@ static struct ref_array_item *new_ref_array_item(const char *refname,
 	return ref;
 }
 
+struct ref_array_item *ref_array_push(struct ref_array *array,
+				      const char *refname,
+				      const struct object_id *oid)
+{
+	struct ref_array_item *ref = new_ref_array_item(refname, oid);
+
+	ALLOC_GROW(array->items, array->nr + 1, array->alloc);
+	array->items[array->nr++] = ref;
+
+	return ref;
+}
+
 static int ref_kind_from_refname(const char *refname)
 {
 	unsigned int i;
@@ -1930,13 +1942,11 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	 * to do its job and the resulting list may yet to be pruned
 	 * by maxcount logic.
 	 */
-	ref = new_ref_array_item(refname, oid);
+	ref = ref_array_push(ref_cbdata->array, refname, oid);
 	ref->commit = commit;
 	ref->flag = flag;
 	ref->kind = kind;
 
-	REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
-	ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
 	return 0;
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 68268f9eb..76cf87cb6 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -135,4 +135,12 @@ void setup_ref_filter_porcelain_msg(void);
 void pretty_print_ref(const char *name, const struct object_id *oid,
 		      const struct ref_format *format);
 
+/*
+ * Push a single ref onto the array; this can be used to construct your own
+ * ref_array without using filter_refs().
+ */
+struct ref_array_item *ref_array_push(struct ref_array *array,
+				      const char *refname,
+				      const struct object_id *oid);
+
 #endif /*  REF_FILTER_H  */
-- 
2.14.3 (Apple Git-98)
Harald Nordgren· Apr 9, 2018, 01:42 UTC · re: Harald Nordgren · lore

[PATCH v14 1/4] ref-filter: use "struct object_id" consistently

From: Jeff King <peff@peff.net>

Internally we store a "struct object_id", and all of our callers have one to pass us. But we insist that they peel it to its bare-sha1 hash, which we then hashcpy() into place. Let's pass it around as an object_id, which future-proofs us for a post-sha1 world.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
 builtin/tag.c        |  2 +-
 builtin/verify-tag.c |  2 +-
 ref-filter.c         | 10 +++++-----
 ref-filter.h         |  2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)
Show changes to 4 files +8 −8

builtin/tag.c, builtin/verify-tag.c, ref-filter.c, ref-filter.h

diff --git a/builtin/tag.c b/builtin/tag.c
index da186691e..42278f516 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -117,7 +117,7 @@ static int verify_tag(const char *name, const char *ref,
 		return -1;
 
 	if (format->format)
-		pretty_print_ref(name, oid->hash, format);
+		pretty_print_ref(name, oid, format);
 
 	return 0;
 }
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index ad7b79fa5..6fa04b751 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -72,7 +72,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 		}
 
 		if (format.format)
-			pretty_print_ref(name, oid.hash, &format);
+			pretty_print_ref(name, &oid, &format);
 	}
 	return had_error;
 }
diff --git a/ref-filter.c b/ref-filter.c
index 45fc56216..ade97a848 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1826,12 +1826,12 @@ static const struct object_id *match_points_at(struct oid_array *points_at,
 
 /* Allocate space for a new ref_array_item and copy the objectname and flag to it */
 static struct ref_array_item *new_ref_array_item(const char *refname,
-						 const unsigned char *objectname,
+						 const struct object_id *oid,
 						 int flag)
 {
 	struct ref_array_item *ref;
 	FLEX_ALLOC_STR(ref, refname, refname);
-	hashcpy(ref->objectname.hash, objectname);
+	oidcpy(&ref->objectname, oid);
 	ref->flag = flag;
 
 	return ref;
@@ -1927,7 +1927,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	 * to do its job and the resulting list may yet to be pruned
 	 * by maxcount logic.
 	 */
-	ref = new_ref_array_item(refname, oid->hash, flag);
+	ref = new_ref_array_item(refname, oid, flag);
 	ref->commit = commit;
 
 	REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
@@ -2165,11 +2165,11 @@ void show_ref_array_item(struct ref_array_item *info,
 	putchar('\n');
 }
 
-void pretty_print_ref(const char *name, const unsigned char *sha1,
+void pretty_print_ref(const char *name, const struct object_id *oid,
 		      const struct ref_format *format)
 {
 	struct ref_array_item *ref_item;
-	ref_item = new_ref_array_item(name, sha1, 0);
+	ref_item = new_ref_array_item(name, oid, 0);
 	ref_item->kind = ref_kind_from_refname(name);
 	show_ref_array_item(ref_item, format);
 	free_array_item(ref_item);
diff --git a/ref-filter.h b/ref-filter.h
index 0d98342b3..68268f9eb 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -132,7 +132,7 @@ void setup_ref_filter_porcelain_msg(void);
  * Print a single ref, outside of any ref-filter. Note that the
  * name must be a fully qualified refname.
  */
-void pretty_print_ref(const char *name, const unsigned char *sha1,
+void pretty_print_ref(const char *name, const struct object_id *oid,
 		      const struct ref_format *format);
 
 #endif /*  REF_FILTER_H  */
-- 
2.14.3 (Apple Git-98)
Harald Nordgren· Apr 9, 2018, 01:42 UTC · re: Harald Nordgren · lore

[PATCH v14 2/4] ref-filter: make ref_array_item allocation more consistent

From: Jeff King <peff@peff.net>

We have a helper function to allocate ref_array_item structs, but it only takes a subset of the possible fields in the struct as initializers. We could have it accept an argument for _every_ field, but that becomes a pain for the fields which some callers don't want to set initially.

Instead, let's be explicit that it takes only the minimum required to create the ref, and that callers should then fill in the rest themselves.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
 ref-filter.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)
Show changes to ref-filter.c +11 −7
diff --git a/ref-filter.c b/ref-filter.c
index ade97a848..c1c3cc948 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1824,15 +1824,18 @@ static const struct object_id *match_points_at(struct oid_array *points_at,
 	return NULL;
 }
 
-/* Allocate space for a new ref_array_item and copy the objectname and flag to it */
+/*
+ * Allocate space for a new ref_array_item and copy the name and oid to it.
+ *
+ * Callers can then fill in other struct members at their leisure.
+ */
 static struct ref_array_item *new_ref_array_item(const char *refname,
-						 const struct object_id *oid,
-						 int flag)
+						 const struct object_id *oid)
 {
 	struct ref_array_item *ref;
+
 	FLEX_ALLOC_STR(ref, refname, refname);
 	oidcpy(&ref->objectname, oid);
-	ref->flag = flag;
 
 	return ref;
 }
@@ -1927,12 +1930,13 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	 * to do its job and the resulting list may yet to be pruned
 	 * by maxcount logic.
 	 */
-	ref = new_ref_array_item(refname, oid, flag);
+	ref = new_ref_array_item(refname, oid);
 	ref->commit = commit;
+	ref->flag = flag;
+	ref->kind = kind;
 
 	REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
 	ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
-	ref->kind = kind;
 	return 0;
 }
 
@@ -2169,7 +2173,7 @@ void pretty_print_ref(const char *name, const struct object_id *oid,
 		      const struct ref_format *format)
 {
 	struct ref_array_item *ref_item;
-	ref_item = new_ref_array_item(name, oid, 0);
+	ref_item = new_ref_array_item(name, oid);
 	ref_item->kind = ref_kind_from_refname(name);
 	show_ref_array_item(ref_item, format);
 	free_array_item(ref_item);
-- 
2.14.3 (Apple Git-98)
Harald Nordgren· Apr 11, 2018, 17:57 UTC · re: Harald Nordgren · lore

Re: [PATCH v14 2/4] ref-filter: make ref_array_item allocation more consistent

There have been no new comments for the last few days. I know Jeff King will be away for the next two weeks, but should we still move forward with this? The initial reactions to the idea seemed positive.

Best regards Harald

On Mon, Apr 9, 2018 at 3:42 AM, Harald Nordgren <haraldnordgren@gmail.com> wrote:

Show 73 quoted lines
> From: Jeff King <peff@peff.net>
>
> We have a helper function to allocate ref_array_item
> structs, but it only takes a subset of the possible fields
> in the struct as initializers. We could have it accept an
> argument for _every_ field, but that becomes a pain for the
> fields which some callers don't want to set initially.
>
> Instead, let's be explicit that it takes only the minimum
> required to create the ref, and that callers should then
> fill in the rest themselves.
>
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> ---
>  ref-filter.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index ade97a848..c1c3cc948 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1824,15 +1824,18 @@ static const struct object_id *match_points_at(struct oid_array *points_at,
>         return NULL;
>  }
>
> -/* Allocate space for a new ref_array_item and copy the objectname and flag to it */
> +/*
> + * Allocate space for a new ref_array_item and copy the name and oid to it.
> + *
> + * Callers can then fill in other struct members at their leisure.
> + */
>  static struct ref_array_item *new_ref_array_item(const char *refname,
> -                                                const struct object_id *oid,
> -                                                int flag)
> +                                                const struct object_id *oid)
>  {
>         struct ref_array_item *ref;
> +
>         FLEX_ALLOC_STR(ref, refname, refname);
>         oidcpy(&ref->objectname, oid);
> -       ref->flag = flag;
>
>         return ref;
>  }
> @@ -1927,12 +1930,13 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
>          * to do its job and the resulting list may yet to be pruned
>          * by maxcount logic.
>          */
> -       ref = new_ref_array_item(refname, oid, flag);
> +       ref = new_ref_array_item(refname, oid);
>         ref->commit = commit;
> +       ref->flag = flag;
> +       ref->kind = kind;
>
>         REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
>         ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
> -       ref->kind = kind;
>         return 0;
>  }
>
> @@ -2169,7 +2173,7 @@ void pretty_print_ref(const char *name, const struct object_id *oid,
>                       const struct ref_format *format)
>  {
>         struct ref_array_item *ref_item;
> -       ref_item = new_ref_array_item(name, oid, 0);
> +       ref_item = new_ref_array_item(name, oid);
>         ref_item->kind = ref_kind_from_refname(name);
>         show_ref_array_item(ref_item, format);
>         free_array_item(ref_item);
> --
> 2.14.3 (Apple Git-98)
>
Stefan Beller· Apr 11, 2018, 18:07 UTC · re: Harald Nordgren · lore

Re: [PATCH v14 2/4] ref-filter: make ref_array_item allocation more consistent

Hi Harald,

On Wed, Apr 11, 2018 at 10:57 AM, Harald Nordgren <haraldnordgren@gmail.com> wrote:

> There have been no new comments for the last few days. I know Jeff
> King will be away for the next two weeks, but should we still move
> forward with this? The initial reactions to the idea seemed positive.

Please see the "What's cooking?" email on the mailing list that is sent out periodically by Junio. the last one is https://public-inbox.org/git/xmqqd0z865pk.fsf@gitster-ct.c.googlers.com/ which says:

Show 9 quoted lines
> * jk/ref-array-push (2018-04-09) 3 commits
> - ref-filter: factor ref_array pushing into its own function
> - ref-filter: make ref_array_item allocation more consistent
> - ref-filter: use "struct object_id" consistently
> (this branch is used by hn/sort-ls-remote.)
>
> API clean-up aournd ref-filter code.
>
> Will merge to 'next'.

It will be merged to next and if no people speak up (due to bugs observed or such) then it will be merged to master eventually, later.

I am not able to find the documentation for the workflow right now, though it is partially covered in Documentation/SubmittingPatches.

Todd Zullinger· Apr 11, 2018, 18:30 UTC · re: Stefan Beller · lore

Re: [PATCH v14 2/4] ref-filter: make ref_array_item allocation more consistent

Hi Stefan,
Stefan Beller wrote:
Show 22 quoted lines
> Please see the "What's cooking?" email on the mailing list that is
> sent out periodically by Junio.
> the last one is
> https://public-inbox.org/git/xmqqd0z865pk.fsf@gitster-ct.c.googlers.com/
> which says:
> 
>> * jk/ref-array-push (2018-04-09) 3 commits
>> - ref-filter: factor ref_array pushing into its own function
>> - ref-filter: make ref_array_item allocation more consistent
>> - ref-filter: use "struct object_id" consistently
>> (this branch is used by hn/sort-ls-remote.)
>>
>> API clean-up aournd ref-filter code.
>>
>> Will merge to 'next'.
> 
> It will be merged to next and if no people speak up (due to bugs
> observed or such)
> then it will be merged to master eventually, later.
> 
> I am not able to find the documentation for the workflow right now,
> though it is partially covered in Documentation/SubmittingPatches.

Perhaps Documentation/howto/maintain-git.txt is the documentation you're thinking of?

https://kernel.org/pub/software/scm/git/docs/howto/maintain-git.html
There's also MaintNotes in the todo branch:
https://raw.githubusercontent.com/git/git/todo/MaintNotes
-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Be who you are and say what you feel because those who mind don't
matter and those who matter don't mind.
    -- Dr Seuss, "Oh the Places You'll Go"
Eric Sunshine· Apr 11, 2018, 18:56 UTC · re: Stefan Beller · lore

Re: [PATCH v14 2/4] ref-filter: make ref_array_item allocation more consistent

On Wed, Apr 11, 2018 at 2:07 PM, Stefan Beller <sbeller@google.com> wrote:
Show 21 quoted lines
> On Wed, Apr 11, 2018 at 10:57 AM, Harald Nordgren
> <haraldnordgren@gmail.com> wrote:
>> There have been no new comments for the last few days. I know Jeff
>> King will be away for the next two weeks, but should we still move
>> forward with this? The initial reactions to the idea seemed positive.
>
> Please see the "What's cooking?" email on the mailing list that is
> sent out periodically by Junio.
>
>> * jk/ref-array-push (2018-04-09) 3 commits
>> - ref-filter: factor ref_array pushing into its own function
>> - ref-filter: make ref_array_item allocation more consistent
>> - ref-filter: use "struct object_id" consistently
>> (this branch is used by hn/sort-ls-remote.)
>>
>> API clean-up aournd ref-filter code.
>>
>> Will merge to 'next'.
>
> It will be merged to next and if no people speak up (due to bugs
> observed or such)

I would guess, however, that Harald is wondering more specifically about the "--sort" patch he added atop Peff's patches. Disposition for _that_ patch is not specified in the latest "What's cooking":

    * hn/sort-ls-remote (2018-04-09) 1 commit
     - ls-remote: create '--sort' option
     (this branch uses jk/ref-array-push.)
     "git ls-remote" learned an option to allow sorting its output based
     on the refnames being shown.
Junio C Hamano· Apr 11, 2018, 23:25 UTC · re: Eric Sunshine · lore

Re: [PATCH v14 2/4] ref-filter: make ref_array_item allocation more consistent

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 20 quoted lines
> On Wed, Apr 11, 2018 at 2:07 PM, Stefan Beller <sbeller@google.com> wrote:
>> On Wed, Apr 11, 2018 at 10:57 AM, Harald Nordgren
>> <haraldnordgren@gmail.com> wrote:
>>> There have been no new comments for the last few days. I know Jeff
>>> King will be away for the next two weeks, but should we still move
>>> forward with this? The initial reactions to the idea seemed positive.
>> ...
>> It will be merged to next and if no people speak up (due to bugs
>> observed or such)
>
> I would guess, however, that Harald is wondering more specifically
> about the "--sort" patch he added atop Peff's patches. Disposition for
> _that_ patch is not specified in the latest "What's cooking":
>
>     * hn/sort-ls-remote (2018-04-09) 1 commit
>      - ls-remote: create '--sort' option
>      (this branch uses jk/ref-array-push.)
>
>      "git ls-remote" learned an option to allow sorting its output based
>      on the refnames being shown.
Thanks all for summarizing.

I think the preliminary clean-up patches by Peff are all independently good and can be advanced, even without him being around. I haven't formed a firm opinion on the doneness of the latest round of the "--sort" thing. I usually do not explicitly write "I haven't formed an opinion" in "What's cooking" report, so please read the lack of "disposition" as such.

Harald Nordgren· Apr 9, 2018, 01:42 UTC · re: Harald Nordgren · lore

[PATCH v14 3/4] ref-filter: factor ref_array pushing into its own function

From: Jeff King <peff@peff.net>

In preparation for callers constructing their own ref_array structs, let's move our own internal push operation into its own function.

While we're at it, we can replace REALLOC_ARRAY() with ALLOC_GROW(), which should give the growth operation amortized linear complexity (as opposed to growing by one, which is potentially quadratic, though in-place realloc growth often makes this faster in practice).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
 ref-filter.c | 16 +++++++++++++---
 ref-filter.h |  8 ++++++++
 2 files changed, 21 insertions(+), 3 deletions(-)
Show changes to 2 files +21 −3

ref-filter.c, ref-filter.h

diff --git a/ref-filter.c b/ref-filter.c
index c1c3cc948..6e9328b27 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1840,6 +1840,18 @@ static struct ref_array_item *new_ref_array_item(const char *refname,
 	return ref;
 }
 
+struct ref_array_item *ref_array_push(struct ref_array *array,
+				      const char *refname,
+				      const struct object_id *oid)
+{
+	struct ref_array_item *ref = new_ref_array_item(refname, oid);
+
+	ALLOC_GROW(array->items, array->nr + 1, array->alloc);
+	array->items[array->nr++] = ref;
+
+	return ref;
+}
+
 static int ref_kind_from_refname(const char *refname)
 {
 	unsigned int i;
@@ -1930,13 +1942,11 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	 * to do its job and the resulting list may yet to be pruned
 	 * by maxcount logic.
 	 */
-	ref = new_ref_array_item(refname, oid);
+	ref = ref_array_push(ref_cbdata->array, refname, oid);
 	ref->commit = commit;
 	ref->flag = flag;
 	ref->kind = kind;
 
-	REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
-	ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
 	return 0;
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 68268f9eb..76cf87cb6 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -135,4 +135,12 @@ void setup_ref_filter_porcelain_msg(void);
 void pretty_print_ref(const char *name, const struct object_id *oid,
 		      const struct ref_format *format);
 
+/*
+ * Push a single ref onto the array; this can be used to construct your own
+ * ref_array without using filter_refs().
+ */
+struct ref_array_item *ref_array_push(struct ref_array *array,
+				      const char *refname,
+				      const struct object_id *oid);
+
 #endif /*  REF_FILTER_H  */
-- 
2.14.3 (Apple Git-98)
Harald Nordgren· Apr 9, 2018, 01:42 UTC · re: Harald Nordgren · lore

[PATCH v14 4/4] ls-remote: create '--sort' option

Create a '--sort' option for ls-remote, based on the one from for-each-ref. This e.g. allows ref names to be sorted by version semantics, so that v1.2 is sorted before v1.10.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
Notes:
    Changes according to Junio Hamano's code review (2)
 Documentation/git-ls-remote.txt | 16 +++++++++++-
 builtin/ls-remote.c             | 28 ++++++++++++++++++---
 t/t5512-ls-remote.sh            | 54 +++++++++++++++++++++++++++++++++++------
 3 files changed, 87 insertions(+), 11 deletions(-)
Show changes to 3 files +87 −11

Documentation/git-ls-remote.txt, builtin/ls-remote.c, t/t5512-ls-remote.sh

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 5f2628c8f..6ad1e34af 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git ls-remote' [--heads] [--tags] [--refs] [--upload-pack=<exec>]
-	      [-q | --quiet] [--exit-code] [--get-url]
+	      [-q | --quiet] [--exit-code] [--get-url] [--sort=<key>]
 	      [--symref] [<repository> [<refs>...]]
 
 DESCRIPTION
@@ -60,6 +60,16 @@ OPTIONS
 	upload-pack only shows the symref HEAD, so it will be the only
 	one shown by ls-remote.
 
+--sort=<key>::
+	Sort based on the key given. Prefix `-` to sort in descending order
+	of the value. Supports "version:refname" or "v:refname" (tag names
+	are treated as versions). The "version:refname" sort order can also
+	be affected by the "versionsort.suffix" configuration variable.
+	See linkgit:git-for-each-ref[1] for more sort options, but be aware
+	keys like `committerdate` that require access to the objects
+	themselves will not work for refs whose objects have not yet been
+	fetched from the remote, and will give a `missing object` error.
+
 <repository>::
 	The "remote" repository to query.  This parameter can be
 	either a URL or the name of a remote (see the GIT URLS and
@@ -90,6 +100,10 @@ EXAMPLES
 	c5db5456ae3b0873fc659c19fafdde22313cc441	refs/tags/v0.99.2
 	7ceca275d047c90c0c7d5afb13ab97efdf51bd6e	refs/tags/v0.99.3
 
+SEE ALSO
+--------
+linkgit:git-check-ref-format[1].
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 540d56429..b26c53670 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -1,6 +1,7 @@
 #include "builtin.h"
 #include "cache.h"
 #include "transport.h"
+#include "ref-filter.h"
 #include "remote.h"
 
 static const char * const ls_remote_usage[] = {
@@ -43,10 +44,13 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	int show_symref_target = 0;
 	const char *uploadpack = NULL;
 	const char **pattern = NULL;
+	int i;
 
 	struct remote *remote;
 	struct transport *transport;
 	const struct ref *ref;
+	struct ref_array ref_array;
+	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
 
 	struct option options[] = {
 		OPT__QUIET(&quiet, N_("do not print remote URL")),
@@ -60,6 +64,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL),
 		OPT_BOOL(0, "get-url", &get_url,
 			 N_("take url.<base>.insteadOf into account")),
+		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
+			     N_("field name to sort on"), &parse_opt_ref_sorting),
 		OPT_SET_INT_F(0, "exit-code", &status,
 			      N_("exit with exit code 2 if no matching refs are found"),
 			      2, PARSE_OPT_NOCOMPLETE),
@@ -68,6 +74,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	memset(&ref_array, 0, sizeof(ref_array));
+
 	argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 	dest = argv[0];
@@ -90,6 +98,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 
 	if (get_url) {
 		printf("%s\n", *remote->url);
+		UNLEAK(sorting);
 		return 0;
 	}
 
@@ -98,20 +107,33 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		transport_set_option(transport, TRANS_OPT_UPLOADPACK, uploadpack);
 
 	ref = transport_get_remote_refs(transport);
-	if (transport_disconnect(transport))
+	if (transport_disconnect(transport)) {
+		UNLEAK(sorting);
 		return 1;
+	}
 
 	if (!dest && !quiet)
 		fprintf(stderr, "From %s\n", *remote->url);
 	for ( ; ref; ref = ref->next) {
+		struct ref_array_item *item;
 		if (!check_ref_type(ref, flags))
 			continue;
 		if (!tail_match(pattern, ref->name))
 			continue;
+		item = ref_array_push(&ref_array, ref->name, &ref->old_oid);
+		item->symref = xstrdup_or_null(ref->symref);
+	}
+
+	if (sorting)
+		ref_array_sort(sorting, &ref_array);
+
+	for (i = 0; i < ref_array.nr; i++) {
+		const struct ref_array_item *ref = ref_array.items[i];
 		if (show_symref_target && ref->symref)
-			printf("ref: %s\t%s\n", ref->symref, ref->name);
-		printf("%s\t%s\n", oid_to_hex(&ref->old_oid), ref->name);
+			printf("ref: %s\t%s\n", ref->symref, ref->refname);
+		printf("%s\t%s\n", oid_to_hex(&ref->objectname), ref->refname);
 		status = 0; /* we found something */
 	}
+
 	return status;
 }
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 02106c922..83cd35c39 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -10,6 +10,9 @@ test_expect_success setup '
 	test_tick &&
 	git commit -m initial &&
 	git tag mark &&
+	git tag mark1.1 &&
+	git tag mark1.2 &&
+	git tag mark1.10 &&
 	git show-ref --tags -d | sed -e "s/ /	/" >expected.tag &&
 	(
 		echo "$(git rev-parse HEAD)	HEAD"
@@ -39,6 +42,39 @@ test_expect_success 'ls-remote self' '
 	test_cmp expected.all actual
 '
 
+test_expect_success 'ls-remote --sort="version:refname" --tags self' '
+	cat >expect <<-EOF &&
+	$(git rev-parse mark)	refs/tags/mark
+	$(git rev-parse mark1.1)	refs/tags/mark1.1
+	$(git rev-parse mark1.2)	refs/tags/mark1.2
+	$(git rev-parse mark1.10)	refs/tags/mark1.10
+	EOF
+	git ls-remote --sort="version:refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-remote --sort="-version:refname" --tags self' '
+	cat >expect <<-EOF &&
+	$(git rev-parse mark1.10)	refs/tags/mark1.10
+	$(git rev-parse mark1.2)	refs/tags/mark1.2
+	$(git rev-parse mark1.1)	refs/tags/mark1.1
+	$(git rev-parse mark)	refs/tags/mark
+	EOF
+	git ls-remote --sort="-version:refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-remote --sort="-refname" --tags self' '
+	cat >expect <<-EOF &&
+	$(git rev-parse mark1.2)	refs/tags/mark1.2
+	$(git rev-parse mark1.10)	refs/tags/mark1.10
+	$(git rev-parse mark1.1)	refs/tags/mark1.1
+	$(git rev-parse mark)	refs/tags/mark
+	EOF
+	git ls-remote --sort="-refname" --tags self >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'dies when no remote specified and no default remotes found' '
 	test_must_fail git ls-remote
 '
@@ -131,7 +167,7 @@ test_expect_success 'Report no-match with --exit-code' '
 
 test_expect_success 'Report match with --exit-code' '
 	git ls-remote --exit-code other.git "refs/tags/*" >actual &&
-	git ls-remote . tags/mark >expect &&
+	git ls-remote . tags/mark* >expect &&
 	test_cmp expect actual
 '
 
@@ -170,14 +206,18 @@ test_expect_success 'overrides work between mixed transfer/upload-pack hideRefs'
 	grep refs/tags/magic actual
 '
 
+git fetch origin
 test_expect_success 'ls-remote --symref' '
-	cat >expect <<-\EOF &&
+	cat >expect <<-EOF &&
 	ref: refs/heads/master	HEAD
-	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	HEAD
-	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/heads/master
-	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/HEAD
-	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/master
-	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
+	$(git rev-parse HEAD)	HEAD
+	$(git rev-parse refs/heads/master)	refs/heads/master
+	$(git rev-parse HEAD)	refs/remotes/origin/HEAD
+	$(git rev-parse refs/remotes/origin/master)	refs/remotes/origin/master
+	$(git rev-parse refs/tags/mark)	refs/tags/mark
+	$(git rev-parse refs/tags/mark1.1)	refs/tags/mark1.1
+	$(git rev-parse refs/tags/mark1.10)	refs/tags/mark1.10
+	$(git rev-parse refs/tags/mark1.2)	refs/tags/mark1.2
 	EOF
 	git ls-remote --symref >actual &&
 	test_cmp expect actual
-- 
2.14.3 (Apple Git-98)
René Scharfe· May 12, 2018, 08:45 UTC · re: Harald Nordgren · lore

Re: [PATCH v14 4/4] ls-remote: create '--sort' option

Am 09.04.2018 um 03:42 schrieb Harald Nordgren:
> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
> index 02106c922..83cd35c39 100755
> --- a/t/t5512-ls-remote.sh
> +++ b/t/t5512-ls-remote.sh
Show 25 quoted lines
> @@ -170,14 +206,18 @@ test_expect_success 'overrides work between mixed transfer/upload-pack hideRefs'
>   	grep refs/tags/magic actual
>   '
>   
> +git fetch origin
>   test_expect_success 'ls-remote --symref' '
> -	cat >expect <<-\EOF &&
> +	cat >expect <<-EOF &&
>   	ref: refs/heads/master	HEAD
> -	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	HEAD
> -	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/heads/master
> -	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/HEAD
> -	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/remotes/origin/master
> -	1bd44cb9d13204b0fe1958db0082f5028a16eb3a	refs/tags/mark
> +	$(git rev-parse HEAD)	HEAD
> +	$(git rev-parse refs/heads/master)	refs/heads/master
> +	$(git rev-parse HEAD)	refs/remotes/origin/HEAD
> +	$(git rev-parse refs/remotes/origin/master)	refs/remotes/origin/master
> +	$(git rev-parse refs/tags/mark)	refs/tags/mark
> +	$(git rev-parse refs/tags/mark1.1)	refs/tags/mark1.1
> +	$(git rev-parse refs/tags/mark1.10)	refs/tags/mark1.10
> +	$(git rev-parse refs/tags/mark1.2)	refs/tags/mark1.2
>   	EOF
>   	git ls-remote --symref >actual &&
>   	test_cmp expect actual

Why is fetch called outside of the test? Its output is shown among the test messages, where it doesn't belong:

    ok 23 - overrides work between mixed transfer/upload-pack hideRefs
    From /home/lsr/src/git/t/trash directory.t5512-ls-remote/
     * [new branch]      master     -> origin/master
    ok 24 - ls-remote --symref
-- >8 --
Subject: [PATCH] t5512: run git fetch inside test

Do the preparatory fetch inside the test of ls-remote --symref to avoid cluttering the test output and to be able to catch unexpected fetch failures.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 t/t5512-ls-remote.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t5512-ls-remote.sh +1 −1
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 83cd35c391..6a949484d0 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -206,8 +206,8 @@ test_expect_success 'overrides work between mixed transfer/upload-pack hideRefs'
 	grep refs/tags/magic actual
 '
 
-git fetch origin
 test_expect_success 'ls-remote --symref' '
+	git fetch origin &&
 	cat >expect <<-EOF &&
 	ref: refs/heads/master	HEAD
 	$(git rev-parse HEAD)	HEAD
-- 
2.17.0
Jeff King· May 12, 2018, 09:55 UTC · re: René Scharfe · lore

Re: [PATCH v14 4/4] ls-remote: create '--sort' option

On Sat, May 12, 2018 at 10:45:23AM +0200, René Scharfe wrote:
Show 7 quoted lines
> Why is fetch called outside of the test?  Its output is shown among the
> test messages, where it doesn't belong:
> 
>     ok 23 - overrides work between mixed transfer/upload-pack hideRefs
>     From /home/lsr/src/git/t/trash directory.t5512-ls-remote/
>      * [new branch]      master     -> origin/master
>     ok 24 - ls-remote --symref

Heh, I just noticed this yesterday, too, but ran out of time before writing up the patch.

Show 6 quoted lines
> -- >8 --
> Subject: [PATCH] t5512: run git fetch inside test
> 
> Do the preparatory fetch inside the test of ls-remote --symref to avoid
> cluttering the test output and to be able to catch unexpected fetch
> failures.
Yep, this looks obviously correct.
-Peff

← back to recent threads