git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 18/18] revert: Propogate errors upwards from do_pick_commit

From
Ramkumar Ramachandra <artagnon@gmail.com>
Date
Jul 28, 2011, 16:52 UTC
Message-ID
<1311871951-3497-19-git-send-email-artagnon@gmail.com>
In-Reply-To
<1311871951-3497-1-git-send-email-artagnon@gmail.com>

Currently, revert_or_cherry_pick can fail in two ways. If it encounters a conflict, it returns a positive number indicating the intended exit status for the git wrapper to pass on; for all other errors, it calls die(). The latter behavior is inconsiderate towards callers, as it denies them the opportunity to recover from errors and do other things.

After this patch, revert_or_cherry_pick will still return a positive return value to indicate an exit status for conflicts as before, while for some other errors, it will print an error message and return -1 instead of die()-ing. The cmd_revert and cmd_cherry_pick are adjusted to handle the fatal errors by die()-ing themselves.

While the full benefits of this patch will only be seen once all the "die" calls are replaced with calls to "error", its immediate impact is to change some "fatal:" messages to say "error:" and to add a new "fatal: cherry-pick failed" message at the end when the operation fails.

Inspired-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/revert.c |   86 +++++++++++++++++++++++++-----------------------------
 1 files changed, 40 insertions(+), 46 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 985f49f..f30dbd4 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -66,15 +66,6 @@ struct replay_opts {
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
-static void fatal(const char *advice, ...)
-{
-	va_list params;
-
-	va_start(params, advice);
-	vreportf("fatal: ", advice, params);
-	va_end(params);
-}
-
 static const char *action_name(const struct replay_opts *opts)
 {
 	return opts->action == REVERT ? "revert" : "cherry-pick";
@@ -359,25 +350,20 @@ static struct tree *empty_tree(void)
 	return tree;
 }
 
-static NORETURN void die_dirty_index(struct replay_opts *opts)
+static int error_dirty_index(struct replay_opts *opts)
 {
-	if (read_cache_unmerged()) {
-		die_resolve_conflict(action_name(opts));
-	} else {
-		if (advice_commit_before_merge) {
-			if (opts->action == REVERT)
-				die(_("Your local changes would be overwritten by revert.\n"
-					  "Please, commit your changes or stash them to proceed."));
-			else
-				die(_("Your local changes would be overwritten by cherry-pick.\n"
-					  "Please, commit your changes or stash them to proceed."));
-		} else {
-			if (opts->action == REVERT)
-				die(_("Your local changes would be overwritten by revert.\n"));
-			else
-				die(_("Your local changes would be overwritten by cherry-pick.\n"));
-		}
-	}
+	if (read_cache_unmerged())
+		return error_resolve_conflict(action_name(opts));
+
+	/* Different translation strings for cherry-pick and revert */
+	if (opts->action == CHERRY_PICK)
+		error(_("Your local changes would be overwritten by cherry-pick."));
+	else
+		error(_("Your local changes would be overwritten by revert."));
+
+	if (advice_commit_before_merge)
+		advise(_("Commit your changes or stash them to proceed."));
+	return -1;
 }
 
 static int fast_forward_to(const unsigned char *to, const unsigned char *from)
@@ -495,9 +481,9 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 			die (_("Your index file is unmerged."));
 	} else {
 		if (get_sha1("HEAD", head))
-			die (_("You do not have a valid HEAD"));
+			return error(_("You do not have a valid HEAD"));
 		if (index_differs_from("HEAD", 0))
-			die_dirty_index(opts);
+			return error_dirty_index(opts);
 	}
 	discard_cache();
 
@@ -510,20 +496,20 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		struct commit_list *p;
 
 		if (!opts->mainline)
-			die(_("Commit %s is a merge but no -m option was given."),
-			    sha1_to_hex(commit->object.sha1));
+			return error(_("Commit %s is a merge but no -m option was given."),
+				sha1_to_hex(commit->object.sha1));
 
 		for (cnt = 1, p = commit->parents;
 		     cnt != opts->mainline && p;
 		     cnt++)
 			p = p->next;
 		if (cnt != opts->mainline || !p)
-			die(_("Commit %s does not have parent %d"),
-			    sha1_to_hex(commit->object.sha1), opts->mainline);
+			return error(_("Commit %s does not have parent %d"),
+				sha1_to_hex(commit->object.sha1), opts->mainline);
 		parent = p->item;
 	} else if (0 < opts->mainline)
-		die(_("Mainline was specified but commit %s is not a merge."),
-		    sha1_to_hex(commit->object.sha1));
+		return error(_("Mainline was specified but commit %s is not a merge."),
+			sha1_to_hex(commit->object.sha1));
 	else
 		parent = commit->parents->item;
 
@@ -533,12 +519,12 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	if (parent && parse_commit(parent) < 0)
 		/* TRANSLATORS: The first %s will be "revert" or
 		   "cherry-pick", the second %s a SHA1 */
-		die(_("%s: cannot parse parent commit %s"),
-		    action_name(opts), sha1_to_hex(parent->object.sha1));
+		return error(_("%s: cannot parse parent commit %s"),
+			action_name(opts), sha1_to_hex(parent->object.sha1));
 
 	if (get_message(commit, &msg) != 0)
