Re: [PATCH] ls-remote: create option to sort by versions

[Harald Nordgren]

Thanks for all the discussion!

I think I figured out a way to reuse more ref-filter.c machinery. I
will submit another patchset shortly.

On Mon, Apr 2, 2018 at 8:32 PM, Junio C Hamano  wrote:
> Ævar Arnfjörð Bjarmason  writes:
>
>> This is a sensible thing to want, but why not follow the UI we have for
>> this with git-tag? I.e. --sort= & -i (or --ignore-case)? Of course
>> ls-remote doesn't just show tags, so maybe we'd want --tag-sort=
>> and --ignore-tag-case or something, but the rest should be equivalent,
>> no?
>
> Yeah, and if we can reuse more of ref-filter.c machinery (which was
> factored out of for-each-ref and tag you suggested borrows from),
> that would be even better.  In the context of ls-remote, however, we
> cannot inspect the object (we typically do not have them yet), so it
> may not be practical, but I agree with your suggestion---we should
> match the behaviour at the UI level at least when we can.
>

Re: [PATCH] ls-remote: create option to sort by versions

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> This is a sensible thing to want, but why not follow the UI we have for
> this with git-tag? I.e. --sort= & -i (or --ignore-case)? Of course
> ls-remote doesn't just show tags, so maybe we'd want --tag-sort=
> and --ignore-tag-case or something, but the rest should be equivalent,
> no?

Yeah, and if we can reuse more of ref-filter.c machinery (which was
factored out of for-each-ref and tag you suggested borrows from),
that would be even better.  In the context of ls-remote, however, we
cannot inspect the object (we typically do not have them yet), so it
may not be practical, but I agree with your suggestion---we should
match the behaviour at the UI level at least when we can.

Re: [PATCH] ls-remote: create option to sort by versions

[Jeff King]

On Mon, Apr 02, 2018 at 06:26:49PM +0200, Harald Nordgren wrote:

> It would be nice to have a uniform option like
> '--sort=version:refname'. But spending a few hours to look over the
> code, it seems that ls-remote.c would require a lot of rewrites if we
> wanted to start using `ref_array` and `ref_array_item` for storing the
> refs.
> 
> Which seems necessary in order to hook in to the sorting flow used in
> other subcommands. That, or reimplement `cmp_ref_sorting`. But maybe
> I'm missing something?

I haven't looked at how painful it might be to use ref-filter.c, but it
would buy us even more if we could. That would open up other options
like --format, I think (OTOH there may be some funny corner cases; that
code assumes we're talking about local refs, so if you were to ask for
"%(committerdate)" or something, we might have to more cleanly handle
the case where we don't actually have the object).

-Peff

Re: [PATCH] ls-remote: create option to sort by versions

[Harald Nordgren]

Both points make sense and it sounds like a very pragmatic approach.
I'll look into it!

On Mon, Apr 2, 2018 at 7:32 PM, Ævar Arnfjörð Bjarmason
 wrote:
>
> On Mon, Apr 02 2018, Harald Nordgren wrote:
>
>> In regards the the print statement, it was only moved down according
>> to the diff because I added more logic above. Basically there is 1)
>> the unrolling of the linked list to an array and 2) the printing
>> logic. I could move it and make the diff smaller, but that probably
>> makes the code a tiny bit more complicated.
>
> I was just wondering since it wasn't explained in the commit message,
> makes sense to copy this explanation into v2, or lead with a purely code
> re-arrangement patch.
>
>> It would be nice to have a uniform option like
>> '--sort=version:refname'. But spending a few hours to look over the
>> code, it seems that ls-remote.c would require a lot of rewrites if we
>> wanted to start using `ref_array` and `ref_array_item` for storing the
>> refs.
>>
>> Which seems necessary in order to hook in to the sorting flow used in
>> other subcommands. That, or reimplement `cmp_ref_sorting`. But maybe
>> I'm missing something?
>
> I'm thinking just in terms of UI. If it's the case that porting this to
> whatever guts git-tag uses for sorting would be hard, then we could
> still use the same command-line option convention (and perhaps just die
> if anything except --sort=version:refname is supplied). Changing the
> underlying implementation is easier than cleaning up UI-differences that
> (seemingly) only arose due to underlying implementation details at the
> time.
>
>> On Mon, Apr 2, 2018 at 8:37 AM, Ævar Arnfjörð Bjarmason
>>  wrote:
>>>
>>> On Mon, Apr 02 2018, Harald Nordgren wrote:
>>>
 Create the options '-V ' and '--version-sort' to sort
 'git ls-remote' output by version semantics. This is useful e.g. for
 the Go repository after the release of version 1.10, where otherwise
 v1.10 is sorted before v1.2. See:

   $ git ls-remote -t https://go.googlesource.com/go
   ...
   205f850ceacfc39d1e9d76a9569416284594ce8crefs/tags/go1.1
   d260448f6b6ac10efe4ae7f6dfe944e72bc2a676refs/tags/go1.1.1
   1d6d8fca241bb611af51e265c1b5a2e9ae904702refs/tags/go1.1.2
   bf86aec25972f3a100c3aa58a6abcbcc35bdea49refs/tags/go1.10
   ac7c0ee26dda18076d5f6c151d8f920b43340ae3refs/tags/go1.10.1
   9ce6b5c2ed5d3d5251b9a6a0c548d5fb2c8567e8refs/tags/go1.10beta1
   594668a5a96267a46282ce3007a584ec07adf705refs/tags/go1.10beta2
   5348aed83e39bd1d450d92d7f627e994c2db6ebfrefs/tags/go1.10rc1
   20e228f2fdb44350c858de941dff4aea9f3127b8refs/tags/go1.10rc2
   1c5438aae896edcd1e9f9618f4776517f08053b3refs/tags/go1.1rc2
   46a6097aa7943a490e9bd2e04274845d0e5e200frefs/tags/go1.1rc3
   402d3590b54e4a0df9fb51ed14b2999e85ce0b76refs/tags/go1.2
   9c9802fad57c1bcb72ea98c5c55ea2652efc5772refs/tags/go1.2.1
   ...
