As a prelude to libification, we would like to propogate errors
upwards through the callchain, so as to correctly handle errors and
clean up afterwards. Also introduce a new function
"error_resolve_conflict" in advice.c in an attempt to unify the way
conflicts are reported by the various components of Git. As a general
guideline for libification, "die" should be only be called in two
cases: by toplevel callers like command-line argument parsing
routines, or when an irrecoverable situation is encountered.
Helped-by: Jonathan Nieder <jrnieder@xxxxxxxxx>
Signed-off-by: Ramkumar Ramachandra <artagnon@xxxxxxxxx>
---
Has the commit message justified this change fully?
How do I trap and handle the exit status from write_message in
do_pick_commit correctly? There's already a call to
do_recursive_merge whose exit status is being trapped -- what happens
when do_recursive_merge succeeds and write_message fails, or
viceversa?
Junio has suggested dropping error_errno, and simply using error and
returning -errno by hand in one email. Considering the number of
times I've used that tecnique, and I think we should get something
like an error_errno atleast for the sake of terseness.
advice.c | 14 +++++
advice.h | 1 +
builtin/revert.c | 158 ++++++++++++++++++++++++++++++-----------------------
3 files changed, 104 insertions(+), 69 deletions(-)
diff --git a/advice.c b/advice.c
index 0be4b5f..3c3c187 100644
--- a/advice.c
+++ b/advice.c
@@ -47,3 +47,17 @@ void NORETURN die_resolve_conflict(const char *me)
else
die("'%s' is not possible because you have unmerged files.", me);
}
+
+int error_resolve_conflict(const char *me)
+{
+ if (advice_resolve_conflict)
+ /*
+ * Message used both when 'git commit' fails and when
+ * other commands doing a merge do.
+ */
+ return error("'%s' is not possible because you have unmerged files.\n"
+ "Please, fix them up in the work tree, and then use 'git add/rm <file>' as\n"
+ "appropriate to mark resolution and make a commit, or use 'git commit -a'.", me);
+ else
+ return error("'%s' is not possible because you have unmerged files.", me);
+}
diff --git a/advice.h b/advice.h
index 3244ebb..7b7cea5 100644
--- a/advice.h
+++ b/advice.h
@@ -13,5 +13,6 @@ extern int advice_detached_head;
int git_default_advice_config(const char *var, const char *value);
extern void NORETURN die_resolve_conflict(const char *me);
+extern int error_resolve_conflict(const char *me);
#endif /* ADVICE_H */
diff --git a/builtin/revert.c b/builtin/revert.c
index f697e66..fefb18b 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -167,9 +167,11 @@ static char *get_encoding(const char *message)
{
const char *p = message, *eol;
- if (!p)
- die (_("Could not read commit message of %s"),
- sha1_to_hex(commit->object.sha1));
+ if (!p) {
+ error(_("Could not read commit message of %s"),
+ sha1_to_hex(commit->object.sha1));
+ return NULL;
+ }
while (*p && *p != '\n') {
for (eol = p + 1; *eol && *eol != '\n'; eol++)
; /* do nothing */
@@ -198,7 +200,7 @@ static void add_message_to_msg(struct strbuf *msgbuf, const char *message)
strbuf_addstr(msgbuf, p);
}
-static void write_cherry_pick_head(void)
+static int write_cherry_pick_head(void)
{
int fd;
struct strbuf buf = STRBUF_INIT;
@@ -206,12 +208,22 @@ static void write_cherry_pick_head(void)
strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
fd = open(git_path("CHERRY_PICK_HEAD"), O_WRONLY | O_CREAT, 0666);
- if (fd < 0)
- die_errno(_("Could not open '%s' for writing"),
- git_path("CHERRY_PICK_HEAD"));
- if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))
- die_errno(_("Could not write to '%s'"), git_path("CHERRY_PICK_HEAD"));
+ if (fd < 0) {
+ int err = errno;
+ strbuf_release(&buf);
+ error(_("Could not open '%s' for writing: %s"),
+ git_path("CHERRY_PICK_HEAD"), strerror(err));
+ return -err;
+ }
+ if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd)) {
+ int err = errno;
+ strbuf_release(&buf);
+ error(_("Could not write to '%s': %s"),
+ git_path("CHERRY_PICK_HEAD"), strerror(err));
+ return -err;
+ }
strbuf_release(&buf);
+ return 0;
}
static void advise(const char *advice, ...)
@@ -243,17 +255,22 @@ static void print_advice(void)
advise("and commit the result with 'git commit'");
}
-static void write_message(struct strbuf *msgbuf, const char *filename)
+static int write_message(struct strbuf *msgbuf, const char *filename)
{
static struct lock_file msg_file;
int msg_fd = hold_lock_file_for_update(&msg_file, filename,
LOCK_DIE_ON_ERROR);
- if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
- die_errno(_("Could not write to %s."), filename);
+ if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0) {
+ int err = errno;
+ strbuf_release(msgbuf);
+ error(_("Could not write to %s: %s"), filename, strerror(err));
+ return -err;
+ }
strbuf_release(msgbuf);
if (commit_lock_file(&msg_file) < 0)
- die(_("Error wrapping up %s"), filename);
+ return error(_("Error wrapping up %s"), filename);
+ return 0;
}
static struct tree *empty_tree(void)
@@ -266,25 +283,20 @@ static struct tree *empty_tree(void)
return tree;
}
-static NORETURN void die_dirty_index(const char *me)
+static int verify_resolution(const char *me)
{
- if (read_cache_unmerged()) {
- die_resolve_conflict(me);
- } else {
- if (advice_commit_before_merge) {
- if (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 (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 0;
+
+ return error_resolve_conflict(me);
+}
+
+static int error_dirty_worktree(const char *me)
+{
+ if (advice_commit_before_merge)
+ return error(_("Your local changes would be overwritten by %s.\n"
+ "Please, commit your changes or stash them to proceed."), me);
+ return error(_("Your local changes would be overwritten by %s.\n"), me);
}
static int fast_forward_to(const unsigned char *to, const unsigned char *from)
@@ -329,10 +341,12 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
next_tree, base_tree, &result);
if (active_cache_changed &&
- (write_cache(index_fd, active_cache, active_nr) ||
- commit_locked_index(&index_lock)))
+ (write_cache(index_fd, active_cache, active_nr) ||
+ commit_locked_index(&index_lock))) {
+ rollback_lock_file(&index_lock);
/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
- die(_("%s: Unable to write new index file"), me);
+ return error(_("%s: Unable to write new index file"), me);
+ }
rollback_lock_file(&index_lock);
if (!clean) {
@@ -397,19 +411,21 @@ static int do_pick_commit(void)
* that represents the "current" state for merge-recursive
* to work on.
*/
- if (write_cache_as_tree(head, 0, NULL))
- die (_("Your index file is unmerged."));
+ if (write_cache_as_tree(head, 0, NULL)) {
+ discard_cache();
+ return error(_("Your index file is unmerged."));
+ }
} else {
if (get_sha1("HEAD", head))
- die (_("You do not have a valid HEAD"));
- if (index_differs_from("HEAD", 0))
- die_dirty_index(me);
+ return error(_("You do not have a valid HEAD"));
+ if (index_differs_from("HEAD", 0) && !verify_resolution(me))
+ return error_dirty_worktree(me);
}
discard_cache();
if (!commit->parents) {
if (action == REVERT)
- die (_("Cannot revert a root commit"));
+ return error(_("Cannot revert a root commit"));
parent = NULL;
}
else if (commit->parents->next) {
@@ -418,19 +434,19 @@ static int do_pick_commit(void)
struct commit_list *p;
if (!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 != mainline && p;
cnt++)
p = p->next;
if (cnt != mainline || !p)
- die(_("Commit %s does not have parent %d"),
+ return error(_("Commit %s does not have parent %d"),
sha1_to_hex(commit->object.sha1), mainline);
parent = p->item;
- } else if (0 < mainline)
- die(_("Mainline was specified but commit %s is not a merge."),
+ } else if (mainline > 0)
+ return error(_("Mainline was specified but commit %s is not a merge."),
sha1_to_hex(commit->object.sha1));
else
parent = commit->parents->item;
@@ -441,11 +457,11 @@ static int do_pick_commit(void)
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"),
+ return error(_("%s: cannot parse parent commit %s"),
me, sha1_to_hex(parent->object.sha1));
if (get_message(commit->buffer, &msg) != 0)
- die(_("Cannot get commit message for %s"),
+ return error(_("Cannot get commit message for %s"),
sha1_to_hex(commit->object.sha1));
/*
@@ -484,7 +500,11 @@ static int do_pick_commit(void)
strbuf_addstr(&msgbuf, ")\n");
}
if (!no_commit)
- write_cherry_pick_head();
+ if ((res = write_cherry_pick_head())) {
+ free_message(&msg);
+ free(defmsg);
+ return res;
+ }
}
if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
@@ -524,44 +544,46 @@ static int do_pick_commit(void)
return res;
}
-static void prepare_revs(struct rev_info *revs)
+static int prepare_revs(struct rev_info *revs)
{
- int argc;
-
init_revisions(revs, NULL);
revs->no_walk = 1;
if (action != REVERT)
revs->reverse = 1;
- argc = setup_revisions(commit_argc, commit_argv, revs, NULL);
- if (argc > 1)
- usage(*revert_or_cherry_pick_usage());
+ if (setup_revisions(commit_argc, commit_argv, revs, NULL) > 1)
+ return error(_("usage: %s"), *revert_or_cherry_pick_usage());
if (prepare_revision_walk(revs))
- die(_("revision walk setup failed"));
+ return error(_("revision walk setup failed"));
if (!revs->commits)
- die(_("empty commit set passed"));
+ return error(_("empty commit set passed"));
+ return 0;
}
-static void read_and_refresh_cache(const char *me)
+static int read_and_refresh_cache(const char *me)
{
static struct lock_file index_lock;
int index_fd = hold_locked_index(&index_lock, 0);
if (read_index_preload(&the_index, NULL) < 0)
- die(_("git %s: failed to read the index"), me);
+ return error(_("%s: failed to read the index"), me);
refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
if (the_index.cache_changed) {
if (write_index(&the_index, index_fd) ||
- commit_locked_index(&index_lock))
- die(_("git %s: failed to refresh the index"), me);
+ commit_locked_index(&index_lock)) {
+ rollback_lock_file(&index_lock);
+ return error(_("%s: failed to refresh the index"), me);
+ }
}
rollback_lock_file(&index_lock);
+ return 0;
}
static int revert_or_cherry_pick(int argc, const char **argv)
{
struct rev_info revs;
+ int res;
git_config(git_default_config, NULL);
me = action == REVERT ? "revert" : "cherry-pick";
@@ -579,17 +601,15 @@ static int revert_or_cherry_pick(int argc, const char **argv)
die(_("cherry-pick --ff cannot be used with --edit"));
}
- read_and_refresh_cache(me);
+ if ((res = read_and_refresh_cache(me)) ||
+ (res = prepare_revs(&revs)))
+ return res;
- prepare_revs(&revs);
+ while ((commit = get_revision(&revs)) &&
+ !(res = do_pick_commit()))
+ ;
- while ((commit = get_revision(&revs))) {
- int res = do_pick_commit();
- if (res)
- return res;
- }
-
- return 0;
+ return res;
}
int cmd_revert(int argc, const char **argv, const char *prefix)
--
1.7.5.GIT
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html