From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sat, 05 Feb 2011 10:14:12 GMT
Subject: Re: [PATCH] cache-tree: do not cache empty trees
Message-ID: <20110205101412.GB1685@elie>
In-Reply-To: <1296899427-1394-1-git-send-email-pclouds@gmail.com>

Hi,

Some quick nits to save myself time.  The basic idea of the patch
seems sound.

Nguyễn Thái Ngọc Duy wrote:

> --- /dev/null
> +++ b/t/t1013-read-tree-empty.sh
> @@ -0,0 +1,38 @@
> +#!/bin/sh
> +
> +test_description='read-tree with empty trees'
> +
> +. ./test-lib.sh
> +
> +T1=f4ec99e8174c01eab488469b4c2680500bbb18da
> +T2=4b825dc642cb6eb9a060e54bf8d69288fbee4904

What are these trees?  Do they need to be hardcoded?

> +
> +test_expect_success 'setup' '
> +	printf "40000 empty\0\x4b\x82\x5d\xc6\x42\xcb\x6e\xb9\xa0\x60\xe5\x4b\xf8\xd6\x92\x88\xfb\xee\x49\x04" >newtree &&

printf '\xab' is unfortunately unportable.  I suppose
this should rather say something like

 test_unequal () {
	printf '%s\n' "$1" >bad &&
	printf '%s\n' "$2" >actual &&
	! test_cmp bad actual
 }

	empty_tree=$(git mktree </dev/null) &&
	tree_with_empty_subtree=$(
		echo "040000 tree $empty_tree	empty" |
		git mktree
	) &&
	test_unequal "$empty_tree" "$tree_with_empty_subtree"

> +test_expect_success 'ls-tree T1 (with empty tree)' '
> +	git ls-tree $T1 >actual &&
> +	cat <<EOF >expected &&
> +040000 tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904	empty
> +EOF
> +	test_cmp expected actual
> +'

	echo "040000 tree $empty_tree	empty" >expect &&
	git ls-tree "$tree_with_empty_subtree" >actual &&
	test_cmp expect actual

> +
> +test_expect_success 'write-tree removes empty tree' '
> +	git read-tree "$T1" &&
> +	git write-tree >actual
> +	echo $T2 >expected
> +	test_cmp expected actual
> +'

	git read-tree "$tree_with_empty_subtree" &&
	...

Sane?
Jonathan