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

[Eric Sunshine]

On Sun, Apr 8, 2018 at 6:16 PM, Junio C Hamano  wrote:
> Harald Nordgren  writes:
>> +--sort=::
>> + Sort based on the key given. [...]
>
> This is a tangent but I suspect that the description of --sort in
> git-for-each-ref documentation is wrong on the use of multiple
> options, or I am misreading parse_opt_ref_sorting(), which I think
> appends later keys to the end of the list, and compare_refs() which
> I think yields results when an earlier key in the last decides two
> are not equal and ignores the later keys.  The commit that added the
> description was c0f6dc9b ("git-for-each-ref.txt: minor
> improvements", 2008-06-05), and I think even back then the code in
> builtin-for-each-ref.c appended later keys at the end, and it seems
> nobody complained, so it is possible I am not reading the code right
> before fully adjusting to timezone change ;-)

I just re-read parse_opt_ref_sorting(), and I'm pretty sure that it is
_prepending_ keys to the list, despite the confusingly named variable
"sorting_tail", thus it agrees with the documentation that
--sort= specified last becomes primary sort key.

For me, at least, that behavior (last --sort= becomes primary key)
feels counterintuitive, but it's too late to change it now.

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

[Junio C Hamano]

Harald Nordgren  writes:

>> It is not too big a deal, but I find that liberal sprinkling of
>> UNLEAK() is somewhat unsightly.  From the code we leave with this
>> change, it is clear what we'd need to do when we make the code fully
>> leak-proof (i.e. we'd have a several lines to free resources held by
>> these two variables here, and then UNLEAK() that appear earlier in
>> the function will become a "goto cleanup;" after setting appropriate
>> value to "status"), so let's not worry about it.
>
> Removed the "extra" unleak annotations within the if-return blocks,
> but kept them at the end.

I actually think that is worse.  The UNLEAK()s near early-return
serve to remind us: "here we are making early return and when we
properly plug the leak, here is one of the places we need to fix".
Please keep them, unless (and I do not recommend to do this) you are
plugging the leaks for real.

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

[Harald Nordgren]

On Mon, Apr 9, 2018 at 12:16 AM, Junio C Hamano  wrote:
>
> I can connect "because it deals only with remotes" and "access to
> the object may fail", but I wonder if we can clarify the former a
> bit better to make it easier to understand for those who are not yet
> familiar with Git.
>
> Keys like `committerdate` that require access to the object will
> not work [***HOW??? Do we get a segfault or something???***] for
> refs whose objects have not yet been fetched from the remote.
>
> I added "for refs whose objects have not yet been fetched", whose
> explicit-ness made "will not work" to be an OK expression, but
> without it I would have suggested to rephrase it to "may not work".
>
> This is a tangent but I suspect that the description of --sort in
> git-for-each-ref documentation is wrong on the use of multiple
> options, or I am misreading parse_opt_ref_sorting(), which I think
> appends later keys to the end of the list, and compare_refs() which
> I think yields results when an earlier key in the last decides two
> are not equal and ignores the later keys.  The commit that added the
> description was c0f6dc9b ("git-for-each-ref.txt: minor
> improvements", 2008-06-05), and I think even back then the code in
> builtin-for-each-ref.c appended later keys at the end, and it seems
> nobody complained, so it is possible I am not reading the code right
> before fully adjusting to timezone change ;-)

Updated the docs.

>
> It is not too big a deal, but I find that liberal sprinkling of
> UNLEAK() is somewhat unsightly.  From the code we leave with this
> change, it is clear what we'd need to do when we make the code fully
> leak-proof (i.e. we'd have a several lines to free resources held by
> these two variables here, and then UNLEAK() that appear earlier in
> the function will become a "goto cleanup;" after setting appropriate
> value to "status"), so let's not worry about it.

Removed the "extra" unleak annotations within the if-return blocks,
but kept them at the end.

> Please do *NOT* step outside the pair of single quotes that protects
> the body of the test scriptlet and execute commands like "git
> rev-parse".  These execute in order to prepare the command line
> argument strings BEFORE test_expect_success can run (or the test
> framework decides if the test will be skipped via GIT_TEST_SKIP).
>
> Instead do it like so:
>
> cat >expect <<-EOF &&
> $(git rev-parse mark)   refs/tags/mark
> $(git rev-parse mark1.1)refs/tags/mark1.1
> $(git rev-parse mark1.2)refs/tags/mark1.2
> $(git rev-parse mark1.10)   refs/tags/mark1.10
> EOF
>
> I.e. the end token for << that is not quoted allows $var and $(cmd)
> to be substituted.
>
> Same comment applies throughout the remainder of this file.

Ah, thanks! I was looking for some way to allow the values to expand
within the proper test context.

So turns out '<<-\EOF' vs '<<-EOF' makes all the difference :)

> Other than that, this patch was a very pleasant read.  Thanks.

Thank you!

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

[Junio C Hamano]

Harald Nordgren  writes:

> +--sort=::
> + Sort based on the key given. Prefix `-` to sort in descending order
> + of the value. 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.
> + See linkgit:git-for-each-ref[1] for more sort options, but be aware
> + that because `ls-remote` deals only with remotes, any key like
> + `committerdate` that requires access to the object itself will
> + cause a failure.

I can connect "because it deals only with remotes" and "access to
the object may fail", but I wonder if we can clarify the former a
bit better to make it easier to understand for those who are not yet
familiar with Git.  

Keys like `committerdate` that require access to the object will
not work [***HOW??? Do we get a segfault or something???***] for
refs whose objects have not yet been fetched from the remote.

