[PATCH] Return non-zero status from pull if merge fails.

[PATCH] Return non-zero status from pull if merge fails.

[Shawn O. Pearce]

If the merge operation fails during git-pull it usually exits with a
non-zero exit status to let the caller know it failed.  But git-pull
tossed that status away and always returned 0 (success), leading to
confusing output in any higher level interface which supplied both
the console output (showing the merge failure) and the exit status
(showing success).

So now git-pull just exec's git-merge, allowing its exit status to
be reported directly to the caller.

There were also a number of cases within git-merge which terminated
with a success (0) exit status despite having been caused by failure
to merge.  This is equally misleading and should return 1 instead.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 git-merge.sh |   10 +++++-----
 git-pull.sh  |    2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

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
 	;;
@@ -214,7 +214,7 @@ f,*)
 ?,1,*,)
 	# We are not doing octopus, not fast forward, and have only
 	# one common.  See if it is really trivial.
-	git var GIT_COMMITTER_IDENT >/dev/null || exit
+	git var GIT_COMMITTER_IDENT >/dev/null || exit 1
 
 	echo "Trying really trivial in-index merge..."
 	git-update-index --refresh 2>/dev/null
@@ -225,7 +225,7 @@ f,*)
 	    result_commit=$(
 	        echo "$merge_msg" |
 	        git-commit-tree $result_tree -p HEAD -p "$1"
-	    ) || exit
+	    ) || exit 1
 	    finish "$result_commit" "In-index merge"
 	    dropsave
 	    exit 0
@@ -253,7 +253,7 @@ f,*)
 esac
 
 # We are going to make a new commit.
-git var GIT_COMMITTER_IDENT >/dev/null || exit
+git var GIT_COMMITTER_IDENT >/dev/null || exit 1
 
 # At this point, we need a real merge.  No matter what strategy
 # we use, it would operate on the index, possibly affecting the
@@ -327,7 +327,7 @@ done
 if test '' != "$result_tree"
 then
     parents=$(git-show-branch --independent "$head" "$@" | sed -e 's/^/-p /')
-    result_commit=$(echo "$merge_msg" | git-commit-tree $result_tree $parents) || exit
+    result_commit=$(echo "$merge_msg" | git-commit-tree $result_tree $parents) || exit 1
     finish "$result_commit" "Merge made by $wt_strategy."
     dropsave
     exit 0
diff --git a/git-pull.sh b/git-pull.sh
index ed04e7d..d10fcdd 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -102,6 +102,6 @@ case "$strategy_args" in
 esac
 
 merge_name=$(git-fmt-merge-msg <"$GIT_DIR/FETCH_HEAD") || exit
-git-merge "--reflog-action=pull $*" \
+exec git-merge "--reflog-action=pull $*" \
 	$no_summary $no_commit $squash $strategy_args \
 	"$merge_name" HEAD $merge_head
-- 

Re: [PATCH] Return non-zero status from pull if merge fails.

[Junio C Hamano]

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

> @@ -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?

> diff --git a/git-pull.sh b/git-pull.sh
> index ed04e7d..d10fcdd 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -102,6 +102,6 @@ case "$strategy_args" in
>  esac
>  
>  merge_name=$(git-fmt-merge-msg <"$GIT_DIR/FETCH_HEAD") || exit
> -git-merge "--reflog-action=pull $*" \
> +exec git-merge "--reflog-action=pull $*" \
>  	$no_summary $no_commit $squash $strategy_args \
>  	"$merge_name" HEAD $merge_head

I think this is a good change.

Re: [PATCH] Return non-zero status from pull if merge fails.

[Shawn Pearce]

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.

-- 

Re: [PATCH] Return non-zero status from pull if merge fails.

[Junio C Hamano]

Shawn Pearce <spearce@spearce.org> writes:

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

I think fast forward went Ok in that "git-ls-tree HEAD" gives
the correct merge result from pulling next on top of 931233^ (or
whatever).  I am undecided if we want to keep what dropsave is
supposed to remove in that case, but exiting with non-zero to
indicate an error condition is needed.

> Hmm; apparently you are correct.  But that seems like magic shell
> voodoo to me.  I honestly didn't expect exit to behave like that.

Get used to it and learn the real shell programming from a
traditionalist ;-).

	something-that-could-fail || exit

is a well established idiom.

But I do not mind the extra explicit non-zero exit status too
much as long as you are consistent.

Re: [PATCH] Return non-zero status from pull if merge fails.

[Shawn Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> Shawn Pearce <spearce@spearce.org> writes:
> 
> > 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.
> 
> I think fast forward went Ok in that "git-ls-tree HEAD" gives
> the correct merge result from pulling next on top of 931233^ (or
> whatever).

No it didn't.  After doing the pull:

	$ git rev-parse --verify HEAD
	c2e525d97f81bc178567cdf4dd7056ce6224eb58
	$ git rev-parse --verify 931233bc666b^
	c2e525d97f81bc178567cdf4dd7056ce6224eb58

so no the merge result wasn't put into HEAD.  Nothing was done.
Which is good because to do the merge the working directory has
to change but there's a conflict there due to one file being in a
modified state.  Better we don't change HEAD.

> I am undecided if we want to keep what dropsave is
> supposed to remove in that case, but exiting with non-zero to
> indicate an error condition is needed.

I think that elsewhere in git-merge we abort without calling dropsave
when things to south.  Which is why I aborted before.

--