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

From: Jeff King <peff@peff.net>
Date: Apr 4, 2018, 18:55 UTC
Message-ID: <20180404185518.GD9055@sigill.intra.peff.net>
In-Reply-To: <20180404171153.4126-1-haraldnordgren@gmail.com>

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

Show 13 quoted lines

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

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

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

Show 11 quoted lines

> @@ -104,13 +112,28 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>  	if (!dest && !quiet)
>  		fprintf(stderr, "From %s\n", *remote->url);
>  	for ( ; ref; ref = ref->next) {
> +		struct ref_array_item *item;
>  		if (!check_ref_type(ref, flags))
>  			continue;
>  		if (!tail_match(pattern, ref->name))
>  			continue;
> +
> +		FLEX_ALLOC_MEM(item, refname, ref->name, strlen(ref->name));

I think this can use the slightly-simpler FLEX_ALLOC_STR().

> +		item->symref = ref->symref;

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

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

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

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

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

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

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

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

-Peff