{"thread":{"id":"29256","subject":"[Wish] use postimage when showing common context in \"diff -w\"?","startedAt":"2011-12-27T22:16:56Z","lastAt":"2012-01-10T22:58:42Z","messageCount":4,"participants":["Junio C Hamano","René Scharfe"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"181736","messageId":"7vlipx4q3r.fsf@alter.siamese.dyndns.org","threadId":"29256","inReplyTo":null,"subject":"[Wish] use postimage when showing common context in \"diff -w\"?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-27T22:16:56Z","receivedAt":"2011-12-27T22:16:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joey's \"write first for-merge ref to FETCH_HEAD first\" ($gmane/187699)\nwraps an existing large for(;;) loop inside another new loop, making the\nexisting code indented deeper. After queuing the patch, \"git show -w\"\ndisplays a hunk like this [*1*]:\n\n+\t/*\n+\t * The first pass writes objects to be merged and then the\n+\t * second pass writes the rest, in order to allow using\n+\t * FETCH_HEAD as a refname to refer to the ref to be merged.\n+\t */\n+\tfor (want_merge = 1; 0 <= want_merge; want_merge--) {\n \tfor (rm = ref_map; rm; rm = rm->next) {\n \t\tstruct ref *ref = NULL;\n \n+\t\t\tcommit = lookup_commit_reference_gently(rm->old_sha1, 1);\n+\t\t\tif (!commit)\n+\t\t\t\trm->merge = 0;\n+\n+\t\t\tif (rm->merge != want_merge)\n+\t\t\t\tcontinue;\n+\n \t\tif (rm->peer_ref) {\n \t\t\tref = xcalloc(1, sizeof(*ref) + strlen(rm->peer_ref->name) + 1);\n \t\t\tstrcpy(ref->name, rm->peer_ref->name);\n\nThe context lines we can see in the above hunk are shown with incorrect\nindentation level; I think we are showing the lines from the preimage.\n\nIt would be a really nice holiday gift to us, if somebody can fix this to\nshow lines from the postimage. It would make reviewing the change much\nmore pleasant. I obviously cannot throw this into my Amazon wishlist, so\ninstead I am posting it here ;-)\n\n\n[Footnote]\n\n*1* The text has my style fix-ups in it and does not match what was\nposted. The patch lacked a sign-off and needs to be amended anyway. Also\nit needs to adjust some existing tests (at least 5515 seems to break for\nobvious reasons).\n"},{"id":"182030","messageId":"4F072B9C.1030005@lsrfire.ath.cx","threadId":"29256","inReplyTo":"7vlipx4q3r.fsf@alter.siamese.dyndns.org","subject":"[PATCH] xdiff: print post-image for common records instead of pre-image","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-01-06T17:13:00Z","receivedAt":"2012-01-06T17:13:00Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Normally it doesn't matter if we show the pre-image or th post-image\nfor the common parts of a diff because they are the same. If\nwhite-space changes are ignored they can differ, though. The\nnew text after applying the diff is more interesting in that case,\nso show that instead of the old contents.\n\nNote: GNU diff shows the pre-image.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Rene Scharfe \n---\n t/t4015-diff-whitespace.sh | 14 +++++++-------\n xdiff/xemit.c | 12 ++++++------\n 2 files changed, 13 insertions(+), 13 deletions(-)\n\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 9059bcd..cc3db13 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -103,7 +103,7 @@ test_expect_success 'another test, with -w --ignore-space-at-eol' 'test_cmp expe\n git diff -w -b --ignore-space-at-eol > out\n test_expect_success 'another test, with -w -b --ignore-space-at-eol' 'test_cmp expect out'\n \n-tr 'Q' '\\015' << EOF > expect\n+tr 'Q_' '\\015 ' << EOF > expect\n diff --git a/x b/x\n index d99af23..8b32fb5 100644\n --- a/x\n@@ -111,19 +111,19 @@ index d99af23..8b32fb5 100644\n @@ -1,6 +1,6 @@\n -whitespace at beginning\n +\twhitespace at beginning\n- whitespace change\n+ whitespace \t change\n -whitespace in the middle\n +white space in the middle\n- whitespace at end\n+ whitespace at end__\n unchanged line\n- CR at endQ\n+ CR at end\n EOF\n git diff -b > out\n test_expect_success 'another test, with -b' 'test_cmp expect out'\n git diff -b --ignore-space-at-eol > out\n test_expect_success 'another test, with -b --ignore-space-at-eol' 'test_cmp expect out'\n \n-tr 'Q' '\\015' << EOF > expect\n+tr 'Q_' '\\015 ' << EOF > expect\n diff --git a/x b/x\n index d99af23..8b32fb5 100644\n --- a/x\n@@ -135,9 +135,9 @@ index d99af23..8b32fb5 100644\n +\twhitespace at beginning\n +whitespace \t change\n +white space in the middle\n- whitespace at end\n+ whitespace at end__\n unchanged line\n- CR at endQ\n+ CR at end\n EOF\n git diff --ignore-space-at-eol > out\n test_expect_success 'another test, with --ignore-space-at-eol' 'test_cmp expect out'\ndiff --git a/xdiff/xemit.c b/xdiff/xemit.c\nindex 2e669c3..d11dbf9 100644\n--- a/xdiff/xemit.c\n+++ b/xdiff/xemit.c\n@@ -87,7 +87,7 @@ static long def_ff(const char *rec, long len, char *buf, long sz, void *priv)\n \n static int xdl_emit_common(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n xdemitconf_t const *xecfg) {\n-\txdfile_t *xdf = &xe->xdf1;\n+\txdfile_t *xdf = &xe->xdf2;\n \tconst char *rchg = xdf->rchg;\n \tlong ix;\n \n@@ -204,8 +204,8 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\t/*\n \t\t * Emit pre-context.\n \t\t */\n-\t\tfor (; s1 < xch->i1; s1++)\n-\t\t\tif (xdl_emit_record(&xe->xdf1, s1, \" \", ecb) < 0)\n+\t\tfor (; s2 < xch->i2; s2++)\n+\t\t\tif (xdl_emit_record(&xe->xdf2, s2, \" \", ecb) < 0)\n \t\t\t\treturn -1;\n \n \t\tfor (s1 = xch->i1, s2 = xch->i2;; xch = xch->next) {\n@@ -213,7 +213,7 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\t\t * Merge previous with current change atom.\n \t\t\t */\n \t\t\tfor (; s1 < xch->i1 && s2 < xch->i2; s1++, s2++)\n-\t\t\t\tif (xdl_emit_record(&xe->xdf1, s1, \" \", ecb) < 0)\n+\t\t\t\tif (xdl_emit_record(&xe->xdf2, s2, \" \", ecb) < 0)\n \t\t\t\t\treturn -1;\n \n \t\t\t/*\n@@ -239,8 +239,8 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\t/*\n \t\t * Emit post-context.\n \t\t */\n-\t\tfor (s1 = xche->i1 + xche->chg1; s1 < e1; s1++)\n-\t\t\tif (xdl_emit_record(&xe->xdf1, s1, \" \", ecb) < 0)\n+\t\tfor (s2 = xche->i2 + xche->chg2; s2 < e2; s2++)\n+\t\t\tif (xdl_emit_record(&xe->xdf2, s2, \" \", ecb) < 0)\n \t\t\t\treturn -1;\n \t}\n \n-- \n1.7.8.2\n"},{"id":"182033","messageId":"7v39bslk9h.fsf@alter.siamese.dyndns.org","threadId":"29256","inReplyTo":"4F072B9C.1030005@lsrfire.ath.cx","subject":"Re: [PATCH] xdiff: print post-image for common records instead of pre-image","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-06T19:10:34Z","receivedAt":"2012-01-06T19:10:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n"},{"id":"182292","messageId":"7vy5tf88rh.fsf@alter.siamese.dyndns.org","threadId":"29256","inReplyTo":"4F072B9C.1030005@lsrfire.ath.cx","subject":"Re: [PATCH] xdiff: print post-image for common records instead of pre-image","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-10T22:58:42Z","receivedAt":"2012-01-10T22:58:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Normally it doesn't matter if we show the pre-image or th post-image\n> for the common parts of a diff because they are the same. If\n> white-space changes are ignored they can differ, though. The\n> new text after applying the diff is more interesting in that case,\n> so show that instead of the old contents.\n>\n> Note: GNU diff shows the pre-image.\n>\n> Suggested-by: Junio C Hamano \n> Signed-off-by: Rene Scharfe \n\nI was looking at this one again, and I think one possible downside of\nshowing post-image for the context lines is that the resulting patch would\nnot apply to the pre-image tree anymore. Probably GNU folks thought that\nit is a big enough issue. Or perhaps they didn't simply care either way ;-)\n\nIn any case, showing pre-image lines as the context at least makes the\npatch easier to apply, but the result would be different from the intended\npost-image and would appear as if indentation fixes in the patch are\nreverted, so you would need manual fix-up after applying such a patch\ngenerated with (gnu) \"diff -w\".\n\nI tried to generate an output from \"show -w\", with this change, on a\ncommit that is largely indentation fix. The resulting patch seems to apply\ncleanly with \"apply --ignore-space-change\" to the parent of the commit\n\"show -w\" was taken from; of course the result needs some manual fix-ups\nfor the indentation changes, but that is not a news anyway. So I suspect\nit won't be a huge downside and I think the benefit of being able to see\nthe post-image in the context when the user is more interested in how the\nfile looks like after the change outweighs it.\n\nThanks again.\n"}]}