threads / patch / 43156

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

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

## tl;dr

5 messages between Nov 7, 2006 and Nov 8, 2006. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Shawn O. Pearce· Nov 7, 2006, 18:10 UTC · lore

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

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(-)
Show changes to 2 files +6 −6

git-merge.sh, git-pull.sh

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
Junio C Hamano· Nov 8, 2006, 00:05 UTC · re: Shawn O. Pearce · lore
"Shawn O. Pearce" <spearce@spearce.org> writes:
Show 13 quoted lines
> 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?

Show 8 quoted lines
> @@ -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?

Show 12 quoted lines
> 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.
Shawn Pearce· Nov 8, 2006, 05:10 UTC · re: Junio C Hamano · lore
Junio C Hamano <junkio@cox.net> wrote:
Show 18 quoted lines
> "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.
 
Show 11 quoted lines
> > @@ -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.

Junio C Hamano· Nov 8, 2006, 05:36 UTC · re: Shawn Pearce · lore
Shawn Pearce <spearce@spearce.org> writes:
Show 13 quoted lines
> 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.

Shawn Pearce· Nov 8, 2006, 05:52 UTC · re: Junio C Hamano · lore
Junio C Hamano <junkio@cox.net> wrote:
Show 19 quoted lines
> 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.

← back to recent threads