Make error messages from cherry-pick/revert more sensible

The original "rewrite in C" did somewhat a sloppy job while
stealing code from git-write-tree.

The caller pretends as if the write_tree() function would return
an error code and being able to issue a sensible error message
itself, but write_tree() function just calls die() and never
returns an error.  Worse yet, the function claims that it was
running git-write-tree (which is no longer true after
cherry-pick stole it).

Tested-by: Björn Steinbrink <B.Steinbrink@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin-revert.c b/builtin-revert.c
index 358af537476b0b558989f27fe60627af6977635b..e219859f9b04dc94a2444346541e86503085fdea 100644 (file)
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -8,6 +8,7 @@
 #include "exec_cmd.h"
 #include "utf8.h"
 #include "parse-options.h"
+#include "cache-tree.h"
 
 /*
  * This implements the builtins revert and cherry-pick.
@@ -270,7 +271,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
                 * that represents the "current" state for merge-recursive
                 * to work on.
                 */
-               if (write_tree(head, 0, NULL))
+               if (write_cache_as_tree(head, 0, NULL))
                        die ("Your index file is unmerged.");
        } else {
                struct wt_status s;
@@ -357,7 +358,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
        if (merge_recursive(sha1_to_hex(base->object.sha1),
                                sha1_to_hex(head), "HEAD",
                                sha1_to_hex(next->object.sha1), oneline) ||
-                       write_tree(head, 0, NULL)) {
+                       write_cache_as_tree(head, 0, NULL)) {
                add_to_msg("\nConflicts:\n\n");
                read_cache();
                for (i = 0; i < active_nr;) {

diff --git a/builtin-write-tree.c b/builtin-write-tree.c
index d16b9ed0098fd5c12fe83638f51cc3f10fd87ed7..e838d01233eb1d6cfaa0885c6b37c8726079b0ac 100644 (file)
--- a/builtin-write-tree.c
+++ b/builtin-write-tree.c
@@ -11,63 +11,12 @@
 static const char write_tree_usage[] =
 "git-write-tree [--missing-ok] [--prefix=<prefix>/]";
 
-int write_tree(unsigned char *sha1, int missing_ok, const char *prefix)
-{
-       int entries, was_valid, newfd;
-
-       /* We can't free this memory, it becomes part of a linked list parsed atexit() */
-       struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
-
-       newfd = hold_locked_index(lock_file, 1);
-
-       entries = read_cache();
-       if (entries < 0)
-               die("git-write-tree: error reading cache");
-
-       if (!active_cache_tree)
-               active_cache_tree = cache_tree();
-
-       was_valid = cache_tree_fully_valid(active_cache_tree);
-
-       if (!was_valid) {
-               if (cache_tree_update(active_cache_tree,
-                                     active_cache, active_nr,
-                                     missing_ok, 0) < 0)
-                       die("git-write-tree: error building trees");
-               if (0 <= newfd) {
-                       if (!write_cache(newfd, active_cache, active_nr) &&
-                           !commit_lock_file(lock_file))
-                               newfd = -1;
-               }
-               /* Not being able to write is fine -- we are only interested
-                * in updating the cache-tree part, and if the next caller
-                * ends up using the old index with unupdated cache-tree part
-                * it misses the work we did here, but that is just a
-                * performance penalty and not a big deal.
-                */
-       }
-
-       if (prefix) {
-               struct cache_tree *subtree =
-                       cache_tree_find(active_cache_tree, prefix);
-               if (!subtree)
-                       die("git-write-tree: prefix %s not found", prefix);
-               hashcpy(sha1, subtree->sha1);
-       }
-       else
-               hashcpy(sha1, active_cache_tree->sha1);
-
-       if (0 <= newfd)
-               rollback_lock_file(lock_file);
-
-       return 0;
-}
-
 int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
 {
        int missing_ok = 0, ret;
        const char *prefix = NULL;
        unsigned char sha1[20];
+       const char *me = "git-write-tree";
 
        git_config(git_default_config);
        while (1 < argc) {
@@ -84,8 +33,20 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
        if (argc > 2)
                die("too many options");
 
-       ret = write_tree(sha1, missing_ok, prefix);
-       printf("%s\n", sha1_to_hex(sha1));
-
+       ret = write_cache_as_tree(sha1, missing_ok, prefix);
+       switch (ret) {
+       case 0:
+               printf("%s\n", sha1_to_hex(sha1));
+               break;
+       case WRITE_TREE_UNREADABLE_INDEX:
+               die("%s: error reading the index", me);
+               break;
+       case WRITE_TREE_UNMERGED_INDEX:
+               die("%s: error building trees; the index is unmerged?", me);
+               break;
+       case WRITE_TREE_PREFIX_ERROR:
+               die("%s: prefix %s not found", me, prefix);
+               break;
+       }
        return ret;
 }

diff --git a/builtin.h b/builtin.h
index cb675c4d7a80b99a06d6d04156e767ea025eb512..3d1628c597b21cc22b750809c27d4499e725d259 100644 (file)
--- a/builtin.h
+++ b/builtin.h
@@ -8,7 +8,6 @@ extern const char git_usage_string[];
 
 extern void list_common_cmds_help(void);
 extern void help_unknown_cmd(const char *cmd);
-extern int write_tree(unsigned char *sha1, int missing_ok, const char *prefix);
 extern void prune_packed_objects(int);
 
 extern int cmd_add(int argc, const char **argv, const char *prefix);

diff --git a/cache-tree.c b/cache-tree.c
index 50b35264fd0405a299700ef8bf4a61f416f30e46..4e1e7b25cf3e5cdbff99524f197c8341d26ac10a 100644 (file)
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -529,3 +529,58 @@ struct cache_tree *cache_tree_find(struct cache_tree *it, const char *path)
        }
        return it;
 }
+
+int write_cache_as_tree(unsigned char *sha1, int missing_ok, const char *prefix)
+{
+       int entries, was_valid, newfd;
+
+       /*
+        * We can't free this memory, it becomes part of a linked list
+        * parsed atexit()
+        */
+       struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
+
+       newfd = hold_locked_index(lock_file, 1);
+
+       entries = read_cache();
+       if (entries < 0)
+               return WRITE_TREE_UNREADABLE_INDEX;
+
+       if (!active_cache_tree)
+               active_cache_tree = cache_tree();
+
+       was_valid = cache_tree_fully_valid(active_cache_tree);
+
+       if (!was_valid) {
+               if (cache_tree_update(active_cache_tree,
+                                     active_cache, active_nr,
+                                     missing_ok, 0) < 0)
+                       return WRITE_TREE_UNMERGED_INDEX;
+               if (0 <= newfd) {
+                       if (!write_cache(newfd, active_cache, active_nr) &&
+                           !commit_lock_file(lock_file))
+                               newfd = -1;
+               }
+               /* Not being able to write is fine -- we are only interested
+                * in updating the cache-tree part, and if the next caller
+                * ends up using the old index with unupdated cache-tree part
+                * it misses the work we did here, but that is just a
+                * performance penalty and not a big deal.
+                */
+       }
+
+       if (prefix) {
+               struct cache_tree *subtree =
+                       cache_tree_find(active_cache_tree, prefix);
+               if (!subtree)
+                       return WRITE_TREE_PREFIX_ERROR;
+               hashcpy(sha1, subtree->sha1);
+       }
+       else
+               hashcpy(sha1, active_cache_tree->sha1);
+
+       if (0 <= newfd)
+               rollback_lock_file(lock_file);
+
+       return 0;
+}

diff --git a/cache-tree.h b/cache-tree.h
index 8243228e49ffd7078a783582be6ce79c97541a9c..44aad426d319b83eda013e115f35e066cc590cb8 100644 (file)
--- a/cache-tree.h
+++ b/cache-tree.h
@@ -30,4 +30,9 @@ int cache_tree_update(struct cache_tree *, struct cache_entry **, int, int, int)
 
 struct cache_tree *cache_tree_find(struct cache_tree *, const char *);
 
+#define WRITE_TREE_UNREADABLE_INDEX (-1)
+#define WRITE_TREE_UNMERGED_INDEX (-2)
+#define WRITE_TREE_PREFIX_ERROR (-3)
+
+int write_cache_as_tree(unsigned char *sha1, int missing_ok, const char *prefix);
 #endif