{"thread":{"id":"24119","subject":"[PATCH 0/5 v2] unpack_trees: nicer error messages","startedAt":"2010-06-15T12:22:51Z","lastAt":"2010-06-15T13:40:16Z","messageCount":13,"participants":["Diane Gasselin","Matthieu Moy"],"isPatch":true,"patchVersion":2,"patchTotal":5},"messages":[{"id":"143733","messageId":"1276604576-28092-1-git-send-email-diane.gasselin@ensimag.imag.fr","threadId":"24119","inReplyTo":null,"subject":"[PATCH 0/5 v2] unpack_trees: nicer error messages","fromName":"Diane Gasselin","fromEmail":"diane.gasselin@ensimag.imag.fr","sentAt":"2010-06-15T12:22:51Z","receivedAt":"2010-06-15T12:22:51Z","isPatch":true,"sender":{"key":"diane.gasselin@ensimag.imag.fr","avatar":null},"body":"This patch serie aims at grouping porcerlain merge and checkout \nerrors messages by type if possible, listing all the file concerned\nby the error type.\nIt also adds porcelain messages for checkout.\n\nIt was first introduced in the thread:\nhttp://mid.gmane.org/7v63277f92.fsf@alter.siamese.dyndns.org\n\nDiane Gasselin (5):\n  merge-recursive: update merge porcelain messages for checkout\n  unpack_trees: group errors by type\n  unpack_trees_options: update porcelain messages\n  tests: update porcelain expected message\n  t7609: test merge and checkout error messages\n\n builtin/checkout.c             |    4 +-\n builtin/merge.c                |    3 +-\n merge-recursive.c              |   48 ++++++++++------\n merge-recursive.h              |    6 +-\n t/t3030-merge-recursive.sh     |    2 +-\n t/t3400-rebase.sh              |    2 +-\n t/t7609-merge-co-error-msgs.sh |  125 ++++++++++++++++++++++++++++++++++++++++\n tree-walk.c                    |   11 +++-\n unpack-trees.c                 |  119 +++++++++++++++++++++++++++++++++++---\n unpack-trees.h                 |   31 ++++++++++-\n 10 files changed, 316 insertions(+), 35 deletions(-)\n create mode 100755 t/t7609-merge-co-error-msgs.sh\n"},{"id":"143735","messageId":"1276604576-28092-2-git-send-email-diane.gasselin@ensimag.imag.fr","threadId":"24119","inReplyTo":"1276604576-28092-1-git-send-email-diane.gasselin@ensimag.imag.fr","subject":"[PATCH 1/5 v2] merge-recursive: porcelain messages for checkout","fromName":"Diane Gasselin","fromEmail":"diane.gasselin@ensimag.imag.fr","sentAt":"2010-06-15T12:22:52Z","receivedAt":"2010-06-15T12:22:52Z","isPatch":true,"sender":{"key":"diane.gasselin@ensimag.imag.fr","avatar":null},"body":"A porcelain message was first added in checkout.c in the commit\n8ccba008 (Junio C Hamano, Sat May 17 21:03:49 2008, unpack-trees: \nallow Porcelain to give different error messages) so that it better fit \nthe situation.\nThis patch proposes other specific porcelain messages for checkout instead of\nusing merge plumbing error messages. This way, when having a checkout error,\n\"merge\" no longer appears in the error message.\n\nSigned-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>\nSigned-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>\nSigned-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>\n---\n builtin/checkout.c |    1 +\n builtin/merge.c    |    3 ++-\n merge-recursive.c  |   48 +++++++++++++++++++++++++++++++-----------------\n merge-recursive.h  |    6 ++++--\n 4 files changed, 38 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 88b1f43..6f34566 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -372,6 +372,7 @@ static int merge_working_tree(struct checkout_opts *opts,\n \t\ttopts.src_index = &the_index;\n \t\ttopts.dst_index = &the_index;\n \n+\t\ttopts.msgs = get_porcelain_error_msgs(\"checkout\");\n \t\ttopts.msgs.not_uptodate_file = \"You have local changes to '%s'; cannot switch branches.\";\n \n \t\trefresh_cache(REFRESH_QUIET);\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 37d414b..501177f 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -704,7 +704,8 @@ int checkout_fast_forward(const unsigned char *head, const unsigned char *remote\n \topts.verbose_update = 1;\n \topts.merge = 1;\n \topts.fn = twoway_merge;\n-\topts.msgs = get_porcelain_error_msgs();\n+\n+\topts.msgs = get_porcelain_error_msgs(\"merge\");\n \n \ttrees[nr_trees] = parse_tree_indirect(head);\n \tif (!trees[nr_trees++])\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 206c103..80c9744 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -185,7 +185,7 @@ static int git_merge_trees(int index_only,\n \topts.fn = threeway_merge;\n \topts.src_index = &the_index;\n \topts.dst_index = &the_index;\n-\topts.msgs = get_porcelain_error_msgs();\n+\topts.msgs = get_porcelain_error_msgs(\"merge\");\n \n \tinit_tree_desc_from_tree(t+0, common);\n \tinit_tree_desc_from_tree(t+1, head);\n@@ -1178,24 +1178,38 @@ static int process_entry(struct merge_options *o,\n \treturn clean_merge;\n }\n \n-struct unpack_trees_error_msgs get_porcelain_error_msgs(void)\n+struct unpack_trees_error_msgs get_porcelain_error_msgs(const char *cmd)\n {\n-\tstruct unpack_trees_error_msgs msgs = {\n-\t\t/* would_overwrite */\n-\t\t\"Your local changes to '%s' would be overwritten by merge.  Aborting.\",\n-\t\t/* not_uptodate_file */\n-\t\t\"Your local changes to '%s' would be overwritten by merge.  Aborting.\",\n-\t\t/* not_uptodate_dir */\n-\t\t\"Updating '%s' would lose untracked files in it.  Aborting.\",\n-\t\t/* would_lose_untracked */\n-\t\t\"Untracked working tree file '%s' would be %s by merge.  Aborting\",\n-\t\t/* bind_overlap -- will not happen here */\n-\t\tNULL,\n-\t};\n+\tstruct unpack_trees_error_msgs msgs;\n+\n+\t/* would_overwrite */\n+\tmsgs.would_overwrite = malloc(sizeof(char) * 72);\n+\tsprintf((char *)msgs.would_overwrite,\n+\t\t\"Your local changes to '%%s' would be overwritten by %s.  Aborting.\",\n+\t\tcmd);\n+\t/* not_uptodate_file */\n+\tmsgs.not_uptodate_file = msgs.would_overwrite;\n+\t/* not_uptodate_dir */\n+\tmsgs.not_uptodate_dir =\n+\t\t\"Updating '%s' would lose untracked files in it.  Aborting.\";\n+\t/* would_lose_untracked */\n+\tmsgs.would_lose_untracked = malloc(sizeof(char) * 72);\n+\tsprintf((char *)msgs.would_lose_untracked,\n+\t\t\"Untracked working tree file '%%s' would be %%s by %s.  Aborting.\",\n+\t\tcmd);\n+\n \tif (advice_commit_before_merge) {\n-\t\tmsgs.would_overwrite = msgs.not_uptodate_file =\n-\t\t\t\"Your local changes to '%s' would be overwritten by merge.  Aborting.\\n\"\n-\t\t\t\"Please, commit your changes or stash them before you can merge.\";\n+\t\tmsgs.would_overwrite = malloc(sizeof(char) * 140);\n+\t\tsprintf((char *)msgs.would_overwrite,\n+\t\t\t\"Your local changes to '%%s' would be overwritten by %s.  Aborting.\\n\"\n+\t\t\t\"Please, commit your changes or stash them before you can %s.\",\n+\t\t\tcmd, strcmp(cmd,\"checkout\") ? cmd : \"swicth branches\");\n+\t\tmsgs.not_uptodate_file = msgs.would_overwrite;\n+\t\tmsgs.would_lose_untracked = malloc (sizeof(char) * 135);\n+\t\tsprintf((char *)msgs.would_lose_untracked,\n+\t\t\t\"Untracked working tree file '%%s' would be %%s by %s.  Aborting.\\n\"\n+\t\t\t\"Please move or remove them before you can %s.\",\n+\t\t\tcmd, strcmp(cmd,\"checkout\") ? cmd : \"swicth branches\");\n \t}\n \treturn msgs;\n }\ndiff --git a/merge-recursive.h b/merge-recursive.h\nindex 0cc465e..d910ae6 100644\n--- a/merge-recursive.h\n+++ b/merge-recursive.h\n@@ -23,8 +23,10 @@ struct merge_options {\n \tstruct string_list current_directory_set;\n };\n \n-/* Return a list of user-friendly error messages to be used by merge */\n-struct unpack_trees_error_msgs get_porcelain_error_msgs(void);\n+/* Return a list of user-friendly error messages to be used by\n+ * the command cmd which would be either merge or checkout\n+ */\n+struct unpack_trees_error_msgs get_porcelain_error_msgs(const char *cmd);\n \n /* merge_trees() but with recursive ancestor consolidation */\n int merge_recursive(struct merge_options *o,\n-- \n1.6.6.7.ga5fe3\n"},{"id":"143734","messageId":"1276604576-28092-3-git-send-email-diane.gasselin@ensimag.imag.fr","threadId":"24119","inReplyTo":"1276604576-28092-2-git-send-email-diane.gasselin@ensimag.imag.fr","subject":"[PATCH 2/5 v2] unpack_trees: group errors by type","fromName":"Diane Gasselin","fromEmail":"diane.gasselin@ensimag.imag.fr","sentAt":"2010-06-15T12:22:53Z","receivedAt":"2010-06-15T12:22:53Z","isPatch":true,"sender":{"key":"diane.gasselin@ensimag.imag.fr","avatar":null},"body":"When an error is encountered, it calls add_rejected_file() which either\n- directly displays the error message and stops if in plumbing mode\n  (i.e. if show_all_errors is not initialized at 1)\n- or stores it so that it will be displayed at the end with display_error_msgs(),\n\nStoring the files by error type permits to have a list of files for\nwhich there is the same error instead of having a serie of almost\nidentical errors.\n\nAs each bind_overlap error combines a file and an old file, a list cannot be\ndone, therefore, theses errors are not stored but directly displayed.\n\nUpdate t3030 to expect failure for a test based on an error message.\n\nSigned-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>\nSigned-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>\nSigned-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>\n---\n builtin/checkout.c         |    1 +\n builtin/merge.c            |    2 +-\n t/t3030-merge-recursive.sh |    2 +-\n tree-walk.c                |   11 +++-\n unpack-trees.c             |  119 +++++++++++++++++++++++++++++++++++++++++---\n unpack-trees.h             |   31 +++++++++++-\n 6 files changed, 152 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 6f34566..23eae56 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -392,6 +392,7 @@ static int merge_working_tree(struct checkout_opts *opts,\n \t\ttopts.dir = xcalloc(1, sizeof(*topts.dir));\n \t\ttopts.dir->flags |= DIR_SHOW_IGNORED;\n \t\ttopts.dir->exclude_per_dir = \".gitignore\";\n+\t\ttopts.show_all_errors = 1;\n \t\ttree = parse_tree_indirect(old->commit ?\n \t\t\t\t\t   old->commit->object.sha1 :\n \t\t\t\t\t   (unsigned char *)EMPTY_TREE_SHA1_BIN);\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 501177f..2c2f904 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -704,7 +704,7 @@ int checkout_fast_forward(const unsigned char *head, const unsigned char *remote\n \topts.verbose_update = 1;\n \topts.merge = 1;\n \topts.fn = twoway_merge;\n-\n+\topts.show_all_errors = 1;\n \topts.msgs = get_porcelain_error_msgs(\"merge\");\n \n \ttrees[nr_trees] = parse_tree_indirect(head);\ndiff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\nindex 9929f82..7ef8dd4 100755\n--- a/t/t3030-merge-recursive.sh\n+++ b/t/t3030-merge-recursive.sh\n@@ -269,7 +269,7 @@ test_expect_success 'merge-recursive result' '\n \n '\n \n-test_expect_success 'fail if the index has unresolved entries' '\n+test_expect_failure 'fail if the index has unresolved entries' '\n \n \trm -fr [abcd] &&\n \tgit checkout -f \"$c1\" &&\ndiff --git a/tree-walk.c b/tree-walk.c\nindex 67a9a0c..b8b7f00 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -1,5 +1,6 @@\n #include \"cache.h\"\n #include \"tree-walk.h\"\n+#include \"unpack-trees.h\"\n #include \"tree.h\"\n \n static const char *get_mode(const char *str, unsigned int *modep)\n@@ -310,6 +311,7 @@ static void free_extended_entry(struct tree_desc_x *t)\n int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n {\n \tint ret = 0;\n+\tint error = 0;\n \tstruct name_entry *entry = xmalloc(n*sizeof(*entry));\n \tint i;\n \tstruct tree_desc_x *tx = xcalloc(n, sizeof(*tx));\n@@ -377,8 +379,11 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n \t\tif (!mask)\n \t\t\tbreak;\n \t\tret = info->fn(n, mask, dirmask, entry, info);\n-\t\tif (ret < 0)\n-\t\t\tbreak;\n+\t\tif (ret < 0) {\n+\t\t\terror = ret;\n+\t\t\tif (!((struct unpack_trees_options*)(info->data))->show_all_errors)\n+\t\t\t\tbreak;\n+\t\t}\n \t\tmask &= ret;\n \t\tret = 0;\n \t\tfor (i = 0; i < n; i++)\n@@ -389,7 +394,7 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n \tfor (i = 0; i < n; i++)\n \t\tfree_extended_entry(tx + i);\n \tfree(tx);\n-\treturn ret;\n+\treturn error;\n }\n \n static int find_tree_entry(struct tree_desc *t, const char *name, unsigned char *result, unsigned *mode)\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex c29a9e0..78ecdc9 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -60,6 +60,92 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n }\n \n /*\n+ * add error messages on path <path> and action <action>\n+ * corresponding to the type <e> with the message <msg>\n+ * indicating if it should be display in porcelain or not\n+ */\n+static int add_rejected_path(struct unpack_trees_options *o,\n+\t\t\t     enum unpack_trees_error e,\n+\t\t\t     const char *path,\n+\t\t\t     const char *action,\n+\t\t\t     int porcelain,\n+\t\t\t     const char *msg)\n+{\n+\tstruct rejected_paths_list *newentry;\n+\tstruct rejected_paths **rp;\n+\t/*\n+\t * simply display the given error message if in plumbing mode\n+\t */\n+\tif (!porcelain)\n+\t\to->show_all_errors = 0;\n+\tif (!o->show_all_errors)\n+\t\treturn error(msg, path, action);\n+\t/*\n+\t * if there is a porcelain error message defined,\n+\t * the error is stored in order to be nicely displayed later\n+\t */\n+\tif (e == would_lose_untracked_overwritten && !strcmp(action, \"removed\"))\n+\t\te = would_lose_untracked_removed;\n+\n+\trp = &o->unpack_rejects[e];\n+\n+\tif (!o->unpack_rejects[e]) {\n+\t\t*rp = malloc(sizeof(struct rejected_paths));\n+\t\t(*rp)->list = NULL;\n+\t}\n+\tnewentry = malloc(sizeof(struct rejected_paths_list));\n+\tnewentry->path = (char *)path;\n+\tnewentry->next = (*rp)->list;\n+\t(*rp)->list = newentry;\n+\t(*rp)->msg = msg;\n+\t(*rp)->action = (char *)action;\n+\treturn -1;\n+}\n+\n+/*\n+ * free all the structures allocated for the error <e>\n+ */\n+static void free_rejected_paths(struct unpack_trees_options *o,\n+\t\t\t\tenum unpack_trees_error e)\n+{\n+\twhile (o->unpack_rejects[e]->list) {\n+\t\tstruct rejected_paths_list *del = o->unpack_rejects[e]->list;\n+\t\to->unpack_rejects[e]->list = o->unpack_rejects[e]->list->next;\n+\t\tfree(del);\n+\t}\n+\tfree(o->unpack_rejects[e]);\n+}\n+\n+/*\n+ * display all the error messages stored in a nice way\n+ */\n+static void display_error_msgs(struct unpack_trees_options *o)\n+{\n+\tint i;\n+\tint something_is_displayed = 0;\n+\tfor (i = 0; i < NB_UNPACK_TREES_ERROR; i++) {\n+\t\tif (o->unpack_rejects[i] && o->unpack_rejects[i]->list) {\n+\t\t\tstruct rejected_paths *rp = o->unpack_rejects[i];\n+\t\t\tstruct rejected_paths_list *f = rp->list;\n+\t\t\tchar *action = rp->action;\n+\t\t\tstruct strbuf path = STRBUF_INIT;\n+\t\t\tsomething_is_displayed  = 1;\n+\t\t\tfor (f = rp->list; f; f = f->next)\n+\t\t\t\tstrbuf_addf(&path, \"\\t%s\\n\", f->path);\n+\t\t\tif (i == would_lose_untracked_overwritten ||\n+\t\t\t    i == would_lose_untracked_removed)\n+\t\t\t\terror(rp->msg, action, path.buf);\n+\t\t\telse\n+\t\t\t\terror(rp->msg, path.buf, action);\n+\t\t\tstrbuf_release(&path);\n+\t\t\tfree_rejected_paths(o, i);\n+\t\t}\n+\t}\n+\tif (something_is_displayed)\n+\t\tprintf(\"Aborting\\n\");\n+}\n+\n+/*\n  * Unlink the last component and schedule the leading directories for\n  * removal, such that empty directories get removed.\n  */\n@@ -819,6 +905,8 @@ done:\n \treturn ret;\n \n return_failed:\n+\tif (o->show_all_errors)\n+\t\tdisplay_error_msgs(o);\n \tmark_all_ce_unused(o->src_index);\n \tret = unpack_failed(o, NULL);\n \tgoto done;\n@@ -828,7 +916,9 @@ return_failed:\n \n static int reject_merge(struct cache_entry *ce, struct unpack_trees_options *o)\n {\n-\treturn error(ERRORMSG(o, would_overwrite), ce->name);\n+\treturn add_rejected_path(o, would_overwrite, ce->name, NULL,\n+\t\t\t\t (o && (o)->msgs.would_overwrite),\n+\t\t\t\t ERRORMSG(o, would_overwrite));\n }\n \n static int same(struct cache_entry *a, struct cache_entry *b)\n@@ -850,7 +940,7 @@ static int same(struct cache_entry *a, struct cache_entry *b)\n  */\n static int verify_uptodate_1(struct cache_entry *ce,\n \t\t\t\t   struct unpack_trees_options *o,\n-\t\t\t\t   const char *error_msg)\n+\t\t\t\t   enum unpack_trees_error error)\n {\n \tstruct stat st;\n \n@@ -874,8 +964,16 @@ static int verify_uptodate_1(struct cache_entry *ce,\n \t}\n \tif (errno == ENOENT)\n \t\treturn 0;\n-\treturn o->gently ? -1 :\n-\t\terror(error_msg, ce->name);\n+\tif (error == sparse_not_uptodate_file)\n+\t\treturn o->gently ? -1 :\n+\t\t\tadd_rejected_path(o, sparse_not_uptodate_file, ce->name, NULL,\n+\t\t\t\t\t  (o && (o)->msgs.sparse_not_uptodate_file),\n+\t\t\t\t\t  ERRORMSG(o, sparse_not_uptodate_file));\n+\telse\n+\t\treturn o->gently ? -1 :\n+\t\t\tadd_rejected_path(o, not_uptodate_file, ce->name, NULL,\n+\t\t\t\t\t  (o && (o)->msgs.not_uptodate_file),\n+\t\t\t\t\t  ERRORMSG(o, not_uptodate_file));\n }\n \n static int verify_uptodate(struct cache_entry *ce,\n@@ -883,13 +981,13 @@ static int verify_uptodate(struct cache_entry *ce,\n {\n \tif (!o->skip_sparse_checkout && will_have_skip_worktree(ce, o))\n \t\treturn 0;\n-\treturn verify_uptodate_1(ce, o, ERRORMSG(o, not_uptodate_file));\n+\treturn verify_uptodate_1(ce, o, not_uptodate_file);\n }\n \n static int verify_uptodate_sparse(struct cache_entry *ce,\n \t\t\t\t  struct unpack_trees_options *o)\n {\n-\treturn verify_uptodate_1(ce, o, ERRORMSG(o, sparse_not_uptodate_file));\n+\treturn verify_uptodate_1(ce, o, sparse_not_uptodate_file);\n }\n \n static void invalidate_ce_path(struct cache_entry *ce, struct unpack_trees_options *o)\n@@ -976,7 +1074,9 @@ static int verify_clean_subdirectory(struct cache_entry *ce, const char *action,\n \ti = read_directory(&d, pathbuf, namelen+1, NULL);\n \tif (i)\n \t\treturn o->gently ? -1 :\n-\t\t\terror(ERRORMSG(o, not_uptodate_dir), ce->name);\n+\t\t\tadd_rejected_path(o, not_uptodate_dir, ce->name, NULL,\n+\t\t\t\t\t  (o && (o)->msgs.not_uptodate_dir),\n+\t\t\t\t\t  ERRORMSG(o, not_uptodate_dir));\n \tfree(pathbuf);\n \treturn cnt;\n }\n@@ -1058,7 +1158,10 @@ static int verify_absent_1(struct cache_entry *ce, const char *action,\n \t\t}\n \n \t\treturn o->gently ? -1 :\n-\t\t\terror(ERRORMSG(o, would_lose_untracked), ce->name, action);\n+\t\t\tadd_rejected_path(o, would_lose_untracked_overwritten,\n+\t\t\t\t\t  ce->name, action,\n+\t\t\t\t\t  (o && (o)->msgs.would_lose_untracked),\n+\t\t\t\t\t  ERRORMSG(o, would_lose_untracked));\n \t}\n \treturn 0;\n }\ndiff --git a/unpack-trees.h b/unpack-trees.h\nindex ef70eab..1f8e71e 100644\n--- a/unpack-trees.h\n+++ b/unpack-trees.h\n@@ -2,6 +2,7 @@\n #define UNPACK_TREES_H\n \n #define MAX_UNPACK_TREES 8\n+#define NB_UNPACK_TREES_ERROR 6\n \n struct unpack_trees_options;\n struct exclude_list;\n@@ -19,6 +20,27 @@ struct unpack_trees_error_msgs {\n \tconst char *would_lose_orphaned;\n };\n \n+struct rejected_paths_list {\n+\tchar *path;\n+\tstruct rejected_paths_list *next;\n+};\n+\n+struct rejected_paths {\n+\tchar *action;\n+\tconst char *msg;\n+\tstruct rejected_paths_list *list;\n+};\n+\n+\n+enum unpack_trees_error{\n+\twould_overwrite,\n+\tnot_uptodate_file,\n+\tnot_uptodate_dir,\n+\twould_lose_untracked_overwritten,\n+\twould_lose_untracked_removed,\n+\tsparse_not_uptodate_file\n+};\n+\n struct unpack_trees_options {\n \tunsigned int reset,\n \t\t     merge,\n@@ -33,7 +55,8 @@ struct unpack_trees_options {\n \t\t     diff_index_cached,\n \t\t     debug_unpack,\n \t\t     skip_sparse_checkout,\n-\t\t     gently;\n+\t\t     gently,\n+\t\t     show_all_errors;\n \tconst char *prefix;\n \tint cache_bottom;\n \tstruct dir_struct *dir;\n@@ -51,6 +74,12 @@ struct unpack_trees_options {\n \tstruct index_state result;\n \n \tstruct exclude_list *el; /* for internal use */\n+\n+\t/*\n+\t * Store error messages in an array, each case\n+\t * corresponding to a error message type\n+\t */\n+\tstruct rejected_paths *unpack_rejects[NB_UNPACK_TREES_ERROR];\n };\n \n extern int unpack_trees(unsigned n, struct tree_desc *t,\n-- \n1.6.6.7.ga5fe3\n"},{"id":"143737","messageId":"1276604576-28092-4-git-send-email-diane.gasselin@ensimag.imag.fr","threadId":"24119","inReplyTo":"1276604576-28092-3-git-send-email-diane.gasselin@ensimag.imag.fr","subject":"[PATCH 3/5 v2] unpack_trees_options: update porcelain messages","fromName":"Diane Gasselin","fromEmail":"diane.gasselin@ensimag.imag.fr","sentAt":"2010-06-15T12:22:54Z","receivedAt":"2010-06-15T12:22:54Z","isPatch":true,"sender":{"key":"diane.gasselin@ensimag.imag.fr","avatar":null},"body":"Update porcelain messages of unpack_trees_options in order to have a good layout\nand add an advice for would_lose_untracked errors if advice_commit_before_merge\nis enabled.\n\nUpdate t3400 to have an expect_failure for the rebase verbose error message.\n\nSigned-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>\nSigned-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>\nSigned-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>\n---\n builtin/checkout.c |    2 +-\n merge-recursive.c  |   18 +++++++++---------\n t/t3400-rebase.sh  |    2 +-\n 3 files changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 23eae56..b9d056d 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -373,7 +373,7 @@ static int merge_working_tree(struct checkout_opts *opts,\n \t\ttopts.dst_index = &the_index;\n \n \t\ttopts.msgs = get_porcelain_error_msgs(\"checkout\");\n-\t\ttopts.msgs.not_uptodate_file = \"You have local changes to '%s'; cannot switch branches.\";\n+\t\ttopts.msgs.not_uptodate_file = \"You have local changes to the following files:\\n%sCannot switch branches.\";\n \n \t\trefresh_cache(REFRESH_QUIET);\n \ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 80c9744..ee80553 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1183,31 +1183,31 @@ struct unpack_trees_error_msgs get_porcelain_error_msgs(const char *cmd)\n \tstruct unpack_trees_error_msgs msgs;\n \n \t/* would_overwrite */\n-\tmsgs.would_overwrite = malloc(sizeof(char) * 72);\n+\tmsgs.would_overwrite = malloc(sizeof(char) * 80);\n \tsprintf((char *)msgs.would_overwrite,\n-\t\t\"Your local changes to '%%s' would be overwritten by %s.  Aborting.\",\n+\t\t\"Your local changes to the following files would be overwritten by %s:\\n%%s\",\n \t\tcmd);\n \t/* not_uptodate_file */\n \tmsgs.not_uptodate_file = msgs.would_overwrite;\n \t/* not_uptodate_dir */\n \tmsgs.not_uptodate_dir =\n-\t\t\"Updating '%s' would lose untracked files in it.  Aborting.\";\n+\t\t\"Updating the following directories would lose untracked files in it:\\n%s\";\n \t/* would_lose_untracked */\n-\tmsgs.would_lose_untracked = malloc(sizeof(char) * 72);\n+\tmsgs.would_lose_untracked = malloc(sizeof(char) * 80);\n \tsprintf((char *)msgs.would_lose_untracked,\n-\t\t\"Untracked working tree file '%%s' would be %%s by %s.  Aborting.\",\n+\t\t\"The following untracked working tree files would be %%s by %s:\\n%%s\",\n \t\tcmd);\n \n \tif (advice_commit_before_merge) {\n-\t\tmsgs.would_overwrite = malloc(sizeof(char) * 140);\n+\t\tmsgs.would_overwrite = malloc(sizeof(char) * 160);\n \t\tsprintf((char *)msgs.would_overwrite,\n-\t\t\t\"Your local changes to '%%s' would be overwritten by %s.  Aborting.\\n\"\n+\t\t\t\"Your local changes to the following files would be overwritten by %s:\\n%%s\"\n \t\t\t\"Please, commit your changes or stash them before you can %s.\",\n \t\t\tcmd, strcmp(cmd,\"checkout\") ? cmd : \"swicth branches\");\n \t\tmsgs.not_uptodate_file = msgs.would_overwrite;\n-\t\tmsgs.would_lose_untracked = malloc (sizeof(char) * 135);\n+\t\tmsgs.would_lose_untracked = malloc (sizeof(char) * 160);\n \t\tsprintf((char *)msgs.would_lose_untracked,\n-\t\t\t\"Untracked working tree file '%%s' would be %%s by %s.  Aborting.\\n\"\n+\t\t\t\"The following untracked working tree files would be %%s by %s:\\n%%s\"\n \t\t\t\"Please move or remove them before you can %s.\",\n \t\t\tcmd, strcmp(cmd,\"checkout\") ? cmd : \"swicth branches\");\n \t}\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex dbf7dfb..cbf160d 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -121,7 +121,7 @@ test_expect_success 'rebase a single mode change' '\n      GIT_TRACE=1 git rebase master\n '\n \n-test_expect_success 'Show verbose error when HEAD could not be detached' '\n+test_expect_failure 'Show verbose error when HEAD could not be detached' '\n      : > B &&\n      test_must_fail git rebase topic 2> output.err > output.out &&\n      grep \"Untracked working tree file .B. would be overwritten\" output.err\n-- \n1.6.6.7.ga5fe3\n"},{"id":"143738","messageId":"1276604576-28092-5-git-send-email-diane.gasselin@ensimag.imag.fr","threadId":"24119","inReplyTo":"1276604576-28092-4-git-send-email-diane.gasselin@ensimag.imag.fr","subject":"[PATCH 4/5 v2] tests: update porcelain expected message","fromName":"Diane Gasselin","fromEmail":"diane.gasselin@ensimag.imag.fr","sentAt":"2010-06-15T12:22:55Z","receivedAt":"2010-06-15T12:22:55Z","isPatch":true,"sender":{"key":"diane.gasselin@ensimag.imag.fr","avatar":null},"body":"As porcelain messages have been changed, the expected porcelain messages\ntested in t3030 and t3400 need to be changed.\n\nSigned-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>\nSigned-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>\nSigned-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>\n---\n t/t3030-merge-recursive.sh |    4 ++--\n t/t3400-rebase.sh          |    4 ++--\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\nindex 7ef8dd4..77bf0f0 100755\n--- a/t/t3030-merge-recursive.sh\n+++ b/t/t3030-merge-recursive.sh\n@@ -269,7 +269,7 @@ test_expect_success 'merge-recursive result' '\n \n '\n \n-test_expect_failure 'fail if the index has unresolved entries' '\n+test_expect_success 'fail if the index has unresolved entries' '\n \n \trm -fr [abcd] &&\n \tgit checkout -f \"$c1\" &&\n@@ -282,7 +282,7 @@ test_expect_failure 'fail if the index has unresolved entries' '\n \tgrep \"You have not concluded your merge\" out &&\n \trm -f .git/MERGE_HEAD &&\n \ttest_must_fail git merge \"$c5\" 2> out &&\n-\tgrep \"Your local changes to .* would be overwritten by merge.\" out\n+\tgrep \"Your local changes to the following files would be overwritten by merge:\" out\n '\n \n test_expect_success 'merge-recursive remove conflict' '\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex cbf160d..55be0c2 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -121,10 +121,10 @@ test_expect_success 'rebase a single mode change' '\n      GIT_TRACE=1 git rebase master\n '\n \n-test_expect_failure 'Show verbose error when HEAD could not be detached' '\n+test_expect_success 'Show verbose error when HEAD could not be detached' '\n      : > B &&\n      test_must_fail git rebase topic 2> output.err > output.out &&\n-     grep \"Untracked working tree file .B. would be overwritten\" output.err\n+     grep \"The following untracked working tree files would be overwritten by checkout:\" output.err\n '\n \n test_expect_success 'rebase -q is quiet' '\n-- \n1.6.6.7.ga5fe3\n"},{"id":"143736","messageId":"1276604576-28092-6-git-send-email-diane.gasselin@ensimag.imag.fr","threadId":"24119","inReplyTo":"1276604576-28092-5-git-send-email-diane.gasselin@ensimag.imag.fr","subject":"[PATCH 5/5 v2] t7609: test merge and checkout error messages","fromName":"Diane Gasselin","fromEmail":"diane.gasselin@ensimag.imag.fr","sentAt":"2010-06-15T12:22:56Z","receivedAt":"2010-06-15T12:22:56Z","isPatch":true,"sender":{"key":"diane.gasselin@ensimag.imag.fr","avatar":null},"body":"Test porcelain and plumbing error messages for different types of errors\nof merge and checkout.\n\nSigned-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>\nSigned-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>\nSigned-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>\n---\n t/t7609-merge-co-error-msgs.sh |  125 ++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 125 insertions(+), 0 deletions(-)\n create mode 100755 t/t7609-merge-co-error-msgs.sh\n\ndiff --git a/t/t7609-merge-co-error-msgs.sh b/t/t7609-merge-co-error-msgs.sh\nnew file mode 100755\nindex 0000000..b636b75\n--- /dev/null\n+++ b/t/t7609-merge-co-error-msgs.sh\n@@ -0,0 +1,125 @@\n+#!/bin/sh\n+\n+test_description='unpack-trees error messages'\n+\n+. ./test-lib.sh\n+\n+\n+test_expect_success 'setup' '\n+\techo one >one &&\n+\tgit add one &&\n+\tgit commit -a -m First &&\n+\n+\tgit checkout -b branch &&\n+\techo two>two &&\n+\techo three>three &&\n+\techo four>four &&\n+\techo five>five &&\n+\tgit add two three four five &&\n+\tgit commit -m Second &&\n+\n+\tgit checkout master &&\n+\techo other>two &&\n+\techo other>three &&\n+\techo other>four &&\n+\techo other>five\n+'\n+\n+cat> expect <<\\EOF\n+error: The following untracked working tree files would be overwritten by merge:\n+\ttwo\n+\tthree\n+\tfour\n+\tfive\n+Please move or remove them before you can merge.\n+EOF\n+\n+test_expect_success 'untracked files overwritten by merge' '\n+\t! git merge branch 2> out &&\n+\ttest_cmp out expect\n+'\n+\n+cat> expect <<\\EOF\n+error: Your local changes to the following files would be overwritten by merge:\n+\ttwo\n+\tthree\n+\tfour\n+Please, commit your changes or stash them before you can merge.\n+error: The following untracked working tree files would be overwritten by merge:\n+\tfive\n+Please move or remove them before you can merge.\n+EOF\n+\n+test_expect_success 'untracked files or local changes ovewritten by merge' '\n+\tgit add two &&\n+\tgit add three &&\n+\tgit add four &&\n+\t! git merge branch 2> out &&\n+\ttest_cmp out expect\n+'\n+\n+cat> expect <<\\EOF\n+error: You have local changes to the following files:\n+\trep/two\n+\trep/one\n+Cannot switch branches.\n+EOF\n+\n+test_expect_success 'cannot switch branches because of local changes' '\n+\tgit add five &&\n+\tmkdir rep &&\n+\techo one>rep/one &&\n+\techo two>rep/two &&\n+\tgit add rep/one rep/two &&\n+\tgit commit -m Fourth &&\n+\tgit checkout master &&\n+\techo uno>rep/one &&\n+\techo dos>rep/two &&\n+\t! git checkout branch 2> out &&\n+\ttest_cmp out expect\n+'\n+\n+cat> expect <<\\EOF\n+error: Your local changes to the following files would be overwritten by checkout:\n+\trep/two\n+\trep/one\n+Please, commit your changes or stash them before you can swicth branches.\n+EOF\n+\n+test_expect_success 'not uptodate file porcelain checkout error' '\n+\tgit add rep/one rep/two &&\n+\t! git checkout branch 2> out &&\n+\ttest_cmp out expect\n+'\n+\n+cat> expect <<\\EOF\n+error: Updating the following directories would lose untracked files in it:\n+\trep2\n+\trep\n+\n+EOF\n+\n+test_expect_success 'not_uptodate_dir porcelain checkout error' '\n+\tgit init uptodate &&\n+\tcd uptodate &&\n+\tmkdir rep &&\n+\tmkdir rep2 &&\n+\ttouch rep/foo &&\n+\ttouch rep2/foo &&\n+\tgit add rep/foo rep2/foo &&\n+\tgit commit -m init &&\n+\tgit checkout -b branch &&\n+\tgit rm rep -r &&\n+\tgit rm rep2 -r &&\n+\t> rep &&\n+\t> rep2 &&\n+\tgit add rep rep2&&\n+\tgit commit -m \"added test as a file\" &&\n+\tgit checkout master &&\n+\t> rep/untracked-file &&\n+\t> rep2/untracked-file &&\n+\t! git checkout branch 2> out &&\n+\ttest_cmp out ../expect\n+'\n+\n+test_done\n-- \n1.6.6.7.ga5fe3\n"},{"id":"143739","messageId":"vpqr5k8zd4z.fsf@bauges.imag.fr","threadId":"24119","inReplyTo":"1276604576-28092-2-git-send-email-diane.gasselin@ensimag.imag.fr","subject":"Re: [PATCH 1/5 v2] merge-recursive: porcelain messages for checkout","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2010-06-15T12:36:12Z","receivedAt":"2010-06-15T12:36:12Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Diane Gasselin <diane.gasselin@ensimag.imag.fr> writes:\n\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -372,6 +372,7 @@ static int merge_working_tree(struct checkout_opts *opts,\n>  \t\ttopts.src_index = &the_index;\n>  \t\ttopts.dst_index = &the_index;\n>  \n> +\t\ttopts.msgs = get_porcelain_error_msgs(\"checkout\");\n>  \t\ttopts.msgs.not_uptodate_file = \"You have local changes to '%s'; cannot switch branches.\";\n\nIt's nice to get accurate messages for all cases, but then why do you\nkeep the special-case for not_uptodate_file? If there's a good reason\nfor it, a comment in the code would be welcome.\n\n> +\t/* would_overwrite */\n> +\tmsgs.would_overwrite = malloc(sizeof(char) * 72);\n> +\tsprintf((char *)msgs.would_overwrite,\n> +\t\t\"Your local changes to '%%s' would be overwritten by %s.  Aborting.\",\n> +\t\tcmd);\n\nThis yields:\n\n  Your local changes to 'foo' would be overwritten by checkout.  Aborting.\n\nI tend to prefer Junio's wording:\n\n  You have local changes to 'foo'; cannot switch branches.\n\n--\nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"143740","messageId":"vpqljagzc39.fsf@bauges.imag.fr","threadId":"24119","inReplyTo":"1276604576-28092-3-git-send-email-diane.gasselin@ensimag.imag.fr","subject":"Re: [PATCH 2/5 v2] unpack_trees: group errors by type","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2010-06-15T12:58:50Z","receivedAt":"2010-06-15T12:58:50Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Diane Gasselin <diane.gasselin@ensimag.imag.fr> writes:\n\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -60,6 +60,92 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n>  }\n>  \n>  /*\n> + * add error messages on path <path> and action <action>\n> + * corresponding to the type <e> with the message <msg>\n> + * indicating if it should be display in porcelain or not\n> + */\n> +static int add_rejected_path(struct unpack_trees_options *o,\n> +\t\t\t     enum unpack_trees_error e,\n> +\t\t\t     const char *path,\n> +\t\t\t     const char *action,\n> +\t\t\t     int porcelain,\n> +\t\t\t     const char *msg)\n> +{\n> +\tstruct rejected_paths_list *newentry;\n> +\tstruct rejected_paths **rp;\n> +\t/*\n> +\t * simply display the given error message if in plumbing mode\n> +\t */\n> +\tif (!porcelain)\n> +\t\to->show_all_errors = 0;\n> +\tif (!o->show_all_errors)\n> +\t\treturn error(msg, path, action);\n\nI don't fully understand what you're doing with show_all_errors and\nporcelain here. From the caller, \"porcelain\" is true iff the\ncorresponding error message has been set in o. But if you can infer\nwhether you're in porcelain from the error messages, why do you need\nshow_all_errors in addition?\n\n>  static int reject_merge(struct cache_entry *ce, struct unpack_trees_options *o)\n>  {\n> -\treturn error(ERRORMSG(o, would_overwrite), ce->name);\n> +\treturn add_rejected_path(o, would_overwrite, ce->name, NULL,\n> +\t\t\t\t (o && (o)->msgs.would_overwrite),\n\nParenthesis around (o) are distracting and useless. I guess you\ncopy-pasted from a macro (for which parentheses should definitely be\nused in case the macro is called on an arbitrary expression).\n\n> @@ -874,8 +964,16 @@ static int verify_uptodate_1(struct cache_entry *ce,\n>  \t}\n>  \tif (errno == ENOENT)\n>  \t\treturn 0;\n> -\treturn o->gently ? -1 :\n> -\t\terror(error_msg, ce->name);\n> +\tif (error == sparse_not_uptodate_file)\n> +\t\treturn o->gently ? -1 :\n> +\t\t\tadd_rejected_path(o, sparse_not_uptodate_file, ce->name, NULL,\n> +\t\t\t\t\t  (o && (o)->msgs.sparse_not_uptodate_file),\n> +\t\t\t\t\t  ERRORMSG(o, sparse_not_uptodate_file));\n> +\telse\n> +\t\treturn o->gently ? -1 :\n> +\t\t\tadd_rejected_path(o, not_uptodate_file, ce->name, NULL,\n> +\t\t\t\t\t  (o && (o)->msgs.not_uptodate_file),\n> +\t\t\t\t\t  ERRORMSG(o, not_uptodate_file));\n>  }\n\nIsn't that a complex way of saying\n\n\tint porcelain;\n\tif (error == sparse_not_uptodate_file)\n\t\tporcelain = o && o->msgs.sparse_not_uptodate_file;\n\telse\n\t\tporcelain = o && o->msgs.not_uptodate_file;\n\treturn o->gently ? -1 :\n\t\t\tadd_rejected_path(o, error, ce->name, NULL,\n\t\t\t\t\t  porcelain, ERRORMSG(o, error));\n\n?\n\nAlso, I'm not sure I understand why you're attaching the error message\nstring to each rejected_paths entry. Wouldn't it be more sensible to\nuse o->msg in display_error_msgs() instead?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"143741","messageId":"vpqfx0ozbs5.fsf@bauges.imag.fr","threadId":"24119","inReplyTo":"1276604576-28092-4-git-send-email-diane.gasselin@ensimag.imag.fr","subject":"Re: [PATCH 3/5 v2] unpack_trees_options: update porcelain messages","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2010-06-15T13:05:30Z","receivedAt":"2010-06-15T13:05:30Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Diane Gasselin <diane.gasselin@ensimag.imag.fr> writes:\n\n> -\tmsgs.would_overwrite = malloc(sizeof(char) * 72);\n> +\tmsgs.would_overwrite = malloc(sizeof(char) * 80);\n>  \tsprintf((char *)msgs.would_overwrite,\n> -\t\t\"Your local changes to '%%s' would be overwritten by %s.  Aborting.\",\n> +\t\t\"Your local changes to the following files would be overwritten by %s:\\n%%s\",\n\nI hate hardcoded string length (these magic 80 and 72). Can't it be\nstg like\n\nconst char * const msg = \"Your local changes to ....\";\nmsg.would_overwrite = malloc(strlen(msg) + strlen(cmd) + something);\nsprintf(msg.would_overwrite, msg, ...);\n\ninstead?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"143742","messageId":"vpqaaqwzblt.fsf@bauges.imag.fr","threadId":"24119","inReplyTo":"1276604576-28092-6-git-send-email-diane.gasselin@ensimag.imag.fr","subject":"Re: [PATCH 5/5 v2] t7609: test merge and checkout error messages","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2010-06-15T13:09:18Z","receivedAt":"2010-06-15T13:09:18Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Diane Gasselin <diane.gasselin@ensimag.imag.fr> writes:\n\n> +\techo two>two &&\n> +\techo three>three &&\n> +\techo four>four &&\n> +\techo five>five &&\n\nSpace before '>' please :\n\n2010/6/9 Junio C Hamano <gitster@pobox.com>:\n\n>  (1) redirection \">\" and \"<\" stick to the target file and have a SP on the\n>     other end.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"143744","messageId":"AANLkTin381eyaDabz3-z_8jB05N4CVKGmLOqVOprJMW2@mail.gmail.com","threadId":"24119","inReplyTo":"vpqljagzc39.fsf@bauges.imag.fr","subject":"Re: [PATCH 2/5 v2] unpack_trees: group errors by type","fromName":"Diane Gasselin","fromEmail":"diane.gasselin@ensimag.imag.fr","sentAt":"2010-06-15T13:15:36Z","receivedAt":"2010-06-15T13:15:36Z","isPatch":true,"sender":{"key":"diane.gasselin@ensimag.imag.fr","avatar":null},"body":"Le 15 juin 2010 14:58, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> a écrit :\n> Diane Gasselin <diane.gasselin@ensimag.imag.fr> writes:\n>\n>> --- a/unpack-trees.c\n>> +++ b/unpack-trees.c\n>> @@ -60,6 +60,92 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,\n>>  }\n>>\n>>  /*\n>> + * add error messages on path <path> and action <action>\n>> + * corresponding to the type <e> with the message <msg>\n>> + * indicating if it should be display in porcelain or not\n>> + */\n>> +static int add_rejected_path(struct unpack_trees_options *o,\n>> +                          enum unpack_trees_error e,\n>> +                          const char *path,\n>> +                          const char *action,\n>> +                          int porcelain,\n>> +                          const char *msg)\n>> +{\n>> +     struct rejected_paths_list *newentry;\n>> +     struct rejected_paths **rp;\n>> +     /*\n>> +      * simply display the given error message if in plumbing mode\n>> +      */\n>> +     if (!porcelain)\n>> +             o->show_all_errors = 0;\n>> +     if (!o->show_all_errors)\n>> +             return error(msg, path, action);\n>\n> I don't fully understand what you're doing with show_all_errors and\n> porcelain here. From the caller, \"porcelain\" is true iff the\n> corresponding error message has been set in o. But if you can infer\n> whether you're in porcelain from the error messages, why do you need\n> show_all_errors in addition?\n>\n>>  static int reject_merge(struct cache_entry *ce, struct unpack_trees_options *o)\n>>  {\n>> -     return error(ERRORMSG(o, would_overwrite), ce->name);\n>> +     return add_rejected_path(o, would_overwrite, ce->name, NULL,\n>> +                              (o && (o)->msgs.would_overwrite),\n>\n> Parenthesis around (o) are distracting and useless. I guess you\n> copy-pasted from a macro (for which parentheses should definitely be\n> used in case the macro is called on an arbitrary expression).\n>\n>> @@ -874,8 +964,16 @@ static int verify_uptodate_1(struct cache_entry *ce,\n>>       }\n>>       if (errno == ENOENT)\n>>               return 0;\n>> -     return o->gently ? -1 :\n>> -             error(error_msg, ce->name);\n>> +     if (error == sparse_not_uptodate_file)\n>> +             return o->gently ? -1 :\n>> +                     add_rejected_path(o, sparse_not_uptodate_file, ce->name, NULL,\n>> +                                       (o && (o)->msgs.sparse_not_uptodate_file),\n>> +                                       ERRORMSG(o, sparse_not_uptodate_file));\n>> +     else\n>> +             return o->gently ? -1 :\n>> +                     add_rejected_path(o, not_uptodate_file, ce->name, NULL,\n>> +                                       (o && (o)->msgs.not_uptodate_file),\n>> +                                       ERRORMSG(o, not_uptodate_file));\n>>  }\n>\n> Isn't that a complex way of saying\n>\n>        int porcelain;\n>        if (error == sparse_not_uptodate_file)\n>                porcelain = o && o->msgs.sparse_not_uptodate_file;\n>        else\n>                porcelain = o && o->msgs.not_uptodate_file;\n>        return o->gently ? -1 :\n>                        add_rejected_path(o, error, ce->name, NULL,\n>                                          porcelain, ERRORMSG(o, error));\n>\n> ?\n>\n\nThe problem is that \"error\" is an enum unpack_trees_error, and\nERRORMSG takes the name of the field from unpack_trees_error_msgs.\nIf I try to do ERRORMSG(o, error), the compilator would say that the\n\"error\" is not a field of unpack_trees_error_msgs.\n\n> Also, I'm not sure I understand why you're attaching the error message\n> string to each rejected_paths entry. Wouldn't it be more sensible to\n> use o->msg in display_error_msgs() instead?\n>\n\nIn display_error_msgs(), I cannot access o->msg because I would not\nknow which error I am treating.\nIn the same way as previously, I cannot use the enum\nunpack_trees_error to access it.\n\nI know it makes the code a bit \"heavy\" but I did not see a better way to do it.\n\n> --\n> Matthieu Moy\n> http://www-verimag.imag.fr/~moy/\n>\n"},{"id":"143745","messageId":"vpq7hm0whkk.fsf@bauges.imag.fr","threadId":"24119","inReplyTo":"AANLkTin381eyaDabz3-z_8jB05N4CVKGmLOqVOprJMW2@mail.gmail.com","subject":"Re: [PATCH 2/5 v2] unpack_trees: group errors by type","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2010-06-15T13:28:43Z","receivedAt":"2010-06-15T13:28:43Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Diane Gasselin <diane.gasselin@ensimag.imag.fr> writes:\n\n> In display_error_msgs(), I cannot access o->msg because I would not\n> know which error I am treating.\n\nYou do:\n\nstatic void display_error_msgs(struct unpack_trees_options *o)\n{\n...\n\tfor (i = 0; i < NB_UNPACK_TREES_ERROR; i++) {\n\t\t...\n\t}\n\nYou know \"i\", so you know which error it is. The difficulty is that\nthe rejected paths are in an array, while the error messages are in a\nstruct, but you can either:\n\n* Turn the struct into an array, and say msgs[would_overwrite] instead\n  of msgs.would_overwrite (which would also simplify the code\n  elsewhere since you would be able to write \"ERRORMSG(o, error)\" and\n  such things).\n\n* Do\n\nswitch (i) {\ncase would_overwrite:\n\tmsg = o->msg.would_overwrite;\n\tbreak;\ncase not_uptodate_file:\n\tmsg = o->msg.not_uptodate_file;\n\tbreak;\ncase not_uptodate_dir:\n\tmsg = o->msg.not_uptodate_dir;\n\tbreak;\ncase would_lose_untracked_overwritten:\n\tmsg = o->msg.would_lose_untracked_overwritten;\n\tbreak;\ncase would_lose_untracked_removed:\n\tmsg = o->msg.would_lose_untracked_removed;\n\tbreak;\ncase sparse_not_uptodate_file:\n\tmsg = o->msg.sparse_not_uptodate_file;\n\tbreak;\n}\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"143746","messageId":"AANLkTilPJRAqT4LUF4ps9YtK3bFwZiVTVXa6_xigdkTg@mail.gmail.com","threadId":"24119","inReplyTo":"vpq7hm0whkk.fsf@bauges.imag.fr","subject":"Re: [PATCH 2/5 v2] unpack_trees: group errors by type","fromName":"Diane Gasselin","fromEmail":"diane.gasselin@ensimag.imag.fr","sentAt":"2010-06-15T13:40:16Z","receivedAt":"2010-06-15T13:40:16Z","isPatch":true,"sender":{"key":"diane.gasselin@ensimag.imag.fr","avatar":null},"body":"Le 15 juin 2010 15:28, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> a écrit :\n> Diane Gasselin <diane.gasselin@ensimag.imag.fr> writes:\n>\n>> In display_error_msgs(), I cannot access o->msg because I would not\n>> know which error I am treating.\n>\n> You do:\n>\n> static void display_error_msgs(struct unpack_trees_options *o)\n> {\n> ...\n>        for (i = 0; i < NB_UNPACK_TREES_ERROR; i++) {\n>                ...\n>        }\n>\n> You know \"i\", so you know which error it is. The difficulty is that\n> the rejected paths are in an array, while the error messages are in a\n> struct, but you can either:\n>\n> * Turn the struct into an array, and say msgs[would_overwrite] instead\n>  of msgs.would_overwrite (which would also simplify the code\n>  elsewhere since you would be able to write \"ERRORMSG(o, error)\" and\n>  such things).\n>\n> * Do\n>\n> switch (i) {\n> case would_overwrite:\n>        msg = o->msg.would_overwrite;\n>        break;\n> case not_uptodate_file:\n>        msg = o->msg.not_uptodate_file;\n>        break;\n> case not_uptodate_dir:\n>        msg = o->msg.not_uptodate_dir;\n>        break;\n> case would_lose_untracked_overwritten:\n>        msg = o->msg.would_lose_untracked_overwritten;\n>        break;\n> case would_lose_untracked_removed:\n>        msg = o->msg.would_lose_untracked_removed;\n>        break;\n> case sparse_not_uptodate_file:\n>        msg = o->msg.sparse_not_uptodate_file;\n>        break;\n> }\n>\n> --\n> Matthieu Moy\n> http://www-verimag.imag.fr/~moy/\n>\nThanks for your answers.\nI did the switch case at first but thought it was maybe a bit\nrepetitive. That is why, I opted for giving directly the message in\nadd_rejected_path().\n\nOtherwise, I do agree an array would make things easier, especially\nfor my patch. Does anyone has an objection into changing the struct\nunpack_trees_error_msgs into an array?\n"}]}