{"thread":{"id":"43156","subject":"Re: [PATCH] Return non-zero status from pull if merge fails.","startedAt":"2006-11-07T18:10:53Z","lastAt":"2006-11-08T05:52:57Z","messageCount":5,"participants":["Shawn Pearce","Junio C Hamano","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"297560","messageId":"20061107181053.GA26856@spearce.org","threadId":"43156","inReplyTo":null,"subject":"[PATCH] Return non-zero status from pull if merge fails.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-11-07T18:10:53Z","receivedAt":"2006-11-07T18:10:53Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"If the merge operation fails during git-pull it usually exits with a\nnon-zero exit status to let the caller know it failed.  But git-pull\ntossed that status away and always returned 0 (success), leading to\nconfusing output in any higher level interface which supplied both\nthe console output (showing the merge failure) and the exit status\n(showing success).\n\nSo now git-pull just exec's git-merge, allowing its exit status to\nbe reported directly to the caller.\n\nThere were also a number of cases within git-merge which terminated\nwith a success (0) exit status despite having been caused by failure\nto merge.  This is equally misleading and should return 1 instead.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n git-merge.sh |   10 +++++-----\n git-pull.sh  |    2 +-\n 2 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/git-merge.sh b/git-merge.sh\nindex cb09438..7725908 100755\n--- a/git-merge.sh\n+++ b/git-merge.sh\n@@ -203,7 +203,7 @@ f,*)\n \tgit-update-index --refresh 2>/dev/null\n \tnew_head=$(git-rev-parse --verify \"$1^0\") &&\n \tgit-read-tree -u -v -m $head \"$new_head\" &&\n-\tfinish \"$new_head\" \"Fast forward\"\n+\tfinish \"$new_head\" \"Fast forward\" || exit 1\n \tdropsave\n \texit 0\n \t;;\n@@ -214,7 +214,7 @@ f,*)\n ?,1,*,)\n \t# We are not doing octopus, not fast forward, and have only\n \t# one common.  See if it is really trivial.\n-\tgit var GIT_COMMITTER_IDENT >/dev/null || exit\n+\tgit var GIT_COMMITTER_IDENT >/dev/null || exit 1\n \n \techo \"Trying really trivial in-index merge...\"\n \tgit-update-index --refresh 2>/dev/null\n@@ -225,7 +225,7 @@ f,*)\n \t    result_commit=$(\n \t        echo \"$merge_msg\" |\n \t        git-commit-tree $result_tree -p HEAD -p \"$1\"\n-\t    ) || exit\n+\t    ) || exit 1\n \t    finish \"$result_commit\" \"In-index merge\"\n \t    dropsave\n \t    exit 0\n@@ -253,7 +253,7 @@ f,*)\n esac\n \n # We are going to make a new commit.\n-git var GIT_COMMITTER_IDENT >/dev/null || exit\n+git var GIT_COMMITTER_IDENT >/dev/null || exit 1\n \n # At this point, we need a real merge.  No matter what strategy\n # we use, it would operate on the index, possibly affecting the\n@@ -327,7 +327,7 @@ done\n if test '' != \"$result_tree\"\n then\n     parents=$(git-show-branch --independent \"$head\" \"$@\" | sed -e 's/^/-p /')\n-    result_commit=$(echo \"$merge_msg\" | git-commit-tree $result_tree $parents) || exit\n+    result_commit=$(echo \"$merge_msg\" | git-commit-tree $result_tree $parents) || exit 1\n     finish \"$result_commit\" \"Merge made by $wt_strategy.\"\n     dropsave\n     exit 0\ndiff --git a/git-pull.sh b/git-pull.sh\nindex ed04e7d..d10fcdd 100755\n--- a/git-pull.sh\n+++ b/git-pull.sh\n@@ -102,6 +102,6 @@ case \"$strategy_args\" in\n esac\n \n merge_name=$(git-fmt-merge-msg <\"$GIT_DIR/FETCH_HEAD\") || exit\n-git-merge \"--reflog-action=pull $*\" \\\n+exec git-merge \"--reflog-action=pull $*\" \\\n \t$no_summary $no_commit $squash $strategy_args \\\n \t\"$merge_name\" HEAD $merge_head\n-- \n"},{"id":"296588","messageId":"7vy7qmzttk.fsf@assigned-by-dhcp.cox.net","threadId":"43156","inReplyTo":"20061107181053.GA26856@spearce.org","subject":"Re: [PATCH] Return non-zero status from pull if merge fails.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-08T00:05:27Z","receivedAt":"2006-11-08T00:05:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> writes:\n\n> diff --git a/git-merge.sh b/git-merge.sh\n> index cb09438..7725908 100755\n> --- a/git-merge.sh\n> +++ b/git-merge.sh\n> @@ -203,7 +203,7 @@ f,*)\n>  \tgit-update-index --refresh 2>/dev/null\n>  \tnew_head=$(git-rev-parse --verify \"$1^0\") &&\n>  \tgit-read-tree -u -v -m $head \"$new_head\" &&\n> -\tfinish \"$new_head\" \"Fast forward\"\n> +\tfinish \"$new_head\" \"Fast forward\" || exit 1\n>  \tdropsave\n>  \texit 0\n>  \t;;\n\nThe shell function \"finish\" itself exits when there is an error;\nis this needed?\n\n> @@ -214,7 +214,7 @@ f,*)\n> +\tgit var GIT_COMMITTER_IDENT >/dev/null || exit 1\n> @@ -225,7 +225,7 @@ f,*)\n> +\t    ) || exit 1\n> @@ -253,7 +253,7 @@ f,*)\n> +git var GIT_COMMITTER_IDENT >/dev/null || exit 1\n> @@ -327,7 +327,7 @@ done\n> +    result_commit=$(echo \"$merge_msg\" | git-commit-tree $result_tree $parents) || exit 1\n\nAre these needed?  Wouldn't \"something || exit\" already exit non-zero\nwhen something exits non-zero?\n\n> diff --git a/git-pull.sh b/git-pull.sh\n> index ed04e7d..d10fcdd 100755\n> --- a/git-pull.sh\n> +++ b/git-pull.sh\n> @@ -102,6 +102,6 @@ case \"$strategy_args\" in\n>  esac\n>  \n>  merge_name=$(git-fmt-merge-msg <\"$GIT_DIR/FETCH_HEAD\") || exit\n> -git-merge \"--reflog-action=pull $*\" \\\n> +exec git-merge \"--reflog-action=pull $*\" \\\n>  \t$no_summary $no_commit $squash $strategy_args \\\n>  \t\"$merge_name\" HEAD $merge_head\n\nI think this is a good change.\n"},{"id":"293943","messageId":"20061108051035.GA28498@spearce.org","threadId":"43156","inReplyTo":"7vy7qmzttk.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Return non-zero status from pull if merge fails.","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-11-08T05:10:35Z","receivedAt":"2006-11-08T05:10:35Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano <junkio@cox.net> wrote:\n> \"Shawn O. Pearce\" <spearce@spearce.org> writes:\n> \n> > diff --git a/git-merge.sh b/git-merge.sh\n> > index cb09438..7725908 100755\n> > --- a/git-merge.sh\n> > +++ b/git-merge.sh\n> > @@ -203,7 +203,7 @@ f,*)\n> >  \tgit-update-index --refresh 2>/dev/null\n> >  \tnew_head=$(git-rev-parse --verify \"$1^0\") &&\n> >  \tgit-read-tree -u -v -m $head \"$new_head\" &&\n> > -\tfinish \"$new_head\" \"Fast forward\"\n> > +\tfinish \"$new_head\" \"Fast forward\" || exit 1\n> >  \tdropsave\n> >  \texit 0\n> >  \t;;\n> \n> The shell function \"finish\" itself exits when there is an error;\n> is this needed?\n\nYes.  Without it:\n\n  $ git checkout -b 931233bc666b^\n  $ echo broken >builtin-pickaxe.c\n  $ git pull . next && echo good merge\n  Updating c2e525d..522da27\n  builtin-pickaxe.c: needs update\n  fatal: Entry 'builtin-pickaxe.c' not uptodate. Cannot merge.\n  good merge\n\nSay what?  There's no way that fast forward was good!  Granted this\nuse case is horrible but that fast forward went very, very badly,\nbut the caller now thinks it was good.\n \n> > @@ -214,7 +214,7 @@ f,*)\n> > +\tgit var GIT_COMMITTER_IDENT >/dev/null || exit 1\n> > @@ -225,7 +225,7 @@ f,*)\n> > +\t    ) || exit 1\n> > @@ -253,7 +253,7 @@ f,*)\n> > +git var GIT_COMMITTER_IDENT >/dev/null || exit 1\n> > @@ -327,7 +327,7 @@ done\n> > +    result_commit=$(echo \"$merge_msg\" | git-commit-tree $result_tree $parents) || exit 1\n> \n> Are these needed?  Wouldn't \"something || exit\" already exit non-zero\n> when something exits non-zero?\n\nHmm; apparently you are correct.  But that seems like magic shell\nvoodoo to me.  I honestly didn't expect exit to behave like that.\nPlease delete these 4 hunks and apply my patch without them.\n\n-- \n"},{"id":"297178","messageId":"7vu01av6tb.fsf@assigned-by-dhcp.cox.net","threadId":"43156","inReplyTo":"20061108051035.GA28498@spearce.org","subject":"Re: [PATCH] Return non-zero status from pull if merge fails.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-08T05:36:00Z","receivedAt":"2006-11-08T05:36:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shawn Pearce <spearce@spearce.org> writes:\n\n> Yes.  Without it:\n>\n>   $ git checkout -b 931233bc666b^\n>   $ echo broken >builtin-pickaxe.c\n>   $ git pull . next && echo good merge\n>   Updating c2e525d..522da27\n>   builtin-pickaxe.c: needs update\n>   fatal: Entry 'builtin-pickaxe.c' not uptodate. Cannot merge.\n>   good merge\n>\n> Say what?  There's no way that fast forward was good!  Granted this\n> use case is horrible but that fast forward went very, very badly,\n> but the caller now thinks it was good.\n\nI think fast forward went Ok in that \"git-ls-tree HEAD\" gives\nthe correct merge result from pulling next on top of 931233^ (or\nwhatever).  I am undecided if we want to keep what dropsave is\nsupposed to remove in that case, but exiting with non-zero to\nindicate an error condition is needed.\n\n> Hmm; apparently you are correct.  But that seems like magic shell\n> voodoo to me.  I honestly didn't expect exit to behave like that.\n\nGet used to it and learn the real shell programming from a\ntraditionalist ;-).\n\n\tsomething-that-could-fail || exit\n\nis a well established idiom.\n\nBut I do not mind the extra explicit non-zero exit status too\nmuch as long as you are consistent.\n"},{"id":"298361","messageId":"20061108055257.GC28498@spearce.org","threadId":"43156","inReplyTo":"7vu01av6tb.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Return non-zero status from pull if merge fails.","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-11-08T05:52:57Z","receivedAt":"2006-11-08T05:52:57Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano <junkio@cox.net> wrote:\n> Shawn Pearce <spearce@spearce.org> writes:\n> \n> > Yes.  Without it:\n> >\n> >   $ git checkout -b 931233bc666b^\n> >   $ echo broken >builtin-pickaxe.c\n> >   $ git pull . next && echo good merge\n> >   Updating c2e525d..522da27\n> >   builtin-pickaxe.c: needs update\n> >   fatal: Entry 'builtin-pickaxe.c' not uptodate. Cannot merge.\n> >   good merge\n> >\n> > Say what?  There's no way that fast forward was good!  Granted this\n> > use case is horrible but that fast forward went very, very badly,\n> > but the caller now thinks it was good.\n> \n> I think fast forward went Ok in that \"git-ls-tree HEAD\" gives\n> the correct merge result from pulling next on top of 931233^ (or\n> whatever).\n\nNo it didn't.  After doing the pull:\n\n\t$ git rev-parse --verify HEAD\n\tc2e525d97f81bc178567cdf4dd7056ce6224eb58\n\t$ git rev-parse --verify 931233bc666b^\n\tc2e525d97f81bc178567cdf4dd7056ce6224eb58\n\nso no the merge result wasn't put into HEAD.  Nothing was done.\nWhich is good because to do the merge the working directory has\nto change but there's a conflict there due to one file being in a\nmodified state.  Better we don't change HEAD.\n\n> I am undecided if we want to keep what dropsave is\n> supposed to remove in that case, but exiting with non-zero to\n> indicate an error condition is needed.\n\nI think that elsewhere in git-merge we abort without calling dropsave\nwhen things to south.  Which is why I aborted before.\n\n-- \n"}]}