On Mon, Apr 9, 2018 at 12:16 AM, Junio C Hamano <gitster@xxxxxxxxx> 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!
