bug: transform a binary file into a symlink in one commit => invalid binary patch

bug: transform a binary file into a symlink in one commit => invalid binary patch

[Pixel]

hi, 

i hit a bug (git 1.6.1): when you transform a binary file into a
symlink in one commit, the binary patch can't be used in "git apply".
Is it a known issue?


reproducer: 

----------
#!/bin/sh -e

# the bug: if you transform a binary file into a symlink in one commit, 
# the binary patch can't be used in "git apply"
#
# nb: if you uncomment the "##" lines, the problem disappears
# since the first patch will empty the binary file then the non-binary file
# will be transformed into a symlink

rm -rf t t2
mkdir t
dd if=/dev/urandom of=t/a count=10
cp -r t t2

cd t
git init
git add .
git commit -m initial

##echo -n > a
##git commit -a -m foo
ln -sf zzz a
git commit -a -m foo2
git format-patch :/initial

cd ../t2
git apply ../t/*.patch

cd ..
rm -rf t t2
----------

Re: bug: transform a binary file into a symlink in one commit => invalid binary patch

[Michael J Gruber]

Pixel venit, vidit, dixit 23.01.2009 13:25:
> hi, 
> 
> i hit a bug (git 1.6.1): when you transform a binary file into a
> symlink in one commit, the binary patch can't be used in "git apply".
> Is it a known issue?
> 
> 
> reproducer: 
> 
> ----------
> #!/bin/sh -e
> 
> # the bug: if you transform a binary file into a symlink in one commit, 
> # the binary patch can't be used in "git apply"
> #
> # nb: if you uncomment the "##" lines, the problem disappears
> # since the first patch will empty the binary file then the non-binary file
> # will be transformed into a symlink
> 
> rm -rf t t2
> mkdir t
> dd if=/dev/urandom of=t/a count=10
> cp -r t t2
> 
> cd t
> git init
> git add .
> git commit -m initial
> 
> ##echo -n > a
> ##git commit -a -m foo
> ln -sf zzz a
> git commit -a -m foo2
> git format-patch :/initial
> 
> cd ../t2
> git apply ../t/*.patch
> 
> cd ..
> rm -rf t t2
> ----------

You're not even initialising a repo in t2. I assume you want to "cp -a t
t2" after the "initial" commit, don't you? Doing that I get

error: binary patch to 'a' creates incorrect result (expecting
e132db255c0e0e3e9309c3c58a0a9c9eb97dd942, got
e69de29bb2d1d6434b8b29ae775ad8c2e48c5391)
error: a: patch does not apply
error: a: already exists in working directory

which is not that magnificient either. If that's the error you're after
your test script needs to be modified.

Michael

Re: bug: transform a binary file into a symlink in one commit => invalid binary patch

[Jeff King]

On Fri, Jan 23, 2009 at 01:25:30PM +0100, Pixel wrote:

> i hit a bug (git 1.6.1): when you transform a binary file into a
> symlink in one commit, the binary patch can't be used in "git apply".
> Is it a known issue?

Not that I know of.

Below is a patch against the test suite that fairly neatly displays the
problem. I didn't get a chance to look into actually fixing it, though
(I'm not even sure the problem is in apply, and not in the generated
patch).

---
diff --git a/t/t4130-apply-symlink-binary.sh b/t/t4130-apply-symlink-binary.sh
new file mode 100755
index 0000000..0ee2ba1
--- /dev/null
+++ b/t/t4130-apply-symlink-binary.sh
@@ -0,0 +1,30 @@
+#!/bin/sh
+
+test_description='apply handles binary to symlink conversion'
+. ./test-lib.sh
+
+test_expect_success 'create commit with binary' '
+	echo content >file && git add file &&
+	printf "\0" > binary && git add binary &&
+	git commit -m one
+'
+
+test_expect_success 'convert binary to symlink' '
+	rm binary &&
+	ln -s file binary &&
+	git add binary &&
+	git commit -m two
+'
+
+test_expect_success 'create patch' '
+	git diff-tree --binary HEAD^ HEAD >patch
+'
+
+test_expect_success 'apply patch' '
+	git reset --hard HEAD^ &&
+	git apply patch &&
+	test -h binary &&
+	test_cmp binary file
+'
+
+test_done

Re: bug: transform a binary file into a symlink in one commit => invalid binary patch

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jan 23, 2009 at 01:25:30PM +0100, Pixel wrote:
>
>> i hit a bug (git 1.6.1): when you transform a binary file into a
>> symlink in one commit, the binary patch can't be used in "git apply".
>> Is it a known issue?
>
> Not that I know of.
>
> Below is a patch against the test suite that fairly neatly displays the
> problem. I didn't get a chance to look into actually fixing it, though
> (I'm not even sure the problem is in apply, and not in the generated
> patch).

The generated diff is wrong.

A filepair that changes type must be split into deletion followed by
creation, which means the "index" line should say 0{40} on the right hand
side for the first half and then 0{40} on the left hand side for the
second half.  The patch generated by this step:

> +test_expect_success 'create patch' '
> +	git diff-tree --binary HEAD^ HEAD >patch
> +'

However says the blob contents change from "\0" to "file" on both.

This is because diff.c::run_diff() computes "index" only once and reuses
it for both halves.

[PATCH] diff.c: output correct index lines for a split diff

[Junio C Hamano]

A patch that changes the filetype (e.g. regular file to symlink) of a path
must be split into a deletion event followed by a creation event, which
means that we need to have two independent metainfo lines for each.
However, the code reused the single set of metainfo lines.

As the blob object names recorded on the index lines are usually not used
nor validated on the receiving end, this is not an issue with normal use
of the resulting patch.  However, when accepting a binary patch to delete
a blob, git-apply verified that the postimage blob object name on the
index line is 0{40}, hence a patch that deletes a regular file blob that
records binary contents to create a blob with different filetype (e.g. a
symbolic link) failed to apply.  "git am -3" also uses the blob object
names recorded on the index line, so it would also misbehave when
synthesizing a preimage tree.

This moves the code to generate metainfo lines around, so that two
independent sets of metainfo lines are used for the split halves.

The fix revealed that one test expected the incorrect behaviour, which is
also fixed here.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

  Junio C Hamano <gitster@pobox.com> writes:

  > Jeff King <peff@peff.net> writes:
  > ...
  >> Below is a patch against the test suite that fairly neatly displays the
  >> problem. I didn't get a chance to look into actually fixing it, though
  >> (I'm not even sure the problem is in apply, and not in the generated
  >> patch).
  >
  > The generated diff is wrong.
  >
  > A filepair that changes type must be split into deletion followed by
  > creation, which means the "index" line should say 0{40} on the right hand
  > side for the first half and then 0{40} on the left hand side for the
  > second half.  The patch generated by this step:
  >
  >> +test_expect_success 'create patch' '
  >> +	git diff-tree --binary HEAD^ HEAD >patch
  >> +'
  >
  > However says the blob contents change from "\0" to "file" on both.
  >
  > This is because diff.c::run_diff() computes "index" only once and reuses
  > it for both halves.

I did not include your new test script here; perhaps we can add it to an
existing typechange diff/apply test, like t4114?

 diff.c                   |  146 +++++++++++++++++++++++++---------------------
 t/t4030-diff-textconv.sh |    2 +-
 2 files changed, 81 insertions(+), 67 deletions(-)

diff --git a/diff.c b/diff.c
index 972b3da..7e18364 100644
--- a/diff.c
+++ b/diff.c
@@ -2081,16 +2081,86 @@ static void run_external_diff(const char *pgm,
 	}
 }
 
+static int similarity_index(struct diff_filepair *p)
+{
+	return p->score * 100 / MAX_SCORE;
+}
+
+static void fill_metainfo(struct strbuf *msg,
+			  const char *name,
+			  const char *other,
+			  struct diff_filespec *one,
+			  struct diff_filespec *two,
+			  struct diff_options *o,
+			  struct diff_filepair *p)
+{
+	strbuf_init(msg, PATH_MAX * 2 + 300);
+	switch (p->status) {
+	case DIFF_STATUS_COPIED:
+		strbuf_addf(msg, "similarity index %d%%", similarity_index(p));
+		strbuf_addstr(msg, "\ncopy from ");
+		quote_c_style(name, msg, NULL, 0);
+		strbuf_addstr(msg, "\ncopy to ");
+		quote_c_style(other, msg, NULL, 0);
+		strbuf_addch(msg, '\n');
+		break;
+	case DIFF_STATUS_RENAMED:
+		strbuf_addf(msg, "similarity index %d%%", similarity_index(p));
+		strbuf_addstr(msg, "\nrename from ");
+		quote_c_style(name, msg, NULL, 0);
+		strbuf_addstr(msg, "\nrename to ");
+		quote_c_style(other, msg, NULL, 0);
+		strbuf_addch(msg, '\n');
+		break;
+	case DIFF_STATUS_MODIFIED:
+		if (p->score) {
+			strbuf_addf(msg, "dissimilarity index %d%%\n",
+				    similarity_index(p));
+			break;
+		}
+		/* fallthru */
+	default:
+		/* nothing */
+		;
+	}
+	if (one && two && hashcmp(one->sha1, two->sha1)) {
+		int abbrev = DIFF_OPT_TST(o, FULL_INDEX) ? 40 : DEFAULT_ABBREV;
+
+		if (DIFF_OPT_TST(o, BINARY)) {
+			mmfile_t mf;
+			if ((!fill_mmfile(&mf, one) && diff_filespec_is_binary(one)) ||
+			    (!fill_mmfile(&mf, two) && diff_filespec_is_binary(two)))
+				abbrev = 40;
+		}
+		strbuf_addf(msg, "index %.*s..%.*s",
+			    abbrev, sha1_to_hex(one->sha1),
+			    abbrev, sha1_to_hex(two->sha1));
+		if (one->mode == two->mode)
+			strbuf_addf(msg, " %06o", one->mode);
+		strbuf_addch(msg, '\n');
+	}
+	if (msg->len)
+		strbuf_setlen(msg, msg->len - 1);
+}
+
 static void run_diff_cmd(const char *pgm,
 			 const char *name,
 			 const char *other,
 			 const char *attr_path,
 			 struct diff_filespec *one,
 			 struct diff_filespec *two,
-			 const char *xfrm_msg,
+			 struct strbuf *msg,
 			 struct diff_options *o,
-			 int complete_rewrite)
+			 struct diff_filepair *p)
 {
+	const char *xfrm_msg = NULL;
+	int complete_rewrite = (p->status == DIFF_STATUS_MODIFIED) && p->score;
+
+	if (msg) {
+		fill_metainfo(msg, name, other, one, two, o, p);
+		xfrm_msg = msg->len ? msg->buf : NULL;
+	}
+
 	if (!DIFF_OPT_TST(o, ALLOW_EXTERNAL))
 		pgm = NULL;
 	else {
@@ -2130,11 +2200,6 @@ static void diff_fill_sha1_info(struct diff_filespec *one)
 		hashclr(one->sha1);
 }
 
-static int similarity_index(struct diff_filepair *p)
-{
-	return p->score * 100 / MAX_SCORE;
-}
-
 static void strip_prefix(int prefix_length, const char **namep, const char **otherp)
 {
 	/* Strip the prefix but do not molest /dev/null and absolute paths */
@@ -2148,13 +2213,11 @@ static void run_diff(struct diff_filepair *p, struct diff_options *o)
 {
 	const char *pgm = external_diff();
 	struct strbuf msg;
-	char *xfrm_msg;
 	struct diff_filespec *one = p->one;
 	struct diff_filespec *two = p->two;
 	const char *name;
 	const char *other;
 	const char *attr_path;
-	int complete_rewrite = 0;
 
 	name  = p->one->path;
 	other = (strcmp(name, p->two->path) ? p->two->path : NULL);
@@ -2164,83 +2227,34 @@ static void run_diff(struct diff_filepair *p, struct diff_options *o)
 
 	if (DIFF_PAIR_UNMERGED(p)) {
 		run_diff_cmd(pgm, name, NULL, attr_path,
-			     NULL, NULL, NULL, o, 0);
+			     NULL, NULL, NULL, o, p);
 		return;
 	}
 
 	diff_fill_sha1_info(one);
 	diff_fill_sha1_info(two);
 
-	strbuf_init(&msg, PATH_MAX * 2 + 300);
-	switch (p->status) {
-	case DIFF_STATUS_COPIED:
-		strbuf_addf(&msg, "similarity index %d%%", similarity_index(p));
-		strbuf_addstr(&msg, "\ncopy from ");
-		quote_c_style(name, &msg, NULL, 0);
-		strbuf_addstr(&msg, "\ncopy to ");
-		quote_c_style(other, &msg, NULL, 0);
-		strbuf_addch(&msg, '\n');
-		break;
-	case DIFF_STATUS_RENAMED:
-		strbuf_addf(&msg, "similarity index %d%%", similarity_index(p));
-		strbuf_addstr(&msg, "\nrename from ");
-		quote_c_style(name, &msg, NULL, 0);
-		strbuf_addstr(&msg, "\nrename to ");
-		quote_c_style(other, &msg, NULL, 0);
-		strbuf_addch(&msg, '\n');
-		break;
-	case DIFF_STATUS_MODIFIED:
-		if (p->score) {
-			strbuf_addf(&msg, "dissimilarity index %d%%\n",
-					similarity_index(p));
-			complete_rewrite = 1;
-			break;
-		}
-		/* fallthru */
-	default:
-		/* nothing */
-		;
-	}
-
-	if (hashcmp(one->sha1, two->sha1)) {
-		int abbrev = DIFF_OPT_TST(o, FULL_INDEX) ? 40 : DEFAULT_ABBREV;
-
-		if (DIFF_OPT_TST(o, BINARY)) {
-			mmfile_t mf;
-			if ((!fill_mmfile(&mf, one) && diff_filespec_is_binary(one)) ||
-			    (!fill_mmfile(&mf, two) && diff_filespec_is_binary(two)))
-				abbrev = 40;
-		}
-		strbuf_addf(&msg, "index %.*s..%.*s",
-				abbrev, sha1_to_hex(one->sha1),
-				abbrev, sha1_to_hex(two->sha1));
-		if (one->mode == two->mode)
-			strbuf_addf(&msg, " %06o", one->mode);
-		strbuf_addch(&msg, '\n');
-	}
-
-	if (msg.len)
-		strbuf_setlen(&msg, msg.len - 1);
-	xfrm_msg = msg.len ? msg.buf : NULL;
-
 	if (!pgm &&
 	    DIFF_FILE_VALID(one) && DIFF_FILE_VALID(two) &&
 	    (S_IFMT & one->mode) != (S_IFMT & two->mode)) {
-		/* a filepair that changes between file and symlink
+		/*
+		 * a filepair that changes between file and symlink
 		 * needs to be split into deletion and creation.
 		 */
 		struct diff_filespec *null = alloc_filespec(two->path);
 		run_diff_cmd(NULL, name, other, attr_path,
-			     one, null, xfrm_msg, o, 0);
+			     one, null, &msg, o, p);
 		free(null);
+		strbuf_release(&msg);
+
 		null = alloc_filespec(one->path);
 		run_diff_cmd(NULL, name, other, attr_path,
-			     null, two, xfrm_msg, o, 0);
+			     null, two, &msg, o, p);
 		free(null);
 	}
 	else
 		run_diff_cmd(pgm, name, other, attr_path,
-			     one, two, xfrm_msg, o, complete_rewrite);
+			     one, two, &msg, o, p);
 
 	strbuf_release(&msg);
 }
diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh
index 2f27a0b..a3f0897 100755
--- a/t/t4030-diff-textconv.sh
+++ b/t/t4030-diff-textconv.sh
@@ -104,7 +104,7 @@ cat >expect.typechange <<'EOF'
 -1
 diff --git a/file b/file
 new file mode 120000
-index ad8b3d2..67be421
+index 0000000..67be421
 --- /dev/null
 +++ b/file
 @@ -0,0 +1 @@
-- 
1.6.1.1.248.g7f6d2

Re: [PATCH] diff.c: output correct index lines for a split diff

[Jeff King]

On Mon, Jan 26, 2009 at 12:33:56AM -0800, Junio C Hamano wrote:

> This moves the code to generate metainfo lines around, so that two
> independent sets of metainfo lines are used for the split halves.

The patch and the generated output look correct to me, so

Acked-by: Jeff King <peff@peff.net>

> I did not include your new test script here; perhaps we can add it to an
> existing typechange diff/apply test, like t4114?

I think it makes sense to add to t4114. Please squash in the test below.

One think to note, though: test 8 (binary -> symlink) shows breakage
with the current master, but test 9 (symlink -> binary) does not.
However, if you run the test under "-d" you can see that the diff has
bogus metainfo. It's just that "apply" doesn't care.

Your test fixes the test failure, and I verified manually that the diff
output is sensible. I don't think an additional test is worth it, but we
could add one that explicitly checks the metainfo if you want to be
extra paranoid.