I added "for refs whose objects have not yet been fetched", whose
explicit-ness made "will not work" to be an OK expression, but
without it I would have suggested to rephrase it to "may not work".

This is a tangent but I suspect that the description of --sort in
git-for-each-ref documentation is wrong on the use of multiple
options, or I am misreading parse_opt_ref_sorting(), which I think
appends later keys to the end of the list, and compare_refs() which
I think yields results when an earlier key in the last decides two
are not equal and ignores the later keys.  The commit that added the
description was c0f6dc9b ("git-for-each-ref.txt: minor
improvements", 2008-06-05), and I think even back then the code in
builtin-for-each-ref.c appended later keys at the end, and it seems
nobody complained, so it is possible I am not reading the code right
before fully adjusting to timezone change ;-)

> + for (i = 0; i < ref_array.nr; i++) {
> + const struct ref_array_item *ref = ref_array.items[i];
>   if (show_symref_target && ref->symref)
> - printf("ref: %s\t%s\n", ref->symref, ref->name);
> - printf("%s\t%s\n", oid_to_hex(>old_oid), ref->name);
> + printf("ref: %s\t%s\n", ref->symref, ref->refname);
> + printf("%s\t%s\n", oid_to_hex(>objectname), ref->refname);
>   status = 0; /* we found something */
>   }
> +
> + UNLEAK(sorting);
> + UNLEAK(ref_array);
>   return status;
>  }

It is not too big a deal, but I find that liberal sprinkling of
UNLEAK() is somewhat unsightly.  From the code we leave with this
change, it is clear what we'd need to do when we make the code fully
leak-proof (i.e. we'd have a several lines to free resources held by
these two variables here, and then UNLEAK() that appear earlier in
the function will become a "goto cleanup;" after setting appropriate
value to "status"), so let's not worry about it.

> +test_expect_success 'ls-remote --sort="version:refname" --tags self' '
> + cat >expect <<-\EOF &&
> + '$(git rev-parse mark)' refs/tags/mark
> + '$(git rev-parse mark1.1)'  refs/tags/mark1.1
> + '$(git rev-parse mark1.2)'  refs/tags/mark1.2
> + '$(git rev-parse mark1.10)' refs/tags/mark1.10
> + EOF

Please do *NOT* step outside the pair of single quotes that protects
the body of the test scriptlet and execute commands like "git
rev-parse".  These execute in order to prepare the command line
argument strings BEFORE test_expect_success can run (or the test
framework decides if the test will be skipped via GIT_TEST_SKIP).

Instead do it like so:

cat >expect <<-EOF &&
$(git rev-parse mark)   refs/tags/mark
$(git rev-parse mark1.1)refs/tags/mark1.1
$(git rev-parse mark1.2)refs/tags/mark1.2
$(git rev-parse mark1.10)   refs/tags/mark1.10
EOF

I.e. the end token for << that is not quoted allows $var and $(cmd)
to be substituted.

Same comment applies throughout the remainder of this file.

Other than that, this patch was a very pleasant read.  Thanks.

[PATCH v12 4/4] ls-remote: create '--sort' option

[Harald Nordgren]

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

Notes:
Changes according to Eric Sunshine's code review

 Documentation/git-ls-remote.txt | 16 -
 builtin/ls-remote.c | 30 +---
 t/t5512-ls-remote.sh| 52 -
 3 files changed, 88 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 5f2628c8f..80a09b518 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. 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.
+   See linkgit:git-for-each-ref[1] for more sort options, but be aware
+   that because `ls-remote` deals only with remotes, any key like
+   `committerdate` that requires access to the object itself will
+   cause a failure.
+
 ::
The "remote" repository to query.  This parameter can be
either a URL or the name of a remote (see the GIT URLS and
@@ -90,6 +100,10 @@ EXAMPLES
c5db5456ae3b0873fc659c19fafdde22313cc441refs/tags/v0.99.2
7ceca275d047c90c0c7d5afb13ab97efdf51bd6erefs/tags/v0.99.3
 
+SEE ALSO
+
+linkgit:git-check-ref-format[1].
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 540d56429..d3851074c 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[] = {
@@ -43,10 +44,13 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
int show_symref_target = 0;
const char *uploadpack = NULL;
const char **pattern = NULL;
+   int i;
 
struct remote *remote;
struct transport *transport;
const struct ref *ref;
+   struct ref_array ref_array;
+   static struct ref_sorting *sorting = NULL, **sorting_tail = 
 
struct option options[] = {
OPT__QUIET(, N_("do not print remote URL")),
@@ -60,6 +64,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 +74,8 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
OPT_END()
};
 
+   memset(_array, 0, sizeof(ref_array));
+
argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
 PARSE_OPT_STOP_AT_NON_OPTION);
dest = argv[0];
@@ -90,6 +98,7 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
 
if (get_url) {
printf("%s\n", *remote->url);
+   UNLEAK(sorting);
return 0;
}
 
@@ -98,20 +107,35 @@ int cmd_ls_remote(int argc, const char **argv, const char 
*prefix)
transport_set_option(transport, TRANS_OPT_UPLOADPACK, 
uploadpack);
 
ref = transport_get_remote_refs(transport);
-   if (transport_disconnect(transport))
+   if (transport_disconnect(transport)) {
+   UNLEAK(sorting);
return 1;
+   }
 
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;
+   item = ref_array_push(_array, ref->name, >old_oid);
+   item->symref = xstrdup_or_null(ref->symref);
+