threads / discuss / 29256

[Wish] use postimage when showing common context in "diff -w"?

Subject: [Wish] use postimage when showing common context in "diff -w"?

## tl;dr

4 messages between Dec 27, 2011 and Jan 10, 2012.

replies: 3people: 2as markdown or json

Junio C Hamano· Dec 27, 2011, 22:16 UTC · lore

Joey's "write first for-merge ref to FETCH_HEAD first" ($gmane/187699) wraps an existing large for(;;) loop inside another new loop, making the existing code indented deeper. After queuing the patch, "git show -w" displays a hunk like this [*1*]:

+	/*
+	 * The first pass writes objects to be merged and then the
+	 * second pass writes the rest, in order to allow using
+	 * FETCH_HEAD as a refname to refer to the ref to be merged.
+	 */
+	for (want_merge = 1; 0 <= want_merge; want_merge--) {
 	for (rm = ref_map; rm; rm = rm->next) {
 		struct ref *ref = NULL;
 
+			commit = lookup_commit_reference_gently(rm->old_sha1, 1);
+			if (!commit)
+				rm->merge = 0;
+
+			if (rm->merge != want_merge)
+				continue;
+
 		if (rm->peer_ref) {
 			ref = xcalloc(1, sizeof(*ref) + strlen(rm->peer_ref->name) + 1);
 			strcpy(ref->name, rm->peer_ref->name);

The context lines we can see in the above hunk are shown with incorrect indentation level; I think we are showing the lines from the preimage.

It would be a really nice holiday gift to us, if somebody can fix this to show lines from the postimage. It would make reviewing the change much more pleasant. I obviously cannot throw this into my Amazon wishlist, so instead I am posting it here ;-)

[Footnote]

*1* The text has my style fix-ups in it and does not match what was posted. The patch lacked a sign-off and needs to be amended anyway. Also it needs to adjust some existing tests (at least 5515 seems to break for obvious reasons).

René Scharfe· Jan 6, 2012, 17:13 UTC · re: Junio C Hamano · lore

[PATCH] xdiff: print post-image for common records instead of pre-image

Normally it doesn't matter if we show the pre-image or th post-image for the common parts of a diff because they are the same. If white-space changes are ignored they can differ, though. The new text after applying the diff is more interesting in that case, so show that instead of the old contents.

Note: GNU diff shows the pre-image.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 t/t4015-diff-whitespace.sh |   14 +++++++-------
 xdiff/xemit.c              |   12 ++++++------
 2 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 9059bcd..cc3db13 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -103,7 +103,7 @@ test_expect_success 'another test, with -w --ignore-space-at-eol' 'test_cmp expe
 git diff -w -b --ignore-space-at-eol > out
 test_expect_success 'another test, with -w -b --ignore-space-at-eol' 'test_cmp expect out'
 
-tr 'Q' '\015' << EOF > expect
+tr 'Q_' '\015 ' << EOF > expect
 diff --git a/x b/x
 index d99af23..8b32fb5 100644
 --- a/x
@@ -111,19 +111,19 @@ index d99af23..8b32fb5 100644
 @@ -1,6 +1,6 @@
 -whitespace at beginning
 +	whitespace at beginning
- whitespace change
+ whitespace 	 change
 -whitespace in the middle
 +white space in the middle
- whitespace at end
+ whitespace at end__
  unchanged line
- CR at endQ
+ CR at end
 EOF
 git diff -b > out
 test_expect_success 'another test, with -b' 'test_cmp expect out'
 git diff -b --ignore-space-at-eol > out
 test_expect_success 'another test, with -b --ignore-space-at-eol' 'test_cmp expect out'
 
-tr 'Q' '\015' << EOF > expect
+tr 'Q_' '\015 ' << EOF > expect
 diff --git a/x b/x
 index d99af23..8b32fb5 100644
 --- a/x
@@ -135,9 +135,9 @@ index d99af23..8b32fb5 100644
 +	whitespace at beginning
 +whitespace 	 change
 +white space in the middle
- whitespace at end
+ whitespace at end__
  unchanged line
- CR at endQ
+ CR at end
 EOF
 git diff --ignore-space-at-eol > out
 test_expect_success 'another test, with --ignore-space-at-eol' 'test_cmp expect out'
diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 2e669c3..d11dbf9 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -87,7 +87,7 @@ static long def_ff(const char *rec, long len, char *buf, long sz, void *priv)
 
 static int xdl_emit_common(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
                            xdemitconf_t const *xecfg) {
-	xdfile_t *xdf = &xe->xdf1;
+	xdfile_t *xdf = &xe->xdf2;
 	const char *rchg = xdf->rchg;
 	long ix;
 
@@ -204,8 +204,8 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 		/*
 		 * Emit pre-context.
 		 */
-		for (; s1 < xch->i1; s1++)
-			if (xdl_emit_record(&xe->xdf1, s1, " ", ecb) < 0)
+		for (; s2 < xch->i2; s2++)
+			if (xdl_emit_record(&xe->xdf2, s2, " ", ecb) < 0)
 				return -1;
 
 		for (s1 = xch->i1, s2 = xch->i2;; xch = xch->next) {
@@ -213,7 +213,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 			 * Merge previous with current change atom.
 			 */
 			for (; s1 < xch->i1 && s2 < xch->i2; s1++, s2++)
-				if (xdl_emit_record(&xe->xdf1, s1, " ", ecb) < 0)
+				if (xdl_emit_record(&xe->xdf2, s2, " ", ecb) < 0)
 					return -1;
 
 			/*
@@ -239,8 +239,8 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
 		/*
 		 * Emit post-context.
 		 */
-		for (s1 = xche->i1 + xche->chg1; s1 < e1; s1++)
-			if (xdl_emit_record(&xe->xdf1, s1, " ", ecb) < 0)
+		for (s2 = xche->i2 + xche->chg2; s2 < e2; s2++)
+			if (xdl_emit_record(&xe->xdf2, s2, " ", ecb) < 0)
 				return -1;
 	}
 

-- 
1.7.8.2
Junio C Hamano· Jan 10, 2012, 22:58 UTC · re: René Scharfe · lore

Re: [PATCH] xdiff: print post-image for common records instead of pre-image

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
Show 10 quoted lines
> Normally it doesn't matter if we show the pre-image or th post-image
> for the common parts of a diff because they are the same.  If
> white-space changes are ignored they can differ, though.  The
> new text after applying the diff is more interesting in that case,
> so show that instead of the old contents.
>
> Note: GNU diff shows the pre-image.
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>

I was looking at this one again, and I think one possible downside of showing post-image for the context lines is that the resulting patch would not apply to the pre-image tree anymore. Probably GNU folks thought that it is a big enough issue. Or perhaps they didn't simply care either way ;-)

In any case, showing pre-image lines as the context at least makes the patch easier to apply, but the result would be different from the intended post-image and would appear as if indentation fixes in the patch are reverted, so you would need manual fix-up after applying such a patch generated with (gnu) "diff -w".

I tried to generate an output from "show -w", with this change, on a commit that is largely indentation fix. The resulting patch seems to apply cleanly with "apply --ignore-space-change" to the parent of the commit "show -w" was taken from; of course the result needs some manual fix-ups for the indentation changes, but that is not a news anyway. So I suspect it won't be a huge downside and I think the benefit of being able to see the post-image in the context when the user is more interested in how the file looks like after the change outweighs it.

Thanks again.

← back to recent threads