Possible bug in Git

Possible bug in Git

[David Martínez Martí]

(Please add deavidsedice@gmail.com and desarrollo@gestiweb.com to CC 
because I'm not suscripted to this list)

Here I've a strange behaviour in GIT:

We noticed a file in our repo is empty: 
facturacion/facturacion/scripts/lineasfacturascli.qs

We didn't found the reason using gitk --all -- 
facturacion/facturacion/scripts/lineasfacturascli.qs

With git bisect we found the problematic commit: 
dac6a95f559604978ff9e1fac24ad752c54382ae

With git show, it doesn't show anything about the modification:

$ git show dac6a95f5 -p --pretty=raw -- 
facturacion/facturacion/scripts/lineasfacturascli.qs
commit dac6a95f559604978ff9e1fac24ad752c54382ae
tree f1472f1d289f9813df5fd691c9cea6ccb04313a7
parent 5dfdd78f24f4578cdfeed876b8bacb45790af9ef
parent 6ed54c8c6508b6d5551290b45785eaf44dd05d75
author David Martinez Marti <desarrollo@gestiweb.com> 1260959309 +0100
committer David Martinez Marti <desarrollo@gestiweb.com> 1260959309 +0100

     Merge remote branch 'origin/infosial'

     Conflicts:
         contabilidad/informes/scripts/flcontinfo.qs
         facturacion/almacen/forms/articulos.ui
         facturacion/almacen/scripts/articulos.qs
         facturacion/almacen/tables/factalma_general.mtd
         facturacion/facturacion/forms/albaranescli.ui
         facturacion/facturacion/forms/albaranesprov.ui
         facturacion/facturacion/forms/facturasprov.ui
         facturacion/facturacion/forms/pedidoscli.ui
         facturacion/facturacion/forms/pedidosprov.ui
         facturacion/facturacion/forms/presupuestoscli.ui
         facturacion/facturacion/scripts/facturascli.qs
         facturacion/facturacion/scripts/facturasprov.qs
         facturacion/facturacion/scripts/flfacturac.qs
         facturacion/facturacion/scripts/lineasalbaranescli.qs
         facturacion/facturacion/scripts/lineasfacturascli.qs
         facturacion/facturacion/scripts/lineaspresupuestoscli.qs
         facturacion/informes/flfactinfo.xml
         facturacion/informes/forms/i_reciboscli.ui
         facturacion/informes/forms/i_recibosprov.ui
         facturacion/informes/queries/i_reciboscli.qry
         facturacion/informes/queries/i_recibosprov.qry
         facturacion/informes/queries/i_resreciboscli.qry
         facturacion/informes/queries/i_resrecibosprov.qry
         facturacion/informes/scripts/flfactinfo.qs
         facturacion/informes/scripts/i_masterinventario.qs
         facturacion/principal/flfactppal.xml
         facturacion/principal/forms/agentes.ui
         facturacion/principal/forms/clientes.ui
         facturacion/principal/forms/empresa.ui
         facturacion/principal/forms/flfactppal.ui
         facturacion/principal/forms/proveedores.ui
         facturacion/principal/scripts/clientes.qs
         facturacion/principal/scripts/proveedores.qs
         facturacion/principal/tables/clientes.mtd
         facturacion/principal/tables/proveedores.mtd
         facturacion/tesoreria/tables/reciboscli.mtd




There was a conflict on that file but it doesn't show anything changed 
about it. Gitk does the same.

git show commit^1:path and git show commit^2:path shows the files:

$ git show 
dac6a95f5^1:facturacion/facturacion/scripts/lineasfacturascli.qs | head
/***************************************************************************
                  lineasfacturascli.qs  -  description
                              -------------------
     begin                : lun abr 26 2004
     copyright            : (C) 2004 by InfoSiAL S.L.
     email                : mail@infosial.com
  ***************************************************************************/
/***************************************************************************
  *                                                                         *
  *   This program is free software; you can redistribute it and/or 
modify  *


$ git show 
dac6a95f5^2:facturacion/facturacion/scripts/lineasfacturascli.qs | head
/***************************************************************************
                  lineasfacturascli.qs  -  description
                              -------------------
     begin                : lun abr 26 2004
     copyright            : (C) 2004 by InfoSiAL S.L.
     email                : mail@infosial.com
  ***************************************************************************/
/***************************************************************************
  *                                                                         *
  *   This program is free software; you can redistribute it and/or 
modify  *


git show commit:path shows empty file:
$ git show 
dac6a95f5:facturacion/facturacion/scripts/lineasfacturascli.qs | wc
       0       0       0



I believe that changes *should* appear on git log -p and on gitk, and 
they doesn't, so I think it may be a bug in some git tools.

Tell me if I can do anything to fdetermine if it's really a bug and 
where it is to help GIT devs resovling it.

PD: I know my merge failed somehow, and I corrected it in a newer commit.

[PATCH] combined diff: correctly handle truncated file

[Thomas Rast]

Consider an evil merge of two commits A and B, both of which have a
file 'foo', but the merge result does not have that file.

The combined-diff code learned in 4462731 (combine-diff: do not punt
on removed or added files., 2006-02-06) to concisely show only the
removal, since that is the evil part and the previous contents are
presumably uninteresting.

However, to diagnose an empty merge result, it overloaded the variable
that holds the file's length.  This means that the check also triggers
for truncated files.  Consequently, such files were not shown in the
diff at all despite the merge being clearly evil.

Fix this by adding a new variable that distinguishes whether the file
was deleted (which is the case 4462731 handled) or truncated.  In the
truncated case, we show the full combined diff again, which is rather
spammy but at least does not hide the evilness.

Reported-by: David Martínez Martí <desarrollo@gestiweb.com>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 combine-diff.c           |   14 ++++++++------
 t/t4038-diff-combined.sh |    8 ++++++++
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index 6162691..3480dae 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -204,7 +204,7 @@ static void consume_line(void *state_, char *line, unsigned long len)
 static void combine_diff(const unsigned char *parent, unsigned int mode,
 			 mmfile_t *result_file,
 			 struct sline *sline, unsigned int cnt, int n,
-			 int num_parent)
+			 int num_parent, int result_deleted)
 {
 	unsigned int p_lno, lno;
 	unsigned long nmask = (1UL << n);
@@ -215,7 +215,7 @@ static void combine_diff(const unsigned char *parent, unsigned int mode,
 	struct combine_diff_state state;
 	unsigned long sz;
 
-	if (!cnt)
+	if (result_deleted)
 		return; /* result deleted */
 
 	parent_file.ptr = grab_blob(parent, mode, &sz);
@@ -517,7 +517,7 @@ static void show_line_to_eol(const char *line, int len, const char *reset)
 }
 
 static void dump_sline(struct sline *sline, unsigned long cnt, int num_parent,
-		       int use_color)
+		       int use_color, int result_deleted)
 {
 	unsigned long mark = (1UL<<num_parent);
 	unsigned long no_pre_delete = (2UL<<num_parent);
@@ -530,7 +530,7 @@ static void dump_sline(struct sline *sline, unsigned long cnt, int num_parent,
 	const char *c_plain = diff_get_color(use_color, DIFF_PLAIN);
 	const char *c_reset = diff_get_color(use_color, DIFF_RESET);
 
-	if (!cnt)
+	if (result_deleted)
 		return; /* result deleted */
 
 	while (1) {
@@ -687,6 +687,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 {
 	struct diff_options *opt = &rev->diffopt;
 	unsigned long result_size, cnt, lno;
+	int result_deleted = 0;
 	char *result, *cp;
 	struct sline *sline; /* survived lines */
 	int mode_differs = 0;
@@ -767,6 +768,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 		}
 		else {
 		deleted_file:
+			result_deleted = 1;
 			result_size = 0;
 			elem->mode = 0;
 			result = xcalloc(1, 1);
@@ -823,7 +825,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			combine_diff(elem->parent[i].sha1,
 				     elem->parent[i].mode,
 				     &result_file, sline,
-				     cnt, i, num_parent);
+				     cnt, i, num_parent, result_deleted);
 		if (elem->parent[i].mode != elem->mode)
 			mode_differs = 1;
 	}
@@ -889,7 +891,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			dump_quoted_path("+++ ", b_prefix, elem->path,
 					 c_meta, c_reset);
 		dump_sline(sline, cnt, num_parent,
-			   DIFF_OPT_TST(opt, COLOR_DIFF));
+			   DIFF_OPT_TST(opt, COLOR_DIFF), result_deleted);
 	}
 	free(result);
 
diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh
index 7584efa..09e8c8a 100755
--- a/t/t4038-diff-combined.sh
+++ b/t/t4038-diff-combined.sh
@@ -81,4 +81,12 @@ test_expect_success 'check combined output (2)' '
 	verify_helper sidesansone
 '
 
+test_expect_success 'diagnose truncated file' '
+	: > file &&
+	git add file &&
+	git commit --amend -C HEAD &&
+	git show > out &&
+	grep "diff --cc file" out
+'
+
 test_done
-- 
1.7.1.rc1.265.g77471

Re: Possible bug in Git

[Avery Pennarun]

On Thu, Apr 15, 2010 at 6:47 AM, David Martínez Martí
<desarrollo@gestiweb.com> wrote:
> $ git show dac6a95f5 -p --pretty=raw --
> facturacion/facturacion/scripts/lineasfacturascli.qs
> commit dac6a95f559604978ff9e1fac24ad752c54382ae
> tree f1472f1d289f9813df5fd691c9cea6ccb04313a7
> parent 5dfdd78f24f4578cdfeed876b8bacb45790af9ef
> parent 6ed54c8c6508b6d5551290b45785eaf44dd05d75
> author David Martinez Marti <desarrollo@gestiweb.com> 1260959309 +0100
> committer David Martinez Marti <desarrollo@gestiweb.com> 1260959309 +0100
>
>    Merge remote branch 'origin/infosial'
>
>    Conflicts:
>       [...]
>    [empty diff]
>
> There was a conflict on that file but it doesn't show anything changed about
> it. Gitk does the same.

