{"thread":{"id":"11943","subject":"[PATCH v2 01/11] Allow callers of unpack_trees() to handle failure","startedAt":"2008-02-07T16:39:48Z","lastAt":"2008-02-07T16:39:48Z","messageCount":1,"participants":["Daniel Barkalow"],"isPatch":true,"patchVersion":2,"patchTotal":11},"messages":[{"id":"67823","messageId":"alpine.LNX.1.00.0802071117080.13593@iabervon.org","threadId":"11943","inReplyTo":null,"subject":"[PATCH v2 01/11] Allow callers of unpack_trees() to handle failure","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-02-07T16:39:48Z","receivedAt":"2008-02-07T16:39:48Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"Return an error from unpack_trees() instead of calling die(), and exit\nwith an error in read-tree, builtin-commit, and diff-lib. merge-recursive \nalready expected an error return from unpack_trees, so it doesn't need to \nbe changed. The merge function can return negative to abort.\n\nThis will be used in builtin-checkout -m.\n\nSigned-off-by: Daniel Barkalow <barkalow@iabervon.org>\n---\n builtin-commit.c    |    3 +-\n builtin-read-tree.c |    3 +-\n diff-lib.c          |    6 ++-\n unpack-trees.c      |   85 ++++++++++++++++++++++++++++----------------------\n 4 files changed, 56 insertions(+), 41 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex a6ecd30..ae34bed 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -204,7 +204,8 @@ static void create_base_index(void)\n \t\tdie(\"failed to unpack HEAD tree object\");\n \tparse_tree(tree);\n \tinit_tree_desc(&t, tree->buffer, tree->size);\n-\tunpack_trees(1, &t, &opts);\n+\tif (unpack_trees(1, &t, &opts))\n+\t\texit(128); /* We've already reported the error, finish dying */\n }\n \n static char *prepare_index(int argc, const char **argv, const char *prefix)\ndiff --git a/builtin-read-tree.c b/builtin-read-tree.c\nindex 5785401..1d9d125 100644\n--- a/builtin-read-tree.c\n+++ b/builtin-read-tree.c\n@@ -268,7 +268,8 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \t\tparse_tree(tree);\n \t\tinit_tree_desc(t+i, tree->buffer, tree->size);\n \t}\n-\tunpack_trees(nr_trees, t, &opts);\n+\tif (unpack_trees(nr_trees, t, &opts))\n+\t\treturn 128;\n \n \t/*\n \t * When reading only one tree (either the most basic form,\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 03eaa7c..94b150e 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -737,7 +737,8 @@ int run_diff_index(struct rev_info *revs, int cached)\n \topts.unpack_data = revs;\n \n \tinit_tree_desc(&t, tree->buffer, tree->size);\n-\tunpack_trees(1, &t, &opts);\n+\tif (unpack_trees(1, &t, &opts))\n+\t\texit(128);\n \n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\n@@ -789,6 +790,7 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)\n \topts.unpack_data = &revs;\n \n \tinit_tree_desc(&t, tree->buffer, tree->size);\n-\tunpack_trees(1, &t, &opts);\n+\tif (unpack_trees(1, &t, &opts))\n+\t\texit(128);\n \treturn 0;\n }\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex ec558f9..9271bf3 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -219,6 +219,8 @@ static int unpack_trees_rec(struct tree_entry_list **posns, int len,\n \t\t\t\t}\n #endif\n \t\t\t\tret = o->fn(src, o, remove);\n+\t\t\t\tif (ret < 0)\n+\t\t\t\t\treturn ret;\n \n #if DBRT_DEBUG > 1\n \t\t\t\tprintf(\"Added %d entries\\n\", ret);\n@@ -359,7 +361,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t}\n \n \tif (o->trivial_merges_only && o->nontrivial_merge)\n-\t\tdie(\"Merge requires file-level merging\");\n+\t\treturn error(\"Merge requires file-level merging\");\n \n \tcheck_updates(active_cache, active_nr, o);\n \treturn 0;\n@@ -367,10 +369,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \n /* Here come the merge functions */\n \n-static void reject_merge(struct cache_entry *ce)\n+static int reject_merge(struct cache_entry *ce)\n {\n-\tdie(\"Entry '%s' would be overwritten by merge. Cannot merge.\",\n-\t    ce->name);\n+\treturn error(\"Entry '%s' would be overwritten by merge. Cannot merge.\",\n+\t\t     ce->name);\n }\n \n static int same(struct cache_entry *a, struct cache_entry *b)\n@@ -388,18 +390,18 @@ static int same(struct cache_entry *a, struct cache_entry *b)\n  * When a CE gets turned into an unmerged entry, we\n  * want it to be up-to-date\n  */\n-static void verify_uptodate(struct cache_entry *ce,\n+static int verify_uptodate(struct cache_entry *ce,\n \t\tstruct unpack_trees_options *o)\n {\n \tstruct stat st;\n \n \tif (o->index_only || o->reset)\n-\t\treturn;\n+\t\treturn 0;\n \n \tif (!lstat(ce->name, &st)) {\n \t\tunsigned changed = ce_match_stat(ce, &st, CE_MATCH_IGNORE_VALID);\n \t\tif (!changed)\n-\t\t\treturn;\n+\t\t\treturn 0;\n \t\t/*\n \t\t * NEEDSWORK: the current default policy is to allow\n \t\t * submodule to be out of sync wrt the supermodule\n@@ -408,12 +410,12 @@ static void verify_uptodate(struct cache_entry *ce,\n \t\t * checked out.\n \t\t */\n \t\tif (S_ISGITLINK(ce->ce_mode))\n-\t\t\treturn;\n+\t\t\treturn 0;\n \t\terrno = 0;\n \t}\n \tif (errno == ENOENT)\n-\t\treturn;\n-\tdie(\"Entry '%s' not uptodate. Cannot merge.\", ce->name);\n+\t\treturn 0;\n+\treturn error(\"Entry '%s' not uptodate. Cannot merge.\", ce->name);\n }\n \n static void invalidate_ce_path(struct cache_entry *ce)\n@@ -479,7 +481,8 @@ static int verify_clean_subdirectory(struct cache_entry *ce, const char *action,\n \t\t * ce->name is an entry in the subdirectory.\n \t\t */\n \t\tif (!ce_stage(ce)) {\n-\t\t\tverify_uptodate(ce, o);\n+\t\t\tif (verify_uptodate(ce, o))\n+\t\t\t\treturn -1;\n \t\t\tce->ce_flags |= CE_REMOVE;\n \t\t}\n \t\tcnt++;\n@@ -498,8 +501,8 @@ static int verify_clean_subdirectory(struct cache_entry *ce, const char *action,\n \t\td.exclude_per_dir = o->dir->exclude_per_dir;\n \ti = read_directory(&d, ce->name, pathbuf, namelen+1, NULL);\n \tif (i)\n-\t\tdie(\"Updating '%s' would lose untracked files in it\",\n-\t\t    ce->name);\n+\t\treturn error(\"Updating '%s' would lose untracked files in it\",\n+\t\t\t     ce->name);\n \tfree(pathbuf);\n \treturn cnt;\n }\n@@ -508,16 +511,16 @@ static int verify_clean_subdirectory(struct cache_entry *ce, const char *action,\n  * We do not want to remove or overwrite a working tree file that\n  * is not tracked, unless it is ignored.\n  */\n-static void verify_absent(struct cache_entry *ce, const char *action,\n-\t\tstruct unpack_trees_options *o)\n+static int verify_absent(struct cache_entry *ce, const char *action,\n+\t\t\t struct unpack_trees_options *o)\n {\n \tstruct stat st;\n \n \tif (o->index_only || o->reset || !o->update)\n-\t\treturn;\n+\t\treturn 0;\n \n \tif (has_symlink_leading_path(ce->name, NULL))\n-\t\treturn;\n+\t\treturn 0;\n \n \tif (!lstat(ce->name, &st)) {\n \t\tint cnt;\n@@ -528,7 +531,7 @@ static void verify_absent(struct cache_entry *ce, const char *action,\n \t\t\t * ce->name is explicitly excluded, so it is Ok to\n \t\t\t * overwrite it.\n \t\t\t */\n-\t\t\treturn;\n+\t\t\treturn 0;\n \t\tif (S_ISDIR(st.st_mode)) {\n \t\t\t/*\n \t\t\t * We are checking out path \"foo\" and\n@@ -557,7 +560,7 @@ static void verify_absent(struct cache_entry *ce, const char *action,\n \t\t\t * deleted entries here.\n \t\t\t */\n \t\t\to->pos += cnt;\n-\t\t\treturn;\n+\t\t\treturn 0;\n \t\t}\n \n \t\t/*\n@@ -569,12 +572,13 @@ static void verify_absent(struct cache_entry *ce, const char *action,\n \t\tif (0 <= cnt) {\n \t\t\tstruct cache_entry *ce = active_cache[cnt];\n \t\t\tif (ce->ce_flags & CE_REMOVE)\n-\t\t\t\treturn;\n+\t\t\t\treturn 0;\n \t\t}\n \n-\t\tdie(\"Untracked working tree file '%s' \"\n-\t\t    \"would be %s by merge.\", ce->name, action);\n+\t\treturn error(\"Untracked working tree file '%s' \"\n+\t\t\t     \"would be %s by merge.\", ce->name, action);\n \t}\n+\treturn 0;\n }\n \n static int merged_entry(struct cache_entry *merge, struct cache_entry *old,\n@@ -592,12 +596,14 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,\n \t\tif (same(old, merge)) {\n \t\t\tmemcpy(merge, old, offsetof(struct cache_entry, name));\n \t\t} else {\n-\t\t\tverify_uptodate(old, o);\n+\t\t\tif (verify_uptodate(old, o))\n+\t\t\t\treturn -1;\n \t\t\tinvalidate_ce_path(old);\n \t\t}\n \t}\n \telse {\n-\t\tverify_absent(merge, \"overwritten\", o);\n+\t\tif (verify_absent(merge, \"overwritten\", o))\n+\t\t\treturn -1;\n \t\tinvalidate_ce_path(merge);\n \t}\n \n@@ -609,10 +615,12 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,\n static int deleted_entry(struct cache_entry *ce, struct cache_entry *old,\n \t\tstruct unpack_trees_options *o)\n {\n-\tif (old)\n-\t\tverify_uptodate(old, o);\n-\telse\n-\t\tverify_absent(ce, \"removed\", o);\n+\tif (old) {\n+\t\tif (verify_uptodate(old, o))\n+\t\t\treturn -1;\n+\t} else\n+\t\tif (verify_absent(ce, \"removed\", o))\n+\t\t\treturn -1;\n \tce->ce_flags |= CE_REMOVE;\n \tadd_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n \tinvalidate_ce_path(ce);\n@@ -700,7 +708,7 @@ int threeway_merge(struct cache_entry **stages,\n \t/* #14, #14ALT, #2ALT */\n \tif (remote && !df_conflict_head && head_match && !remote_match) {\n \t\tif (index && !same(index, remote) && !same(index, head))\n-\t\t\treject_merge(index);\n+\t\t\treturn reject_merge(index);\n \t\treturn merged_entry(remote, index, o);\n \t}\n \t/*\n@@ -708,7 +716,7 @@ int threeway_merge(struct cache_entry **stages,\n \t * make sure that it matches head.\n \t */\n \tif (index && !same(index, head)) {\n-\t\treject_merge(index);\n+\t\treturn reject_merge(index);\n \t}\n \n \tif (head) {\n@@ -759,8 +767,10 @@ int threeway_merge(struct cache_entry **stages,\n \t\t\tremove_entry(remove);\n \t\t\tif (index)\n \t\t\t\treturn deleted_entry(index, index, o);\n-\t\t\telse if (ce && !head_deleted)\n-\t\t\t\tverify_absent(ce, \"removed\", o);\n+\t\t\telse if (ce && !head_deleted) {\n+\t\t\t\tif (verify_absent(ce, \"removed\", o))\n+\t\t\t\t\treturn -1;\n+\t\t\t}\n \t\t\treturn 0;\n \t\t}\n \t\t/*\n@@ -776,7 +786,8 @@ int threeway_merge(struct cache_entry **stages,\n \t * conflict resolution files.\n \t */\n \tif (index) {\n-\t\tverify_uptodate(index, o);\n+\t\tif (verify_uptodate(index, o))\n+\t\t\treturn -1;\n \t}\n \n \tremove_entry(remove);\n@@ -856,11 +867,11 @@ int twoway_merge(struct cache_entry **src,\n \t\t\t/* all other failures */\n \t\t\tremove_entry(remove);\n \t\t\tif (oldtree)\n-\t\t\t\treject_merge(oldtree);\n+\t\t\t\treturn reject_merge(oldtree);\n \t\t\tif (current)\n-\t\t\t\treject_merge(current);\n+\t\t\t\treturn reject_merge(current);\n \t\t\tif (newtree)\n-\t\t\t\treject_merge(newtree);\n+\t\t\t\treturn reject_merge(newtree);\n \t\t\treturn -1;\n \t\t}\n \t}\n@@ -887,7 +898,7 @@ int bind_merge(struct cache_entry **src,\n \t\treturn error(\"Cannot do a bind merge of %d trees\\n\",\n \t\t\t     o->merge_size);\n \tif (a && old)\n-\t\tdie(\"Entry '%s' overlaps.  Cannot bind.\", a->name);\n+\t\treturn error(\"Entry '%s' overlaps.  Cannot bind.\", a->name);\n \tif (!a)\n \t\treturn keep_entry(old, o);\n \telse\n-- \n1.5.4\n"}]}