Re: [PATCH v12 4/4] ls-remote: create '--sort' option
- From
Harald Nordgren <haraldnordgren@gmail.com>
- Date
- Apr 9, 2018, 00:09 UTC
- Message-ID
- <CAHwyqnWe=KmmdGJJ_DnCK+D_FoC0sAQG_8=_p+4+Yxmr8s8sPA@mail.gmail.com>
- In-Reply-To
- <xmqq7eph9wds.fsf@gitster-ct.c.googlers.com>
On Mon, Apr 9, 2018 at 12:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 25 quoted lines
>
> 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.
Show 8 quoted lines
> > 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.
Show 19 quoted lines
> 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!