You're trying to show the log of a merge commit.  git tries to be
clever about showing diffs on such commits, but it screws up rather
too often to be reliable, and this bites a lot of people (including
myself).

The core of the problem is this: if you're looking at 'git log -p',
you'll see all the individual commits.  But a merge commit is
essentially a combination of all the commits on branch #2 as you add
them to branch #1.  (Or alternatively: all the commits on branch #1 as
you add them to branch #2.)  So if git did the obvious thing and just
showed the *entire* diff that the merge applies to branch #1, it would
be a big long patch, but worse, it would be *equivalent* to the
patches you've already seen from other individual commits.

So what git tries to do is reduce the diff output of a merge commit
down to just the "unexpected" parts: the parts that are different from
what you'd get if you just merged in the individual changes in the
obvious way.  Unfortunately, actually calculating that would be really
complicated; it would require redoing the whole merge just to find out
what happened, and that turns out to be too slow.  So git uses
heuristics to try to guess what happened, and those heuristics don't
always work out.  In your case, it seems to have decided that "nothing
special happened" during the merge even though apparently the file was
entirely blanked out, which seems obviously wrong to me, but I don't
actually know how the heuristics work.

The "git log" options -c and --cc are documented (in 'man git-log') as
supposedly affecting this behaviour.  However, it seems that "--cc"
became the default somehow, and there's no obvious way to turn it back
to the "supposedly default" plain or -c behaviour.  Maybe someone else
on the list can comment more.

Have fun,

Avery

Re: Possible bug in Git

[Junio C Hamano]

Avery Pennarun <apenwarr@gmail.com> writes:

> So what git tries to do is reduce the diff output of a merge commit
> down to just the "unexpected" parts: the parts that are different from
> what you'd get if you just merged in the individual changes in the
> obvious way.