>>>
>>> This is a sensible thing to want, but why not follow the UI we have for
>>> this with git-tag? I.e. --sort= & -i (or --ignore-case)? Of course
>>> ls-remote doesn't just show tags, so maybe we'd want --tag-sort=
>>> and --ignore-tag-case or something, but the rest should be equivalent,
>>> no?
>>>
 [...]
 @@ -101,13 +115,22 @@ int cmd_ls_remote(int argc, const char **argv, const 
 char *prefix)
   if (transport_disconnect(transport))
   return 1;

 - if (!dest && !quiet)
 - fprintf(stderr, "From %s\n", *remote->url);
   for ( ; ref; ref = ref->next) {
   if (!check_ref_type(ref, flags))
   continue;
   if (!tail_match(pattern, ref->name))
   continue;
 + REALLOC_ARRAY(refs, nr + 1);
 + refs[nr++] = ref;
 + }
 +
 + if (version_sort)
 + QSORT(refs, nr, cmp_ref_versions);
 +
 + if (!dest && !quiet)
 + fprintf(stderr, "From %s\n", *remote->url);
>>>
>>> Is there some subtlety here I'm missing which means that when sorting
>>> we'd now need to print this "From" line later (i.e. after sorting?

Re: [PATCH] ls-remote: create option to sort by versions

[Ævar Arnfjörð Bjarmason]

On Mon, Apr 02 2018, Harald Nordgren wrote:

> In regards the the print statement, it was only moved down according
> to the diff because I added more logic above. Basically there is 1)
> the unrolling of the linked list to an array and 2) the printing
> logic. I could move it and make the diff smaller, but that probably
> makes the code a tiny bit more complicated.

I was just wondering since it wasn't explained in the commit message,
makes sense to copy this explanation into v2, or lead with a purely code
re-arrangement patch.

> It would be nice to have a uniform option like
> '--sort=version:refname'. But spending a few hours to look over the
> code, it seems that ls-remote.c would require a lot of rewrites if we
> wanted to start using `ref_array` and `ref_array_item` for storing the
> refs.
>
> Which seems necessary in order to hook in to the sorting flow used in
> other subcommands. That, or reimplement `cmp_ref_sorting`. But maybe
> I'm missing something?

I'm thinking just in terms of UI. If it's the case that porting this to
whatever guts git-tag uses for sorting would be hard, then we could
still use the same command-line option convention (and perhaps just die
if anything except --sort=version:refname is supplied). Changing the
underlying implementation is easier than cleaning up UI-differences that
(seemingly) only arose due to underlying implementation details at the
time.

