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

DORMANTno replies

Revision rfc of 3 in this series.

13 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Diane Gasselin <hidden>
Date: 2016-06-15 22:48:56

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 2/5] unpack_trees: group errors by type

From: Diane Gasselin <hidden>
Date: 2016-06-15 22:48:56

From: Diane <redacted>

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 <redacted>
Signed-off-by: Axel Bonnet <redacted>
Signed-off-by: Clément Poulain <redacted>
---
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

From: Diane Gasselin <hidden>
Date: 2016-06-15 22:48:56

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 [off-list ref] a écrit :
quoted hunk
From: Diane <redacted>

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 <redacted>
Signed-off-by: Axel Bonnet <redacted>
Signed-off-by: Clément Poulain <redacted>
---
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 0/5] unpack_trees: nicer error messages

From: Diane Gasselin <hidden>
Date: 2016-06-15 22:48:56

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 5/5] t7609: test merge and checkout error messages

From: Diane Gasselin <hidden>
Date: 2016-06-15 22:48:56

From: Diane <redacted>

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

Signed-off-by: Diane Gasselin <redacted>
Signed-off-by: Axel Bonnet <redacted>
Signed-off-by: Clément Poulain <redacted>
---
 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

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

From: Diane Gasselin <hidden>
Date: 2016-06-15 22:48:56

From: Diane <redacted>

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

Signed-off-by: Diane Gasselin <redacted>
Signed-off-by: Axel Bonnet <redacted>
Signed-off-by: Clément Poulain <redacted>
---
 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 1/5] tree-walk: do not stop when an error is detected

From: Diane Gasselin <hidden>
Date: 2016-06-15 22:48:56

From: Diane <redacted>

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 <redacted>
Signed-off-by: Axel Bonnet <redacted>
Signed-off-by: Clément Poulain <redacted>
---
 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 4/5] t3030: update porcelain expected message

From: Diane Gasselin <hidden>
Date: 2016-06-15 22:48:56

From: Diane <redacted>

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

Signed-off-by: Diane Gasselin <redacted>
Signed-off-by: Axel Bonnet <redacted>
Signed-off-by: Clément Poulain <redacted>
---
 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

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:48:56

Diane Gasselin [off-list ref] 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 2/5] unpack_trees: group errors by type

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:48:56

Diane Gasselin [off-list ref] 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 4/5] t3030: update porcelain expected message

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:48:56

Diane Gasselin [off-list ref] writes:
quoted hunk
From: Diane <redacted>

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

Signed-off-by: Diane Gasselin <redacted>
Signed-off-by: Axel Bonnet <redacted>
Signed-off-by: Clément Poulain <redacted>
---
 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 1/5] tree-walk: do not stop when an error is detected

From: Diane Gasselin <hidden>
Date: 2016-06-15 22:48:56

Le 9 juin 2010 18:49, Junio C Hamano [off-list ref] a écrit :
Diane Gasselin [off-list ref] writes:
quoted
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 2/5] unpack_trees: group errors by type

From: Diane Gasselin <hidden>
Date: 2016-06-15 22:48:56

Thanks for your comments.

Le 9 juin 2010 18:50, Junio C Hamano [off-list ref] a écrit :
Diane Gasselin [off-list ref] writes:
quoted
+/*
+ * 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.
quoted
+     if (!porcelain) {
+             error(msg,file,action);
+             return -1;
+     }
Style:
       if (!porcelain)
               return error(msg, file, action);
quoted
+static void free_rejected_files(unpack_trees_error e)
+{
+     while(unpack_rejects[e]->list) {
Style:
       while (unpack_rejects[e]->list) {
quoted
+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".
quoted
+     for (i=0; i<NB_UNPACK_TREES_ERROR; i++) {
Style:

       for (i = 0; i < NB_UNPACK_TREES_ERROR; i++) {
quoted
+             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);
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help