This (and everything you said after this paragraph) is not quite correct.
What is shown is the pieces that do not match either of the parent.

So if you have a conflicted merge and resolve it by taking what one side
did literally (think: "merge -X ours"), that hunk becomes uninteresting
because the end result matches what one of the parents had.

Re: Possible bug in Git

[Avery Pennarun]

On Thu, Apr 15, 2010 at 8:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Avery Pennarun <apenwarr@gmail.com> writes:
>
>> So what git tries to do is reduce the diff output of a merge commit
>> down to just the "unexpected" parts: the parts that are different from
>> what you'd get if you just merged in the individual changes in the
>> obvious way.
>
> This (and everything you said after this paragraph) is not quite correct.
> What is shown is the pieces that do not match either of the parent.
>
> So if you have a conflicted merge and resolve it by taking what one side
> did literally (think: "merge -X ours"), that hunk becomes uninteresting
> because the end result matches what one of the parents had.

Hmm, but David also said:

>>> git show commit:path shows empty file:
>>> $ git show dac6a95f5:facturacion/facturacion/scripts/lineasfacturascli.qs | wc
>>>     0       0       0

The output seems to match neither parent in any way, and yet the
simplification has resulted in no diff at all.  Strange, no?

Avery

Re: Possible bug in Git

[Avery Pennarun]

On Thu, Apr 15, 2010 at 8:06 PM, Avery Pennarun <apenwarr@gmail.com> wrote:
> On Thu, Apr 15, 2010 at 8:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Avery Pennarun <apenwarr@gmail.com> writes:
>>
>>> So what git tries to do is reduce the diff output of a merge commit
>>> down to just the "unexpected" parts: the parts that are different from
>>> what you'd get if you just merged in the individual changes in the
>>> obvious way.
>>
>> This (and everything you said after this paragraph) is not quite correct.
>> What is shown is the pieces that do not match either of the parent.
>>
>> So if you have a conflicted merge and resolve it by taking what one side
>> did literally (think: "merge -X ours"), that hunk becomes uninteresting
>> because the end result matches what one of the parents had.
>
> Hmm, but David also said:
>
>>>> git show commit:path shows empty file:
>>>> $ git show dac6a95f5:facturacion/facturacion/scripts/lineasfacturascli.qs | wc
>>>>     0       0       0
>
> The output seems to match neither parent in any way, and yet the
> simplification has resulted in no diff at all.  Strange, no?
>
> Avery
>

 mkdir testy
 cd testy
 git init
 cp /etc/profile .
 git add profile
 git commit -m root
 git checkout -b a master
 cat /etc/profile >>profile
 git commit -a -m test1
 git checkout -b b master
 cat /etc/profile | tr A-Za-z a-zA-Z >profile
 git commit -a -m test1b
 git merge a
   # produces a conflict
 true >profile   # blank the file
 git commit -a -m resolved
 git show HEAD:profile
   # no output

Note that if you instead replace the file with a nonempty (but
different) file, you get something appropriate as the output.

Have fun,

Avery

Re: Possible bug in Git

[Jay Soffian]

On Thu, Apr 15, 2010 at 8:13 PM, Avery Pennarun <apenwarr@gmail.com> wrote:
>  true >profile   # blank the file
>  git commit -a -m resolved
>  git show HEAD:profile
>   # no output

Well, I wouldn't expect output from show HEAD:profile. But this is odd::

$ git diff HEAD^1 --name-status
M       profile

$ git diff HEAD^2 --name-status
M       profile

$ git diff-tree HEAD --name-status
<no output>

Hmfph. "git log -m -p -1" at least gives the diffs from each side.

j.

Re: Possible bug in Git

[Avery Pennarun]

