Re: [PATCH v11 4/4] ls-remote: create '--sort' option
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Apr 8, 2018, 01:48 UTC
- Message-ID
- <CAPig+cQnkvs8uPU-=woEzYC=mu1Bbi2u_TgH60aVUyh+m3DKDA@mail.gmail.com>
- In-Reply-To
- <20180407164250.54303-4-haraldnordgren@gmail.com>
On Sat, Apr 7, 2018 at 12:42 PM, Harald Nordgren <haraldnordgren@gmail.com> wrote:
Show 12 quoted lines
> 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?
Show 7 quoted lines
> + 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...Show 6 quoted lines
> 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.
Show 9 quoted lines
> @@ -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.
Show 13 quoted lines
> + } > 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/
Show 21 quoted lines
> +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?)
Show 7 quoted lines
> @@ -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?