{"thread":{"id":"11874","subject":"[PATCH 1/9] Allow callers of unpack_trees() to handle failure","startedAt":"2008-02-04T18:35:35Z","lastAt":"2008-02-05T23:43:43Z","messageCount":4,"participants":["Daniel Barkalow","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"67399","messageId":"alpine.LNX.1.00.0802041334450.13593@iabervon.org","threadId":"11874","inReplyTo":null,"subject":"[PATCH 1/9] Allow callers of unpack_trees() to handle failure","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-02-04T18:35:35Z","receivedAt":"2008-02-04T18:35:35Z","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 and builtin-commit. merge-recursive already\nexpected an error return from unpack_trees, so it doesn't need to be\nchanged. 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 unpack-trees.c      |   85 ++++++++++++++++++++++++++++----------------------\n 3 files changed, 52 insertions(+), 39 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 2ffca40..6c817ea 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/unpack-trees.c b/unpack-trees.c\nindex ff46fd6..9e6587f 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@@ -527,7 +530,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@@ -556,7 +559,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@@ -568,12 +571,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@@ -591,12 +595,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@@ -608,10 +614,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@@ -699,7 +707,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@@ -707,7 +715,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@@ -758,8 +766,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@@ -775,7 +785,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@@ -855,11 +866,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@@ -886,7 +897,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"},{"id":"67461","messageId":"alpine.LSU.1.00.0802050107440.8543@racer.site","threadId":"11874","inReplyTo":"alpine.LNX.1.00.0802041334450.13593@iabervon.org","subject":"Re: [PATCH 1/9] Allow callers of unpack_trees() to handle failure","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-05T01:11:02Z","receivedAt":"2008-02-05T01:11:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 4 Feb 2008, Daniel Barkalow wrote:\n\n> Return an error from unpack_trees() instead of calling die(), and exit\n> with an error in read-tree and builtin-commit.\n\nTechnically, there is still a possibility that an xcalloc() fails with a \ndie(), but I agree that it is a good change.\n\nBTW after the whole series (was too lazy to check the individual \npatches), I count one instance in builtin-checkout.c and two in diff-lib.c \nwhere unpack_trees()' return value is not checked.  Is this intended?\n\nCiao,\nDscho\n"},{"id":"67547","messageId":"alpine.LNX.1.00.0802051428180.13593@iabervon.org","threadId":"11874","inReplyTo":"alpine.LSU.1.00.0802050107440.8543@racer.site","subject":"Re: [PATCH 1/9] Allow callers of unpack_trees() to handle failure","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-02-05T20:38:26Z","receivedAt":"2008-02-05T20:38:26Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Tue, 5 Feb 2008, Johannes Schindelin wrote:\n\n> Hi,\n> \n> On Mon, 4 Feb 2008, Daniel Barkalow wrote:\n> \n> > Return an error from unpack_trees() instead of calling die(), and exit\n> > with an error in read-tree and builtin-commit.\n> \n> Technically, there is still a possibility that an xcalloc() fails with a \n> die(), but I agree that it is a good change.\n\nRight, this is just for the case of the data causing the operation \nrequested to be unsafe or impossible.\n\n> BTW after the whole series (was too lazy to check the individual \n> patches), I count one instance in builtin-checkout.c and two in diff-lib.c \n> where unpack_trees()' return value is not checked.  Is this intended?\n\nI don't think any of them can fail (a diff really ought not abort due to \nthe things you're comparing being incompatible, and the reset_clean_to_new \nsimilarly can't have any conflicts), but it's probably best to check and \nexit anyway, if only in the conversion commit. (If we remove the check \nafterward, there will be a change in the history that documents that we \ndon't need to check)\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"67588","messageId":"7vd4rb7yvk.fsf@gitster.siamese.dyndns.org","threadId":"11874","inReplyTo":"alpine.LNX.1.00.0802051428180.13593@iabervon.org","subject":"Re: [PATCH 1/9] Allow callers of unpack_trees() to handle failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-05T23:43:43Z","receivedAt":"2008-02-05T23:43:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> writes:\n\n> On Tue, 5 Feb 2008, Johannes Schindelin wrote:\n> ...\n>> BTW after the whole series (was too lazy to check the individual \n>> patches), I count one instance in builtin-checkout.c and two in diff-lib.c \n>> where unpack_trees()' return value is not checked.  Is this intended?\n>\n> ..., but it's probably best to check and \n> exit anyway, if only in the conversion commit. (If we remove the check \n> afterward, there will be a change in the history that documents that we \n> don't need to check)\n\nI think that is a sensible thing to do.\n"}]}