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

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

[Diane Gasselin]

This patch serie aims at grouping merge and checkout errors messages by type
if possible, listing all the file concerned by the error type.

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

Diane (5):
  tree-walk: do not stop when an error is detected
  unpack_trees: group errors by type
  unpack_trees_options: update porcelain messages
  t3030: update porcelain expected message
  t7609: test merge and checkout error messages

 builtin/checkout.c             |    2 +-
 merge-recursive.c              |   10 ++--
 t/t3030-merge-recursive.sh     |    8 ++-
 t/t7609-merge-co-error-msgs.sh |  122 ++++++++++++++++++++++++++++++++++++++
 tree-walk.c                    |    5 +-
 unpack-trees.c                 |  128 +++++++++++++++++++++++++++++++++++++---
 unpack-trees.h                 |   12 ++++
 7 files changed, 270 insertions(+), 17 deletions(-)
 create mode 100755 t/t7609-merge-co-error-msgs.sh

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

[Diane Gasselin]

This patch serie aims at grouping merge and checkout errors messages by type
if possible, listing all the file concerned by the error type.

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

Diane (5):
  tree-walk: do not stop when an error is detected
  unpack_trees: group errors by type
  unpack_trees_options: update porcelain messages
  t3030: update porcelain expected message
  t7609: test merge and checkout error messages

 builtin/checkout.c             |    2 +-
 merge-recursive.c              |   10 ++--
 t/t3030-merge-recursive.sh     |    8 ++-
 t/t7609-merge-co-error-msgs.sh |  122 ++++++++++++++++++++++++++++++++++++++
 tree-walk.c                    |    5 +-
 unpack-trees.c                 |  128 +++++++++++++++++++++++++++++++++++++---
 unpack-trees.h                 |   12 ++++
 7 files changed, 270 insertions(+), 17 deletions(-)
 create mode 100755 t/t7609-merge-co-error-msgs.sh

[RFC/ PATCH 1/5] tree-walk: do not stop when an error is detected

[Diane Gasselin]

From: Diane <diane.gasselin@ensimag.imag.fr>

When an error is detected, traverse_trees() is not stopped anymore.
The whole tree is traversed so that all the merging errors can be detected.

Signed-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
Signed-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>
Signed-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>
---
 tree-walk.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/tree-walk.c b/tree-walk.c
index 67a9a0c..04072aa 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -310,6 +310,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));
@@ -378,7 +379,7 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
 			break;
 		ret = info->fn(n, mask, dirmask, entry, info);
 		if (ret < 0)
-			break;
+			error = ret;
 		mask &= ret;
 		ret = 0;
 		for (i = 0; i < n; i++)
@@ -389,7 +390,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)
-- 
1.6.6.7.ga5fe3

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

[Diane Gasselin]

From: Diane <diane.gasselin@ensimag.imag.fr>

When an error is encountered, it calls add_rejected_file() which either
- directly displays the error message if in plumbing mode
- or stores it so that it will be displayed at the end of 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.

Signed-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
Signed-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>
Signed-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>
---
It appears that in verify_absent_sparse(), verify_absent_1() is called with
ERRORMSG(o, would_lose_orphaned) as the error message.
Yet, in verify_absent_1(), this error message error_msg does not
seem to be used and at the end of the function, a would_lose_untracked error
is treated (before displayed and now added). Is it normal?

 unpack-trees.c |  128 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
 unpack-trees.h |   12 +++++
 2 files changed, 132 insertions(+), 8 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index c29a9e0..1e2f48d 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -45,6 +45,21 @@ static struct unpack_trees_error_msgs unpack_plumbing_errors = {
 	? ((o)->msgs.fld) \
 	: (unpack_plumbing_errors.fld) )
 
