[PATCH 0/5 v2] unpack_trees: nicer error messages

[PATCH 0/5 v2] unpack_trees: nicer error messages

[Diane Gasselin]

This patch serie aims at grouping porcerlain merge and checkout 
errors messages by type if possible, listing all the file concerned
by the error type.
It also adds porcelain messages for checkout.

It was first introduced in the thread:
http://mid.gmane.org/7v63277f92.fsf@alter.siamese.dyndns.org

Diane Gasselin (5):
  merge-recursive: update merge porcelain messages for checkout
  unpack_trees: group errors by type
  unpack_trees_options: update porcelain messages
  tests: update porcelain expected message
  t7609: test merge and checkout error messages

 builtin/checkout.c             |    4 +-
 builtin/merge.c                |    3 +-
 merge-recursive.c              |   48 ++++++++++------
 merge-recursive.h              |    6 +-
 t/t3030-merge-recursive.sh     |    2 +-
 t/t3400-rebase.sh              |    2 +-
 t/t7609-merge-co-error-msgs.sh |  125 ++++++++++++++++++++++++++++++++++++++++
 tree-walk.c                    |   11 +++-
 unpack-trees.c                 |  119 +++++++++++++++++++++++++++++++++++---
 unpack-trees.h                 |   31 ++++++++++-
 10 files changed, 316 insertions(+), 35 deletions(-)
 create mode 100755 t/t7609-merge-co-error-msgs.sh

[PATCH 1/5 v2] merge-recursive: porcelain messages for checkout

[Diane Gasselin]

A porcelain message was first added in checkout.c in the commit
8ccba008 (Junio C Hamano, Sat May 17 21:03:49 2008, unpack-trees: 
allow Porcelain to give different error messages) so that it better fit 
the situation.
This patch proposes other specific porcelain messages for checkout instead of
using merge plumbing error messages. This way, when having a checkout error,
"merge" no longer appears in the error message.