---
diff --git a/t/t4114-apply-typechange.sh b/t/t4114-apply-typechange.sh
index 5533492..0f185ca 100755
--- a/t/t4114-apply-typechange.sh
+++ b/t/t4114-apply-typechange.sh
@@ -25,6 +25,10 @@ test_expect_success 'setup repository and commits' '
 	git update-index foo &&
 	git commit -m "foo back to file" &&
 	git branch foo-back-to-file &&
+	printf "\0" > foo &&
+	git update-index foo &&
+	git commit -m "foo becomes binary" &&
+	git branch foo-becomes-binary &&
 	rm -f foo &&
 	git update-index --remove foo &&
 	mkdir foo &&
@@ -85,6 +89,20 @@ test_expect_success 'symlink becomes file' '
 	'
 test_debug 'cat patch'
 
+test_expect_success 'binary file becomes symlink' '
+	git checkout -f foo-becomes-binary &&
+	git diff-tree -p --binary HEAD foo-symlinked-to-bar > patch &&
+	git apply --index < patch
+	'
+test_debug 'cat patch'
+
+test_expect_success 'symlink becomes binary file' '
+	git checkout -f foo-symlinked-to-bar &&
+	git diff-tree -p --binary HEAD foo-becomes-binary > patch &&
+	git apply --index < patch
+	'
+test_debug 'cat patch'
+
 
 test_expect_success 'symlink becomes directory' '
 	git checkout -f foo-symlinked-to-bar &&

Re: [PATCH] diff.c: output correct index lines for a split diff

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Jan 26, 2009 at 12:33:56AM -0800, Junio C Hamano wrote:
>
>> This moves the code to generate metainfo lines around, so that two
>> independent sets of metainfo lines are used for the split halves.
>
> The patch and the generated output look correct to me, so
>
> Acked-by: Jeff King <peff@peff.net>
>
>> I did not include your new test script here; perhaps we can add it to an
>> existing typechange diff/apply test, like t4114?
>
> I think it makes sense to add to t4114. Please squash in the test below.

Thanks.