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 <sunshine@xxxxxxxxxxxxxx> 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 https://github.com/peff/git/tree/jk/ref-array-push 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]: https://public-inbox.org/git/20180406185831.GA11108@xxxxxxxxxxxxxxxxxxxxx/ 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 needed. > 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: From git@xxxxxxxxxx:git/git.git fatal: missing object f0d0fd3a5985d5e588da1e1d11c85fba0ae132f8 for refs/pull/10/head 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]: https://public-inbox.org/git/20180325192055.841459-10-sandals@xxxxxxxxxxxxxxxxxxxx/ 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.
