[PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails

DORMANTno replies

3 messages, 1 author, 2016-06-15 · open the first message on its own page

[PATCH 0/2] Reinstate the helpful message when `git pull --rebase` fails

From: Johannes Schindelin <hidden>
Date: 2016-06-15 23:06:48

Brendan Forster noticed that we no longer see the helpful message after
a failed `git pull --rebase`. It turns out that the builtin `am` calls
the recursive merge function directly, not via a separate process. But
that function was not really safe to be called that way, as it die()s
pretty liberally.

As a consequence, the code that wanted to see whether the merge failed
is not even executed, and the helpful message advising how to fix the
mess is not displayed.

This topic branch fixes this.

Please note that there are a couple of unhandled die() calls in
merge-recursive.c, most of which indicate code paths that should
never be reached (except in the case of a bug).

But there are two other functions that can die(): `update_file_flags()`
(which returns void) and `merge_file_1()`.

The latter function is already nested quite deeply so that the code
would have to be made much uglier to handle the `gentle` flag. It is
also not quite clear to me whether those error cases can be hit in a
regular `git pull --rebase` (which is what I really care about most).

As `update_file_flags()` is called by functions returning void and
that are again called in turn by other functions that also return void,
fixing this part is more involved, so I would like to avoid it, unless
it is deemed absolutely necessary to address in this patch series.


Johannes Schindelin (2):
  merge_recursive_options: introduce the "gentle" flag
  pull --rebase: reinstate helpful message on abort

 builtin/am.c      |  1 +
 merge-recursive.c | 44 +++++++++++++++++++++++++++++++++++---------
 merge-recursive.h |  1 +
 3 files changed, 37 insertions(+), 9 deletions(-)

-- 
2.6.1.windows.1

[PATCH 1/2] merge_recursive_options: introduce the "gentle" flag

From: Johannes Schindelin <hidden>
Date: 2016-06-15 23:06:48

Traditionally, all of Git's operations were intended as single
executables to be run once and exit, not as library functions with
careful error code paths. Therefore, it was okay for those operations
to simply die() with an error.

However, this assumption no longer holds true: builtin `am` calls
merge_recursive_generic() as a regular library function whose return
value indicates whether there was a problem.

Throughout Git's source code, that paradigm (to return an error instead
of die()ing) is called "gentle".

Let's introduce this flag and heed it in as many places as is easily
done.

Signed-off-by: Johannes Schindelin <redacted>
---
 merge-recursive.c | 44 +++++++++++++++++++++++++++++++++++---------
 merge-recursive.h |  1 +
 2 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 44d85be..37528c9 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c

@@ -266,8 +266,12 @@ struct tree *write_tree_from_memory(struct merge_options *o)
 		active_cache_tree = cache_tree();
 
 	if (!cache_tree_fully_valid(active_cache_tree) &&
-	    cache_tree_update(&the_index, 0) < 0)
-		die(_("error building trees"));
+	    cache_tree_update(&the_index, 0) < 0) {
+		if (!o->gentle)
+			die(_("error building trees"));
+		error(_("error building trees"));
+		return NULL;
+	}
 
 	result = lookup_tree(active_cache_tree->sha1);

@@ -712,6 +716,8 @@ static int make_room_for_path(struct merge_options *o, const char *path)
 			error(msg, path, _(": perhaps a D/F conflict?"));
 			return -1;
 		}
+		if (o->gentle)
+			return error(msg, path, "");
 		die(msg, path, "");
 	}