> On Mon, Apr 2, 2018 at 8:37 AM, Ævar Arnfjörð Bjarmason
>  wrote:
>>
>> On Mon, Apr 02 2018, Harald Nordgren wrote:
>>
>>> Create the options '-V ' and '--version-sort' to sort
>>> 'git ls-remote' output by version semantics. This is useful e.g. for
>>> the Go repository after the release of version 1.10, where otherwise
>>> v1.10 is sorted before v1.2. See:
>>>
>>>   $ git ls-remote -t https://go.googlesource.com/go
>>>   ...
>>>   205f850ceacfc39d1e9d76a9569416284594ce8crefs/tags/go1.1
>>>   d260448f6b6ac10efe4ae7f6dfe944e72bc2a676refs/tags/go1.1.1
>>>   1d6d8fca241bb611af51e265c1b5a2e9ae904702refs/tags/go1.1.2
>>>   bf86aec25972f3a100c3aa58a6abcbcc35bdea49refs/tags/go1.10
>>>   ac7c0ee26dda18076d5f6c151d8f920b43340ae3refs/tags/go1.10.1
>>>   9ce6b5c2ed5d3d5251b9a6a0c548d5fb2c8567e8refs/tags/go1.10beta1
>>>   594668a5a96267a46282ce3007a584ec07adf705refs/tags/go1.10beta2
>>>   5348aed83e39bd1d450d92d7f627e994c2db6ebfrefs/tags/go1.10rc1
>>>   20e228f2fdb44350c858de941dff4aea9f3127b8refs/tags/go1.10rc2
>>>   1c5438aae896edcd1e9f9618f4776517f08053b3refs/tags/go1.1rc2
>>>   46a6097aa7943a490e9bd2e04274845d0e5e200frefs/tags/go1.1rc3
>>>   402d3590b54e4a0df9fb51ed14b2999e85ce0b76refs/tags/go1.2
>>>   9c9802fad57c1bcb72ea98c5c55ea2652efc5772refs/tags/go1.2.1
>>>   ...
>>
>> This is a sensible thing to want, but why not follow the UI we have for
>> this with git-tag? I.e. --sort= & -i (or --ignore-case)? Of course
>> ls-remote doesn't just show tags, so maybe we'd want --tag-sort=
>> and --ignore-tag-case or something, but the rest should be equivalent,
>> no?
>>
>>> [...]
>>> @@ -101,13 +115,22 @@ int cmd_ls_remote(int argc, const char **argv, const 
>>> char *prefix)
>>>   if (transport_disconnect(transport))
>>>   return 1;
>>>
>>> - if (!dest && !quiet)
>>> - fprintf(stderr, "From %s\n", *remote->url);
>>>   for ( ; ref; ref = ref->next) {
>>>   if (!check_ref_type(ref, flags))
>>>   continue;
>>>   if (!tail_match(pattern, ref->name))
>>>   continue;
>>> + REALLOC_ARRAY(refs, nr + 1);
>>> + refs[nr++] = ref;
>>> + }
>>> +
>>> + if (version_sort)
>>> + QSORT(refs, nr, cmp_ref_versions);
>>> +
>>> + if (!dest && !quiet)
>>> + fprintf(stderr, "From %s\n", *remote->url);
>>
>> Is there some subtlety here I'm missing which means that when sorting
>> we'd now need to print this "From" line later (i.e. after sorting?

Re: [PATCH] ls-remote: create option to sort by versions

[Harald Nordgren]

Thanks for your comment Ævar!

In regards the the print statement, it was only moved down according
to the diff because I added more logic above. Basically there is 1)
the unrolling of the linked list to an array and 2) the printing
logic. I could move it and make the diff smaller, but that probably
makes the code a tiny bit more complicated.

It would be nice to have a uniform option like
'--sort=version:refname'. But spending a few hours to look over the
code, it seems that ls-remote.c would require a lot of rewrites if we
wanted to start using `ref_array` and `ref_array_item` for storing the
refs.

Which seems necessary in order to hook in to the sorting flow used in
other subcommands. That, or reimplement `cmp_ref_sorting`. But maybe
I'm missing something?

On Mon, Apr 2, 2018 at 8:37 AM, Ævar Arnfjörð Bjarmason
 wrote:
