From: Shawn Pearce <spearce@spearce.org>
Date: Wed, 08 Nov 2006 05:10:35 GMT
Subject: Re: [PATCH] Return non-zero status from pull if merge fails.
Message-ID: <20061108051035.GA28498@spearce.org>
In-Reply-To: <7vy7qmzttk.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano <junkio@cox.net> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> > diff --git a/git-merge.sh b/git-merge.sh
> > index cb09438..7725908 100755
> > --- a/git-merge.sh
> > +++ b/git-merge.sh
> > @@ -203,7 +203,7 @@ f,*)
> >  	git-update-index --refresh 2>/dev/null
> >  	new_head=$(git-rev-parse --verify "$1^0") &&
> >  	git-read-tree -u -v -m $head "$new_head" &&
> > -	finish "$new_head" "Fast forward"
> > +	finish "$new_head" "Fast forward" || exit 1
> >  	dropsave
> >  	exit 0
> >  	;;
> 
> The shell function "finish" itself exits when there is an error;
> is this needed?

Yes.  Without it:

  $ git checkout -b 931233bc666b^
  $ echo broken >builtin-pickaxe.c
  $ git pull . next && echo good merge
  Updating c2e525d..522da27
  builtin-pickaxe.c: needs update
  fatal: Entry 'builtin-pickaxe.c' not uptodate. Cannot merge.
  good merge

Say what?  There's no way that fast forward was good!  Granted this
use case is horrible but that fast forward went very, very badly,
but the caller now thinks it was good.
 
> > @@ -214,7 +214,7 @@ f,*)
> > +	git var GIT_COMMITTER_IDENT >/dev/null || exit 1
> > @@ -225,7 +225,7 @@ f,*)
> > +	    ) || exit 1
> > @@ -253,7 +253,7 @@ f,*)
> > +git var GIT_COMMITTER_IDENT >/dev/null || exit 1
> > @@ -327,7 +327,7 @@ done
> > +    result_commit=$(echo "$merge_msg" | git-commit-tree $result_tree $parents) || exit 1
> 
> Are these needed?  Wouldn't "something || exit" already exit non-zero
> when something exits non-zero?

Hmm; apparently you are correct.  But that seems like magic shell
voodoo to me.  I honestly didn't expect exit to behave like that.
Please delete these 4 hunks and apply my patch without them.

--