Re: [PATCH v11 1/4] ref-filter: use "struct object_id" consistently

From: Harald Nordgren <haraldnordgren@gmail.com>
Date: Apr 8, 2018, 12:27 UTC
Message-ID: <CAHwyqnVWhwmDKhgoJ1p-UuxL-=pAczMCe3KdDbkhayh4n88jPg@mail.gmail.com>
In-Reply-To: <CAPig+cTQ1ZzWTp6Qsj96nE8QwhuRuqB51orTNsCQjGKD=_veyg@mail.gmail.com>

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@sunshineco.com> 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.

Show 9 quoted lines

> 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@sigill.intra.peff.net/

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.

Show 8 quoted lines

> 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@github.com: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.

Show 9 quoted lines

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

Show 10 quoted lines

> 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@crustytoothpaste.net/

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.

Show 9 quoted lines

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