threads / patch / 24930

patchMake sure show_all_errors when using porcelain error messages

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

## tl;dr

16 messages between Sep 1, 2010 and Sep 3, 2010. Diffs are folded; open one to read it.

replies: 15people: 4as markdown or json

Matthieu Moy· Sep 1, 2010, 06:04 UTC · lore

Re: [PATCH for next] insert missing newline in a diagnostic

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 21 quoted lines
> Jim Meyering wrote:
>
>> When merging, I would get a message like this:
>> 
>>   error: The following untracked working tree files would be overwritten by merge:
>>   FILE_NAMEPlease move or remove them before you can merge.
>> 
>> This change inserts the newline after FILE_NAME.
>
> I fear it is more complicated.  With your patch, in some situations
> (e.g., when running t7609-merge-co-error-msgs.sh) I get a leading tab
> and extra newline:
>
>  error: The following untrack...
> 	FILE_NAME
>
>  Please move or remove them before you can merge.
>
> In unpack-trees, display_error_msgs() prints the version with a tab
> but you are getting the message from add_rejected_path which suggests
> to me that o->show_all_errors is unset.

That sounds like an explanation. The patch series did two things: override plumbing error messages, and introducing show_all_errors. The two are meant to work together, and clearly, if you set one without the other, you get surprising results. A fix would be to make sure that we set both in the same places.

Patch follows, untested. Jim, can you tell us whether it fixes the problem? If not, can your give us a reproduction script (preferably as a patch to t7609-merge-co-error-msgs.sh)?

Thanks,
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Matthieu Moy· Sep 1, 2010, 06:05 UTC · re: Matthieu Moy · lore

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.

There was a missing opts.show_all_errors in merge-recursive.c, and a case where it was not obvious enough in merge.c.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
If this is the issue, we should probably change
set_porcelain_error_msgs to set show_all_errors itself (but that
requires changing the argument type)
 builtin/checkout.c |    2 +-
 merge-recursive.c  |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)
Show changes to 2 files +2 −1

builtin/checkout.c, merge-recursive.c

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 7250e5c..8a7f994 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -376,6 +376,7 @@ static int merge_working_tree(struct checkout_opts *opts,
 		topts.src_index = &the_index;
 		topts.dst_index = &the_index;
 
+		topts.show_all_errors = 1;
 		set_porcelain_error_msgs(topts.msgs, "checkout");
 
 		refresh_cache(REFRESH_QUIET);
@@ -395,7 +396,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/merge-recursive.c b/merge-recursive.c
index 638076e..c7fc7a7 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -180,6 +180,7 @@ static int git_merge_trees(int index_only,
 	opts.fn = threeway_merge;
 	opts.src_index = &the_index;
 	opts.dst_index = &the_index;
+	opts.show_all_errors = 1;
 	set_porcelain_error_msgs(opts.msgs, "merge");
 
 	init_tree_desc_from_tree(t+0, common);
-- 
1.7.2.2.175.ga619d.dirty
Junio C Hamano· Sep 1, 2010, 14:59 UTC · re: Matthieu Moy · lore

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

Matthieu Moy <Matthieu.Moy@imag.fr> writes:
> 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.

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.

The function is not about setting the error messages anymore (and it sounds like it never has been since the current behaviour was introduced) but is about declaring that we are a Porcelain and won't stop at the first error. We might end up introducing different behaviours later in this function.

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?
Matthieu Moy· Sep 1, 2010, 17:04 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 10 quoted lines
> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> 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.

Show 5 quoted lines
> 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).

Show 6 quoted lines
> 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/
Junio C Hamano· Sep 1, 2010, 17:54 UTC · re: Matthieu Moy · lore

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

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 15 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>>
>>> 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.

I am not saying it is a bug. The description makes it sound like it is a bug and I was wondering if it can be worded better not to do so.

Matthieu Moy· Sep 2, 2010, 11:57 UTC · re: Matthieu Moy · lore

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

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
Jim Meyering· Sep 3, 2010, 14:18 UTC · re: Matthieu Moy · lore

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

Matthieu Moy wrote:
Show 8 quoted lines
> 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?

Hi Matthieu, Thanks for all of those patches. Unfortunately, I was unable to reproduce the state that provoked my problem.

Matthieu Moy· Sep 3, 2010, 15:25 UTC · re: Jim Meyering · lore

[PATCH] t7609-merge-co-error-msgs: test non-fast forward case too.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
This comes on top of the patch serie, and adds a test to make sure the
bug doesn't come back.

I did check manually that the git of git.git doesn't pass this (while the one after the patch serie does).

 t/t7609-merge-co-error-msgs.sh |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)
Show changes to t/t7609-merge-co-error-msgs.sh +10 −2
diff --git a/t/t7609-merge-co-error-msgs.sh b/t/t7609-merge-co-error-msgs.sh
index 1a109b4..6b58299 100755
--- a/t/t7609-merge-co-error-msgs.sh
+++ b/t/t7609-merge-co-error-msgs.sh
@@ -34,9 +34,17 @@ error: The following untracked working tree files would be overwritten by merge:
 Please move or remove them before you can merge.
 EOF
 
