# bug: transform a binary file into a symlink in one commit => invalid binary patch 7 messages from 2009-01-23 to 2009-01-28. Participants: Pixel, Michael J Gruber, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/17327 ## Pixel, 2009-01-23 12:25 Subject: bug: transform a binary file into a symlink in one commit => invalid binary patch Message-ID: URL: https://gitlist.dev/e/lyhc3q9pl1.fsf%40leia.mandriva.com ``` 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 ---------- ``` ## Michael J Gruber, 2009-01-23 13:13 Subject: Re: bug: transform a binary file into a symlink in one commit => invalid binary patch Message-ID: <4979C295.1000803@drmicha.warpmail.net> URL: https://gitlist.dev/e/4979C295.1000803%40drmicha.warpmail.net In-Reply-To: ``` 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 ``` ## Jeff King, 2009-01-26 00:35 Subject: Re: bug: transform a binary file into a symlink in one commit => invalid binary patch Message-ID: <20090126003556.GA19368@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090126003556.GA19368%40coredump.intra.peff.net In-Reply-To: ``` 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 ``` ## Junio C Hamano, 2009-01-26 07:37 Subject: Re: bug: transform a binary file into a symlink in one commit => invalid binary patch Message-ID: <7vy6wy8qmm.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vy6wy8qmm.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20090126003556.GA19368@coredump.intra.peff.net> ``` Jeff King 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. ``` ## Junio C Hamano, 2009-01-26 08:33 Subject: [PATCH] diff.c: output correct index lines for a split diff Message-ID: <7vhc3m8o0b.fsf_-_@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vhc3m8o0b.fsf_-_%40gitster.siamese.dyndns.org In-Reply-To: <7vy6wy8qmm.fsf@gitster.siamese.dyndns.org> ``` 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 --- Junio C Hamano writes: > Jeff King 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 ``` ## Jeff King, 2009-01-26 09:07 Subject: Re: [PATCH] diff.c: output correct index lines for a split diff Message-ID: <20090126090712.GA12648@coredump.intra.peff.net> URL: https://gitlist.dev/e/20090126090712.GA12648%40coredump.intra.peff.net In-Reply-To: <7vhc3m8o0b.fsf_-_@gitster.siamese.dyndns.org> ``` 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 > 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 && ``` ## Junio C Hamano, 2009-01-28 21:32 Subject: Re: [PATCH] diff.c: output correct index lines for a split diff Message-ID: <7vr62nktgr.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vr62nktgr.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20090126090712.GA12648@coredump.intra.peff.net> ``` Jeff King 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 > >> 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. ```