From: Stefan Sperling <stsp@stsp.name>
Subject: show original content in merge conflicts
To: gameoftrees@openbsd.org
Date: Wed, 9 Oct 2019 12:25:23 +0200

I have spent some time trying to make sense of the weird diff3.c code.
While doing so I have committed some mechanical and stylisitic changes
without review. Please take a look at these changes if you have time.

I have now arrived at a point where I can finally make the functional
change I've been working towards, and I'm switching back into pre-commit
review mode. Please check my diff and the proposed functional change.

This diff makes conflict markers include the original group of lines
which were changed in conflicting ways in two derived versions of the
same file.

For instance, during 'got update', conflicts used to appear like this:

  <<<<<<<
  content of new file in repository
  =======
  content of locally modified file
  >>>>>>>

With the change below, conflicts appear like this instead:

  <<<<<<<
  content of new file in repository
  |||||||
  content of original file in repository
  =======
  content of locally modified file
  >>>>>>>

This behaviour matches the default behaviour of Subversion 1.12 and
should make it somewhat easier to make sense of merge conflicts.
Feedback from SVN users about this behaviour change has been very positive.

Note that no label ("commit 1234abc...") is printed after ||||||| yet.
I intend to work on that next if this change gets comitted.

ok?

diff d59c0cb27bc304bc11f9b1094c6eb85f248c7c5f /home/stsp/src/got
blob - e7951e14f2548e423ecb820f33291e316d6819ee
file + lib/diff3.c
--- lib/diff3.c
+++ lib/diff3.c
@@ -117,6 +117,7 @@ struct off_range {
 struct diff {
 	struct line_range old;
 	struct line_range new;
+	struct off_range oldo;
 	struct off_range newo;
 };
 
@@ -129,7 +130,8 @@ struct diff3_state {
 	/*
 	 * "de" is used to gather editing scripts.  These are later spewed out
 	 * in reverse order.  Its first element must be all zero, the "new"
-	 * component of "de" contains line positions and byte positions.
+	 * component of "de" contains line positions, and "oldo" and "newo"
+	 * components contain byte positions.
 	 * Array overlap indicates which sections in "de" correspond to lines
 	 * that are different in all three files.
 	 */
@@ -152,7 +154,7 @@ struct diff3_state {
 };
 
 
-static const struct got_error *duplicate(int *, struct line_range *,
+static const struct got_error *duplicate(int *, int, struct line_range *,
     struct line_range *, struct diff3_state *);
 static const struct got_error *edit(struct diff *, int, int *,
     struct diff3_state *);
@@ -758,7 +760,7 @@ merge(size_t m1, size_t m2, struct diff3_state *d3s)
 		}
 		/* stuff peculiar to third file or different in all */
 		if (d1->new.from == d2->new.from && d1->new.to == d2->new.to) {
-			err = duplicate(&dpl, &d1->old, &d2->old, d3s);
+			err = duplicate(&dpl, j, &d1->old, &d2->old, d3s);
 			if (err)
 				return err;
 
@@ -851,9 +853,12 @@ skip(size_t *nskipped, int i, int from, struct diff3_s
 /*
  * Set *dpl to 1 or 0 according as the old range (in file 1) contains exactly
  * the same data as the new range (in file 2).
+ *
+ * If this change could overlap, remember start/end offsets in file 2 so we
+ * can write out the original lines of text if a merge conflict occurs.
  */
 static const struct got_error *
-duplicate(int *dpl, struct line_range *r1, struct line_range *r2,
+duplicate(int *dpl, int j, struct line_range *r1, struct line_range *r2,
     struct diff3_state *d3s)
 {
 	const struct got_error *err = NULL;
@@ -861,6 +866,7 @@ duplicate(int *dpl, struct line_range *r1, struct line
 	int nchar;
 	int nline;
 	size_t nskipped;
+	off_t off;
 
 	*dpl = 0;
 
@@ -870,6 +876,12 @@ duplicate(int *dpl, struct line_range *r1, struct line
 	err = skip(&nskipped, 0, r1->from, d3s);
 	if (err)
 		return err;
+
+	off = ftello(d3s->fp[1]);
+	if (off == -1)
+		return got_error_from_errno("ftello");
+	d3s->de[j + 1].oldo.from = off; /* original lines start here */
+
 	err = skip(&nskipped, 1, r2->from, d3s);
 	if (err)
 		return err;
@@ -883,8 +895,24 @@ duplicate(int *dpl, struct line_range *r1, struct line
 			if (d == EOF)
 				return got_ferror(d3s->fp[1], GOT_ERR_EOF);
 			nchar++;
-			if (c != d)
+			if (c != d) {
+				long orig_line_len = nchar;
+				while (d != '\n') {
+					d = getc(d3s->fp[1]);
+					if (d == EOF)
+						break;
+					orig_line_len++;
+				}
+				if (orig_line_len > nchar &&
+				    fseek(d3s->fp[1], -(orig_line_len - nchar),
+				    SEEK_CUR) == -1)
+					return got_ferror(d3s->fp[1],
+						GOT_ERR_IO);
+				/* original lines end here */
+				d3s->de[j + 1].oldo.to = off + orig_line_len;
+
 				return repos(nchar, d3s);
+			}
 		} while (c != '\n');
 	}
 	err = repos(nchar, d3s);
@@ -952,7 +980,26 @@ edscript(int n, struct diff3_state *d3s)
 				return err;
 		} else {
 			err = diff_output(d3s->diffbuf, "%da\n%s\n",
-			    d3s->de[n].old.to -1, GOT_DIFF_CONFLICT_MARKER_SEP);
+			    d3s->de[n].old.to -1,
+			    GOT_DIFF_CONFLICT_MARKER_ORIG);
+			if (err)
+				return err;
+			if (fseeko(d3s->fp[1], d3s->de[n].oldo.from, SEEK_SET)
+			    == -1)
+				return got_error_from_errno("fseeko");
+			k = (size_t)(d3s->de[n].oldo.to - d3s->de[n].oldo.from);
+			for (; k > 0; k -= len) {
+				len = k > BUFSIZ ? BUFSIZ : k;
+				if (fread(block, 1, len, d3s->fp[1]) != len)
+					return got_ferror(d3s->fp[1],
+					    GOT_ERR_IO);
+				block[len] = '\0';
+				err = diff_output(d3s->diffbuf, "%s", block);
+				if (err)
+					return err;
+			}
+			err = diff_output(d3s->diffbuf, "%s\n",
+			    GOT_DIFF_CONFLICT_MARKER_SEP);
 			if (err)
 				return err;
 		}
blob - f3ddb3fb288bda4b679437664c0f0f403351969d
file + lib/got_lib_diff.h
--- lib/got_lib_diff.h
+++ lib/got_lib_diff.h
@@ -135,6 +135,7 @@ struct got_diff_args {
 };
 
 #define GOT_DIFF_CONFLICT_MARKER_BEGIN	"<<<<<<<"
+#define GOT_DIFF_CONFLICT_MARKER_ORIG	"|||||||"
 #define GOT_DIFF_CONFLICT_MARKER_SEP	"======="
 #define GOT_DIFF_CONFLICT_MARKER_END	">>>>>>>"
 
blob - d5083e047e683d5211b8352a65aeb41fea0facb2
file + regress/cmdline/diff.sh
--- regress/cmdline/diff.sh
+++ regress/cmdline/diff.sh
@@ -138,10 +138,12 @@ function test_diff_shows_conflict {
 	echo 'file + numbers' >> $testroot/stdout.expected
 	echo '--- numbers' >> $testroot/stdout.expected
 	echo '+++ numbers' >> $testroot/stdout.expected
-	echo '@@ -1,5 +1,9 @@' >> $testroot/stdout.expected
+	echo '@@ -1,5 +1,11 @@' >> $testroot/stdout.expected
 	echo ' 1' >> $testroot/stdout.expected
 	echo "+<<<<<<< commit $head_rev" >> $testroot/stdout.expected
 	echo ' 22' >> $testroot/stdout.expected
+	echo '+|||||||' >> $testroot/stdout.expected
+	echo '+1' >> $testroot/stdout.expected
 	echo '+=======' >> $testroot/stdout.expected
 	echo '+77' >> $testroot/stdout.expected
 	echo '+>>>>>>> numbers' >> $testroot/stdout.expected
blob - 0d64a54e87019fb4decc9861f1e2c2da67aba905
file + regress/cmdline/rebase.sh
--- regress/cmdline/rebase.sh
+++ regress/cmdline/rebase.sh
@@ -218,6 +218,8 @@ function test_rebase_continue {
 
 	echo "<<<<<<< commit $orig_commit1" > $testroot/content.expected
 	echo "modified alpha on branch" >> $testroot/content.expected
+	echo "|||||||" >> $testroot/content.expected
+	echo "alpha" >> $testroot/content.expected
 	echo "=======" >> $testroot/content.expected
 	echo "modified alpha on master" >> $testroot/content.expected
 	echo '>>>>>>> alpha' >> $testroot/content.expected
@@ -347,6 +349,8 @@ function test_rebase_abort {
 
 	echo "<<<<<<< commit $orig_commit1" > $testroot/content.expected
 	echo "modified alpha on branch" >> $testroot/content.expected
+	echo "|||||||" >> $testroot/content.expected
+	echo "alpha" >> $testroot/content.expected
 	echo "=======" >> $testroot/content.expected
 	echo "modified alpha on master" >> $testroot/content.expected
 	echo '>>>>>>> alpha' >> $testroot/content.expected
@@ -455,6 +459,8 @@ function test_rebase_no_op_change {
 
 	echo "<<<<<<< commit $orig_commit1" > $testroot/content.expected
 	echo "modified alpha on branch" >> $testroot/content.expected
+	echo "|||||||" >> $testroot/content.expected
+	echo "alpha" >> $testroot/content.expected
 	echo "=======" >> $testroot/content.expected
 	echo "modified alpha on master" >> $testroot/content.expected
 	echo '>>>>>>> alpha' >> $testroot/content.expected
@@ -560,6 +566,8 @@ function test_rebase_in_progress {
 
 	echo "<<<<<<< commit $orig_commit1" > $testroot/content.expected
 	echo "modified alpha on branch" >> $testroot/content.expected
+	echo "|||||||" >> $testroot/content.expected
+	echo "alpha" >> $testroot/content.expected
 	echo "=======" >> $testroot/content.expected
 	echo "modified alpha on master" >> $testroot/content.expected
 	echo '>>>>>>> alpha' >> $testroot/content.expected
blob - 1cdd63392f3e865270de6ffdb9b10cb96f773a41
file + regress/cmdline/update.sh
--- regress/cmdline/update.sh
+++ regress/cmdline/update.sh
@@ -680,6 +680,8 @@ function test_update_merges_file_edits {
 	git_show_head $testroot/repo >> $testroot/content.expected
 	echo >> $testroot/content.expected
 	echo "modified alpha" >> $testroot/content.expected
+	echo "|||||||" >> $testroot/content.expected
+	echo "alpha" >> $testroot/content.expected
 	echo "=======" >> $testroot/content.expected
 	echo "modified alpha, too" >> $testroot/content.expected
 	echo '>>>>>>> alpha' >> $testroot/content.expected
@@ -883,6 +885,7 @@ function test_update_conflict_wt_add_vs_repo_add {
 	git_show_head $testroot/repo >> $testroot/content.expected
 	echo >> $testroot/content.expected
 	echo "new" >> $testroot/content.expected
+	echo "|||||||" >> $testroot/content.expected
 	echo "=======" >> $testroot/content.expected
 	echo "also new" >> $testroot/content.expected
 	echo '>>>>>>> gamma/new' >> $testroot/content.expected
@@ -1364,6 +1367,8 @@ function test_update_to_another_branch {
 	git_show_head $testroot/repo >> $testroot/content.expected
 	echo >> $testroot/content.expected
 	echo "modified alpha on new branch" >> $testroot/content.expected
+	echo "|||||||" >> $testroot/content.expected
+	echo "alpha" >> $testroot/content.expected
 	echo "=======" >> $testroot/content.expected
 	echo "modified alpha in work tree" >> $testroot/content.expected
 	echo '>>>>>>> alpha' >> $testroot/content.expected