-test_expect_success 'untracked files overwritten by merge' '
+test_expect_success 'untracked files overwritten by merge (fast and non-fast forward)' '
 	test_must_fail git merge branch 2>out &&
-	test_cmp out expect
+	test_cmp out expect &&
+	git commit --allow-empty -m empty &&
+	(
+		GIT_MERGE_VERBOSITY=0 &&
+		export GIT_MERGE_VERBOSITY &&
+		test_must_fail git merge branch 2>out2
+	) &&
+	test_cmp out2 expect &&
+	git reset --hard HEAD^	
 '
 
 cat >expect <<\EOF
-- 
1.7.2.2.284.g95cc.dirty
Matthieu Moy· Sep 2, 2010, 11:57 UTC · re: Matthieu Moy · lore

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

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 <Matthieu.Moy@imag.fr>
---
 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(-)
Show changes to 6 files +54 −54

builtin/checkout.c, builtin/merge.c, merge-recursive.c, merge-recursive.h, unpack-trees.c, unpack-trees.h

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
Matthieu Moy· Sep 2, 2010, 11:57 UTC · re: Matthieu Moy · lore

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

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

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 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(-)
Show changes to 5 files +8 −5

builtin/checkout.c, builtin/merge.c, merge-recursive.c, unpack-trees.c, unpack-trees.h

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
Matthieu Moy· Sep 2, 2010, 11:57 UTC · re: Matthieu Moy · lore

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

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 <Matthieu.Moy@imag.fr>
---
 builtin/checkout.c |    1 -
 builtin/merge.c    |    1 -
 unpack-trees.c     |    8 ++------
 unpack-trees.h     |    2 +-
 4 files changed, 3 insertions(+), 9 deletions(-)
Show changes to 4 files +3 −9

builtin/checkout.c, builtin/merge.c, unpack-trees.c, unpack-trees.h

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
Junio C Hamano· Sep 2, 2010, 15:52 UTC · re: Matthieu Moy · lore

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

Matthieu Moy <Matthieu.Moy@imag.fr> writes:
Show 9 quoted lines
> 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.
Do you mean inefficient or ineffective?
The code in this patch looks fine, though.
Thanks.
Matthieu Moy· Sep 2, 2010, 16:06 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 13 quoted lines
> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> 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.
>
> Do you mean inefficient or ineffective?
Ineffective, yes.

<disgression> I should have listened better during my english courses ;-). Both words translate to "inefficace" in French, and it's a common mistake for french people. </disgression>

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Matthieu Moy· Sep 2, 2010, 16:08 UTC · re: Matthieu Moy · lore

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

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 ineffective 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 <Matthieu.Moy@imag.fr>
---
Just s/inefficient/ineffective/ compared to v1
 builtin/checkout.c |    1 -
 builtin/merge.c    |    1 -
 unpack-trees.c     |    8 ++------
 unpack-trees.h     |    2 +-
 4 files changed, 3 insertions(+), 9 deletions(-)
Show changes to 4 files +3 −9

builtin/checkout.c, builtin/merge.c, unpack-trees.c, unpack-trees.h

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
Jim Meyering· Sep 2, 2010, 08:20 UTC · re: Matthieu Moy · lore

Re: [PATCH for next] insert missing newline in a diagnostic

Matthieu Moy wrote:
Show 29 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Jim Meyering wrote:
>>
>>> When merging, I would get a message like this:
>>>
>>>   error: The following untracked working tree files would be overwritten by merge:
>>>   FILE_NAMEPlease move or remove them before you can merge.
>>>
>>> This change inserts the newline after FILE_NAME.
>>
>> I fear it is more complicated.  With your patch, in some situations
>> (e.g., when running t7609-merge-co-error-msgs.sh) I get a leading tab
>> and extra newline:
>>
>>  error: The following untrack...
>> 	FILE_NAME
>>
>>  Please move or remove them before you can merge.
>>
>> In unpack-trees, display_error_msgs() prints the version with a tab
>> but you are getting the message from add_rejected_path which suggests
>> to me that o->show_all_errors is unset.
>
> That sounds like an explanation. The patch series did two things:
> override plumbing error messages, and introducing show_all_errors. The
> two are meant to work together, and clearly, if you set one without
> the other, you get surprising results. A fix would be to make sure
> that we set both in the same places.
Thanks to both of you.
> Patch follows, untested. Jim, can you tell us whether it fixes the
> problem? If not, can your give us a reproduction script (preferably as
> a patch to t7609-merge-co-error-msgs.sh)?

Matthieu, should your patch have gone to the mailing list? I do not see it there.

Matthieu Moy· Sep 2, 2010, 08:42 UTC · re: Jim Meyering · lore

Re: [PATCH for next] insert missing newline in a diagnostic

Jim Meyering <jim@meyering.net> writes:
Show 6 quoted lines
>> Patch follows, untested. Jim, can you tell us whether it fixes the
>> problem? If not, can your give us a reproduction script (preferably as
>> a patch to t7609-merge-co-error-msgs.sh)?
>
> Matthieu, should your patch have gone to the mailing list?
> I do not see it there.
http://article.gmane.org/gmane.comp.version-control.git/155016
Junio replied to it, so it must have gone through ;-).
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

← back to recent threads