Re: [PATCH] Make sure show_all_errors when using porcelain error messages

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

Re: [PATCH] Make sure show_all_errors when using porcelain error messages

From: Matthieu Moy <hidden>
Date: 2016-06-15 22:49:27

Junio C Hamano [off-list ref] writes:

Matthieu Moy [off-list ref] writes:

quoted

opts.show_all_errors controls the number of errors that can be displayed,
and set_porcelain_error_msgs gives the format. But the formatting would
be incorrect if set_porcelain_error_msgs is called without setting
opts.show_all_errors.

That makes it sound like a design bug of set_porcelain_error_msgs(), in
that the caller _cannot_ choose to stop at the first error if it wants to
use friendlier message than the plumbing one.

You're right that the caller cannot stop at the first error and get
friendly message, but I don't think this is a bug. As a user, I don't
see any reason not to want the complete list. We kept the
"show_all_errors = false" behavior to allow the slight performance
improvement of stopping at the first error, which may (but I'm not
even sure) benefit to scripts.

But if that is (and I do not think it is such a bad thing if we decide
that Porcelains have no option of stopping at the first error) then we
should set opts.show_all_errors in the set_porcelain_error_msgs()
function, and probably should rename the function to something
saner.

Yes (actually, I send a quick patch to make sure this was the problem
of the OP, but the intention was to have a better one when I have
time. I forgot to add RFC in the title).

How about making it

	int setup_unpack_trees_porcelain(struct unpack_trees_options *opt,
						const char *cmd);

and possibly moving it from merge-recursive.c to unpack-trees.c?

Sounds good, yes. Will do (probably tomorrow).

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

[PATCH 2/3] setup_unpack_trees_porcelain: take the whole options struct as parameter

From: Matthieu Moy <hidden>
Date: 2016-06-15 22:49:28

This is a preparation patch to let setup_unpack_trees_porcelain set
show_all_errors itself.

Signed-off-by: Matthieu Moy <redacted>
---
 builtin/checkout.c |    2 +-
 builtin/merge.c    |    2 +-
 merge-recursive.c  |    2 +-
 unpack-trees.c     |    4 +++-
 unpack-trees.h     |    3 ++-
 5 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index e5c0ef0..b26dfd0 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c