+/*
+ * Store error messages in an array, each case
+ * corresponding to a error message type
+ */
+typedef enum {
+	would_overwrite,
+	not_uptodate_file,
+	not_uptodate_dir,
+	would_lose_untracked,
+	would_lose_untracked_removed,
+	sparse_not_uptodate_file
+} unpack_trees_error;
+#define NB_UNPACK_TREES_ERROR 6
+struct rejected_files *unpack_rejects[NB_UNPACK_TREES_ERROR];
+
 static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
 	unsigned int set, unsigned int clear)
 {
@@ -60,6 +75,88 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
 }
 
 /*
+ * add error messages on file <file> 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_file(unpack_trees_error e,
+			     const char *file,
+			     const char *action,
+			     int porcelain,
+			     const char *msg)
+{
+	struct rejected_files_list *newentry;
+	/*
+	 * simply display the given error message if in plumbing mode
+	 */
+	if (!porcelain) {
+		error(msg,file,action);
+		return -1;
+	}
+	/*
+	 * if there is a porcelain error message defined,
+	 * the error is stored in order to be nicely displayed later
+	 */
+	if (e == would_lose_untracked && !strcmp(action,"removed"))
+		e = would_lose_untracked_removed;
+
+	if (!unpack_rejects[e]) {
+		unpack_rejects[e] = malloc(sizeof(struct rejected_files));
+		unpack_rejects[e]->list = NULL;
+		unpack_rejects[e]->size = 0;
+	}
+	newentry = malloc(sizeof(struct rejected_files_list));
+	newentry->file = (char *)file;
+	newentry->next = unpack_rejects[e]->list;
+	unpack_rejects[e]->list = newentry;
+	unpack_rejects[e]->msg = msg;
+	unpack_rejects[e]->action = (char *)action;
+	unpack_rejects[e]->size += strlen(file)+strlen("\n")+strlen("\t");
+	return -1;
+}
+
+/*
+ * free all the structures allocated for the error <e>
+ */
+static void free_rejected_files(unpack_trees_error e)
+{
+	while(unpack_rejects[e]->list) {
+		struct rejected_files_list *del = unpack_rejects[e]->list;
+		unpack_rejects[e]->list = unpack_rejects[e]->list->next;
+		free(del);
+	}
+	free(unpack_rejects[e]);
+}
+
+/*
+ * display all the error messages stored in a nice way
+ */
+static void display_error_msgs()
+{
+	int i;
+	int hasPorcelain = 0;
+	for (i=0; i<NB_UNPACK_TREES_ERROR; i++) {
+		if (unpack_rejects[i] && unpack_rejects[i]->list) {
+			hasPorcelain = 1;
+			struct rejected_files_list *f = unpack_rejects[i]->list;
+			char *action = unpack_rejects[i]->action;
+			char *file = malloc(unpack_rejects[i]->size+1);
+			*file = '\0';
+			while (f) {
+				strcat(file,"\t");
+				strcat(file,f->file);
+				strcat(file,"\n");
+				f = f->next;
+			}
+			error(unpack_rejects[i]->msg,file,action);
+			free_rejected_files(i);
+		}
+	}
+	if (hasPorcelain)
+		printf("Aborting\n");
+}
+
+/*
  * Unlink the last component and schedule the leading directories for
  * removal, such that empty directories get removed.
  */
@@ -819,6 +916,7 @@ done:
 	return ret;
 
 return_failed:
+	display_error_msgs();
 	mark_all_ce_unused(o->src_index);
 	ret = unpack_failed(o, NULL);
 	goto done;
