On Sat, Apr 7, 2018 at 12:42 PM, Harald Nordgren
<haraldnordgren@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxx/
> +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?
