Thanks for your very thorough review Eric! I think I address all the
points, but if I missed something please let me know.

On Sun, Apr 8, 2018 at 3:06 AM, Eric Sunshine <> wrote:

> You incorrectly dropped Peff's sign-off[1] when re-sending the patches
> he authored in the series. And, your sign-off should follow his.

I just rebased on
where there were no sign-off tags.

But I amended the commit messages now to add the sign-off. And also
added my own sign-off.

> Also, if you made any changes to Peff's patch, it's a good idea to
> state so with a bracketed comment at the end of the commit message
> (before the sign-offs). For instance:
>     [hn: tweaked doodle blap]
> or such.
> [1]: 

As a sign of respect I probably wouldn't want to tweak the patches.

They currently work well together with my implementation, so there is no need.

> 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"...
> To what does "Also" refer?

This was copied wholesale from Documentation/git-tag.txt.

I minimized the text now and removed all the references to using
'sort' multiple times. Although you technically could use multiple
keys, since 'ls-remote' only outputs two columns, I guess it's never

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

This will be the output when trying to sort by commiterdate:

    fatal: missing object f0d0fd3a5985d5e588da1e1d11c85fba0ae132f8 for

I added a a note in the documentation saying that it will "cause a
failure". Should we be even more explicit than that?

This is one side-effect of borrowing the ref-filter logic in this
patch. We inherit things that won't work on remotes.

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

Added a "linkgit" to git-for-each-ref. And a "See also" section at the bottom.

> Can we have a more meaningful name than 'array'? Even a name a simple
> as 'refs' would convey more information.

I think 'refs' would be confusing considering that the return value of
'transport_get_remote_refs' is stored as 'ref'. I renamed it to
'ref_array'. I hope it's not a problem that is shadows its struct
name. But I've seen us this naming paradigm in other places -- and in
this file.

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

Since 'cmd_ls_remote' is always (?) called from another process I
don't think we need to worry explicitly freeing the memory. I added
UNLEAK annotations to my code.

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

Added 'git rev-parse' calls to my tests do decrease the reliance on
SHA-1's. For the test

    ls-remote --symref

I couldn't figure out a way to extract the hash for
'refs/remotes/origin/HEAD' so I re-used HEAD. I tried different ways
of fetching origin, making another commit and pushing, but origin/HEAD
is still unavailable.

By calling 'git fetch origin' before the test, at least I am able to
extract 'git rev-parse refs/remotes/origin/master'.

This all makes the tests a bit more complicated, so I hope it helps us
in the future! :)

> Do we want a test verifying that multiple sort keys are respected? (Is
> it even possible to construct such a test?)

To add to my previous point: Since 'ls-remote' outputs only two
columns, I don't see a use case for multiple keys.

And I don't know what the test would look like either.

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

I added the asterisk to 'mark*' because without it, the test

    Report match with --exit-code

will fail because the input 'git ls-remote refs/tags/*' now gives
gives back more tags (mark, mark1.1, mark1.2, mark1.10). So I think
the asterisk is the cleanest solution to not have to create e.g. two
separate setup functions.

Reply via email to