@@ -376,7 +376,7 @@ static int merge_working_tree(struct checkout_opts *opts,
 		topts.src_index = &the_index;
 		topts.dst_index = &the_index;
 
-		setup_unpack_trees_porcelain(topts.msgs, "checkout");
+		setup_unpack_trees_porcelain(&topts, "checkout");
 
 		refresh_cache(REFRESH_QUIET);

diff --git a/builtin/merge.c b/builtin/merge.c
index da52b10..389e79c 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -706,7 +706,7 @@ int checkout_fast_forward(const unsigned char *head, const unsigned char *remote
 	opts.merge = 1;
 	opts.fn = twoway_merge;
 	opts.show_all_errors = 1;
-	setup_unpack_trees_porcelain(opts.msgs, "merge");
+	setup_unpack_trees_porcelain(&opts, "merge");
 
 	trees[nr_trees] = parse_tree_indirect(head);
 	if (!trees[nr_trees++])

diff --git a/merge-recursive.c b/merge-recursive.c
index 61e237b..ebe6700 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c

@@ -180,7 +180,7 @@ static int git_merge_trees(int index_only,
 	opts.fn = threeway_merge;
 	opts.src_index = &the_index;
 	opts.dst_index = &the_index;
-	setup_unpack_trees_porcelain(opts.msgs, "merge");
+	setup_unpack_trees_porcelain(&opts, "merge");
 
 	init_tree_desc_from_tree(t+0, common);
 	init_tree_desc_from_tree(t+1, head);

diff --git a/unpack-trees.c b/unpack-trees.c
index 4520aa0..17501d3 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c

@@ -50,8 +50,10 @@ const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
 	  ? ((o)->msgs[(type)])      \
 	  : (unpack_plumbing_errors[(type)]) )
 
-void setup_unpack_trees_porcelain(const char **msgs, const char *cmd)
+void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
+				  const char *cmd)
 {
+	const char **msgs = opts->msgs;
 	const char *msg;
 	char *tmp;
 	const char *cmd2 = strcmp(cmd, "checkout") ? cmd : "switch branches";

diff --git a/unpack-trees.h b/unpack-trees.h
index d62bba9..fad680d 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h

@@ -26,7 +26,8 @@ enum unpack_trees_error_types {
  * Sets the list of user-friendly error messages to be used by the
  * command "cmd" (either merge or checkout)
  */
-void setup_unpack_trees_porcelain(const char **msgs, const char *cmd);
+void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
+				  const char *cmd);
 
 struct rejected_paths_list {
 	char *path;

-- 
1.7.2.2.175.ga619d.dirty

[PATCH 0/3] (hopefully) Proper fix to set show_all_errors where needed

From: Matthieu Moy <hidden>
Date: 2016-06-15 22:49:28

So, after my toy untested patch, here's a cleaner version. I splitted
it into really small patches, to avoid modifying code while I move it,
but Junio, feel free to squash 1/3 and 2/3 if you prefer.

This fixes a regression, so this should be in the next release if the
patch is OK.

Jim: can you confirm if it fixes your problem?

Matthieu Moy (3):
  Move set_porcelain_error_msgs to unpack-trees.c and rename it
  setup_unpack_trees_porcelain: take the whole options struct as
    parameter
  Move "show_all_errors = 1" to setup_unpack_trees_porcelain()

 builtin/checkout.c |    3 +-
 builtin/merge.c    |    3 +-
 merge-recursive.c  |   46 +-----------------------------------------
 merge-recursive.h  |    6 -----
 unpack-trees.c     |   56 +++++++++++++++++++++++++++++++++++++++++++++------
 unpack-trees.h     |    7 ++++++
 6 files changed, 59 insertions(+), 62 deletions(-)

-- 
1.7.2.2.175.ga619d.dirty

[PATCH 3/3] Move "show_all_errors = 1" to setup_unpack_trees_porcelain()

From: Matthieu Moy <hidden>
Date: 2016-06-15 22:49:28

Not only this makes the code clearer since setting up the porcelain error
message is meant to work with show_all_errors, but this fixes a call to
setup_unpack_trees_porcelain() in git_merge_trees() which did not set
show_all_errors.

add_rejected_path() used to double-check whether it was running in
plumbing mode. This check was inefficient since it was setting
show_all_errors too late for traverse_trees() to see it, and is made
useless by this patch. Remove it.

Signed-off-by: Matthieu Moy <redacted>
---
 builtin/checkout.c |    1 -
 builtin/merge.c    |    1 -
 unpack-trees.c     |    8 ++------
 unpack-trees.h     |    2 +-
 4 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index b26dfd0..1532669 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c

@@ -395,7 +395,6 @@ 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 389e79c..bfd3d32 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -705,7 +705,6 @@ 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;
 	setup_unpack_trees_porcelain(&opts, "merge");
 
 	trees[nr_trees] = parse_tree_indirect(head);

diff --git a/unpack-trees.c b/unpack-trees.c
index 17501d3..803445a 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c

@@ -94,6 +94,8 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 		"The following Working tree files would be overwritten by sparse checkout update:\n%s";
 	msgs[ERROR_WOULD_LOSE_ORPHANED_REMOVED] =
 		"The following Working tree files would be removed by sparse checkout update:\n%s";
+
+	opts->show_all_errors = 1;
 }
 
 static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,

@@ -123,12 +125,6 @@ static int add_rejected_path(struct unpack_trees_options *o,
 			     const char *path)
 {
 	struct rejected_paths_list *newentry;
-	int porcelain = o && (o)->msgs[e];
-	/*
-	 * simply display the given error message if in plumbing mode
-	 */
-	if (!porcelain)
-		o->show_all_errors = 0;
 	if (!o->show_all_errors)
 		return error(ERRORMSG(o, e), path);

diff --git a/unpack-trees.h b/unpack-trees.h
index fad680d..7c0187d 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h

@@ -24,7 +24,7 @@ enum unpack_trees_error_types {
 
 /*
  * Sets the list of user-friendly error messages to be used by the
- * command "cmd" (either merge or checkout)
+ * command "cmd" (either merge or checkout), and show_all_errors to 1.
  */
 void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 				  const char *cmd);

-- 
1.7.2.2.175.ga619d.dirty

[PATCH 1/3] Move set_porcelain_error_msgs to unpack-trees.c and rename it

From: Matthieu Moy <hidden>
Date: 2016-06-15 22:49:28

The function is currently dealing only with error messages, but the
intent of calling it is really to notify the unpack-tree mechanics that
it is running in porcelain mode.

Signed-off-by: Matthieu Moy <redacted>
---
 builtin/checkout.c |    2 +-
 builtin/merge.c    |    2 +-
 merge-recursive.c  |   46 +---------------------------------------------
 merge-recursive.h  |    6 ------
 unpack-trees.c     |   46 +++++++++++++++++++++++++++++++++++++++++++++-
 unpack-trees.h     |    6 ++++++
 6 files changed, 54 insertions(+), 54 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 7250e5c..e5c0ef0 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c

@@ -376,7 +376,7 @@ static int merge_working_tree(struct checkout_opts *opts,
 		topts.src_index = &the_index;
 		topts.dst_index = &the_index;
 
-		set_porcelain_error_msgs(topts.msgs, "checkout");
+		setup_unpack_trees_porcelain(topts.msgs, "checkout");
 
 		refresh_cache(REFRESH_QUIET);

diff --git a/builtin/merge.c b/builtin/merge.c
index 47e705b..da52b10 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -706,7 +706,7 @@ int checkout_fast_forward(const unsigned char *head, const unsigned char *remote
 	opts.merge = 1;
 	opts.fn = twoway_merge;
 	opts.show_all_errors = 1;
-	set_porcelain_error_msgs(opts.msgs, "merge");
+	setup_unpack_trees_porcelain(opts.msgs, "merge");
 
 	trees[nr_trees] = parse_tree_indirect(head);
 	if (!trees[nr_trees++])

diff --git a/merge-recursive.c b/merge-recursive.c
index df90be4..61e237b 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c

@@ -180,7 +180,7 @@ static int git_merge_trees(int index_only,
 	opts.fn = threeway_merge;
 	opts.src_index = &the_index;
 	opts.dst_index = &the_index;
-	set_porcelain_error_msgs(opts.msgs, "merge");
+	setup_unpack_trees_porcelain(opts.msgs, "merge");
 
 	init_tree_desc_from_tree(t+0, common);
 	init_tree_desc_from_tree(t+1, head);

@@ -1238,50 +1238,6 @@ static int process_df_entry(struct merge_options *o,
 	return clean_merge;
 }
 
-void set_porcelain_error_msgs(const char **msgs, const char *cmd)
-{
-	const char *msg;
-	char *tmp;
-	const char *cmd2 = strcmp(cmd, "checkout") ? cmd : "switch branches";
-	if (advice_commit_before_merge)
-		msg = "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.";
-	else
-		msg = "Your local changes to the following files would be overwritten by %s:\n%%s";
-	tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen(cmd2) - 2);
-	sprintf(tmp, msg, cmd, cmd2);
-	msgs[ERROR_WOULD_OVERWRITE] = tmp;
-	msgs[ERROR_NOT_UPTODATE_FILE] = tmp;
-
-	msgs[ERROR_NOT_UPTODATE_DIR] =
-		"Updating the following directories would lose untracked files in it:\n%s";
-
-	if (advice_commit_before_merge)
-		msg = "The following untracked working tree files would be %s by %s:\n%%s"
-			"Please move or remove them before you can %s.";
-	else
-		msg = "The following untracked working tree files would be %s by %s:\n%%s";
-	tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen("removed") + strlen(cmd2) - 4);
-	sprintf(tmp, msg, "removed", cmd, cmd2);
-	msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = tmp;
-	tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen("overwritten") + strlen(cmd2) - 4);
-	sprintf(tmp, msg, "overwritten", cmd, cmd2);
-	msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = tmp;
-
-	/*
-	 * Special case: ERROR_BIND_OVERLAP refers to a pair of paths, we
-	 * cannot easily display it as a list.
-	 */
-	msgs[ERROR_BIND_OVERLAP] = "Entry '%s' overlaps with '%s'.  Cannot bind.";
-
-	msgs[ERROR_SPARSE_NOT_UPTODATE_FILE] =
-		"Cannot update sparse checkout: the following entries are not up-to-date:\n%s";
-	msgs[ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN] =
-		"The following Working tree files would be overwritten by sparse checkout update:\n%s";
-	msgs[ERROR_WOULD_LOSE_ORPHANED_REMOVED] =
-		"The following Working tree files would be removed by sparse checkout update:\n%s";
-}
-
 int merge_trees(struct merge_options *o,
 		struct tree *head,
 		struct tree *merge,

diff --git a/merge-recursive.h b/merge-recursive.h
index 08f9850..f79917c 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h

@@ -23,12 +23,6 @@ struct merge_options {
 	struct string_list current_directory_set;
 };
 
-/*
- * Sets the list of user-friendly error messages to be used by the
- * command "cmd" (either merge or checkout)
- */
-void set_porcelain_error_msgs(const char **msgs, const char *cmd);
-
 /* merge_trees() but with recursive ancestor consolidation */
 int merge_recursive(struct merge_options *o,
 		    struct commit *h1,

diff --git a/unpack-trees.c b/unpack-trees.c
index 3c7a7c9..4520aa0 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c

@@ -14,7 +14,7 @@
  * read-tree.  Non-scripted Porcelain is not required to use these messages
  * and in fact are encouraged to reword them to better suit their particular
  * situation better.  See how "git checkout" and "git merge" replaces
- * them using set_porcelain_error_msgs(), for example.
+ * them using setup_unpack_trees_porcelain(), for example.
  */
 const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
 	/* ERROR_WOULD_OVERWRITE */

@@ -50,6 +50,50 @@ const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
 	  ? ((o)->msgs[(type)])      \
 	  : (unpack_plumbing_errors[(type)]) )
 
+void setup_unpack_trees_porcelain(const char **msgs, const char *cmd)
+{
+	const char *msg;
+	char *tmp;
+	const char *cmd2 = strcmp(cmd, "checkout") ? cmd : "switch branches";
+	if (advice_commit_before_merge)
+		msg = "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.";
+	else
+		msg = "Your local changes to the following files would be overwritten by %s:\n%%s";
+	tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen(cmd2) - 2);
+	sprintf(tmp, msg, cmd, cmd2);
+	msgs[ERROR_WOULD_OVERWRITE] = tmp;
+	msgs[ERROR_NOT_UPTODATE_FILE] = tmp;
+
+	msgs[ERROR_NOT_UPTODATE_DIR] =
+		"Updating the following directories would lose untracked files in it:\n%s";
+
+	if (advice_commit_before_merge)
+		msg = "The following untracked working tree files would be %s by %s:\n%%s"
+			"Please move or remove them before you can %s.";
+	else
+		msg = "The following untracked working tree files would be %s by %s:\n%%s";
+	tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen("removed") + strlen(cmd2) - 4);
+	sprintf(tmp, msg, "removed", cmd, cmd2);
+	msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = tmp;
+	tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen("overwritten") + strlen(cmd2) - 4);
+	sprintf(tmp, msg, "overwritten", cmd, cmd2);
+	msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = tmp;
+
+	/*
+	 * Special case: ERROR_BIND_OVERLAP refers to a pair of paths, we
+	 * cannot easily display it as a list.
+	 */
+	msgs[ERROR_BIND_OVERLAP] = "Entry '%s' overlaps with '%s'.  Cannot bind.";
+
+	msgs[ERROR_SPARSE_NOT_UPTODATE_FILE] =
+		"Cannot update sparse checkout: the following entries are not up-to-date:\n%s";
+	msgs[ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN] =
+		"The following Working tree files would be overwritten by sparse checkout update:\n%s";
+	msgs[ERROR_WOULD_LOSE_ORPHANED_REMOVED] =
+		"The following Working tree files would be removed by sparse checkout update:\n%s";
+}
+
 static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
 	unsigned int set, unsigned int clear)
 {

diff --git a/unpack-trees.h b/unpack-trees.h
index 6e049b0..d62bba9 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h

@@ -22,6 +22,12 @@ enum unpack_trees_error_types {
 	NB_UNPACK_TREES_ERROR_TYPES
 };
 
+/*
+ * Sets the list of user-friendly error messages to be used by the
+ * command "cmd" (either merge or checkout)
+ */
+void setup_unpack_trees_porcelain(const char **msgs, const char *cmd);
+
 struct rejected_paths_list {
 	char *path;
 	struct rejected_paths_list *next;

-- 
1.7.2.2.175.ga619d.dirty

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help