[PATCH for next] insert missing newline in a diagnostic

[PATCH for next] insert missing newline in a diagnostic

[Jim Meyering]

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.

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 merge-recursive.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index aadd48c..e81c995 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1307,7 +1307,7 @@ void set_porcelain_error_msgs(const char **msgs, const char *cmd)
 		"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"
+		msg = "The following untracked working tree files would be %s by %s:\n%%s\n"
 			"Please move or remove them before you can %s.";
 	else
 		msg = "The following untracked working tree files would be %s by %s:\n%%s";
--
1.7.2.2.510.g7180a

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

[Jonathan Nieder]

(+cc: Matthieu, Junio)

Hi Jim,

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.

Was there some other error before then?

Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/152965/focus=153211

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

[Matthieu Moy]

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.

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/

[PATCH] Make sure show_all_errors when using porcelain error messages

[Matthieu Moy]

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(-)

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

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

[Junio C Hamano]

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?

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

[Matthieu Moy]

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. 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/

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

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> 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.

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

[Jim Meyering]

Matthieu Moy wrote:
> 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.

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

[Matthieu Moy]

Jim Meyering <jim@meyering.net> writes:

>> 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/

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

[Matthieu Moy]

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 1/3] Move set_porcelain_error_msgs to unpack-trees.c and rename it

[Matthieu Moy]

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(-)

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

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

[Matthieu Moy]

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(-)

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 3/3] Move "show_all_errors = 1" to setup_unpack_trees_porcelain()

[Matthieu Moy]

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(-)

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

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

[Junio C Hamano]

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?

The code in this patch looks fine, though.

Thanks.

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

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> 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/

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

[Matthieu Moy]

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(-)

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

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

[Jim Meyering]

Matthieu Moy wrote:
> 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.

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

[Matthieu Moy]

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(-)

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