>
> On Mon, Apr 02 2018, Harald Nordgren wrote:
>
>> Create the options '-V ' and '--version-sort' to sort
>> 'git ls-remote' output by version semantics. This is useful e.g. for
>> the Go repository after the release of version 1.10, where otherwise
>> v1.10 is sorted before v1.2. See:
>>
>>   $ git ls-remote -t https://go.googlesource.com/go
>>   ...
>>   205f850ceacfc39d1e9d76a9569416284594ce8crefs/tags/go1.1
>>   d260448f6b6ac10efe4ae7f6dfe944e72bc2a676refs/tags/go1.1.1
>>   1d6d8fca241bb611af51e265c1b5a2e9ae904702refs/tags/go1.1.2
>>   bf86aec25972f3a100c3aa58a6abcbcc35bdea49refs/tags/go1.10
>>   ac7c0ee26dda18076d5f6c151d8f920b43340ae3refs/tags/go1.10.1
>>   9ce6b5c2ed5d3d5251b9a6a0c548d5fb2c8567e8refs/tags/go1.10beta1
>>   594668a5a96267a46282ce3007a584ec07adf705refs/tags/go1.10beta2
>>   5348aed83e39bd1d450d92d7f627e994c2db6ebfrefs/tags/go1.10rc1
>>   20e228f2fdb44350c858de941dff4aea9f3127b8refs/tags/go1.10rc2
>>   1c5438aae896edcd1e9f9618f4776517f08053b3refs/tags/go1.1rc2
>>   46a6097aa7943a490e9bd2e04274845d0e5e200frefs/tags/go1.1rc3
>>   402d3590b54e4a0df9fb51ed14b2999e85ce0b76refs/tags/go1.2
>>   9c9802fad57c1bcb72ea98c5c55ea2652efc5772refs/tags/go1.2.1
>>   ...
>
> This is a sensible thing to want, but why not follow the UI we have for
> this with git-tag? I.e. --sort= & -i (or --ignore-case)? Of course
> ls-remote doesn't just show tags, so maybe we'd want --tag-sort=
> and --ignore-tag-case or something, but the rest should be equivalent,
> no?
>
>> [...]
>> @@ -101,13 +115,22 @@ int cmd_ls_remote(int argc, const char **argv, const 
>> char *prefix)
>>   if (transport_disconnect(transport))
>>   return 1;
>>
>> - if (!dest && !quiet)
>> - fprintf(stderr, "From %s\n", *remote->url);
>>   for ( ; ref; ref = ref->next) {
>>   if (!check_ref_type(ref, flags))
>>   continue;
>>   if (!tail_match(pattern, ref->name))
>>   continue;
>> + REALLOC_ARRAY(refs, nr + 1);
>> + refs[nr++] = ref;
>> + }
>> +
>> + if (version_sort)
>> + QSORT(refs, nr, cmp_ref_versions);
>> +
>> + if (!dest && !quiet)
>> + fprintf(stderr, "From %s\n", *remote->url);
>
> Is there some subtlety here I'm missing which means that when sorting
> we'd now need to print this "From" line later (i.e. after sorting?

Re: [PATCH] ls-remote: create option to sort by versions

[Ævar Arnfjörð Bjarmason]

On Mon, Apr 02 2018, Harald Nordgren wrote:

> Create the options '-V ' and '--version-sort' to sort
> 'git ls-remote' output by version semantics. This is useful e.g. for
> the Go repository after the release of version 1.10, where otherwise
> v1.10 is sorted before v1.2. See:
>
>   $ git ls-remote -t https://go.googlesource.com/go
>   ...
>   205f850ceacfc39d1e9d76a9569416284594ce8crefs/tags/go1.1
>   d260448f6b6ac10efe4ae7f6dfe944e72bc2a676refs/tags/go1.1.1
>   1d6d8fca241bb611af51e265c1b5a2e9ae904702refs/tags/go1.1.2
>   bf86aec25972f3a100c3aa58a6abcbcc35bdea49refs/tags/go1.10
>   ac7c0ee26dda18076d5f6c151d8f920b43340ae3refs/tags/go1.10.1
>   9ce6b5c2ed5d3d5251b9a6a0c548d5fb2c8567e8refs/tags/go1.10beta1
>   594668a5a96267a46282ce3007a584ec07adf705refs/tags/go1.10beta2
>   5348aed83e39bd1d450d92d7f627e994c2db6ebfrefs/tags/go1.10rc1
>   20e228f2fdb44350c858de941dff4aea9f3127b8refs/tags/go1.10rc2
>   1c5438aae896edcd1e9f9618f4776517f08053b3refs/tags/go1.1rc2
>   46a6097aa7943a490e9bd2e04274845d0e5e200frefs/tags/go1.1rc3
>   402d3590b54e4a0df9fb51ed14b2999e85ce0b76refs/tags/go1.2
>   9c9802fad57c1bcb72ea98c5c55ea2652efc5772refs/tags/go1.2.1
>   ...

This is a sensible thing to want, but why not follow the UI we have for
this with git-tag? I.e. --sort= & -i (or --ignore-case)? Of course
ls-remote doesn't just show tags, so maybe we'd want --tag-sort=
and --ignore-tag-case or something, but the rest should be equivalent,
no?

> [...]
> @@ -101,13 +115,22 @@ int cmd_ls_remote(int argc, const char **argv, const 
> char *prefix)
>   if (transport_disconnect(transport))
>   return 1;
>
> - if (!dest && !quiet)
> - fprintf(stderr, "From %s\n", *remote->url);
>   for ( ; ref; ref = ref->next) {
>   if (!check_ref_type(ref, flags))
>   continue;
>   if (!tail_match(pattern, ref->name))
>   continue;
> + REALLOC_ARRAY(refs, nr + 1);
> + refs[nr++] = ref;
> + }
> +
> + if (version_sort)
> + QSORT(refs, nr, cmp_ref_versions);
> +
> + if (!dest && !quiet)
> + fprintf(stderr, "From %s\n", *remote->url);

Is there some subtlety here I'm missing which means that when sorting
we'd now need to print this "From" line later (i.e. after sorting?