On Thu, Apr 15, 2010 at 8:38 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
> On Thu, Apr 15, 2010 at 8:13 PM, Avery Pennarun <apenwarr@gmail.com> wrote:
>>  true >profile   # blank the file
>>  git commit -a -m resolved
>>  git show HEAD:profile
>>   # no output
>
> Well, I wouldn't expect output from show HEAD:profile. But this is odd::
>
> $ git diff HEAD^1 --name-status
> M       profile
>
> $ git diff HEAD^2 --name-status
> M       profile
>
> $ git diff-tree HEAD --name-status
> <no output>
>
> Hmfph. "git log -m -p -1" at least gives the diffs from each side.

Whoops, actually I meant "git show HEAD profile".  Totally different,
of course.  But it doesn't produce output either :)

Have fun,

Avery

Re: Possible bug in Git

[Linus Torvalds]

On Thu, 15 Apr 2010, Avery Pennarun wrote:
> >
> > The output seems to match neither parent in any way, and yet the
> > simplification has resulted in no diff at all.  Strange, no?
> 
>  mkdir testy
>  cd testy
>  git init
>  cp /etc/profile .
>  git add profile
>  git commit -m root
>  git checkout -b a master
>  cat /etc/profile >>profile
>  git commit -a -m test1
>  git checkout -b b master
>  cat /etc/profile | tr A-Za-z a-zA-Z >profile
>  git commit -a -m test1b
>  git merge a
>    # produces a conflict
>  true >profile   # blank the file
>  git commit -a -m resolved
>  git show HEAD:profile
>    # no output
> 
> Note that if you instead replace the file with a nonempty (but
> different) file, you get something appropriate as the output.

Hmm. That does seem to be a bug. It's clearly an evil merge that doesn't 
match either side, and we should show it as such.

And yes, changing the

	true >profile

to

	echo 1 >profile

changes it to suddenly show it as such.

I guess the issue is that combine-diff decides that there is nothing in 
the result that isn't in either of the sources (because there is nothing 
at all in the result), and as such it's not "interesting" after all.

		Linus

Re: Possible bug in Git

[Thomas Rast]

Linus Torvalds wrote:
> I guess the issue is that combine-diff decides that there is nothing in 
> the result that isn't in either of the sources (because there is nothing 
> at all in the result), and as such it's not "interesting" after all.

It does give me a warm and fuzzy feeling when you are devising and
discussing test cases for a bug that I believe I fixed before this
branch of the thread even began:

  http://article.gmane.org/gmane.comp.version-control.git/144982

:-)

[scnr]

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: Possible bug in Git

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> I guess the issue is that combine-diff decides that there is nothing in 
> the result that isn't in either of the sources (because there is nothing 
> at all in the result), and as such it's not "interesting" after all.

There was already a "fix" posted for the "empty vs missing" confusion.

From: Thomas Rast <trast@student.ethz.ch>
Subject: [PATCH] combined diff: correctly handle truncated file
Date: Thu, 15 Apr 2010 14:59:37 +0200
Message-ID:
 <884b9b68a4478aceda580299c059a9a67417cb1c.1271336073.git.trast@student.ethz.ch>

I've been busy for the past few days, but I have it in my 'to-maint'
queue.  We may actually want to do the same for "missing" case as well,
though.

Re: Possible bug in Git

[Thomas Rast]

Junio C Hamano wrote:
> There was already a "fix" posted for the "empty vs missing" confusion.
> 
> From: Thomas Rast <trast@student.ethz.ch>
> Subject: [PATCH] combined diff: correctly handle truncated file
> Date: Thu, 15 Apr 2010 14:59:37 +0200
> Message-ID:
>  <884b9b68a4478aceda580299c059a9a67417cb1c.1271336073.git.trast@student.ethz.ch>
> 
> I've been busy for the past few days, but I have it in my 'to-maint'
> queue.  We may actually want to do the same for "missing" case as well,
> though.

Why?  The evil deletion is already reported as

  diff --cc foo
  index 5716ca5,7601807..0000000
  deleted file mode 100644,100644
  --- a/foo
  +++ /dev/null

with no diff -- but the absence of a diff is because of your 4462731
(combine-diff: do not punt on removed or added files., 2006-02-06) and
I tend to agree with the reasoning.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch