Re: [PATCH v12 4/4] ls-remote: create '--sort' option

[Harald Nordgren <haraldnordgren@gmail.com>]

On Mon, Apr 9, 2018 at 12:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> I can connect "because it deals only with remotes" and "access to
> the object may fail", but I wonder if we can clarify the former a
> bit better to make it easier to understand for those who are not yet
> familiar with Git.
>
>     Keys like `committerdate` that require access to the object will
>     not work [***HOW??? Do we get a segfault or something???***] for
>     refs whose objects have not yet been fetched from the remote.
>
> I added "for refs whose objects have not yet been fetched", whose
> explicit-ness made "will not work" to be an OK expression, but
> without it I would have suggested to rephrase it to "may not work".
>
> This is a tangent but I suspect that the description of --sort in
> git-for-each-ref documentation is wrong on the use of multiple
> options, or I am misreading parse_opt_ref_sorting(), which I think
> appends later keys to the end of the list, and compare_refs() which
> I think yields results when an earlier key in the last decides two
> are not equal and ignores the later keys.  The commit that added the
> description was c0f6dc9b ("git-for-each-ref.txt: minor
> improvements", 2008-06-05), and I think even back then the code in
> builtin-for-each-ref.c appended later keys at the end, and it seems
> nobody complained, so it is possible I am not reading the code right
> before fully adjusting to timezone change ;-)

Updated the docs.

>
> It is not too big a deal, but I find that liberal sprinkling of
> UNLEAK() is somewhat unsightly.  From the code we leave with this
> change, it is clear what we'd need to do when we make the code fully
> leak-proof (i.e. we'd have a several lines to free resources held by
> these two variables here, and then UNLEAK() that appear earlier in
> the function will become a "goto cleanup;" after setting appropriate
> value to "status"), so let's not worry about it.

Removed the "extra" unleak annotations within the if-return blocks,
but kept them at the end.

> Please do *NOT* step outside the pair of single quotes that protects
> the body of the test scriptlet and execute commands like "git
> rev-parse".  These execute in order to prepare the command line
> argument strings BEFORE test_expect_success can run (or the test
> framework decides if the test will be skipped via GIT_TEST_SKIP).
>
> Instead do it like so:
>
>         cat >expect <<-EOF &&
>         $(git rev-parse mark)   refs/tags/mark
>         $(git rev-parse mark1.1)        refs/tags/mark1.1
>         $(git rev-parse mark1.2)        refs/tags/mark1.2
>         $(git rev-parse mark1.10)       refs/tags/mark1.10
>         EOF
>
> I.e. the end token for << that is not quoted allows $var and $(cmd)
> to be substituted.
>
> Same comment applies throughout the remainder of this file.

Ah, thanks! I was looking for some way to allow the values to expand
within the proper test context.

So turns out '<<-\EOF' vs '<<-EOF' makes all the difference :)

> Other than that, this patch was a very pleasant read.  Thanks.

Thank you!