Re: [PATCH] Adding rebase merge strategy

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Oct 1, 2007, 15:50 UTC
Message-ID: <Pine.LNX.4.64.0710011647030.28395@racer.site>
In-Reply-To: <11912513203420-git-send-email-tom@u2i.com>

Hi,

On Mon, 1 Oct 2007, Tom Clarke wrote:

> Incorporated comments from Johannes Schindlen.

Thanks.

> +# Give up if we are given two or more remotes -- not handling octopus.
> +test $# = 1 || exit 2

I think the user wants to know in this case, too.  How about

test $# = 1 || {
	echo "Cannot handle octopus." >&2
	exit 2
}

Show 9 quoted lines

> diff --git a/t/t3031-merge-rebase.sh b/t/t3031-merge-rebase.sh
> new file mode 100755
> index 0000000..daa03b1
> --- /dev/null
> +++ b/t/t3031-merge-rebase.sh
>
> [...]
>
> +	( git log --pretty=oneline ) >actual &&

Please lose the parentheses here.

Show 5 quoted lines

> +	(
> +		echo "4db7a5a013e67aa623d1fd294e8d46e89b3ace8f onbranch"
> +		echo "893371811dbd13e85c098b72d1ab42bcfd24c2db update"
> +		echo "0e960b10429bf3f1e168ee2cc7d531ac7c622580 initial"
> +	) >expected &&

Why not do it as is done elsewhere in the test suit: use a "cat << EOF" before "test_expect_success"?

> +	(
> +		echo "warning: Message is not used for rebase merge strategy"
> +	) >expected &&

Same here.

Other than that, I like it.

Ciao, Dscho