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

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

[Harald Nordgren]

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

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)

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

[Ævar Arnfjörð Bjarmason]

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?

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

[Harald Nordgren]

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

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

[Ævar Arnfjörð Bjarmason]

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?

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

[Harald Nordgren]

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

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

[Jeff King]

On Mon, Apr 02, 2018 at 06:26:49PM +0200, Harald Nordgren wrote:

> 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

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

[Junio C Hamano]

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

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

[Harald Nordgren]

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

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

[Harald Nordgren]

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

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)

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

[Harald Nordgren]

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

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)

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

[Eric Sunshine]

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

>  Documentation/git-ls-remote.txt | 12 +++++++++++-
>  builtin/ls-remote.c             | 26 ++++++++++++++++++++++++--
>  t/t5512-ls-remote.sh            | 41 ++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 75 insertions(+), 4 deletions(-)

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

[Eric Sunshine]

On Mon, Apr 2, 2018 at 6:53 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 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/

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

[Harald Nordgren]

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

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)

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

[Harald Nordgren]

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)

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

[Harald Nordgren]

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

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

[Harald Nordgren]

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

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

[Jeff King]

On Wed, Apr 04, 2018 at 07:11:53PM +0200, Harald Nordgren wrote:

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

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

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

[Jeff King]

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

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

[Harald Nordgren]

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)

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

[Harald Nordgren]

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

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

[Harald Nordgren]

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

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)

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

[Harald Nordgren]

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

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)

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

[Jeff King]

On Thu, Apr 05, 2018 at 01:11:34AM +0200, Harald Nordgren wrote:

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

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

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

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

[Jeff King]

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

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

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

[Jeff King]

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

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

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

[Jeff King]

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  */
-- 
2.17.0.686.g08b0810b04

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

[Derrick Stolee]

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>

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

[Harald Nordgren]

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

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

[Harald Nordgren]

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

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)

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

[Harald Nordgren]

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

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)

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

[Harald Nordgren]

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

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)

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

[Harald Nordgren]

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

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)

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

[Eric Sunshine]

On Sat, Apr 7, 2018 at 12:42 PM, Harald Nordgren
<haraldnordgren@gmail.com> wrote:
> 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/

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

[Eric Sunshine]

On Sat, Apr 7, 2018 at 12:42 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>
> ---
> 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?

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

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

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

> +       }
> 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/

> +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?)

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

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

[Harald Nordgren]

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.

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

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

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

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

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

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

[Harald Nordgren]

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

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)

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

[Harald Nordgren]

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)

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

[Harald Nordgren]

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

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)

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

[Harald Nordgren]

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

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)

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

[Junio C Hamano]

Harald Nordgren <haraldnordgren@gmail.com> writes:

> +--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 ;-)

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

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

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

[Junio C Hamano]

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.

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.

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

[Harald Nordgren]

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

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)

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

[Harald Nordgren]

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)

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

[Harald Nordgren]

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

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)

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

[Harald Nordgren]

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

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)

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

[Harald Nordgren]

On Mon, Apr 9, 2018 at 12:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.

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

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

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

[Harald Nordgren]

On Mon, Apr 9, 2018 at 12:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.

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

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

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

[Junio C Hamano]

Harald Nordgren <haraldnordgren@gmail.com> writes:

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

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

[Junio C Hamano]

Harald Nordgren <haraldnordgren@gmail.com> writes:

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

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



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

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

[Harald Nordgren]

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

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)

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

[Harald Nordgren]

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)

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

[Harald Nordgren]

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

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)

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

[Harald Nordgren]

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

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)

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

[Harald Nordgren]

On Mon, Apr 9, 2018 at 2:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.

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

[Eric Sunshine]

On Sun, Apr 8, 2018 at 6:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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

[Jeff King]

On Mon, Apr 09, 2018 at 08:18:35AM +0900, Junio C Hamano wrote:

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

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

> 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

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

[Harald Nordgren]

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

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

[Stefan Beller]

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:

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

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

[Todd Zullinger]

Hi Stefan,

Stefan Beller wrote:
> 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"

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

[Eric Sunshine]

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

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

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

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

[René Scharfe]

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

> @@ -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(-)

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

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

[Jeff King]

On Sat, May 12, 2018 at 10:45:23AM +0200, René Scharfe wrote:

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

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