From: Jeff King <peff@peff.net>
Date: Mon, 02 Feb 2015 18:41:16 GMT
Subject: Re: [PATCHv2 1/2] t5304-prune: demonstrate bug in pruning alternates
Message-ID: <20150202184115.GA25421@peff.net>
In-Reply-To: <0BD44E44-686B-44B2-A4C0-9E14A99BA96B@jonathonmah.com>

On Mon, Feb 02, 2015 at 10:33:02AM -0800, Jonathon Mah wrote:

> Signed-off-by: Jonathon Mah <me@JonathonMah.com>
> ---
> Adjust prune test directly, much nicer.

Agreed, this is much nicer. A few comments:

> +test_expect_success 'prune: handle alternate object database' '

This test fails, so we either need expect_failure here, or it just needs
to be squashed in with the fix (I generally prefer the latter).

> +	test_create_repo A && cd A &&

We generally prefer to chdir in a subshell, so that a failure in the
test does not leave further tests in a confusing spot. Like:

  test_create_repo A &&
  (
	cd A &&
	... do stuff in repo ...
	# no need to cd ..
  ) &&
  .. do stuff outside repo ...

> +	echo "Hello World" > file1 &&

Style nit: we prefer ">file1" with no space.

> +	git add file1 &&
> +	git commit -m "Initial commit" file1 &&
> +	cd .. &&
> +	git clone -l -s A B && cd B &&

"-l" is a noop these days. I don't think it is hurting, but I'd prefer
not to propagate bad habits in our tests.

> diff --git a/t/t5710-info-alternate.sh b/t/t5710-info-alternate.sh
> index 5a6e49d..d82844a 100755
> --- a/t/t5710-info-alternate.sh
> +++ b/t/t5710-info-alternate.sh

We can drop this change, then, right?

-Peff