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

[Jeff King <peff@peff.net>]

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