git diff --cc bug

git diff --cc bug

[Johannes Sixt]

While resolving a conflict, I noticed this output of 'git diff':

@@@ -109,7 -130,8 +114,8 @@@ void CValuesView::UpdateValues...
  	lvc.mask = LVCF_TEXT;
  	lvc.pszText = const_cast<LPTSTR>(static_cast<LPCTST...
 -	m_table.SetColumn(1, &lvc);
 +	GetListCtrl().SetColumn(1, &lvc);
+ 	m_columnTitles[1].id = valuesHeader;
  }


@@@ -635,31 -865,171 +641,169 @@@ void CValuesView::EraseVa...
 -LRESULT CValuesView::OnOptionsChanged(WPARAM, LPARAM)
 -{
 -	if (GetDocument()->ShowPointerValues()) {
 -		// invalidate values in the display
 -		CPoint pt(m_x, m_y);	// will be erased
 -		InvalidateValues();
 -		FindFmtStrings();
 -		Update(pt.x, pt.y);
 -	}
 -	return 0;
 -}
 -
  void CValuesView::OnUpdate(CView* pSender, LPARAM lHint, ...
  {
  	switch (lHint) {
++<<<<<<< HEAD:SomUI/ValuesWnd.cpp
++=======
+ 	case UPDATE_MONITORFRAME:
+ 		{
(hunk truncated for exposition)

Notice that there are no context lines at the beginning of the second
hunk. I would not have expected this, and I think it's a bug in git diff.

I don't feel like diving into the diff machinery, and would appreciate if
someone could have a closer look. I have prepared a small repository to
reproduce the behavior, but I don't want to post it to the public.

-- Hannes

Re: git diff --cc bug

[Johannes Sixt]

[-- Attachment #1: Type: text/plain, Size: 1125 bytes --]

Johannes Sixt schrieb:
> @@@ -635,31 -865,171 +641,169 @@@ void CValuesView::EraseVa...
>  -LRESULT CValuesView::OnOptionsChanged(WPARAM, LPARAM)
>  -{
>  -	if (GetDocument()->ShowPointerValues()) {
>  -		// invalidate values in the display
>  -		CPoint pt(m_x, m_y);	// will be erased
>  -		InvalidateValues();
>  -		FindFmtStrings();
>  -		Update(pt.x, pt.y);
>  -	}
>  -	return 0;
>  -}
>  -
>   void CValuesView::OnUpdate(CView* pSender, LPARAM lHint, ...
>   {
>   	switch (lHint) {
> ++<<<<<<< HEAD:SomUI/ValuesWnd.cpp
> ++=======
> + 	case UPDATE_MONITORFRAME:
> + 		{
> (hunk truncated for exposition)
> 
> Notice that there are no context lines at the beginning of the second
> hunk. I would not have expected this, and I think it's a bug in git diff.

Attached is a tar.gz of a repository the exposes the bug when a simple
'git diff' is run. If -U2 is added, the removals at the beginning of the
hunk are not shown, as expected. If -U4 is added, more changes are shown
at the beginning of the hunk, which happen to be separated by 3 lines of
context from the part that is unexpectedly visible with -U3.

-- Hannes


[-- Attachment #2: git-diff-cc-bug.tar.gz --]
[-- Type: application/gzip, Size: 3183 bytes --]

Re: git diff --cc bug

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Johannes Sixt schrieb:
>> @@@ -635,31 -865,171 +641,169 @@@ void CValuesView::EraseVa...
>>  -LRESULT CValuesView::OnOptionsChanged(WPARAM, LPARAM)
>>  -{
>>  -	if (GetDocument()->ShowPointerValues()) {
>>  -		// invalidate values in the display
>>  -		CPoint pt(m_x, m_y);	// will be erased
>>  -		InvalidateValues();
>>  -		FindFmtStrings();
>>  -		Update(pt.x, pt.y);
>>  -	}
>>  -	return 0;
>>  -}
>>  -
>>   void CValuesView::OnUpdate(CView* pSender, LPARAM lHint, ...
>>   {
>>   	switch (lHint) {
>> ++<<<<<<< HEAD:SomUI/ValuesWnd.cpp
>> ++=======
>> + 	case UPDATE_MONITORFRAME:
>> + 		{
>> (hunk truncated for exposition)
>> 
>> Notice that there are no context lines at the beginning of the second
>> hunk. I would not have expected this, and I think it's a bug in git diff.

Interesting.  There indeed is a bug, but it is not a bug that "-LRESULT"
is not preceded by three more context lines.  The bug is that these
deletion are shown when "void CValuesView::" line is shown.

The diff combining logic discards hunks that take the result solely from
a single parent.  So in the above example (and your smaller example in the
tarball), the initial segment that begins with "-LRESULT" is not
interesting at all and should never be shown.

The computation first finds "interesting" parts (done by make_hunks()),
and then the first interesting line it finds actually is "++<<<<<<".

Then it gives a bit of context going back from that line (done by
give_context()).  It goes back by the default 3 lines and stops at the
line "void CValuesView::".  Then the output routine start emitting from
that line.

The problem is that the internal data structure hooks the deleted lines in
front of the surviving line that follows it.  That means that showing the
sline[] that holds "void CValuesView::" will show the unwanted "-LRESULT"
deletion part as part of it.

You can view this in action by running "git diff -U2" in your tarball
repository.  By reducing the context to 2, the leading edge context now
begins at the opening brace after "void CValuesView::" line, which does
not have deletion lines in front of it, so you will see what you expect.

This patch would fix it for the sample repository, but I am not sure if it
has unintended side effects.

 combine-diff.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index 588c58b..9f80a1c 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -84,6 +84,7 @@ struct sline {
 	/* bit 0 up to (N-1) are on if the parent has this line (i.e.
 	 * we did not change it).
 	 * bit N is used for "interesting" lines, including context.
+	 * bit (N+1) is used for "do not show deletion before this".
 	 */
 	unsigned long flag;
 	unsigned long *p_lno;
@@ -308,6 +309,7 @@ static int give_context(struct sline *sline, unsigned long cnt, int num_parent)
 {
 	unsigned long all_mask = (1UL<<num_parent) - 1;
 	unsigned long mark = (1UL<<num_parent);
+	unsigned long no_pre_delete = (2UL<<num_parent);
 	unsigned long i;
 
 	/* Two groups of interesting lines may have a short gap of
@@ -329,7 +331,7 @@ static int give_context(struct sline *sline, unsigned long cnt, int num_parent)
 
 		/* Paint a few lines before the first interesting line. */
 		while (j < i)
-			sline[j++].flag |= mark;
+			sline[j++].flag |= mark | no_pre_delete;
 
 	again:
 		/* we know up to i is to be included.  where does the
@@ -502,6 +504,7 @@ static void dump_sline(struct sline *sline, unsigned long cnt, int num_parent,
 		       int use_color)
 {
 	unsigned long mark = (1UL<<num_parent);
+	unsigned long no_pre_delete = (2UL<<num_parent);
 	int i;
 	unsigned long lno = 0;
 	const char *c_frag = diff_get_color(use_color, DIFF_FRAGINFO);
@@ -581,7 +584,7 @@ static void dump_sline(struct sline *sline, unsigned long cnt, int num_parent,
 			int j;
 			unsigned long p_mask;
 			sl = &sline[lno++];
-			ll = sl->lost_head;
+			ll = (sl->flag & no_pre_delete) ? NULL : sl->lost_head;
 			while (ll) {
 				fputs(c_old, stdout);
 				for (j = 0; j < num_parent; j++) {

Re: git diff --cc bug

[Johannes Sixt]

Junio C Hamano schrieb:
> The problem is that the internal data structure hooks the deleted lines in
> front of the surviving line that follows it.  That means that showing the
> sline[] that holds "void CValuesView::" will show the unwanted "-LRESULT"
> deletion part as part of it.
> 
> You can view this in action by running "git diff -U2" in your tarball
> repository.  By reducing the context to 2, the leading edge context now
> begins at the opening brace after "void CValuesView::" line, which does
> not have deletion lines in front of it, so you will see what you expect.
> 
> This patch would fix it for the sample repository, but I am not sure if it
> has unintended side effects.

I can verify that it fixes the case in hands as well as a "symmetric" one
where the accidentally visible deletion is against the first parent, i.e.
it looks like "- delete_me" instead of the " -delete_me" above. I also did
some brief tests with various "normal" conflict diffs, and looked at a
number of merges in my repository, and I noticed nothing odd with this
patch. So:

Tested-by: Johannes Sixt <johannes.sixt@telecom.at>

-- Hannes