From: Jeff King <peff@peff.net>
Date: Thu, 10 Jul 2014 04:00:19 GMT
Subject: Re: [PATCH v2] tag: support configuring --sort via .gitconfig
Message-ID: <20140710040019.GC28401@sigill.intra.peff.net>
In-Reply-To: <1404945412-10197-1-git-send-email-jacob.e.keller@intel.com>

On Wed, Jul 09, 2014 at 03:36:51PM -0700, Jacob Keller wrote:

> Add support for configuring default sort ordering for git tags. Command
> line option will override this configured value, using the exact same
> syntax.

Thanks, this version looks pretty good to me. A few minor comments:

> +	if (!strcmp(var, "tag.sort")) {
> +		tag_sort = parse_sort_string(value);
> +	}

Our style is to usually avoid braces for a one-liner. However, I think
would actually make sense to "return 0" from this conditional.

> +test_expect_success 'configured lexical sort' '
> +	git config tag.sort "v:refname" &&
> +	git tag -l "foo*" >actual &&
> [...]

Please use:

  test_config tag.sort "v:refname"

here, which will clean up the config value after the test ends (and thus
not pollute any later tests).

Though you will need to add an extra "test_config" to the following
test:

> +test_expect_success 'option override configured sort' '
> +	git tag -l --sort=-refname "foo*" >actual &&
> [...]

I think that's a good thing, though (it makes it more clear in the
second test what is being tested, rather than relying on the state left
by the previous test).

-Peff