Re: [PATCH v3] rev-parse --namespace

From: Jeff King <peff@peff.net>
Date: Jan 19, 2010, 06:56 UTC
Message-ID: <20100119065609.GB3946@coredump.intra.peff.net>
In-Reply-To: <1263808296-30756-1-git-send-email-ilari.liusvaara@elisanet.fi>

On Mon, Jan 18, 2010 at 11:51:36AM +0200, Ilari Liusvaara wrote:

> Add --namespace=<namespace> option to rev-parse and everything that
> accepts its options. This option matches all refs in some subnamespace
> of refs hierarchy.

Thanks, I think the code in this version looks good, but:

Show 9 quoted lines

> --- /dev/null
> +++ b/t/t6018-rev-list-namespace.sh
> [...]
> +compare () {
> +	# Split arguments on whitespace.
> +	git $1 $2 | sort >expected &&
> +	git $1 $3 | sort >actual &&
> +	cmp expected actual
> +}

Please use test_cmp instead of regular cmp.

Also, do you really need to sort? The internal ref code keeps the ref lists sorted by name, and we merge-sort the packed and loose lists in do_for_each_ref. So your --namespace should always operate on the refs in sorted order, producing predictable results.

Show 16 quoted lines

> +test_expect_success 'setup' '
> +
> +	commit master &&
> +	git checkout -b subspace/one master
> +	commit one &&
> +	git checkout -b subspace/two master
> +	commit two &&
> +	git checkout -b subspace-x master
> +	commit subspace-x &&
> +	git checkout -b other/three master
> +	commit three &&
> +	git checkout -b someref master
> +	commit some &&
> +	git checkout master &&
> +	commit master2
> +'

You are still missing some '&&' here to detect setup failures.

-Peff