subject:"\[PATCH v7\] ls\-remote\: create '\-\-sort' option"

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

2018-04-04 Thread Jeff King

On Wed, Apr 04, 2018 at 07:18:42PM +0200, Harald Nordgren wrote:

> Jeff, you are right that 'git ls-remote --sort=committerdate' will not
> work. Do you think we need to do something about this, or it's fine
> that it fails the way you showed?

It's a reasonable-sized footgun, but one that I think most people would
be unlikely to trigger. I think it's probably OK as long as we warn the
user in the documentation (see my other response).

-Peff

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

2018-04-04 Thread Jeff King

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=::
> + Sort based on the key given.  Prefix `-` to sort in
> + descending order of the value. You may use the --sort= 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(>objectname, >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(, ref->name, >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

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

2018-04-04 Thread Harald Nordgren

Links to previous revisions:

[1] 
https://public-inbox.org/git/20180402174614.ga28...@sigill.intra.peff.net/T/#m108fe8c83f3558afaea8e317e680f7eaa136e9a9
[2] 
https://public-inbox.org/git/20180402211920.ga32...@sigill.intra.peff.net/T/#ma9ec4e0ce664160086e535c012e20d76822c60e5
...
[4] 
https://public-inbox.org/git/20180402174614.ga28...@sigill.intra.peff.net/T/#maa02c40c87b192e56c370c312098d469c9fce757
[5] 
https://public-inbox.org/git/20180402174614.ga28...@sigill.intra.peff.net/T/#m52cda3f359e1257e7bdfe19cd9a26f55fa20
[6] 
https://public-inbox.org/git/20180402174614.ga28...@sigill.intra.peff.net/T/#m6d3ce17f0f6dabeddaf03336c92512b7c413422b

On Wed, Apr 4, 2018 at 7:18 PM, Harald Nordgren
 wrote:
> I updated the code to use 'ALLOC_GROW'. I makes sense, I now I realize
> why array.alloc is there ;)
>
> Jeff, you are right that 'git ls-remote --sort=committerdate' will not
> work. Do you think we need to do something about this, or it's fine
> that it fails the way you showed?
>
> On Wed, Apr 4, 2018 at 7:11 PM, Harald Nordgren
>  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 
>> ---
>>
>> Notes:
>> Started using 'ALLOC_GROW'
>>
>>  Documentation/git-ls-remote.txt | 12 +++-
>>  builtin/ls-remote.c | 27 +--
>>  t/t5512-ls-remote.sh| 41 
>> -
>>  3 files changed, 76 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/git-ls-remote.txt 
>> b/Documentation/git-ls-remote.txt
>> index 5f2628c8f..17fae7218 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. You may use the --sort= 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`.
>> +
>>  ::
>> The "remote" repository to query.  This parameter can be
>> either a URL or the name of a remote (see the GIT URLS and
>> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
>> index 540d56429..5bb8ee68a 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 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(, 0, sizeof(array));
>> +
>> argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
>>  PARSE_OPT_STOP_AT_NON_OPTION);
>> dest = argv[0];
>> @@ -104,13 +112,28 @@ int cmd_ls_remote(int argc, const char

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

2018-04-04 Thread Harald Nordgren

I updated the code to use 'ALLOC_GROW'. I makes sense, I now I realize
why array.alloc is there ;)

Jeff, you are right that 'git ls-remote --sort=committerdate' will not
work. Do you think we need to do something about this, or it's fine
that it fails the way you showed?

On Wed, Apr 4, 2018 at 7:11 PM, Harald Nordgren
 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 
> ---
>
> Notes:
> Started using 'ALLOC_GROW'
>
>  Documentation/git-ls-remote.txt | 12 +++-
>  builtin/ls-remote.c | 27 +--
>  t/t5512-ls-remote.sh| 41 
> -
>  3 files changed, 76 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
> index 5f2628c8f..17fae7218 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. You may use the --sort= 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`.
> +
>  ::
> The "remote" repository to query.  This parameter can be
> either a URL or the name of a remote (see the GIT URLS and
> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
> index 540d56429..5bb8ee68a 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 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(, 0, sizeof(array));
> +
> argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
>  PARSE_OPT_STOP_AT_NON_OPTION);
> dest = argv[0];
> @@ -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));
> +   item->symref = ref->symref;
> +   item->objectname = ref->old_oid;
> +
> +   ALLOC_GROW(array.items, array.nr + 1, array.alloc);
> +   array.items[array.nr++] = item;
> +   }
> +
> +   if (sorting)
> +   ref_array_sort(sorting, );
> +
> +   for (i = 0; i < array.nr; i++) {
> +   const

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

2018-04-04 Thread 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:
Started using 'ALLOC_GROW'

 Documentation/git-ls-remote.txt | 12 +++-
 builtin/ls-remote.c | 27 +--
 t/t5512-ls-remote.sh| 41 -
 3 files changed, 76 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 5f2628c8f..17fae7218 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. You may use the --sort= 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`.
+
 ::
The "remote" repository to query.  This parameter can be
either a URL or the name of a remote (see the GIT URLS and
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 540d56429..5bb8ee68a 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 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(, 0, sizeof(array));
+
argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
 PARSE_OPT_STOP_AT_NON_OPTION);
dest = argv[0];
@@ -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));
+   item->symref = ref->symref;
+   item->objectname = ref->old_oid;
+
+   ALLOC_GROW(array.items, array.nr + 1, array.alloc);
+   array.items[array.nr++] = item;
+   }
+
+   if (sorting)
+   ref_array_sort(sorting, );
+
+   for (i = 0; i < array.nr; i++) {
+   const struct ref_array_item *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 */
}
return status;
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 02106c922..66370cd88 100755
---

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

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

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

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

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

5 matches

Site Navigation

Mail list logo

Footer information