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

[Neil Horman <nhorman@tuxdriver.com>]

On Thu, Apr 19, 2012 at 02:19:22PM +0200, Zbigniew Jędrzejewski-Szmek wrote:
> 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
> 
Ok, I'll update it.
Neil