Possible bug in Git

(Please add [email protected] and [email protected] 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 <[email protected]> 1260959309 +0100
committer David Martinez Marti <[email protected]> 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                : [email protected]
 ***************************************************************************/
/***************************************************************************
 *                                                                         *
 *   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                : [email protected]
 ***************************************************************************/
/***************************************************************************
 *                                                                         *
 *   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.

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.

---
 combine-diff.c           |   14 ++++++++------
 t/t4038-diff-combined.sh |    8 ++++++++
 2 files changed, 16 insertions(+), 6 deletions(-)

index 6162691..3480dae 100644
@@ -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);
 

index 7584efa..09e8c8a 100755
@@ -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

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

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.

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

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

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

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

From: Thomas Rast <[email protected]>
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

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.

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

Have fun,

Avery