Signed-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
Signed-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>
Signed-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>
---
 builtin/checkout.c |    1 +
 builtin/merge.c    |    3 ++-
 merge-recursive.c  |   48 +++++++++++++++++++++++++++++++-----------------
 merge-recursive.h  |    6 ++++--
 4 files changed, 38 insertions(+), 20 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 88b1f43..6f34566 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -372,6 +372,7 @@ static int merge_working_tree(struct checkout_opts *opts,
 		topts.src_index = &the_index;
 		topts.dst_index = &the_index;
 
+		topts.msgs = get_porcelain_error_msgs("checkout");
 		topts.msgs.not_uptodate_file = "You have local changes to '%s'; cannot switch branches.";
 
 		refresh_cache(REFRESH_QUIET);
diff --git a/builtin/merge.c b/builtin/merge.c
index 37d414b..501177f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -704,7 +704,8 @@ int checkout_fast_forward(const unsigned char *head, const unsigned char *remote
 	opts.verbose_update = 1;
 	opts.merge = 1;
 	opts.fn = twoway_merge;
-	opts.msgs = get_porcelain_error_msgs();
+
+	opts.msgs = get_porcelain_error_msgs("merge");
 
 	trees[nr_trees] = parse_tree_indirect(head);
 	if (!trees[nr_trees++])
diff --git a/merge-recursive.c b/merge-recursive.c
index 206c103..80c9744 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -185,7 +185,7 @@ static int git_merge_trees(int index_only,
 	opts.fn = threeway_merge;
 	opts.src_index = &the_index;
 	opts.dst_index = &the_index;
-	opts.msgs = get_porcelain_error_msgs();
+	opts.msgs = get_porcelain_error_msgs("merge");
 
 	init_tree_desc_from_tree(t+0, common);
 	init_tree_desc_from_tree(t+1, head);
@@ -1178,24 +1178,38 @@ static int process_entry(struct merge_options *o,
 	return clean_merge;
 }
 
-struct unpack_trees_error_msgs get_porcelain_error_msgs(void)
+struct unpack_trees_error_msgs get_porcelain_error_msgs(const char *cmd)
 {
-	struct unpack_trees_error_msgs msgs = {
-		/* would_overwrite */
-		"Your local changes to '%s' would be overwritten by merge.  Aborting.",
-		/* not_uptodate_file */
-		"Your local changes to '%s' would be overwritten by merge.  Aborting.",
-		/* not_uptodate_dir */
-		"Updating '%s' would lose untracked files in it.  Aborting.",
-		/* would_lose_untracked */
-		"Untracked working tree file '%s' would be %s by merge.  Aborting",
-		/* bind_overlap -- will not happen here */
-		NULL,
-	};
+	struct unpack_trees_error_msgs msgs;
+
+	/* would_overwrite */
+	msgs.would_overwrite = malloc(sizeof(char) * 72);
+	sprintf((char *)msgs.would_overwrite,
+		"Your local changes to '%%s' would be overwritten by %s.  Aborting.",
+		cmd);
+	/* not_uptodate_file */
+	msgs.not_uptodate_file = msgs.would_overwrite;
+	/* not_uptodate_dir */
+	msgs.not_uptodate_dir =
+		"Updating '%s' would lose untracked files in it.  Aborting.";
+	/* would_lose_untracked */
+	msgs.would_lose_untracked = malloc(sizeof(char) * 72);
+	sprintf((char *)msgs.would_lose_untracked,
+		"Untracked working tree file '%%s' would be %%s by %s.  Aborting.",
+		cmd);
+
 	if (advice_commit_before_merge) {
-		msgs.would_overwrite = msgs.not_uptodate_file =
-			"Your local changes to '%s' would be overwritten by merge.  Aborting.\n"
-			"Please, commit your changes or stash them before you can merge.";
+		msgs.would_overwrite = malloc(sizeof(char) * 140);
+		sprintf((char *)msgs.would_overwrite,
+			"Your local changes to '%%s' would be overwritten by %s.  Aborting.\n"
+			"Please, commit your changes or stash them before you can %s.",
+			cmd, strcmp(cmd,"checkout") ? cmd : "swicth branches");
+		msgs.not_uptodate_file = msgs.would_overwrite;
+		msgs.would_lose_untracked = malloc (sizeof(char) * 135);
+		sprintf((char *)msgs.would_lose_untracked,
+			"Untracked working tree file '%%s' would be %%s by %s.  Aborting.\n"
+			"Please move or remove them before you can %s.",
+			cmd, strcmp(cmd,"checkout") ? cmd : "swicth branches");
 	}
 	return msgs;
 }
diff --git a/merge-recursive.h b/merge-recursive.h
index 0cc465e..d910ae6 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -23,8 +23,10 @@ struct merge_options {
 	struct string_list current_directory_set;
 };
 
-/* Return a list of user-friendly error messages to be used by merge */
-struct unpack_trees_error_msgs get_porcelain_error_msgs(void);
+/* Return a list of user-friendly error messages to be used by
+ * the command cmd which would be either merge or checkout
+ */
+struct unpack_trees_error_msgs get_porcelain_error_msgs(const char *cmd);
 
 /* merge_trees() but with recursive ancestor consolidation */
 int merge_recursive(struct merge_options *o,
-- 
1.6.6.7.ga5fe3

[PATCH 2/5 v2] unpack_trees: group errors by type

[Diane Gasselin]

When an error is encountered, it calls add_rejected_file() which either
- directly displays the error message and stops if in plumbing mode
  (i.e. if show_all_errors is not initialized at 1)
- or stores it so that it will be displayed at the end with display_error_msgs(),

Storing the files by error type permits to have a list of files for
which there is the same error instead of having a serie of almost
identical errors.

As each bind_overlap error combines a file and an old file, a list cannot be
done, therefore, theses errors are not stored but directly displayed.

Update t3030 to expect failure for a test based on an error message.

Signed-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
Signed-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>
Signed-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>
---
 builtin/checkout.c         |    1 +
 builtin/merge.c            |    2 +-
 t/t3030-merge-recursive.sh |    2 +-
 tree-walk.c                |   11 +++-
 unpack-trees.c             |  119 +++++++++++++++++++++++++++++++++++++++++---
 unpack-trees.h             |   31 +++++++++++-
 6 files changed, 152 insertions(+), 14 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 6f34566..23eae56 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -392,6 +392,7 @@ static int merge_working_tree(struct checkout_opts *opts,
 		topts.dir = xcalloc(1, sizeof(*topts.dir));
 		topts.dir->flags |= DIR_SHOW_IGNORED;
 		topts.dir->exclude_per_dir = ".gitignore";
+		topts.show_all_errors = 1;
 		tree = parse_tree_indirect(old->commit ?
 					   old->commit->object.sha1 :
 					   (unsigned char *)EMPTY_TREE_SHA1_BIN);
diff --git a/builtin/merge.c b/builtin/merge.c
index 501177f..2c2f904 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -704,7 +704,7 @@ int checkout_fast_forward(const unsigned char *head, const unsigned char *remote
 	opts.verbose_update = 1;
 	opts.merge = 1;
 	opts.fn = twoway_merge;
-
+	opts.show_all_errors = 1;
 	opts.msgs = get_porcelain_error_msgs("merge");
 
 	trees[nr_trees] = parse_tree_indirect(head);
diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 9929f82..7ef8dd4 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -269,7 +269,7 @@ test_expect_success 'merge-recursive result' '
 
 '
 
-test_expect_success 'fail if the index has unresolved entries' '
+test_expect_failure 'fail if the index has unresolved entries' '
 
 	rm -fr [abcd] &&
 	git checkout -f "$c1" &&
diff --git a/tree-walk.c b/tree-walk.c
index 67a9a0c..b8b7f00 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "tree-walk.h"
+#include "unpack-trees.h"
 #include "tree.h"
 
 static const char *get_mode(const char *str, unsigned int *modep)
@@ -310,6 +311,7 @@ static void free_extended_entry(struct tree_desc_x *t)
 int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
 {
 	int ret = 0;
+	int error = 0;
 	struct name_entry *entry = xmalloc(n*sizeof(*entry));
 	int i;
 	struct tree_desc_x *tx = xcalloc(n, sizeof(*tx));
@@ -377,8 +379,11 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
 		if (!mask)
 			break;
 		ret = info->fn(n, mask, dirmask, entry, info);
-		if (ret < 0)
-			break;
+		if (ret < 0) {
+			error = ret;
+			if (!((struct unpack_trees_options*)(info->data))->show_all_errors)
+				break;
+		}
 		mask &= ret;
 		ret = 0;
 		for (i = 0; i < n; i++)
@@ -389,7 +394,7 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
 	for (i = 0; i < n; i++)
 		free_extended_entry(tx + i);
 	free(tx);
-	return ret;
+	return error;
 }
 
 static int find_tree_entry(struct tree_desc *t, const char *name, unsigned char *result, unsigned *mode)
diff --git a/unpack-trees.c b/unpack-trees.c
index c29a9e0..78ecdc9 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -60,6 +60,92 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
 }
 
 /*
+ * add error messages on path <path> and action <action>
+ * corresponding to the type <e> with the message <msg>
+ * indicating if it should be display in porcelain or not
+ */
+static int add_rejected_path(struct unpack_trees_options *o,
+			     enum unpack_trees_error e,
+			     const char *path,
+			     const char *action,
+			     int porcelain,
+			     const char *msg)
+{
+	struct rejected_paths_list *newentry;
+	struct rejected_paths **rp;
+	/*
+	 * simply display the given error message if in plumbing mode
+	 */
+	if (!porcelain)
+		o->show_all_errors = 0;
+	if (!o->show_all_errors)
+		return error(msg, path, action);
+	/*
+	 * if there is a porcelain error message defined,
+	 * the error is stored in order to be nicely displayed later
+	 */
+	if (e == would_lose_untracked_overwritten && !strcmp(action, "removed"))
+		e = would_lose_untracked_removed;
+
+	rp = &o->unpack_rejects[e];
+
+	if (!o->unpack_rejects[e]) {
+		*rp = malloc(sizeof(struct rejected_paths));
+		(*rp)->list = NULL;
+	}
+	newentry = malloc(sizeof(struct rejected_paths_list));
+	newentry->path = (char *)path;
+	newentry->next = (*rp)->list;
+	(*rp)->list = newentry;
+	(*rp)->msg = msg;
+	(*rp)->action = (char *)action;
+	return -1;
+}
+
+/*
+ * free all the structures allocated for the error <e>
+ */
+static void free_rejected_paths(struct unpack_trees_options *o,
+				enum unpack_trees_error e)
+{
+	while (o->unpack_rejects[e]->list) {
+		struct rejected_paths_list *del = o->unpack_rejects[e]->list;
+		o->unpack_rejects[e]->list = o->unpack_rejects[e]->list->next;
+		free(del);
+	}
+	free(o->unpack_rejects[e]);
+}
+
+/*
+ * display all the error messages stored in a nice way
+ */
+static void display_error_msgs(struct unpack_trees_options *o)
+{
+	int i;
+	int something_is_displayed = 0;
+	for (i = 0; i < NB_UNPACK_TREES_ERROR; i++) {
+		if (o->unpack_rejects[i] && o->unpack_rejects[i]->list) {
+			struct rejected_paths *rp = o->unpack_rejects[i];
+			struct rejected_paths_list *f = rp->list;
+			char *action = rp->action;
+			struct strbuf path = STRBUF_INIT;
+			something_is_displayed  = 1;
+			for (f = rp->list; f; f = f->next)
+				strbuf_addf(&path, "\t%s\n", f->path);
+			if (i == would_lose_untracked_overwritten ||
+			    i == would_lose_untracked_removed)
+				error(rp->msg, action, path.buf);
+			else
+				error(rp->msg, path.buf, action);
+			strbuf_release(&path);
+			free_rejected_paths(o, i);
+		}
+	}
+	if (something_is_displayed)
+		printf("Aborting\n");
+}
+
+/*
  * Unlink the last component and schedule the leading directories for
  * removal, such that empty directories get removed.
  */
@@ -819,6 +905,8 @@ done:
 	return ret;
 
 return_failed:
+	if (o->show_all_errors)
+		display_error_msgs(o);
 	mark_all_ce_unused(o->src_index);
 	ret = unpack_failed(o, NULL);
 	goto done;
@@ -828,7 +916,9 @@ return_failed:
 
 static int reject_merge(struct cache_entry *ce, struct unpack_trees_options *o)
 {
-	return error(ERRORMSG(o, would_overwrite), ce->name);
+	return add_rejected_path(o, would_overwrite, ce->name, NULL,
+				 (o && (o)->msgs.would_overwrite),
+				 ERRORMSG(o, would_overwrite));
 }
 
 static int same(struct cache_entry *a, struct cache_entry *b)
@@ -850,7 +940,7 @@ static int same(struct cache_entry *a, struct cache_entry *b)
  */
 static int verify_uptodate_1(struct cache_entry *ce,
 				   struct unpack_trees_options *o,
-				   const char *error_msg)
+				   enum unpack_trees_error error)
 {
 	struct stat st;
 
@@ -874,8 +964,16 @@ static int verify_uptodate_1(struct cache_entry *ce,
 	}
 	if (errno == ENOENT)
 		return 0;
-	return o->gently ? -1 :
-		error(error_msg, ce->name);
+	if (error == sparse_not_uptodate_file)
+		return o->gently ? -1 :
+			add_rejected_path(o, sparse_not_uptodate_file, ce->name, NULL,
+					  (o && (o)->msgs.sparse_not_uptodate_file),
+					  ERRORMSG(o, sparse_not_uptodate_file));
+	else
+		return o->gently ? -1 :
+			add_rejected_path(o, not_uptodate_file, ce->name, NULL,
+					  (o && (o)->msgs.not_uptodate_file),
+					  ERRORMSG(o, not_uptodate_file));
 }
 
 static int verify_uptodate(struct cache_entry *ce,
@@ -883,13 +981,13 @@ static int verify_uptodate(struct cache_entry *ce,
 {
 	if (!o->skip_sparse_checkout && will_have_skip_worktree(ce, o))
 		return 0;
-	return verify_uptodate_1(ce, o, ERRORMSG(o, not_uptodate_file));
+	return verify_uptodate_1(ce, o, not_uptodate_file);
 }
 
 static int verify_uptodate_sparse(struct cache_entry *ce,
 				  struct unpack_trees_options *o)
 {
-	return verify_uptodate_1(ce, o, ERRORMSG(o, sparse_not_uptodate_file));
+	return verify_uptodate_1(ce, o, sparse_not_uptodate_file);
 }
 
 static void invalidate_ce_path(struct cache_entry *ce, struct unpack_trees_options *o)
@@ -976,7 +1074,9 @@ static int verify_clean_subdirectory(struct cache_entry *ce, const char *action,
 	i = read_directory(&d, pathbuf, namelen+1, NULL);
 	if (i)
 		return o->gently ? -1 :
-			error(ERRORMSG(o, not_uptodate_dir), ce->name);
+			add_rejected_path(o, not_uptodate_dir, ce->name, NULL,
+					  (o && (o)->msgs.not_uptodate_dir),
+					  ERRORMSG(o, not_uptodate_dir));
 	free(pathbuf);
 	return cnt;
 }
@@ -1058,7 +1158,10 @@ static int verify_absent_1(struct cache_entry *ce, const char *action,
 		}
 
 		return o->gently ? -1 :
-			error(ERRORMSG(o, would_lose_untracked), ce->name, action);
+			add_rejected_path(o, would_lose_untracked_overwritten,
+					  ce->name, action,
+					  (o && (o)->msgs.would_lose_untracked),
+					  ERRORMSG(o, would_lose_untracked));
 	}
 	return 0;
 }
diff --git a/unpack-trees.h b/unpack-trees.h
index ef70eab..1f8e71e 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -2,6 +2,7 @@
 #define UNPACK_TREES_H
 
 #define MAX_UNPACK_TREES 8
+#define NB_UNPACK_TREES_ERROR 6
 
 struct unpack_trees_options;
 struct exclude_list;
@@ -19,6 +20,27 @@ struct unpack_trees_error_msgs {
 	const char *would_lose_orphaned;
 };
 
+struct rejected_paths_list {
+	char *path;
+	struct rejected_paths_list *next;
+};
+
+struct rejected_paths {
+	char *action;
+	const char *msg;
+	struct rejected_paths_list *list;
+};
+
+
+enum unpack_trees_error{
+	would_overwrite,
+	not_uptodate_file,
+	not_uptodate_dir,
+	would_lose_untracked_overwritten,
+	would_lose_untracked_removed,
+	sparse_not_uptodate_file
+};
+
 struct unpack_trees_options {
 	unsigned int reset,
 		     merge,
@@ -33,7 +55,8 @@ struct unpack_trees_options {
 		     diff_index_cached,
 		     debug_unpack,
 		     skip_sparse_checkout,
-		     gently;
+		     gently,
+		     show_all_errors;
 	const char *prefix;
 	int cache_bottom;
 	struct dir_struct *dir;
@@ -51,6 +74,12 @@ struct unpack_trees_options {
 	struct index_state result;
 
 	struct exclude_list *el; /* for internal use */
+
+	/*
+	 * Store error messages in an array, each case
+	 * corresponding to a error message type
+	 */
+	struct rejected_paths *unpack_rejects[NB_UNPACK_TREES_ERROR];
 };
 
 extern int unpack_trees(unsigned n, struct tree_desc *t,
-- 
1.6.6.7.ga5fe3

[PATCH 3/5 v2] unpack_trees_options: update porcelain messages

[Diane Gasselin]

Update porcelain messages of unpack_trees_options in order to have a good layout
and add an advice for would_lose_untracked errors if advice_commit_before_merge
is enabled.

Update t3400 to have an expect_failure for the rebase verbose error message.

Signed-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
Signed-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>
Signed-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>
---
 builtin/checkout.c |    2 +-
 merge-recursive.c  |   18 +++++++++---------
 t/t3400-rebase.sh  |    2 +-
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 23eae56..b9d056d 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -373,7 +373,7 @@ static int merge_working_tree(struct checkout_opts *opts,
 		topts.dst_index = &the_index;
 
 		topts.msgs = get_porcelain_error_msgs("checkout");
-		topts.msgs.not_uptodate_file = "You have local changes to '%s'; cannot switch branches.";
+		topts.msgs.not_uptodate_file = "You have local changes to the following files:\n%sCannot switch branches.";
 
 		refresh_cache(REFRESH_QUIET);
 
diff --git a/merge-recursive.c b/merge-recursive.c
index 80c9744..ee80553 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1183,31 +1183,31 @@ struct unpack_trees_error_msgs get_porcelain_error_msgs(const char *cmd)
 	struct unpack_trees_error_msgs msgs;
 
 	/* would_overwrite */
-	msgs.would_overwrite = malloc(sizeof(char) * 72);
+	msgs.would_overwrite = malloc(sizeof(char) * 80);
 	sprintf((char *)msgs.would_overwrite,
-		"Your local changes to '%%s' would be overwritten by %s.  Aborting.",
+		"Your local changes to the following files would be overwritten by %s:\n%%s",
 		cmd);
 	/* not_uptodate_file */
 	msgs.not_uptodate_file = msgs.would_overwrite;
 	/* not_uptodate_dir */
 	msgs.not_uptodate_dir =
-		"Updating '%s' would lose untracked files in it.  Aborting.";
+		"Updating the following directories would lose untracked files in it:\n%s";
 	/* would_lose_untracked */
-	msgs.would_lose_untracked = malloc(sizeof(char) * 72);
+	msgs.would_lose_untracked = malloc(sizeof(char) * 80);
 	sprintf((char *)msgs.would_lose_untracked,
-		"Untracked working tree file '%%s' would be %%s by %s.  Aborting.",
+		"The following untracked working tree files would be %%s by %s:\n%%s",
 		cmd);
 
 	if (advice_commit_before_merge) {
-		msgs.would_overwrite = malloc(sizeof(char) * 140);
+		msgs.would_overwrite = malloc(sizeof(char) * 160);
 		sprintf((char *)msgs.would_overwrite,
-			"Your local changes to '%%s' would be overwritten by %s.  Aborting.\n"
+			"Your local changes to the following files would be overwritten by %s:\n%%s"
 			"Please, commit your changes or stash them before you can %s.",
 			cmd, strcmp(cmd,"checkout") ? cmd : "swicth branches");
 		msgs.not_uptodate_file = msgs.would_overwrite;
-		msgs.would_lose_untracked = malloc (sizeof(char) * 135);
+		msgs.would_lose_untracked = malloc (sizeof(char) * 160);
 		sprintf((char *)msgs.would_lose_untracked,
-			"Untracked working tree file '%%s' would be %%s by %s.  Aborting.\n"
+			"The following untracked working tree files would be %%s by %s:\n%%s"
 			"Please move or remove them before you can %s.",
 			cmd, strcmp(cmd,"checkout") ? cmd : "swicth branches");
 	}
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index dbf7dfb..cbf160d 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -121,7 +121,7 @@ test_expect_success 'rebase a single mode change' '
      GIT_TRACE=1 git rebase master
 '
 
-test_expect_success 'Show verbose error when HEAD could not be detached' '
+test_expect_failure 'Show verbose error when HEAD could not be detached' '
      : > B &&
      test_must_fail git rebase topic 2> output.err > output.out &&
      grep "Untracked working tree file .B. would be overwritten" output.err
-- 
1.6.6.7.ga5fe3

[PATCH 4/5 v2] tests: update porcelain expected message

[Diane Gasselin]

As porcelain messages have been changed, the expected porcelain messages
tested in t3030 and t3400 need to be changed.

Signed-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
Signed-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>
Signed-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>
---
 t/t3030-merge-recursive.sh |    4 ++--
 t/t3400-rebase.sh          |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 7ef8dd4..77bf0f0 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -269,7 +269,7 @@ test_expect_success 'merge-recursive result' '
 
 '
 
-test_expect_failure 'fail if the index has unresolved entries' '
+test_expect_success 'fail if the index has unresolved entries' '
 
 	rm -fr [abcd] &&
 	git checkout -f "$c1" &&
@@ -282,7 +282,7 @@ test_expect_failure 'fail if the index has unresolved entries' '
 	grep "You have not concluded your merge" out &&
 	rm -f .git/MERGE_HEAD &&
 	test_must_fail git merge "$c5" 2> out &&
-	grep "Your local changes to .* would be overwritten by merge." out
+	grep "Your local changes to the following files would be overwritten by merge:" out
 '
 
 test_expect_success 'merge-recursive remove conflict' '
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index cbf160d..55be0c2 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -121,10 +121,10 @@ test_expect_success 'rebase a single mode change' '
      GIT_TRACE=1 git rebase master
 '
 
-test_expect_failure 'Show verbose error when HEAD could not be detached' '
+test_expect_success 'Show verbose error when HEAD could not be detached' '
      : > B &&
      test_must_fail git rebase topic 2> output.err > output.out &&
-     grep "Untracked working tree file .B. would be overwritten" output.err
+     grep "The following untracked working tree files would be overwritten by checkout:" output.err
 '
 
 test_expect_success 'rebase -q is quiet' '
-- 
1.6.6.7.ga5fe3

[PATCH 5/5 v2] t7609: test merge and checkout error messages

[Diane Gasselin]

Test porcelain and plumbing error messages for different types of errors
of merge and checkout.

Signed-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
Signed-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>
Signed-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>
---
 t/t7609-merge-co-error-msgs.sh |  125 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 125 insertions(+), 0 deletions(-)
 create mode 100755 t/t7609-merge-co-error-msgs.sh

diff --git a/t/t7609-merge-co-error-msgs.sh b/t/t7609-merge-co-error-msgs.sh
new file mode 100755
index 0000000..b636b75
--- /dev/null
+++ b/t/t7609-merge-co-error-msgs.sh
@@ -0,0 +1,125 @@
+#!/bin/sh
+
+test_description='unpack-trees error messages'
+
+. ./test-lib.sh
+
+
+test_expect_success 'setup' '
+	echo one >one &&
+	git add one &&
+	git commit -a -m First &&
+
+	git checkout -b branch &&
+	echo two>two &&
+	echo three>three &&
+	echo four>four &&
+	echo five>five &&
+	git add two three four five &&
+	git commit -m Second &&
+
+	git checkout master &&
+	echo other>two &&
+	echo other>three &&
+	echo other>four &&
+	echo other>five
+'
+
+cat> expect <<\EOF
+error: The following untracked working tree files would be overwritten by merge:
+	two
+	three
+	four
+	five
+Please move or remove them before you can merge.
+EOF
+
+test_expect_success 'untracked files overwritten by merge' '
+	! git merge branch 2> out &&
+	test_cmp out expect
+'
+
+cat> expect <<\EOF
+error: Your local changes to the following files would be overwritten by merge:
+	two
+	three
+	four
+Please, commit your changes or stash them before you can merge.
+error: The following untracked working tree files would be overwritten by merge:
+	five
+Please move or remove them before you can merge.
+EOF
+
+test_expect_success 'untracked files or local changes ovewritten by merge' '
+	git add two &&
+	git add three &&
+	git add four &&
+	! git merge branch 2> out &&
+	test_cmp out expect
+'
+
+cat> expect <<\EOF
+error: You have local changes to the following files:
+	rep/two
+	rep/one
+Cannot switch branches.
+EOF
+
+test_expect_success 'cannot switch branches because of local changes' '
+	git add five &&
+	mkdir rep &&
+	echo one>rep/one &&
+	echo two>rep/two &&
+	git add rep/one rep/two &&
+	git commit -m Fourth &&
+	git checkout master &&
+	echo uno>rep/one &&
+	echo dos>rep/two &&
+	! git checkout branch 2> out &&
+	test_cmp out expect
+'
+
+cat> expect <<\EOF
+error: Your local changes to the following files would be overwritten by checkout:
+	rep/two
+	rep/one
+Please, commit your changes or stash them before you can swicth branches.
+EOF
+
+test_expect_success 'not uptodate file porcelain checkout error' '
+	git add rep/one rep/two &&
+	! git checkout branch 2> out &&
+	test_cmp out expect
+'
+
+cat> expect <<\EOF
+error: Updating the following directories would lose untracked files in it:
+	rep2
+	rep
+
+EOF
+
+test_expect_success 'not_uptodate_dir porcelain checkout error' '
+	git init uptodate &&
+	cd uptodate &&
+	mkdir rep &&
+	mkdir rep2 &&
+	touch rep/foo &&
+	touch rep2/foo &&
+	git add rep/foo rep2/foo &&
+	git commit -m init &&
+	git checkout -b branch &&
+	git rm rep -r &&
+	git rm rep2 -r &&
+	> rep &&
+	> rep2 &&
+	git add rep rep2&&
+	git commit -m "added test as a file" &&
+	git checkout master &&
+	> rep/untracked-file &&
+	> rep2/untracked-file &&
+	! git checkout branch 2> out &&
+	test_cmp out ../expect
+'
+
+test_done
-- 
1.6.6.7.ga5fe3

Re: [PATCH 1/5 v2] merge-recursive: porcelain messages for checkout

[Matthieu Moy]

Diane Gasselin <diane.gasselin@ensimag.imag.fr> writes:

> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -372,6 +372,7 @@ static int merge_working_tree(struct checkout_opts *opts,
>  		topts.src_index = &the_index;
>  		topts.dst_index = &the_index;
>  
> +		topts.msgs = get_porcelain_error_msgs("checkout");
>  		topts.msgs.not_uptodate_file = "You have local changes to '%s'; cannot switch branches.";

It's nice to get accurate messages for all cases, but then why do you
keep the special-case for not_uptodate_file? If there's a good reason
for it, a comment in the code would be welcome.

> +	/* would_overwrite */
> +	msgs.would_overwrite = malloc(sizeof(char) * 72);
> +	sprintf((char *)msgs.would_overwrite,
> +		"Your local changes to '%%s' would be overwritten by %s.  Aborting.",
> +		cmd);

This yields:

  Your local changes to 'foo' would be overwritten by checkout.  Aborting.

I tend to prefer Junio's wording:

  You have local changes to 'foo'; cannot switch branches.

--
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 2/5 v2] unpack_trees: group errors by type

[Matthieu Moy]

Diane Gasselin <diane.gasselin@ensimag.imag.fr> writes:

> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -60,6 +60,92 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
>  }
>  
>  /*
> + * add error messages on path <path> and action <action>
> + * corresponding to the type <e> with the message <msg>
> + * indicating if it should be display in porcelain or not
> + */
> +static int add_rejected_path(struct unpack_trees_options *o,
> +			     enum unpack_trees_error e,
> +			     const char *path,
> +			     const char *action,
> +			     int porcelain,
> +			     const char *msg)
> +{
> +	struct rejected_paths_list *newentry;
> +	struct rejected_paths **rp;
> +	/*
> +	 * simply display the given error message if in plumbing mode
> +	 */
> +	if (!porcelain)
> +		o->show_all_errors = 0;
> +	if (!o->show_all_errors)
> +		return error(msg, path, action);

I don't fully understand what you're doing with show_all_errors and
porcelain here. From the caller, "porcelain" is true iff the
corresponding error message has been set in o. But if you can infer
whether you're in porcelain from the error messages, why do you need
show_all_errors in addition?

>  static int reject_merge(struct cache_entry *ce, struct unpack_trees_options *o)
>  {
> -	return error(ERRORMSG(o, would_overwrite), ce->name);
> +	return add_rejected_path(o, would_overwrite, ce->name, NULL,
> +				 (o && (o)->msgs.would_overwrite),

Parenthesis around (o) are distracting and useless. I guess you
copy-pasted from a macro (for which parentheses should definitely be
used in case the macro is called on an arbitrary expression).

> @@ -874,8 +964,16 @@ static int verify_uptodate_1(struct cache_entry *ce,
>  	}
>  	if (errno == ENOENT)
>  		return 0;
> -	return o->gently ? -1 :
> -		error(error_msg, ce->name);
> +	if (error == sparse_not_uptodate_file)
> +		return o->gently ? -1 :
> +			add_rejected_path(o, sparse_not_uptodate_file, ce->name, NULL,
> +					  (o && (o)->msgs.sparse_not_uptodate_file),
> +					  ERRORMSG(o, sparse_not_uptodate_file));
> +	else
> +		return o->gently ? -1 :
> +			add_rejected_path(o, not_uptodate_file, ce->name, NULL,
> +					  (o && (o)->msgs.not_uptodate_file),
> +					  ERRORMSG(o, not_uptodate_file));
>  }

Isn't that a complex way of saying

	int porcelain;
	if (error == sparse_not_uptodate_file)
		porcelain = o && o->msgs.sparse_not_uptodate_file;
	else
		porcelain = o && o->msgs.not_uptodate_file;
	return o->gently ? -1 :
			add_rejected_path(o, error, ce->name, NULL,
					  porcelain, ERRORMSG(o, error));

?

Also, I'm not sure I understand why you're attaching the error message
string to each rejected_paths entry. Wouldn't it be more sensible to
use o->msg in display_error_msgs() instead?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 3/5 v2] unpack_trees_options: update porcelain messages

[Matthieu Moy]

Diane Gasselin <diane.gasselin@ensimag.imag.fr> writes:

> -	msgs.would_overwrite = malloc(sizeof(char) * 72);
> +	msgs.would_overwrite = malloc(sizeof(char) * 80);
>  	sprintf((char *)msgs.would_overwrite,
> -		"Your local changes to '%%s' would be overwritten by %s.  Aborting.",
> +		"Your local changes to the following files would be overwritten by %s:\n%%s",

I hate hardcoded string length (these magic 80 and 72). Can't it be
stg like

const char * const msg = "Your local changes to ....";
msg.would_overwrite = malloc(strlen(msg) + strlen(cmd) + something);
sprintf(msg.would_overwrite, msg, ...);

instead?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 5/5 v2] t7609: test merge and checkout error messages

[Matthieu Moy]

Diane Gasselin <diane.gasselin@ensimag.imag.fr> writes:

> +	echo two>two &&
> +	echo three>three &&
> +	echo four>four &&
> +	echo five>five &&

Space before '>' please :

2010/6/9 Junio C Hamano <gitster@pobox.com>:

>  (1) redirection ">" and "<" stick to the target file and have a SP on the
>     other end.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 2/5 v2] unpack_trees: group errors by type

[Diane Gasselin]

Le 15 juin 2010 14:58, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> a écrit :
> Diane Gasselin <diane.gasselin@ensimag.imag.fr> writes:
>
>> --- a/unpack-trees.c
>> +++ b/unpack-trees.c
>> @@ -60,6 +60,92 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
>>  }
>>
>>  /*
>> + * add error messages on path <path> and action <action>
>> + * corresponding to the type <e> with the message <msg>
>> + * indicating if it should be display in porcelain or not
>> + */
>> +static int add_rejected_path(struct unpack_trees_options *o,
>> +                          enum unpack_trees_error e,
>> +                          const char *path,
>> +                          const char *action,
>> +                          int porcelain,
>> +                          const char *msg)
>> +{
>> +     struct rejected_paths_list *newentry;
>> +     struct rejected_paths **rp;
>> +     /*
>> +      * simply display the given error message if in plumbing mode
>> +      */
>> +     if (!porcelain)
>> +             o->show_all_errors = 0;
>> +     if (!o->show_all_errors)
>> +             return error(msg, path, action);
>
> I don't fully understand what you're doing with show_all_errors and
> porcelain here. From the caller, "porcelain" is true iff the
> corresponding error message has been set in o. But if you can infer
> whether you're in porcelain from the error messages, why do you need
> show_all_errors in addition?
>
>>  static int reject_merge(struct cache_entry *ce, struct unpack_trees_options *o)
>>  {
>> -     return error(ERRORMSG(o, would_overwrite), ce->name);
>> +     return add_rejected_path(o, would_overwrite, ce->name, NULL,
>> +                              (o && (o)->msgs.would_overwrite),
>
> Parenthesis around (o) are distracting and useless. I guess you
> copy-pasted from a macro (for which parentheses should definitely be
> used in case the macro is called on an arbitrary expression).
>
>> @@ -874,8 +964,16 @@ static int verify_uptodate_1(struct cache_entry *ce,
>>       }
>>       if (errno == ENOENT)
>>               return 0;
>> -     return o->gently ? -1 :
>> -             error(error_msg, ce->name);
>> +     if (error == sparse_not_uptodate_file)
>> +             return o->gently ? -1 :
>> +                     add_rejected_path(o, sparse_not_uptodate_file, ce->name, NULL,
>> +                                       (o && (o)->msgs.sparse_not_uptodate_file),
>> +                                       ERRORMSG(o, sparse_not_uptodate_file));
>> +     else
>> +             return o->gently ? -1 :
>> +                     add_rejected_path(o, not_uptodate_file, ce->name, NULL,
>> +                                       (o && (o)->msgs.not_uptodate_file),
>> +                                       ERRORMSG(o, not_uptodate_file));
>>  }
>
> Isn't that a complex way of saying
>
>        int porcelain;
>        if (error == sparse_not_uptodate_file)
>                porcelain = o && o->msgs.sparse_not_uptodate_file;
>        else
>                porcelain = o && o->msgs.not_uptodate_file;
>        return o->gently ? -1 :
>                        add_rejected_path(o, error, ce->name, NULL,
>                                          porcelain, ERRORMSG(o, error));
>
> ?
>

The problem is that "error" is an enum unpack_trees_error, and
ERRORMSG takes the name of the field from unpack_trees_error_msgs.
If I try to do ERRORMSG(o, error), the compilator would say that the
"error" is not a field of unpack_trees_error_msgs.

> Also, I'm not sure I understand why you're attaching the error message
> string to each rejected_paths entry. Wouldn't it be more sensible to
> use o->msg in display_error_msgs() instead?
>

In display_error_msgs(), I cannot access o->msg because I would not
know which error I am treating.
In the same way as previously, I cannot use the enum
unpack_trees_error to access it.

I know it makes the code a bit "heavy" but I did not see a better way to do it.

> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
>

Re: [PATCH 2/5 v2] unpack_trees: group errors by type

[Matthieu Moy]

Diane Gasselin <diane.gasselin@ensimag.imag.fr> writes:

> In display_error_msgs(), I cannot access o->msg because I would not
> know which error I am treating.

You do:

static void display_error_msgs(struct unpack_trees_options *o)
{
...
	for (i = 0; i < NB_UNPACK_TREES_ERROR; i++) {
		...
	}

You know "i", so you know which error it is. The difficulty is that
the rejected paths are in an array, while the error messages are in a
struct, but you can either:

* Turn the struct into an array, and say msgs[would_overwrite] instead
  of msgs.would_overwrite (which would also simplify the code
  elsewhere since you would be able to write "ERRORMSG(o, error)" and
  such things).

* Do

switch (i) {
case would_overwrite:
	msg = o->msg.would_overwrite;
	break;
case not_uptodate_file:
	msg = o->msg.not_uptodate_file;
	break;
case not_uptodate_dir:
	msg = o->msg.not_uptodate_dir;
	break;
case would_lose_untracked_overwritten:
	msg = o->msg.would_lose_untracked_overwritten;
	break;
case would_lose_untracked_removed:
	msg = o->msg.would_lose_untracked_removed;
	break;
case sparse_not_uptodate_file:
	msg = o->msg.sparse_not_uptodate_file;
	break;
}

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 2/5 v2] unpack_trees: group errors by type

[Diane Gasselin]

Le 15 juin 2010 15:28, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> a écrit :
> Diane Gasselin <diane.gasselin@ensimag.imag.fr> writes:
>
>> In display_error_msgs(), I cannot access o->msg because I would not
>> know which error I am treating.
>
> You do:
>
> static void display_error_msgs(struct unpack_trees_options *o)
> {
> ...
>        for (i = 0; i < NB_UNPACK_TREES_ERROR; i++) {
>                ...
>        }
>
> You know "i", so you know which error it is. The difficulty is that
> the rejected paths are in an array, while the error messages are in a
> struct, but you can either:
>
> * Turn the struct into an array, and say msgs[would_overwrite] instead
>  of msgs.would_overwrite (which would also simplify the code
>  elsewhere since you would be able to write "ERRORMSG(o, error)" and
>  such things).
>
> * Do
>
> switch (i) {
> case would_overwrite:
>        msg = o->msg.would_overwrite;
>        break;
> case not_uptodate_file:
>        msg = o->msg.not_uptodate_file;
>        break;
> case not_uptodate_dir:
>        msg = o->msg.not_uptodate_dir;
>        break;
> case would_lose_untracked_overwritten:
>        msg = o->msg.would_lose_untracked_overwritten;
>        break;
> case would_lose_untracked_removed:
>        msg = o->msg.would_lose_untracked_removed;
>        break;
> case sparse_not_uptodate_file:
>        msg = o->msg.sparse_not_uptodate_file;
>        break;
> }
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
>
Thanks for your answers.
I did the switch case at first but thought it was maybe a bit
repetitive. That is why, I opted for giving directly the message in
add_rejected_path().

Otherwise, I do agree an array would make things easier, especially
for my patch. Does anyone has an objection into changing the struct
unpack_trees_error_msgs into an array?