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

"Shawn O. Pearce" [off-list ref] writes:

The shell function "finish" itself exits when there is an error;
is this needed?

Are these needed?  Wouldn't "something || exit" already exit non-zero
when something exits non-zero?

I think this is a good change.

Junio C Hamano [off-list ref] wrote:

"Shawn O. Pearce" [off-list ref] writes:

quoted

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.
 

quoted

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

--