{"thread":{"id":"13912","subject":"git diff --cc bug","startedAt":"2008-06-12T11:48:10Z","lastAt":"2008-06-19T08:11:35Z","messageCount":4,"participants":["Johannes Sixt","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"79586","messageId":"48510CFA.3060101@viscovery.net","threadId":"13912","inReplyTo":null,"subject":"git diff --cc bug","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-06-12T11:48:10Z","receivedAt":"2008-06-12T11:48:10Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"While resolving a conflict, I noticed this output of 'git diff':\n\n@@@ -109,7 -130,8 +114,8 @@@ void CValuesView::UpdateValues...\n \tlvc.mask = LVCF_TEXT;\n \tlvc.pszText = const_cast(static_castShowPointerValues()) {\n -\t\t// invalidate values in the display\n -\t\tCPoint pt(m_x, m_y);\t// will be erased\n -\t\tInvalidateValues();\n -\t\tFindFmtStrings();\n -\t\tUpdate(pt.x, pt.y);\n -\t}\n -\treturn 0;\n -}\n -\n void CValuesView::OnUpdate(CView* pSender, LPARAM lHint, ...\n {\n \tswitch (lHint) {\n++<<<<<<< HEAD:SomUI/ValuesWnd.cpp\n++=======\n+ \tcase UPDATE_MONITORFRAME:\n+ \t\t{\n(hunk truncated for exposition)\n\nNotice that there are no context lines at the beginning of the second\nhunk. I would not have expected this, and I think it's a bug in git diff.\n\nI don't feel like diving into the diff machinery, and would appreciate if\nsomeone could have a closer look. I have prepared a small repository to\nreproduce the behavior, but I don't want to post it to the public.\n\n-- Hannes\n"},{"id":"80021","messageId":"48565311.9070407@viscovery.net","threadId":"13912","inReplyTo":"48510CFA.3060101@viscovery.net","subject":"Re: git diff --cc bug","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-06-16T11:48:33Z","receivedAt":"2008-06-16T11:48:33Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Johannes Sixt schrieb:\n> @@@ -635,31 -865,171 +641,169 @@@ void CValuesView::EraseVa...\n> -LRESULT CValuesView::OnOptionsChanged(WPARAM, LPARAM)\n> -{\n> -\tif (GetDocument()->ShowPointerValues()) {\n> -\t\t// invalidate values in the display\n> -\t\tCPoint pt(m_x, m_y);\t// will be erased\n> -\t\tInvalidateValues();\n> -\t\tFindFmtStrings();\n> -\t\tUpdate(pt.x, pt.y);\n> -\t}\n> -\treturn 0;\n> -}\n> -\n> void CValuesView::OnUpdate(CView* pSender, LPARAM lHint, ...\n> {\n> \tswitch (lHint) {\n> ++<<<<<<< HEAD:SomUI/ValuesWnd.cpp\n> ++=======\n> + \tcase UPDATE_MONITORFRAME:\n> + \t\t{\n> (hunk truncated for exposition)\n> \n> Notice that there are no context lines at the beginning of the second\n> hunk. I would not have expected this, and I think it's a bug in git diff.\n\nAttached is a tar.gz of a repository the exposes the bug when a simple\n'git diff' is run. If -U2 is added, the removals at the beginning of the\nhunk are not shown, as expected. If -U4 is added, more changes are shown\nat the beginning of the hunk, which happen to be separated by 3 lines of\ncontext from the part that is unexpectedly visible with -U3.\n\n-- Hannes\n\n"},{"id":"80290","messageId":"7vhcbpx6bq.fsf@gitster.siamese.dyndns.org","threadId":"13912","inReplyTo":"48565311.9070407@viscovery.net","subject":"Re: git diff --cc bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-19T06:54:49Z","receivedAt":"2008-06-19T06:54:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Johannes Sixt schrieb:\n>> @@@ -635,31 -865,171 +641,169 @@@ void CValuesView::EraseVa...\n>> -LRESULT CValuesView::OnOptionsChanged(WPARAM, LPARAM)\n>> -{\n>> -\tif (GetDocument()->ShowPointerValues()) {\n>> -\t\t// invalidate values in the display\n>> -\t\tCPoint pt(m_x, m_y);\t// will be erased\n>> -\t\tInvalidateValues();\n>> -\t\tFindFmtStrings();\n>> -\t\tUpdate(pt.x, pt.y);\n>> -\t}\n>> -\treturn 0;\n>> -}\n>> -\n>> void CValuesView::OnUpdate(CView* pSender, LPARAM lHint, ...\n>> {\n>> \tswitch (lHint) {\n>> ++<<<<<<< HEAD:SomUI/ValuesWnd.cpp\n>> ++=======\n>> + \tcase UPDATE_MONITORFRAME:\n>> + \t\t{\n>> (hunk truncated for exposition)\n>> \n>> Notice that there are no context lines at the beginning of the second\n>> hunk. I would not have expected this, and I think it's a bug in git diff.\n\nInteresting. There indeed is a bug, but it is not a bug that \"-LRESULT\"\nis not preceded by three more context lines. The bug is that these\ndeletion are shown when \"void CValuesView::\" line is shown.\n\nThe diff combining logic discards hunks that take the result solely from\na single parent. So in the above example (and your smaller example in the\ntarball), the initial segment that begins with \"-LRESULT\" is not\ninteresting at all and should never be shown.\n\nThe computation first finds \"interesting\" parts (done by make_hunks()),\nand then the first interesting line it finds actually is \"++<<<<<<\".\n\nThen it gives a bit of context going back from that line (done by\ngive_context()). It goes back by the default 3 lines and stops at the\nline \"void CValuesView::\". Then the output routine start emitting from\nthat line.\n\nThe problem is that the internal data structure hooks the deleted lines in\nfront of the surviving line that follows it. That means that showing the\nsline[] that holds \"void CValuesView::\" will show the unwanted \"-LRESULT\"\ndeletion part as part of it.\n\nYou can view this in action by running \"git diff -U2\" in your tarball\nrepository. By reducing the context to 2, the leading edge context now\nbegins at the opening brace after \"void CValuesView::\" line, which does\nnot have deletion lines in front of it, so you will see what you expect.\n\nThis patch would fix it for the sample repository, but I am not sure if it\nhas unintended side effects.\n\n combine-diff.c | 7 +++++--\n 1 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 588c58b..9f80a1c 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -84,6 +84,7 @@ struct sline {\n \t/* bit 0 up to (N-1) are on if the parent has this line (i.e.\n \t * we did not change it).\n \t * bit N is used for \"interesting\" lines, including context.\n+\t * bit (N+1) is used for \"do not show deletion before this\".\n \t */\n \tunsigned long flag;\n \tunsigned long *p_lno;\n@@ -308,6 +309,7 @@ static int give_context(struct sline *sline, unsigned long cnt, int num_parent)\n {\n \tunsigned long all_mask = (1UL<lost_head;\n+\t\t\tll = (sl->flag & no_pre_delete) ? NULL : sl->lost_head;\n \t\t\twhile (ll) {\n \t\t\t\tfputs(c_old, stdout);\n \t\t\t\tfor (j = 0; j < num_parent; j++) {\n"},{"id":"80296","messageId":"485A14B7.8080807@viscovery.net","threadId":"13912","inReplyTo":"7vhcbpx6bq.fsf@gitster.siamese.dyndns.org","subject":"Re: git diff --cc bug","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-06-19T08:11:35Z","receivedAt":"2008-06-19T08:11:35Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> The problem is that the internal data structure hooks the deleted lines in\n> front of the surviving line that follows it. That means that showing the\n> sline[] that holds \"void CValuesView::\" will show the unwanted \"-LRESULT\"\n> deletion part as part of it.\n> \n> You can view this in action by running \"git diff -U2\" in your tarball\n> repository. By reducing the context to 2, the leading edge context now\n> begins at the opening brace after \"void CValuesView::\" line, which does\n> not have deletion lines in front of it, so you will see what you expect.\n> \n> This patch would fix it for the sample repository, but I am not sure if it\n> has unintended side effects.\n\nI can verify that it fixes the case in hands as well as a \"symmetric\" one\nwhere the accidentally visible deletion is against the first parent, i.e.\nit looks like \"- delete_me\" instead of the \" -delete_me\" above. I also did\nsome brief tests with various \"normal\" conflict diffs, and looked at a\nnumber of merges in my repository, and I noticed nothing odd with this\npatch. So:\n\nTested-by: Johannes Sixt \n\n-- Hannes\n"}]}