@@ -828,7 +926,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_file(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 +950,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)
+				   unpack_trees_error error)
 {
 	struct stat st;
 
@@ -874,8 +974,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_file(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_file(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 +991,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 +1084,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_file(not_uptodate_dir, ce->name, NULL,
+					  (o && (o)->msgs.not_uptodate_dir),
+					  ERRORMSG(o, not_uptodate_dir));
 	free(pathbuf);
 	return cnt;
 }
@@ -1058,7 +1168,9 @@ 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_file(would_lose_untracked, 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..49cc1ee 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -19,6 +19,18 @@ struct unpack_trees_error_msgs {
 	const char *would_lose_orphaned;
 };
 
+struct rejected_files_list {
+	char *file;
+	struct rejected_files_list *next;
+};
+
+struct rejected_files {
+	char *action;
+	const char *msg;
+	size_t size;
+	struct rejected_files_list *list;
+};
+
 struct unpack_trees_options {
 	unsigned int reset,
 		     merge,
-- 
1.6.6.7.ga5fe3

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

[Diane Gasselin]

From: Diane <diane.gasselin@ensimag.imag.fr>

Update porcelain messages of unpack_trees_options in order to have a good layout.

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

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 88b1f43..9b2dca6 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -372,7 +372,7 @@ static int merge_working_tree(struct checkout_opts *opts,
 		topts.src_index = &the_index;
 		topts.dst_index = &the_index;
 
-		topts.msgs.not_uptodate_file = "You have local changes to '%s'; cannot switch branches.";
+		topts.msgs.not_uptodate_file = "You have local changes to:\n%scannot switch branches.";
 
 		refresh_cache(REFRESH_QUIET);
 
diff --git a/merge-recursive.c b/merge-recursive.c
index 206c103..62c07ab 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1182,19 +1182,19 @@ struct unpack_trees_error_msgs get_porcelain_error_msgs(void)
 {
 	struct unpack_trees_error_msgs msgs = {
 		/* would_overwrite */
-		"Your local changes to '%s' would be overwritten by merge.  Aborting.",
+		"Your local changes to the files:\n%swould be overwritten by merge.",
 		/* not_uptodate_file */
-		"Your local changes to '%s' would be overwritten by merge.  Aborting.",
+		"Your local changes to the files:\n%swould be overwritten by merge.",
 		/* not_uptodate_dir */
-		"Updating '%s' would lose untracked files in it.  Aborting.",
+		"Updating the directories:\n%swould lose untracked files in it.",
 		/* would_lose_untracked */
-		"Untracked working tree file '%s' would be %s by merge.  Aborting",
+		"Untracked working tree files:\n%swould be %s by merge.",
 		/* bind_overlap -- will not happen here */
 		NULL,
 	};
 	if (advice_commit_before_merge) {
 		msgs.would_overwrite = msgs.not_uptodate_file =
-			"Your local changes to '%s' would be overwritten by merge.  Aborting.\n"
+			"Your local changes to the files:\n%swould be overwritten by merge.\n"
 			"Please, commit your changes or stash them before you can merge.";
 	}
 	return msgs;
-- 
1.6.6.7.ga5fe3

[RFC/ PATCH 4/5] t3030: update porcelain expected message

[Diane Gasselin]

From: Diane <diane.gasselin@ensimag.imag.fr>

As porcelain messages have been changed, the expected porcelain message
tested in this test needs to be changed.

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

diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 9929f82..9ac5df8 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -268,6 +268,11 @@ test_expect_success 'merge-recursive result' '
 	test_cmp expected actual
 
 '
+cat> expected2 <<EOF
+error: Your local changes to the files:
+	a
+would be overwritten by merge.
+EOF
 
 test_expect_success 'fail if the index has unresolved entries' '
 
@@ -282,7 +287,8 @@ test_expect_success '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 -A 2 "Your local changes to" out > tmp &&
+	test_cmp expected2 tmp
 '
 
 test_expect_success 'merge-recursive remove conflict' '
-- 
1.6.6.7.ga5fe3

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

[Diane Gasselin]

From: Diane <diane.gasselin@ensimag.imag.fr>

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: Axel Bonnet <axel.bonnet@ensimag.imag.fr>
Signed-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>
---
 t/t7609-merge-co-error-msgs.sh |  122 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 122 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..8461e10
--- /dev/null
+++ b/t/t7609-merge-co-error-msgs.sh
@@ -0,0 +1,122 @@
+#!/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: Untracked working tree files:
+	two
+	three
+	four
+	five
+would be overwritten by 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 files:
+	two
+	three
+	four
+would be overwritten by merge.
+Please, commit your changes or stash them before you can merge.
+error: Untracked working tree files:
+	five
+would be overwritten by 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:
+	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: Entry 'rep/one' would be overwritten by merge. Cannot merge.
+error: Entry 'rep/two' would be overwritten by merge. Cannot merge.
+EOF
+
+test_expect_success 'not uptodate file plumbing error' '
+	git add rep/one rep/two &&
+	! git checkout branch 2> out &&
+	test_cmp out expect
+'
+
+cat> expect <<EOF
+error: Updating 'rep' would lose untracked files in it
+error: Updating 'rep2' would lose untracked files in it
+EOF
+
+test_expect_success 'not_uptodate_dir plumbing 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 &&
+	touch rep &&
+	touch rep2 &&
+	git add rep rep2&&
+	git commit -m "added test as a file" &&
+	git checkout master &&
+	touch rep/untracked-file &&
+	touch rep2/untracked-file &&
+	! git checkout branch 2> out &&
+	test_cmp out ../expect
+'
+
+test_done
-- 
1.6.6.7.ga5fe3

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

[Matthieu Moy]

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

> +error: Untracked working tree files:
> +	five
> +would be overwritten by merge.
> +EOF

In my experience, newbies are often confused by this message (typical
example is students who don't believe me when I tell them not to
exchange files by email, getting untracked files from their co-workers
outside Git, and then trying to pull).

Perhaps this deserves an advice like the one we have for modified
files:

> +error: Your local changes to the files:
> +	two
> +	three
> +	four
> +would be overwritten by merge.
> +Please, commit your changes or stash them before you can merge.

(also disabled by advice.commitBeforeMerge)

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

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

[Diane Gasselin]

Le 9 juin 2010 22:47, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> a écrit :
> Diane Gasselin <diane.gasselin@ensimag.imag.fr> writes:
>
>> +error: Untracked working tree files:
>> +     five
>> +would be overwritten by merge.
>> +EOF
>
> In my experience, newbies are often confused by this message (typical
> example is students who don't believe me when I tell them not to
> exchange files by email, getting untracked files from their co-workers
> outside Git, and then trying to pull).
>
> Perhaps this deserves an advice like the one we have for modified
> files:
>
>> +error: Your local changes to the files:
>> +     two
>> +     three
>> +     four
>> +would be overwritten by merge.
>> +Please, commit your changes or stash them before you can merge.
>
> (also disabled by advice.commitBeforeMerge)
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
>

It seems a good idea, I will introduce that in my next version.
Should the message directly advice that files should not be exchanged
outside of Git and propose a solution for resolving the situation?

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

[Matthieu Moy]

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

> It seems a good idea, I will introduce that in my next version.
> Should the message directly advice that files should not be exchanged
> outside of Git and propose a solution for resolving the situation?

I don't think the tool should try to guess the origin of the problem,
but give a solution (like "please move or remove the files before you
can merge").

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

Re: [RFC/ PATCH 4/5] t3030: update porcelain expected message

[Junio C Hamano]

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

> From: Diane <diane.gasselin@ensimag.imag.fr>
>
> As porcelain messages have been changed, the expected porcelain message
> tested in this test needs to be changed.
>
> Signed-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
> Signed-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>
> Signed-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>
> ---
>  t/t3030-merge-recursive.sh |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
> index 9929f82..9ac5df8 100755
> --- a/t/t3030-merge-recursive.sh
> +++ b/t/t3030-merge-recursive.sh
> @@ -268,6 +268,11 @@ test_expect_success 'merge-recursive result' '
>  	test_cmp expected actual
>  
>  '
> +cat> expected2 <<EOF

Style:

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

 (2) if you are not actively $substituting inside here document,
     quote EOF to assure readers that nothing funny is going on.

i.e.

	cat >expected2 <<\EOF
        ... your HERE document here ...
	EOF

The same comment applies to [PATCH 5/5].  Also when you want to create an
empty file, don't use "touch F"; say ">F" instead.

> -	grep "Your local changes to .* would be overwritten by merge." out
> +	grep -A 2 "Your local changes to" out > tmp &&

I think "grep -A $n" is a GNUism, not even in POSIX.  Avoid it.

Perhaps

	sed -n "/^Your local changes to/,\$p" out >tmp &&

Re: [RFC/ PATCH 4/5] t3030: update porcelain expected message

[Matthieu Moy]

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

> From: Diane <diane.gasselin@ensimag.imag.fr>

You did something strange with git format-patch or send-email. This
From header should appear in the header of your email, but not in the
body.

> As porcelain messages have been changed, the expected porcelain message
> tested in this test needs to be changed.

We usually try to have the test-suite pass for each commit (so that
"git bisect" can be used easily among other reasons). So, you probably
want to squash this patch with the one that actually changes the
message. Also, I think it eases reviewing: the changes to the
test-suite can be seen as a specification (particularly clear here: we
know what the rest of the patch serie does reading this patch).

Another trick is to set the tests as "test_expect_failure" before
introducing the feature, and mark them as "test_expect_success" when
appropriate. This way, you can add new tests before adding the
feature, without introducing broken commits.

> +cat> expected2 <<EOF
> +error: Your local changes to the files:
> +	a
> +would be overwritten by merge.
> +EOF

I'd have phrased it like this:

error: Your local changes to these files would be overwritten by merge:
	a

to avoid splitting the message in two parts. It's more consistant with
the rest of Git (git status or git reset for example). Also, your
version would become hard to read if the file list is long.

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

Re: [RFC/ PATCH 4/5] t3030: update porcelain expected message

[Jeff King]

On Wed, Jun 09, 2010 at 10:40:20PM +0200, Matthieu Moy wrote:

> Diane Gasselin <diane.gasselin@ensimag.imag.fr> writes:
> 
> > From: Diane <diane.gasselin@ensimag.imag.fr>
> 
> You did something strange with git format-patch or send-email. This
> From header should appear in the header of your email, but not in the
> body.

The commit author is missing the last name, so send-email correctly
includes the extra "From" header. Probably the user.name config variable
needs updated (and the commit can be rebased and amended with
--reset-author to take the new author).

> > +cat> expected2 <<EOF
> > +error: Your local changes to the files:
> > +	a
> > +would be overwritten by merge.
> > +EOF
> 
> I'd have phrased it like this:
> 
> error: Your local changes to these files would be overwritten by merge:
> 	a
> 
> to avoid splitting the message in two parts. It's more consistant with
> the rest of Git (git status or git reset for example). Also, your
> version would become hard to read if the file list is long.

Yes, I think your version is much more readable.

-Peff

Re: [RFC/ PATCH 4/5] t3030: update porcelain expected message

[Diane Gasselin]

Le 10 juin 2010 03:59, Jeff King <peff@peff.net> a écrit :
> On Wed, Jun 09, 2010 at 10:40:20PM +0200, Matthieu Moy wrote:
>
>> Diane Gasselin <diane.gasselin@ensimag.imag.fr> writes:
>>
>> > From: Diane <diane.gasselin@ensimag.imag.fr>
>>
>> You did something strange with git format-patch or send-email. This
>> From header should appear in the header of your email, but not in the
>> body.
>
> The commit author is missing the last name, so send-email correctly
> includes the extra "From" header. Probably the user.name config variable
> needs updated (and the commit can be rebased and amended with
> --reset-author to take the new author).
>
I had my user name changed at a moment so I thought maybe it was due to that.
Thanks for the tip

>> > +cat> expected2 <<EOF
>> > +error: Your local changes to the files:
>> > +   a
>> > +would be overwritten by merge.
>> > +EOF
>>
>> I'd have phrased it like this:
>>
>> error: Your local changes to these files would be overwritten by merge:
>>       a
>>
>> to avoid splitting the message in two parts. It's more consistant with
>> the rest of Git (git status or git reset for example). Also, your
>> version would become hard to read if the file list is long.
>
> Yes, I think your version is much more readable.
>
> -Peff
>
Yes, I changed it. It makes grep much more easier for the tests.
For some errors, if in porcelain, the order of the arguments file and
action needs to be reversed but this is not a problem.

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

[Diane Gasselin]

Sorry about this.
We had a problem sending our patch, only one message was sent and we
don't know what happened to the others (they have not been returned to
us).
As soon as the problem is fixed, we will send the entire patch.
Sorry again for the noise.

Le 9 juin 2010 14:44, Diane Gasselin <diane.gasselin@ensimag.imag.fr> a écrit :
> From: Diane <diane.gasselin@ensimag.imag.fr>
>
> When an error is encountered, it calls add_rejected_file() which either
> - directly displays the error message if in plumbing mode
> - or stores it so that it will be displayed at the end of 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.
>
> Signed-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
> Signed-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>
> Signed-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>
> ---
> It appears that in verify_absent_sparse(), verify_absent_1() is called with
> ERRORMSG(o, would_lose_orphaned) as the error message.
> Yet, in verify_absent_1(), this error message error_msg does not
> seem to be used and at the end of the function, a would_lose_untracked error
> is treated (before displayed and now added). Is it normal?
>
>  unpack-trees.c |  128 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  unpack-trees.h |   12 +++++
>  2 files changed, 132 insertions(+), 8 deletions(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index c29a9e0..1e2f48d 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -45,6 +45,21 @@ static struct unpack_trees_error_msgs unpack_plumbing_errors = {
>        ? ((o)->msgs.fld) \
>        : (unpack_plumbing_errors.fld) )
>
> +/*
> + * Store error messages in an array, each case
> + * corresponding to a error message type
> + */
> +typedef enum {
> +       would_overwrite,
> +       not_uptodate_file,
> +       not_uptodate_dir,
> +       would_lose_untracked,
> +       would_lose_untracked_removed,
> +       sparse_not_uptodate_file
> +} unpack_trees_error;
> +#define NB_UNPACK_TREES_ERROR 6
> +struct rejected_files *unpack_rejects[NB_UNPACK_TREES_ERROR];
> +
>  static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
>        unsigned int set, unsigned int clear)
>  {
> @@ -60,6 +75,88 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
>  }
>
>  /*
> + * add error messages on file <file> 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_file(unpack_trees_error e,
> +                            const char *file,
> +                            const char *action,
> +                            int porcelain,
> +                            const char *msg)
> +{
> +       struct rejected_files_list *newentry;
> +       /*
> +        * simply display the given error message if in plumbing mode
> +        */
> +       if (!porcelain) {
> +               error(msg,file,action);
> +               return -1;
> +       }
> +       /*
> +        * if there is a porcelain error message defined,
> +        * the error is stored in order to be nicely displayed later
> +        */
> +       if (e == would_lose_untracked && !strcmp(action,"removed"))
> +               e = would_lose_untracked_removed;
> +
> +       if (!unpack_rejects[e]) {
> +               unpack_rejects[e] = malloc(sizeof(struct rejected_files));
> +               unpack_rejects[e]->list = NULL;
> +               unpack_rejects[e]->size = 0;
> +       }
> +       newentry = malloc(sizeof(struct rejected_files_list));
> +       newentry->file = (char *)file;
> +       newentry->next = unpack_rejects[e]->list;
> +       unpack_rejects[e]->list = newentry;
> +       unpack_rejects[e]->msg = msg;
> +       unpack_rejects[e]->action = (char *)action;
> +       unpack_rejects[e]->size += strlen(file)+strlen("\n")+strlen("\t");
> +       return -1;
> +}
> +
> +/*
> + * free all the structures allocated for the error <e>
> + */
> +static void free_rejected_files(unpack_trees_error e)
> +{
> +       while(unpack_rejects[e]->list) {
> +               struct rejected_files_list *del = unpack_rejects[e]->list;
> +               unpack_rejects[e]->list = unpack_rejects[e]->list->next;
> +               free(del);
> +       }
> +       free(unpack_rejects[e]);
> +}
> +
> +/*
> + * display all the error messages stored in a nice way
> + */
> +static void display_error_msgs()
> +{
> +       int i;
> +       int hasPorcelain = 0;
> +       for (i=0; i<NB_UNPACK_TREES_ERROR; i++) {
> +               if (unpack_rejects[i] && unpack_rejects[i]->list) {
> +                       hasPorcelain = 1;
> +                       struct rejected_files_list *f = unpack_rejects[i]->list;
> +                       char *action = unpack_rejects[i]->action;
> +                       char *file = malloc(unpack_rejects[i]->size+1);
> +                       *file = '\0';
> +                       while (f) {
> +                               strcat(file,"\t");
> +                               strcat(file,f->file);
> +                               strcat(file,"\n");
> +                               f = f->next;
> +                       }
> +                       error(unpack_rejects[i]->msg,file,action);
> +                       free_rejected_files(i);
> +               }
> +       }
> +       if (hasPorcelain)
> +               printf("Aborting\n");
> +}
> +
> +/*
>  * Unlink the last component and schedule the leading directories for
>  * removal, such that empty directories get removed.
>  */
> @@ -819,6 +916,7 @@ done:
>        return ret;
>
>  return_failed:
> +       display_error_msgs();
>        mark_all_ce_unused(o->src_index);
>        ret = unpack_failed(o, NULL);
>        goto done;
> @@ -828,7 +926,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_file(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 +950,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)
> +                                  unpack_trees_error error)
>  {
>        struct stat st;
>
> @@ -874,8 +974,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_file(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_file(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 +991,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 +1084,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_file(not_uptodate_dir, ce->name, NULL,
> +                                         (o && (o)->msgs.not_uptodate_dir),
> +                                         ERRORMSG(o, not_uptodate_dir));
>        free(pathbuf);
>        return cnt;
>  }
> @@ -1058,7 +1168,9 @@ 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_file(would_lose_untracked, 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..49cc1ee 100644
> --- a/unpack-trees.h
> +++ b/unpack-trees.h
> @@ -19,6 +19,18 @@ struct unpack_trees_error_msgs {
>        const char *would_lose_orphaned;
>  };
>
> +struct rejected_files_list {
> +       char *file;
> +       struct rejected_files_list *next;
> +};
> +
> +struct rejected_files {
> +       char *action;
> +       const char *msg;
> +       size_t size;
> +       struct rejected_files_list *list;
> +};
> +
>  struct unpack_trees_options {
>        unsigned int reset,
>                     merge,
> --
> 1.6.6.7.ga5fe3
>
>

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

[Junio C Hamano]

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

> +/*
> + * Store error messages in an array, each case
> + * corresponding to a error message type
> + */
> +typedef enum {
> +	would_overwrite,
> +	not_uptodate_file,
> +	not_uptodate_dir,
> +	would_lose_untracked,
> +	would_lose_untracked_removed,
> +	sparse_not_uptodate_file
> +} unpack_trees_error;
> +#define NB_UNPACK_TREES_ERROR 6
> +struct rejected_files *unpack_rejects[NB_UNPACK_TREES_ERROR];

You folks seem to like global variables a lot...  Isn't there a struct
passed throughout the callchain in unpack_trees that you can attach this
information to?

Also "rejected_files" is not as technically correct (there are symlinks)
as "rejected_paths".

Style: we don't encourage "typedef enum { ... } unpack_trees_error";
instead we tend to just say "enum unpack_trees_error" both in the
definition and in the use.

> +	if (!porcelain) {
> +		error(msg,file,action);
> +		return -1;
> +	}

Style:
	if (!porcelain)
        	return error(msg, file, action);

> +static void free_rejected_files(unpack_trees_error e)
> +{
> +	while(unpack_rejects[e]->list) {

Style:
	while (unpack_rejects[e]->list) {

> +static void display_error_msgs()
> +{
> +	int i;
> +	int hasPorcelain = 0;

Style: we don't encourage camelCase.

Whichever way spelled, "has porcelain?" is puzzling.

Is this about "are we issuing error messages as a Porcelain program, or
are we a plumbing without noisy error messages?"  Or is this about "have
we said anything in the loop, and if so finish the message with
'Aborting'"?  If the former, I would name it after "we are Porcelain";
if the latter, I would name it after "we said something".

> +	for (i=0; i<NB_UNPACK_TREES_ERROR; i++) {

Style:

	for (i = 0; i < NB_UNPACK_TREES_ERROR; i++) {

> +		if (unpack_rejects[i] && unpack_rejects[i]->list) {
> +			hasPorcelain = 1;
> +			struct rejected_files_list *f = unpack_rejects[i]->list;
> +			char *action = unpack_rejects[i]->action;
> +			char *file = malloc(unpack_rejects[i]->size+1);
> +			*file = '\0';
> +			while (f) {
> +				strcat(file,"\t");
> +				strcat(file,f->file);
> +				strcat(file,"\n");
> +				f = f->next;
> +			}
> +			error(unpack_rejects[i]->msg,file,action);
> +			free_rejected_files(i);

It feels wrong to malloc() inside the loop (and without freeing, which is
worse).  At least the code should use strbuf to do something like:

	struct strbuf indented = STRBUF_INIT;
	for (f = unpack_rejects[i]->list; f; f = f->next)
                strbuf_addf(&indented, "\t%s\n", f->file);
	error(..., indented.buf, action);
        strbuf_release(&indented);

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

[Diane Gasselin]

Thanks for your comments.

Le 9 juin 2010 18:50, Junio C Hamano <gitster@pobox.com> a écrit :
> Diane Gasselin <diane.gasselin@ensimag.imag.fr> writes:
>
>> +/*
>> + * Store error messages in an array, each case
>> + * corresponding to a error message type
>> + */
>> +typedef enum {
>> +     would_overwrite,
>> +     not_uptodate_file,
>> +     not_uptodate_dir,
>> +     would_lose_untracked,
>> +     would_lose_untracked_removed,
>> +     sparse_not_uptodate_file
>> +} unpack_trees_error;
>> +#define NB_UNPACK_TREES_ERROR 6
>> +struct rejected_files *unpack_rejects[NB_UNPACK_TREES_ERROR];
>
> You folks seem to like global variables a lot...  Isn't there a struct
> passed throughout the callchain in unpack_trees that you can attach this
> information to?
>
At first, I wanted to avoid of having a global variable but I was not
sure if I could add my error structure to an existing structure and I
did not want to overload the callchain with a new parameter.
So now, I attached my structure to struct unpack_trees_options.

I also corrected all the style errors and the following remarks.
Thanks.

> Also "rejected_files" is not as technically correct (there are symlinks)
> as "rejected_paths".
>
> Style: we don't encourage "typedef enum { ... } unpack_trees_error";
> instead we tend to just say "enum unpack_trees_error" both in the
> definition and in the use.
>
>> +     if (!porcelain) {
>> +             error(msg,file,action);
>> +             return -1;
>> +     }
>
> Style:
>        if (!porcelain)
>                return error(msg, file, action);
>
>> +static void free_rejected_files(unpack_trees_error e)
>> +{
>> +     while(unpack_rejects[e]->list) {
>
> Style:
>        while (unpack_rejects[e]->list) {
>
>> +static void display_error_msgs()
>> +{
>> +     int i;
>> +     int hasPorcelain = 0;
>
> Style: we don't encourage camelCase.
>
> Whichever way spelled, "has porcelain?" is puzzling.
>
> Is this about "are we issuing error messages as a Porcelain program, or
> are we a plumbing without noisy error messages?"  Or is this about "have
> we said anything in the loop, and if so finish the message with
> 'Aborting'"?  If the former, I would name it after "we are Porcelain";
> if the latter, I would name it after "we said something".
>
>> +     for (i=0; i<NB_UNPACK_TREES_ERROR; i++) {
>
> Style:
>
>        for (i = 0; i < NB_UNPACK_TREES_ERROR; i++) {
>
>> +             if (unpack_rejects[i] && unpack_rejects[i]->list) {
>> +                     hasPorcelain = 1;
>> +                     struct rejected_files_list *f = unpack_rejects[i]->list;
>> +                     char *action = unpack_rejects[i]->action;
>> +                     char *file = malloc(unpack_rejects[i]->size+1);
>> +                     *file = '\0';
>> +                     while (f) {
>> +                             strcat(file,"\t");
>> +                             strcat(file,f->file);
>> +                             strcat(file,"\n");
>> +                             f = f->next;
>> +                     }
>> +                     error(unpack_rejects[i]->msg,file,action);
>> +                     free_rejected_files(i);
>
> It feels wrong to malloc() inside the loop (and without freeing, which is
> worse).  At least the code should use strbuf to do something like:
>
>        struct strbuf indented = STRBUF_INIT;
>        for (f = unpack_rejects[i]->list; f; f = f->next)
>                strbuf_addf(&indented, "\t%s\n", f->file);
>        error(..., indented.buf, action);
>        strbuf_release(&indented);
>

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

[Matthieu Moy]

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

> +		error(msg,file,action);

Style: spaces after commas. grep your patch for ',[^ ]' to find many
other occurences.

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

Re: [RFC/ PATCH 1/5] tree-walk: do not stop when an error is detected

[Junio C Hamano]

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

> When an error is detected, traverse_trees() is not stopped anymore.
> The whole tree is traversed so that all the merging errors can be detected.

A small worry is if we have some codepath that uses this function like
this:

    if (traverse trees finishes successfully) {
    	be happy, all is well;
    } else {
	attempt a different strategy to achieve
        what we wanted to with traverse trees, if
        it worked fine.
    }

In such a case, spending extra cycles in traverse-trees only to collect
more errors would actively degrade performance in the "alternative
implementation" codepath.  For "try 'quick but limited' version first, and
if it doesn't work, try more elaborate version spending extra cycles"
pattern to work well, the 'quick but limited' version needs to fail
quickly without wasting extra cycles when it hits its limitation.  In the
original code, we deliberately returned early upon seeing the first error
exactly for this reason.

I don't think of concrete examples offhand (fallbacks "merge -s resolve -s
recursive" or "am -3" use come close, perhaps), though, so I may be
worried needlessly in this case.  I honestly don't know offhand.

With our attention focused only on UI issues, I however would agree that
it makes a lot of sense to collect all errors and give them all to the
user, especially because the extra cycles (compared to the current code)
spent to do so is only in the error codepath.

Re: [RFC/ PATCH 1/5] tree-walk: do not stop when an error is detected

[Diane Gasselin]

Le 9 juin 2010 18:49, Junio C Hamano <gitster@pobox.com> a écrit :
> Diane Gasselin <diane.gasselin@ensimag.imag.fr> writes:
>
>> When an error is detected, traverse_trees() is not stopped anymore.
>> The whole tree is traversed so that all the merging errors can be detected.
>
> A small worry is if we have some codepath that uses this function like
> this:
>
>    if (traverse trees finishes successfully) {
>        be happy, all is well;
>    } else {
>        attempt a different strategy to achieve
>        what we wanted to with traverse trees, if
>        it worked fine.
>    }
>
> In such a case, spending extra cycles in traverse-trees only to collect
> more errors would actively degrade performance in the "alternative
> implementation" codepath.  For "try 'quick but limited' version first, and
> if it doesn't work, try more elaborate version spending extra cycles"
> pattern to work well, the 'quick but limited' version needs to fail
> quickly without wasting extra cycles when it hits its limitation.  In the
> original code, we deliberately returned early upon seeing the first error
> exactly for this reason.
>
> I don't think of concrete examples offhand (fallbacks "merge -s resolve -s
> recursive" or "am -3" use come close, perhaps), though, so I may be
> worried needlessly in this case.  I honestly don't know offhand.
>
> With our attention focused only on UI issues, I however would agree that
> it makes a lot of sense to collect all errors and give them all to the
> user, especially because the extra cycles (compared to the current code)
> spent to do so is only in the error codepath.
>

Seems pretty fair.
Can I add in this case an option to git pull and git merge to specify
that we do want to collect all the errors?

Re: [RFC/ PATCH 1/5] tree-walk: do not stop when an error is detected

[Matthieu Moy]

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

>> A small worry is if we have some codepath that uses this function like
>> this:
>>
>>    if (traverse trees finishes successfully) {
>>        be happy, all is well;
>>    } else {
>>        attempt a different strategy to achieve
>>        what we wanted to with traverse trees, if
>>        it worked fine.
>>    }
[...]
> Seems pretty fair.
> Can I add in this case an option to git pull and git merge to specify
> that we do want to collect all the errors?

I don't think this should ever been exposed in the user interface. It
may be interesting to add this in the API (an extra argument letting
the caller decide whether to fail early or not), but the user running
"git pull" normally expects it to succeed, and can clearly accept that
a failure takes as much time as a success, especially when the better
error message lets him save a few tens of seconds, if not minutes.

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