Re: [PATCH v7 4/4] git-rebase: add keep_empty flag

From: Thomas Rast <trast@student.ethz.ch>
Date: Apr 19, 2012, 11:49 UTC
Message-ID: <873980q6vm.fsf@thomas.inf.ethz.ch>
In-Reply-To: <4F8FE2CD.3070300@in.waw.pl>

Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:

Show 19 quoted lines

> 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:

Show 5 quoted lines

>> 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"
}

-- 
Thomas Rast
trast@{inf,student}.ethz.ch