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

[Jeff King]

On Mon, Apr 02, 2018 at 10:07:36PM +0200, Harald Nordgren wrote:

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

We don't allow mixed declarations and code; this line needs to go at the
top of the block.

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

This will grow by one each time, which ends up doing quadratic work in
the number of items (although it can often be less if the realloc is
able to just extend the block). It should probably use ALLOC_GROW()
to move it more aggressively.

Interestingly, the ref-filter code itself seems to have the same
problem, but I couldn't measure any speedup. So either my analysis is
totally wrong, or we end up reusing the block most of the time in
practice.

> +
> + if (sorting) {
> + ref_array_sort(sorting, );
> + }

Minor style nit: we usually omit the braces for a one-liner block.

The ref-filter code lets you sort on any arbitrary field. So you could
do:

  git ls-remote --sort=committerdate

What should that do?

With your patch, I think we'll just get something like:

  fatal: missing object 468165c1d8a442994a825f3684528361727cd8c0 for HEAD

which is perhaps the best we can do (it might even work if you've
recently fetched from the other side).

-Peff

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

[Harald Nordgren]

I shortened the commit message. Since we already have this feature for
other sub-commands I guess I don't have to explain what version
semantics are.

Have to say I'm a bit confused about 'git send-mail'. This time I tried

git send-email --subject-prefix="PATCH v4"
--in-reply-to='20180402005248.52418-1-haraldnordg...@gmail.com'
--compose -1

...and the supplied 'git@vger.kernel.org' on the prompt. hopefully
everyone received the latest patches. However, at least what it looks
like from Gmail / Google Inbox, the discussion is now split into two
threads. Hopefully the patches still make sense though.

I will post replies to the original email chain after this message.

On Mon, Apr 2, 2018 at 10:12 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Harald Nordgren <haraldnordg...@gmail.com> writes:
>
>> Subject: Re: [PATCH] ls-remote: create '--sort' option
>
> It would be more helpful to mark as [PATCH v2] a rerolled patch like
> this.
>
>> Create a '--sort' option for ls-remote. This is useful e.g. for the Go
>> repository after the release of version 1.10, where by default v1.10 is
>> sorted before v1.2. See:
>>
>>   $ git ls-remote -t https://go.googlesource.com/go
>
> Does this sample command line also need updating for the refined
> design?
>
>>   ...
>>   205f850ceacfc39d1e9d76a9569416284594ce8crefs/tags/go1.1
>>   d260448f6b6ac10efe4ae7f6dfe944e72bc2a676refs/tags/go1.1.1
>>   1d6d8fca241bb611af51e265c1b5a2e9ae904702refs/tags/go1.1.2
>>   bf86aec25972f3a100c3aa58a6abcbcc35bdea49refs/tags/go1.10
>>   ac7c0ee26dda18076d5f6c151d8f920b43340ae3refs/tags/go1.10.1
>> ...
>> + git ls-remote --sort="-version:refname" --tags self >actual &&
>> + test_cmp expect actual
>> +...
>> + git ls-remote --sort="-refname" --tags self >actual &&
>
> These both look sensible (also the variant without the dash prefix
> which I omitted from the quote).
>
>
>

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

[Junio C Hamano]

Harald Nordgren <haraldnordg...@gmail.com> writes:

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

It would be more helpful to mark as [PATCH v2] a rerolled patch like
this.

> Create a '--sort' option for ls-remote. This is useful e.g. for the Go
> repository after the release of version 1.10, where by default v1.10 is
> sorted before v1.2. See:
>
>   $ git ls-remote -t https://go.googlesource.com/go

Does this sample command line also need updating for the refined
design?

>   ...
>   205f850ceacfc39d1e9d76a9569416284594ce8crefs/tags/go1.1
>   d260448f6b6ac10efe4ae7f6dfe944e72bc2a676refs/tags/go1.1.1
>   1d6d8fca241bb611af51e265c1b5a2e9ae904702refs/tags/go1.1.2
>   bf86aec25972f3a100c3aa58a6abcbcc35bdea49refs/tags/go1.10
>   ac7c0ee26dda18076d5f6c151d8f920b43340ae3refs/tags/go1.10.1
> ...
> + git ls-remote --sort="-version:refname" --tags self >actual &&
> + test_cmp expect actual
> +...
> + git ls-remote --sort="-refname" --tags self >actual &&

These both look sensible (also the variant without the dash prefix
which I omitted from the quote).

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

[Harald Nordgren]

Create a '--sort' option for ls-remote. This is useful e.g. for the Go
repository after the release of version 1.10, where by default v1.10 is
sorted before v1.2. See:

$ git ls-remote -t https://go.googlesource.com/go
...
205f850ceacfc39d1e9d76a9569416284594ce8crefs/tags/go1.1
d260448f6b6ac10efe4ae7f6dfe944e72bc2a676refs/tags/go1.1.1
1d6d8fca241bb611af51e265c1b5a2e9ae904702refs/tags/go1.1.2
bf86aec25972f3a100c3aa58a6abcbcc35bdea49refs/tags/go1.10
ac7c0ee26dda18076d5f6c151d8f920b43340ae3refs/tags/go1.10.1
9ce6b5c2ed5d3d5251b9a6a0c548d5fb2c8567e8refs/tags/go1.10beta1
594668a5a96267a46282ce3007a584ec07adf705refs/tags/go1.10beta2
5348aed83e39bd1d450d92d7f627e994c2db6ebfrefs/tags/go1.10rc1
20e228f2fdb44350c858de941dff4aea9f3127b8refs/tags/go1.10rc2
1c5438aae896edcd1e9f9618f4776517f08053b3refs/tags/go1.1rc2
46a6097aa7943a490e9bd2e04274845d0e5e200frefs/tags/go1.1rc3
402d3590b54e4a0df9fb51ed14b2999e85ce0b76refs/tags/go1.2
9c9802fad57c1bcb72ea98c5c55ea2652efc5772refs/tags/go1.2.1
...

Signed-off-by: Harald Nordgren 
---
 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=]
- [-q | --quiet] [--exit-code] [--get-url]
+ [-q | --quiet] [--exit-code] [--get-url] [--sort=]
  [--symref] [ [...]]
 
 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=::
+   Sort based on the key given.  Prefix `-` to sort in
+   descending order of the value. You may use the --sort= 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`.
+
 ::
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 = 
 
struct option options[] = {
OPT__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", , N_("do not show peeled tags"), 
REF_NORMAL),
OPT_BOOL(0, "get-url", _url,
 N_("take url..insteadOf into account")),
+   OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
+N_("field name to sort on"), 
_opt_ref_sorting),
OPT_SET_INT_F(0, "exit-code", ,
  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(, 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) {
+