[09/12] merge-tree: provide a list of which files have conflicts

Commit Message

Elijah Newren Jan. 22, 2022, 9:55 p.m. UTC

From: Elijah Newren <[e-mail]>

Callers of `git merge-tree --write-tree` will often want to know which
files had conflicts.  While they could potentially attempt to parse the
CONFLICT notices printed, those messages are not meant to be machine
readable.  Provide a simpler mechanism of just printing the files (in
the same format as `git ls-files` with quoting, but restricted to
unmerged files) in the output before the free-form messages.

Signed-off-by: Elijah Newren <[e-mail]>
---
 Documentation/git-merge-tree.txt |  8 ++++++++
 builtin/merge-tree.c             | 24 ++++++++++++++++++++++--
 t/t4301-merge-tree-real.sh       | 11 +++++++++++
 3 files changed, 41 insertions(+), 2 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Jan. 24, 2022, 10:01 a.m. UTC | #1

On Sat, Jan 22 2022, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <[e-mail]>
>
> Callers of `git merge-tree --write-tree` will often want to know which
> files had conflicts.  While they could potentially attempt to parse the
> CONFLICT notices printed, those messages are not meant to be machine
> readable.  Provide a simpler mechanism of just printing the files (in
> the same format as `git ls-files` with quoting, but restricted to
> [...]
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index 560640ad911..d8eeeb3f306 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -11,6 +11,9 @@
>  #include "blob.h"
>  #include "exec-cmd.h"
>  #include "merge-blobs.h"
> +#include "quote.h"
> +
> +static int line_termination = '\n';

But unlike ls-files we don't do anything with line_termination as a !=
'\n', maybe in a later commit?

>  struct merge_list {
>  	struct merge_list *next;
> @@ -395,7 +398,8 @@ struct merge_tree_options {
>  };
>  
>  static int real_merge(struct merge_tree_options *o,
> -		      const char *branch1, const char *branch2)
> +		      const char *branch1, const char *branch2,
> +		      const char *prefix)
>  {
>  	struct commit *parent1, *parent2;
>  	struct commit_list *common;
> @@ -449,6 +453,22 @@ static int real_merge(struct merge_tree_options *o,
>  		o->show_messages = !result.clean;
>  
>  	printf("%s\n", oid_to_hex(&result.tree->object.oid));
> +	if (!result.clean) {
> +		struct string_list conflicted_files = STRING_LIST_INIT_NODUP;
> +		const char *last = NULL;
> +		int i;
> +
> +		merge_get_conflicted_files(&result, &conflicted_files);
> +		for (i = 0; i < conflicted_files.nr; i++) {
> +			const char *name = conflicted_files.items[i].string;
> +			if (last && !strcmp(last, name))
> +				continue;
> +			write_name_quoted_relative(
> +				name, prefix, stdout, line_termination);

But here it's never \0 or whatever.

Elijah Newren Jan. 24, 2022, 5:18 p.m. UTC | #2

On Mon, Jan 24, 2022 at 2:02 AM Ævar Arnfjörð Bjarmason
<[e-mail]> wrote:
>
> On Sat, Jan 22 2022, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <[e-mail]>
> >
> > Callers of `git merge-tree --write-tree` will often want to know which
> > files had conflicts.  While they could potentially attempt to parse the
> > CONFLICT notices printed, those messages are not meant to be machine
> > readable.  Provide a simpler mechanism of just printing the files (in
> > the same format as `git ls-files` with quoting, but restricted to
> > [...]
> > diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> > index 560640ad911..d8eeeb3f306 100644
> > --- a/builtin/merge-tree.c
> > +++ b/builtin/merge-tree.c
> > @@ -11,6 +11,9 @@
> >  #include "blob.h"
> >  #include "exec-cmd.h"
> >  #include "merge-blobs.h"
> > +#include "quote.h"
> > +
> > +static int line_termination = '\n';
>
> But unlike ls-files we don't do anything with line_termination as a !=
> '\n', maybe in a later commit?
>
> >  struct merge_list {
> >       struct merge_list *next;
> > @@ -395,7 +398,8 @@ struct merge_tree_options {
> >  };
> >
> >  static int real_merge(struct merge_tree_options *o,
> > -                   const char *branch1, const char *branch2)
> > +                   const char *branch1, const char *branch2,
> > +                   const char *prefix)
> >  {
> >       struct commit *parent1, *parent2;
> >       struct commit_list *common;
> > @@ -449,6 +453,22 @@ static int real_merge(struct merge_tree_options *o,
> >               o->show_messages = !result.clean;
> >
> >       printf("%s\n", oid_to_hex(&result.tree->object.oid));
> > +     if (!result.clean) {
> > +             struct string_list conflicted_files = STRING_LIST_INIT_NODUP;
> > +             const char *last = NULL;
> > +             int i;
> > +
> > +             merge_get_conflicted_files(&result, &conflicted_files);
> > +             for (i = 0; i < conflicted_files.nr; i++) {
> > +                     const char *name = conflicted_files.items[i].string;
> > +                     if (last && !strcmp(last, name))
> > +                             continue;
> > +                     write_name_quoted_relative(
> > +                             name, prefix, stdout, line_termination);
>
> But here it's never \0 or whatever.

Correct, I didn't add any option for changing it.  But why hardcode it
to "\n"?  Leaving it this way makes it easier to change later if folks
say they want NUL-terminated output.  Since the series is RFC and the
output already has changed drastically and appears to be the primary
discussion and disagreement point, I wanted to provide what seemed
like a reasonable suggestion and maintain flexiibility to address
feedback (though who knows -- I might need to just completely redo the
output again in ways much bigger than adding a -z option).

Johannes Schindelin Jan. 28, 2022, 4:57 p.m. UTC | #3

Hi Elijah,

On Sat, 22 Jan 2022, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <[e-mail]>
>
> Callers of `git merge-tree --write-tree` will often want to know which
> files had conflicts.  While they could potentially attempt to parse the
> CONFLICT notices printed, those messages are not meant to be machine
> readable.  Provide a simpler mechanism of just printing the files (in
> the same format as `git ls-files` with quoting, but restricted to
> unmerged files) in the output before the free-form messages.
>
> Signed-off-by: Elijah Newren <[e-mail]>
> ---
>  Documentation/git-merge-tree.txt |  8 ++++++++
>  builtin/merge-tree.c             | 24 ++++++++++++++++++++++--
>  t/t4301-merge-tree-real.sh       | 11 +++++++++++
>  3 files changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
> index fd7a867de60..041a4ac2785 100644
> --- a/Documentation/git-merge-tree.txt
> +++ b/Documentation/git-merge-tree.txt
> @@ -58,6 +58,7 @@ simply one line:
>  Whereas for a conflicted merge, the output is by default of the form:
>
>  	<OID of toplevel tree>
> +	<Conflicted file list>
>  	<Informational messages>

To distinguish between the list of conflicted files and the informational
messages, I think it would be good to insert an empty line, as a
separator, like. And...

>
>  These are discussed individually below.
> @@ -69,6 +70,13 @@ This is a tree object that represents what would be checked out in the
>  working tree at the end of `git merge`.  If there were conflicts, then
>  files within this tree may have embedded conflict markers.
>
> +Conflicted file list
> +~~~~~~~~~~~~~~~~~~~~
> +
> +This is a sequence of lines containing a filename on each line, quoted
> +as explained for the configuration variable `core.quotePath` (see
> +linkgit:git-config[1]).
> +
>  Informational messages
>  ~~~~~~~~~~~~~~~~~~~~~~
>
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index 560640ad911..d8eeeb3f306 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -11,6 +11,9 @@
>  #include "blob.h"
>  #include "exec-cmd.h"
>  #include "merge-blobs.h"
> +#include "quote.h"
> +
> +static int line_termination = '\n';
>
>  struct merge_list {
>  	struct merge_list *next;
> @@ -395,7 +398,8 @@ struct merge_tree_options {
>  };
>
>  static int real_merge(struct merge_tree_options *o,
> -		      const char *branch1, const char *branch2)
> +		      const char *branch1, const char *branch2,
> +		      const char *prefix)
>  {
>  	struct commit *parent1, *parent2;
>  	struct commit_list *common;
> @@ -449,6 +453,22 @@ static int real_merge(struct merge_tree_options *o,
>  		o->show_messages = !result.clean;
>
>  	printf("%s\n", oid_to_hex(&result.tree->object.oid));
> +	if (!result.clean) {
> +		struct string_list conflicted_files = STRING_LIST_INIT_NODUP;
> +		const char *last = NULL;
> +		int i;
> +
> +		merge_get_conflicted_files(&result, &conflicted_files);
> +		for (i = 0; i < conflicted_files.nr; i++) {
> +			const char *name = conflicted_files.items[i].string;
> +			if (last && !strcmp(last, name))
> +				continue;
> +			write_name_quoted_relative(
> +				name, prefix, stdout, line_termination);
> +			last = name;
> +		}
> +		string_list_clear(&conflicted_files, 1);
> +	}
>  	if (o->show_messages) {
>  		printf("\n");

... it seems that we do this already...

>  		merge_display_update_messages(&opt, &result, stdout);
> @@ -502,7 +522,7 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
>
>  	/* Do the relevant type of merge */
>  	if (o.real)
> -		return real_merge(&o, argv[0], argv[1]);
> +		return real_merge(&o, argv[0], argv[1], prefix);
>  	else
>  		return trivial_merge(argv[0], argv[1], argv[2]);
>  }
> diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh
> index c34f8e6c1ed..43c9950dedb 100755
> --- a/t/t4301-merge-tree-real.sh
> +++ b/t/t4301-merge-tree-real.sh
> @@ -94,6 +94,8 @@ test_expect_success 'test conflict notices and such' '
>  	#   "whatever" has *both* a modify/delete and a file/directory conflict
>  	cat <<-EOF >expect &&
>  	HASH
> +	greeting
> +	whatever~side1
>
>  	Auto-merging greeting
>  	CONFLICT (content): Merge conflict in greeting

... as illustrated by the test, too. I guess the documentation should show
the empty line, too?

Ciao,
Dscho

> @@ -105,4 +107,13 @@ test_expect_success 'test conflict notices and such' '
>  	test_cmp expect actual
>  '
>
> +test_expect_success 'Just the conflicted files without the messages' '
> +	test_expect_code 1 git merge-tree --write-tree --no-messages side1 side2 >out &&
> +	sed -e "s/[0-9a-f]\{40,\}/HASH/g" out >actual &&
> +
> +	test_write_lines HASH greeting whatever~side1 >expect &&
> +
> +	test_cmp expect actual
> +'
> +
>  test_done
> --
> gitgitgadget
>
>

Elijah Newren Jan. 29, 2022, 6:21 a.m. UTC | #4

On Fri, Jan 28, 2022 at 8:57 AM Johannes Schindelin
<[e-mail]> wrote:
>
> Hi Elijah,
>
> On Sat, 22 Jan 2022, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <[e-mail]>
> >
[...]
> > diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
> > index fd7a867de60..041a4ac2785 100644
> > --- a/Documentation/git-merge-tree.txt
> > +++ b/Documentation/git-merge-tree.txt
> > @@ -58,6 +58,7 @@ simply one line:
> >  Whereas for a conflicted merge, the output is by default of the form:
> >
> >       <OID of toplevel tree>
> > +     <Conflicted file list>
> >       <Informational messages>
>
> To distinguish between the list of conflicted files and the informational
> messages, I think it would be good to insert an empty line, as a
> separator, like.

Yes, I agree; that's why I did so.  :-)

[...]
> >       if (o->show_messages) {
> >               printf("\n");
>
> ... it seems that we do this already...

Yep.

[...]
> > diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh
> > index c34f8e6c1ed..43c9950dedb 100755
> > --- a/t/t4301-merge-tree-real.sh
> > +++ b/t/t4301-merge-tree-real.sh
> > @@ -94,6 +94,8 @@ test_expect_success 'test conflict notices and such' '
> >       #   "whatever" has *both* a modify/delete and a file/directory conflict
> >       cat <<-EOF >expect &&
> >       HASH
> > +     greeting
> > +     whatever~side1
> >
> >       Auto-merging greeting
> >       CONFLICT (content): Merge conflict in greeting
>
> ... as illustrated by the test, too. I guess the documentation should show
> the empty line, too?

It does:

    Informational messages
    ~~~~~~~~~~~~~~~~~~~~~~

    This always starts with a blank line to separate it from the previous
    sections, and then has free-form messages about the merge, such as:

The newline should not be split out separately, because --no-messages
suppresses the newline.  (Without the messages section, the newline
isn't needed.)

Johannes Schindelin Feb. 4, 2022, 11:12 p.m. UTC | #5

Hi Elijah,

On Fri, 28 Jan 2022, Elijah Newren wrote:

> On Fri, Jan 28, 2022 at 8:57 AM Johannes Schindelin
> <[e-mail]> wrote:
> >
> > Hi Elijah,
> >
> > On Sat, 22 Jan 2022, Elijah Newren via GitGitGadget wrote:
> >
> > > From: Elijah Newren <[e-mail]>
> > >
> [...]
> > > diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
> > > index fd7a867de60..041a4ac2785 100644
> > > --- a/Documentation/git-merge-tree.txt
> > > +++ b/Documentation/git-merge-tree.txt
> > > @@ -58,6 +58,7 @@ simply one line:
> > >  Whereas for a conflicted merge, the output is by default of the form:
> > >
> > >       <OID of toplevel tree>
> > > +     <Conflicted file list>
> > >       <Informational messages>
> >
> > To distinguish between the list of conflicted files and the informational
> > messages, I think it would be good to insert an empty line, as a
> > separator, like.
>
> Yes, I agree; that's why I did so.  :-)

My concern was that I did not see this empty line reflected in the quoted
diff. I would have expected an empty line between the `<Conflicted [...]>`
and the `<Informational [...]>` line.

Thanks,
Dscho

Johannes Schindelin Feb. 21, 2022, 9:31 a.m. UTC | #6

Hi Elijah,

[reinstating the Cc: list]

On Sat, 12 Feb 2022, Elijah Newren wrote:

> On Fri, Feb 4, 2022 at 3:12 PM Johannes Schindelin
> <[e-mail]> wrote:
> >
> > On Fri, 28 Jan 2022, Elijah Newren wrote:
> >
> > > On Fri, Jan 28, 2022 at 8:57 AM Johannes Schindelin
> > > <[e-mail]> wrote:
> > > >
> > > > Hi Elijah,
> > > >
> > > > On Sat, 22 Jan 2022, Elijah Newren via GitGitGadget wrote:
> > > >
> > > > > From: Elijah Newren <[e-mail]>
> > > > >
> > > [...]
> > > > > diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
> > > > > index fd7a867de60..041a4ac2785 100644
> > > > > --- a/Documentation/git-merge-tree.txt
> > > > > +++ b/Documentation/git-merge-tree.txt
> > > > > @@ -58,6 +58,7 @@ simply one line:
> > > > >  Whereas for a conflicted merge, the output is by default of the form:
> > > > >
> > > > >       <OID of toplevel tree>
> > > > > +     <Conflicted file list>
> > > > >       <Informational messages>
> > > >
> > > > To distinguish between the list of conflicted files and the informational
> > > > messages, I think it would be good to insert an empty line, as a
> > > > separator, like.
> > >
> > > Yes, I agree; that's why I did so.  :-)
> >
> > My concern was that I did not see this empty line reflected in the quoted
> > diff. I would have expected an empty line between the `<Conflicted [...]>`
> > and the `<Informational [...]>` line.
>
> As stated later in the same email, the newline is only printed if the
> <Informational messages> section is printed.  As such, it's part of
> the <Informational messages> section and listing it between the
> sections could be misleading.

Thank you for the clarification. I still think that it might cause
confusion in the documentation not to see the explicit newline, quite
possible when _I_ re-read this in six months from now. But then, if `-z`
is in effect, it won't be an explicit newline, it will be a NUL instead.

In short: I am fine with leaving this as-is, it might actually reduce even
_my_ confusion to the minimum possible.

Thanks,
Dscho

Patch

diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
index fd7a867de60..041a4ac2785 100644
--- a/Documentation/git-merge-tree.txt
+++ b/Documentation/git-merge-tree.txt
@@ -58,6 +58,7 @@  simply one line:
 Whereas for a conflicted merge, the output is by default of the form:
 
 	<OID of toplevel tree>
+	<Conflicted file list>
 	<Informational messages>
 
 These are discussed individually below.
@@ -69,6 +70,13 @@  This is a tree object that represents what would be checked out in the
 working tree at the end of `git merge`.  If there were conflicts, then
 files within this tree may have embedded conflict markers.
 
+Conflicted file list
+~~~~~~~~~~~~~~~~~~~~
+
+This is a sequence of lines containing a filename on each line, quoted
+as explained for the configuration variable `core.quotePath` (see
+linkgit:git-config[1]).
+
 Informational messages
 ~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 560640ad911..d8eeeb3f306 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -11,6 +11,9 @@ 
 #include "blob.h"
 #include "exec-cmd.h"
 #include "merge-blobs.h"
+#include "quote.h"
+
+static int line_termination = '\n';
 
 struct merge_list {
 	struct merge_list *next;
@@ -395,7 +398,8 @@  struct merge_tree_options {
 };
 
 static int real_merge(struct merge_tree_options *o,
-		      const char *branch1, const char *branch2)
+		      const char *branch1, const char *branch2,
+		      const char *prefix)
 {
 	struct commit *parent1, *parent2;
 	struct commit_list *common;
@@ -449,6 +453,22 @@  static int real_merge(struct merge_tree_options *o,
 		o->show_messages = !result.clean;
 
 	printf("%s\n", oid_to_hex(&result.tree->object.oid));
+	if (!result.clean) {
+		struct string_list conflicted_files = STRING_LIST_INIT_NODUP;
+		const char *last = NULL;
+		int i;
+
+		merge_get_conflicted_files(&result, &conflicted_files);
+		for (i = 0; i < conflicted_files.nr; i++) {
+			const char *name = conflicted_files.items[i].string;
+			if (last && !strcmp(last, name))
+				continue;
+			write_name_quoted_relative(
+				name, prefix, stdout, line_termination);
+			last = name;
+		}
+		string_list_clear(&conflicted_files, 1);
+	}
 	if (o->show_messages) {
 		printf("\n");
 		merge_display_update_messages(&opt, &result, stdout);
@@ -502,7 +522,7 @@  int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 
 	/* Do the relevant type of merge */
 	if (o.real)
-		return real_merge(&o, argv[0], argv[1]);
+		return real_merge(&o, argv[0], argv[1], prefix);
 	else
 		return trivial_merge(argv[0], argv[1], argv[2]);
 }
diff --git a/t/t4301-merge-tree-real.sh b/t/t4301-merge-tree-real.sh
index c34f8e6c1ed..43c9950dedb 100755
--- a/t/t4301-merge-tree-real.sh
+++ b/t/t4301-merge-tree-real.sh
@@ -94,6 +94,8 @@  test_expect_success 'test conflict notices and such' '
 	#   "whatever" has *both* a modify/delete and a file/directory conflict
 	cat <<-EOF >expect &&
 	HASH
+	greeting
+	whatever~side1
 
 	Auto-merging greeting
 	CONFLICT (content): Merge conflict in greeting
@@ -105,4 +107,13 @@  test_expect_success 'test conflict notices and such' '
 	test_cmp expect actual
 '
 
+test_expect_success 'Just the conflicted files without the messages' '
+	test_expect_code 1 git merge-tree --write-tree --no-messages side1 side2 >out &&
+	sed -e "s/[0-9a-f]\{40,\}/HASH/g" out >actual &&
+
+	test_write_lines HASH greeting whatever~side1 >expect &&
+
+	test_cmp expect actual
+'
+
 test_done