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

[Junio C Hamano <gitster@pobox.com>]

Harald Nordgren <haraldnordgren@gmail.com> writes:

> +--sort=<key>::
> +	Sort based on the key given. Prefix `-` to sort in descending order
> +	of the value. Supports "version:refname" or "v:refname" (tag names
> +	are treated as versions). The "version:refname" sort order can also
> +	be affected by the "versionsort.suffix" configuration variable.
> +	See linkgit:git-for-each-ref[1] for more sort options, but be aware
> +	that because `ls-remote` deals only with remotes, any key like
> +	`committerdate` that requires access to the object itself will
> +	cause a failure.

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 ;-)

> +	for (i = 0; i < ref_array.nr; i++) {
> +		const struct ref_array_item *ref = ref_array.items[i];
>  		if (show_symref_target && ref->symref)
> -			printf("ref: %s\t%s\n", ref->symref, ref->name);
> -		printf("%s\t%s\n", oid_to_hex(&ref->old_oid), ref->name);
> +			printf("ref: %s\t%s\n", ref->symref, ref->refname);
> +		printf("%s\t%s\n", oid_to_hex(&ref->objectname), ref->refname);
>  		status = 0; /* we found something */
>  	}
> +
> +	UNLEAK(sorting);
> +	UNLEAK(ref_array);
>  	return status;
>  }

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.

> +test_expect_success 'ls-remote --sort="version:refname" --tags self' '
> +	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

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.

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