From: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Date: Thu, 19 Apr 2012 12:19:22 GMT
Subject: Re: [PATCH v7 4/4] git-rebase: add keep_empty flag
Message-ID: <4F9002CA.6040302@in.waw.pl>
In-Reply-To: <873980q6vm.fsf@thomas.inf.ethz.ch>

On 04/19/2012 01:49 PM, Thomas Rast wrote:
> Zbigniew Jędrzejewski-Szmek<zbyszek@in.waw.pl>  writes:
>
>> On 04/18/2012 09:18 PM, Neil Horman wrote:
>>> Add a command line switch to git-rebase to allow a user the ability to specify
>>> that they want to keep any commits in a series that are empty.
>>>
>>> When git-rebase's type is am, then this option will automatically keep any
>>> commit that has a tree object identical to its parent.
>>>
>>> This patch changes the default behavior of interactive rebases as well.  With
>>> this patch, git-rebase -i will produce a revision set passed to
>>> git-revision-editor, in which empty commits are commented out.  Empty commits
>>> may be kept manually by uncommenting them.  If the new --keep-empty option is
>>> used in an interactive rebase the empty commits will automatically all be
>>> uncommented in the editor.
>>>
>>> Signed-off-by: Neil Horman<nhorman@tuxdriver.com>
>>
>> Hi,
>> this one seems to breaks many tests when /bin/sh=dash. (Both v6 in pu
>> and this v7).
>
> Probably because of the strange return in this function:
>
>>> is_empty_commit() {
>>> 	tree=$(git rev-parse "$1"^{tree})
>>> 	ptree=$(git rev-parse "$1"^^{tree})
>>> 	return $(test "$tree" = "$ptree")
>>> }
>
> bash seems to pass on the exit status from $() to the caller, while dash
> doesn't.  It seems bash is actually more correct here, because POSIX
> says about 'return [n]':
>
>      EXIT STATUS
>         The value of the special parameter '?' shall be set to n, an
>         unsigned decimal integer, or to the exit status of the last
>         command executed if n is not specified.
>
> Either way, it should simply be spelled as
>
> is_empty_commit() {
> 	tree=$(git rev-parse "$1"^{tree})
> 	ptree=$(git rev-parse "$1"^^{tree})
> 	test "$tree" = "$ptree"
> }
Yes, this change fixes the problem (all tests pass).

Thanks!

Zbyszek