-		die(_("Cannot get commit message for %s"),
-				sha1_to_hex(commit->object.sha1));
+		return error(_("Cannot get commit message for %s"),
+			sha1_to_hex(commit->object.sha1));
 
 	/*
 	 * "commit" is an existing commit.  We would want to apply
@@ -990,27 +976,28 @@ static int pick_revisions(struct replay_opts *opts)
 
 		walk_revs_populate_todo(&todo_list, opts);
 		if (create_seq_dir() < 0) {
-			fatal(_("A cherry-pick or revert is in progress."));
+			error(_("A cherry-pick or revert is in progress."));
 			advise(_("Use --continue to continue the operation"));
 			advise(_("or --reset to forget about it"));
-			exit(128);
+			return -1;
 		}
 		if (get_sha1("HEAD", sha1)) {
 			if (opts->action == REVERT)
-				die(_("Can't revert as initial commit"));
-			die(_("Can't cherry-pick into empty head"));
+				return error(_("Can't revert as initial commit"));
+			return error(_("Can't cherry-pick into empty head"));
 		}
 		save_head(sha1_to_hex(sha1));
 		save_opts(opts);
 	}
 	return pick_commits(todo_list, opts);
 error:
-	die(_("No %s in progress"), action_name(opts));
+	return error(_("No %s in progress"), action_name(opts));
 }
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	struct replay_opts opts;
+	int res;
 
 	memset(&opts, 0, sizeof(opts));
 	if (isatty(0))
@@ -1024,16 +1011,23 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	 * cherry-pick should be handled differently from an existing
 	 * one that is being continued
 	 */
-	return pick_revisions(&opts);
+	res = pick_revisions(&opts);
+	if (res < 0)
+		die(_("revert failed"));
+	return res;
 }
 
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
 	struct replay_opts opts;
+	int res;
 
 	memset(&opts, 0, sizeof(opts));
 	opts.action = CHERRY_PICK;
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
-	return pick_revisions(&opts);
+	res = pick_revisions(&opts);
+	if (res < 0)
+		die(_("cherry-pick failed"));
+	return res;
 }

-- 
1.7.4.rc1.7.g2cf08.dirty
Previous: Ramkumar RamachandraNext: Ramkumar Ramachandra
Message 35 of 36 in “Sequencer for inclusion v4”
  1. 00/18 Sequencer for inclusion v4Ramkumar Ramachandra, Jul 28, 2011
  2. 01/18 advice: Introduce error_resolve_conflictRamkumar Ramachandra, Jul 28, 2011
  3. 02/18 config: Introduce functions to write non-standard fileRamkumar Ramachandra, Jul 28, 2011
  4. Jonathan NiederJul 28, 2011
  5. Ramkumar RamachandraJul 28, 2011
  6. 03/18 revert: Simplify and inline add_message_to_msgRamkumar Ramachandra, Jul 28, 2011
  7. 04/18 revert: Don't check lone argument in get_encodingRamkumar Ramachandra, Jul 28, 2011
  8. 05/18 revert: Rename no_replay to record_originRamkumar Ramachandra, Jul 28, 2011
  9. 06/18 revert: Eliminate global "commit" variableRamkumar Ramachandra, Jul 28, 2011
  10. 07/18 revert: Introduce struct to keep command-line optionsRamkumar Ramachandra, Jul 28, 2011
  11. 08/18 revert: Separate cmdline parsing from functional codeRamkumar Ramachandra, Jul 28, 2011
  12. 09/18 revert: Don't create invalid replay_opts in parse_argsRamkumar Ramachandra, Jul 28, 2011
  13. 10/18 revert: Save data for continuing after conflict resolutionRamkumar Ramachandra, Jul 28, 2011
  14. Jonathan NiederJul 28, 2011
  15. Ramkumar RamachandraJul 28, 2011
  16. 11/18 revert: Save command-line options for continuing operationRamkumar Ramachandra, Jul 28, 2011
  17. Christian CouderJul 31, 2011
  18. Ramkumar RamachandraAug 1, 2011
  19. 12/18 revert: Make pick_commits functionally act on a commit listRamkumar Ramachandra, Jul 28, 2011
  20. Christian CouderJul 31, 2011
  21. Christian CouderJul 31, 2011
  22. Ramkumar RamachandraAug 1, 2011
  23. 13/18 revert: Introduce --reset to remove sequencer stateRamkumar Ramachandra, Jul 28, 2011
  24. Christian CouderJul 31, 2011
  25. Ramkumar RamachandraAug 1, 2011
  26. 14/18 reset: Make reset remove the sequencer stateRamkumar Ramachandra, Jul 28, 2011
  27. 15/18 revert: Remove sequencer state when no commits are pendingRamkumar Ramachandra, Jul 28, 2011
  28. 16/18 revert: Don't implictly stomp pending sequencer operationRamkumar Ramachandra, Jul 28, 2011
  29. 17/18 revert: Introduce --continue to continue the operationRamkumar Ramachandra, Jul 28, 2011
  30. Christian CouderJul 31, 2011
  31. Ramkumar RamachandraAug 1, 2011
  32. Jonathan NiederAug 3, 2011
  33. Junio C HamanoAug 3, 2011
  34. Ramkumar RamachandraAug 3, 2011
  35. 18/18 revert: Propogate errors upwards from do_pick_commitRamkumar Ramachandra, Jul 28, 2011
  36. Ramkumar RamachandraJul 28, 2011

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.