@@ -1340,8 +1346,11 @@ static int process_renames(struct merge_options *o,
 			const char *ren2_src = ren2->pair->one->path;
 			const char *ren2_dst = ren2->pair->two->path;
 			enum rename_type rename_type;
-			if (strcmp(ren1_src, ren2_src) != 0)
+			if (strcmp(ren1_src, ren2_src) != 0) {
+				if (o->gentle)
+					return error("ren1_src != ren2_src");
 				die("ren1_src != ren2_src");
+			}
 			ren2->dst_entry->processed = 1;
 			ren2->processed = 1;
 			if (strcmp(ren1_dst, ren2_dst) != 0) {

@@ -1374,8 +1383,11 @@ static int process_renames(struct merge_options *o,
 			char *ren2_dst;
 			ren2 = lookup->util;
 			ren2_dst = ren2->pair->two->path;
-			if (strcmp(ren1_dst, ren2_dst) != 0)
+			if (strcmp(ren1_dst, ren2_dst) != 0) {
+				if (o->gentle)
+					return error("ren1_dst != ren2_dst");
 				die("ren1_dst != ren2_dst");
+			}
 
 			clean_merge = 0;
 			ren2->processed = 1;

@@ -1818,6 +1830,11 @@ int merge_trees(struct merge_options *o,
 	code = git_merge_trees(o->call_depth, common, head, merge);
 
 	if (code != 0) {
+		if (o->gentle)
+			return error(_("merging of trees %s and %s failed"),
+			    sha1_to_hex(head->object.sha1),
+			    sha1_to_hex(merge->object.sha1));
+
 		if (show(o, 4) || o->call_depth)
 			die(_("merging of trees %s and %s failed"),
 			    sha1_to_hex(head->object.sha1),

@@ -1864,8 +1881,8 @@ int merge_trees(struct merge_options *o,
 	else
 		clean = 1;
 
-	if (o->call_depth)
-		*result = write_tree_from_memory(o);
+	if (o->call_depth && !(*result = write_tree_from_memory(o)))
+		return -1;
 
 	return clean;
 }

@@ -1940,14 +1957,18 @@ int merge_recursive(struct merge_options *o,
 		saved_b2 = o->branch2;
 		o->branch1 = "Temporary merge branch 1";
 		o->branch2 = "Temporary merge branch 2";
-		merge_recursive(o, merged_common_ancestors, iter->item,
-				NULL, &merged_common_ancestors);
+		if (merge_recursive(o, merged_common_ancestors, iter->item,
+				NULL, &merged_common_ancestors) < 0)
+			return -1;
 		o->branch1 = saved_b1;
 		o->branch2 = saved_b2;
 		o->call_depth--;
 
-		if (!merged_common_ancestors)
+		if (!merged_common_ancestors) {
+			if (o->gentle)
+				return error(_("merge returned no commit"));
 			die(_("merge returned no commit"));
+		}
 	}
 
 	discard_cache();

@@ -1957,6 +1978,8 @@ int merge_recursive(struct merge_options *o,
 	o->ancestor = "merged common ancestors";
 	clean = merge_trees(o, h1->tree, h2->tree, merged_common_ancestors->tree,
 			    &mrtree);
+	if (clean < 0)
+		return clean;
 
 	if (o->call_depth) {
 		*result = make_virtual_commit(mrtree, "merged tree");

@@ -2013,6 +2036,9 @@ int merge_recursive_generic(struct merge_options *o,
 	hold_locked_index(lock, 1);
 	clean = merge_recursive(o, head_commit, next_commit, ca,
 			result);
+	if (clean < 0)
+		return clean;
+
 	if (active_cache_changed &&
 	    write_locked_index(&the_index, lock, COMMIT_LOCK))
 		return error(_("Unable to write index."));

diff --git a/merge-recursive.h b/merge-recursive.h
index 9e090a3..06f9b7e 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h

@@ -15,6 +15,7 @@ struct merge_options {
 	const char *subtree_shift;
 	unsigned buffer_output : 1;
 	unsigned renormalize : 1;
+	unsigned gentle : 1;
 	long xdl_opts;
 	int verbosity;
 	int diff_rename_limit;

-- 
2.6.1.windows.1

[PATCH 2/2] pull --rebase: reinstate helpful message on abort

From: Johannes Schindelin <hidden>
Date: 2016-06-15 23:06:48

When calling `git pull --rebase`, things can go wrong. In such a case,
we want to tell the user about the most common ways out of this fix:

	Patch failed at [...]
	[...]

	When you have resolved this problem, run "git rebase
	--continue". If you prefer to skip this patch, run "git rebase
	--skip" instead. To check out the original branch and stop
	rebasing, run "git rebase --abort".

However, with the switch to the builtin `git-am` we call the merge
recursive function directly, which usually die()s in case of an
error. Not what we want.

So let's set the newly-introduced `gentle` flag to get a chance to
print the helpful advice.

Reported by Brendan Forster.

Signed-off-by: Johannes Schindelin <redacted>
---
 builtin/am.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/am.c b/builtin/am.c
index 4f77e07..c472937 100644
--- a/builtin/am.c
+++ b/builtin/am.c

@@ -1653,6 +1653,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa
 
 	init_merge_options(&o);
 
+	o.gentle = 1;
 	o.branch1 = "HEAD";
 	his_tree_name = xstrfmt("%.*s", linelen(state->msg), state->msg);
 	o.branch2 = his_tree_name;

-- 
2.6.1.windows.1

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help