stgit truncates binary files to zero length when applying patches

stgit truncates binary files to zero length when applying patches

[Karl Hasselström]

When applying patches and not fast-forwarding, stgit truncates the
binary files to zero length:

  $ cg-init .
  defaulting to local storage area
  Committing initial tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904
  Committed as 73161b6ee428ac8b1c1b16b560c40e13330693d2.

  $ stg init

  $ stg new foo
  Invoking the editor: "emacs .stgit.msg"... done (exit code: 0)

  $ cp /bin/bash .

  $ stg add bash

  $ stg refresh
  Refreshing patch "foo"... done

  $ ls -l
  total 584
  -rwxr-xr-x  1 kha vtech 593304 Nov 15 15:34 bash*

  $ stg pop
  Popping patch "foo"... done
  No patches applied

  $ stg new bar
  Invoking the editor: "emacs .stgit.msg"... done (exit code: 0)

  $ echo bar > bar.txt

  $ stg add bar.txt

  $ stg refresh
  Refreshing patch "bar"... done

  $ stg push foo
  Pushing patch "foo"... done
  Now at patch "foo"

  $ ls -l
  total 4
  -rw-r--r--  1 kha vtech 4 Nov 15 15:34 bar.txt
  -rwxr-xr-x  1 kha vtech 0 Nov 15 15:35 bash*

Without the "bar" patch, popping and then pushing "foo" works as
expected.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: stgit truncates binary files to zero length when applying patches

[Catalin Marinas]

On 15/11/05, Karl Hasselström <kha@treskal.com> wrote:
> When applying patches and not fast-forwarding, stgit truncates the
> binary files to zero length:

I've never tried binaries with StGIT before. When pushing a patch, if
a merge is needed (like in your case, the base of the foo patch has
changed), StGIT first tries "git-diff-tree | git-apply" for speed
reasons. If this fails, it falls back to a three-way merge.

Unfortunately, git-apply doesn't fail for patches including binary
files and simply creates an empty file. I think git-apply should be
changed to fail to apply this kind of patches.

--
Catalin

Re: stgit truncates binary files to zero length when applying patches

[Karl Hasselström]

On 2005-11-16 11:11:56 +0000, Catalin Marinas wrote:

> On 15/11/05, Karl Hasselström <kha@treskal.com> wrote:
>
> > When applying patches and not fast-forwarding, stgit truncates the
> > binary files to zero length:
>
> I've never tried binaries with StGIT before.

I don't blame you. Binary patches aren't something I normally create
either. It's just that I find stgit patches a good way to logically
structure a largeish change that I'm working on before committing it.
(I could probably accoplish the same thing with one branch instead of
each stgit patch, but then it would be quite a lot of work to manually
push updates through all the branches.)

> When pushing a patch, if a merge is needed (like in your case, the
> base of the foo patch has changed), StGIT first tries "git-diff-tree
> | git-apply" for speed reasons. If this fails, it falls back to a
> three-way merge.
>
> Unfortunately, git-apply doesn't fail for patches including binary
> files and simply creates an empty file. I think git-apply should be
> changed to fail to apply this kind of patches.

Yes, at least if stgit is going to continue to use it like this.
Refusing to handle binary files is somewhat disappointing, but still
OK; agreeing to handle them and then silently wiping them is a bit
less OK. (But don't worry; it is a perfect world, after all, so of
course I had backups. :-)

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: stgit truncates binary files to zero length when applying patches

[Catalin Marinas]

On 16/11/05, Karl Hasselström <kha@treskal.com> wrote:
> On 2005-11-16 11:11:56 +0000, Catalin Marinas wrote:
> > Unfortunately, git-apply doesn't fail for patches including binary
> > files and simply creates an empty file. I think git-apply should be
> > changed to fail to apply this kind of patches.
>
> Yes, at least if stgit is going to continue to use it like this.
> Refusing to handle binary files is somewhat disappointing, but still
> OK; agreeing to handle them and then silently wiping them is a bit
> less OK.

A workaround for this would be to add a config option for StGIT to
always use the three-way merge for pushing patches. The problem with
this is speed since git-diff-tree | git-apply is much faster (and
pretty safe since fuzzy patching is not allowed) and most of the
patches would apply cleanly with only this.

--
Catalin

Re: stgit truncates binary files to zero length when applying patches

[Karl Hasselström]

On 2005-11-16 12:31:27 +0000, Catalin Marinas wrote:

> On 16/11/05, Karl Hasselström <kha@treskal.com> wrote:
>
> > On 2005-11-16 11:11:56 +0000, Catalin Marinas wrote:
> >
> > > Unfortunately, git-apply doesn't fail for patches including
> > > binary files and simply creates an empty file. I think git-apply
> > > should be changed to fail to apply this kind of patches.
> >
> > Yes, at least if stgit is going to continue to use it like this.
> > Refusing to handle binary files is somewhat disappointing, but
> > still OK; agreeing to handle them and then silently wiping them is
> > a bit less OK.
>
> A workaround for this would be to add a config option for StGIT to
> always use the three-way merge for pushing patches. The problem with
> this is speed since git-diff-tree | git-apply is much faster (and
> pretty safe since fuzzy patching is not allowed) and most of the
> patches would apply cleanly with only this.

The proper fix has to be to convince git-apply to either handle
patches with binary files, or to make it fail; in both cases, stgit
will be fine. If the former is somehow intractable or undesirable, and
the latter would break existing callers (and/or inconvenience users),
perhaps it could fail on binary files only when a --text-only flag was
given.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: stgit truncates binary files to zero length when applying patches

[Junio C Hamano]

Catalin Marinas <catalin.marinas@gmail.com> writes:

> ... When pushing a patch, if
> a merge is needed (like in your case, the base of the foo patch has
> changed), StGIT first tries "git-diff-tree | git-apply" for speed
> reasons. If this fails, it falls back to a three-way merge.

I think many of the scripts rely on git-apply failing reliably
for unapplicable patches.  I'll do a new test script in git.git/t
and if it fails on binary files, try to fix it today.

Incidentally, for the last couple of days, I was working on
adding a very limited binary file diff support to "diff piped to
apply" pattern, and the result has been posted as "reworked
rebase" patches.  It is very limited in the sense that the diff
output does not attempt to be useful if the patch consumer does
not have both pre- and post-image blob, but for the use of
StGIT's internal patch replaying purposes that is not a concern,
so you might be interested in taking a look.

[PATCH] git-apply: fail if a patch cannot be applied.

[Junio C Hamano]

Recently we fixed 'git-apply --stat' not to barf on a binary
differences.  But it accidentally broke the error detection when
we actually attempt to apply them.

This commit fixes the problem and adds test cases.

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

 Junio C Hamano <junkio@cox.net> writes:

 > Catalin Marinas <catalin.marinas@gmail.com> writes:
 >
 >> ... When pushing a patch, if
 >> a merge is needed (like in your case, the base of the foo patch has
 >> changed), StGIT first tries "git-diff-tree | git-apply" for speed
 >> reasons. If this fails, it falls back to a three-way merge.
 >
 > I think many of the scripts rely on git-apply failing reliably
 > for unapplicable patches.  I'll do a new test script in git.git/t
 > and if it fails on binary files, try to fix it today.

 apply.c                 |   11 ++++---
 t/t4103-apply-binary.sh |   78 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 85 insertions(+), 4 deletions(-)
 create mode 100644 t/t4103-apply-binary.sh

applies-to: 4dda103301d7b70ec6e6e361d4a8d2cac24eccb4
92927ed0aac56a86f85049215791fcd106af4b62
diff --git a/apply.c b/apply.c
index 590adc6..a002e15 100644
--- a/apply.c
+++ b/apply.c
@@ -891,7 +891,7 @@ static int parse_chunk(char *buffer, uns
 
 	patchsize = parse_single_patch(buffer + offset + hdrsize, size - offset - hdrsize, patch);
 
-	if (!patchsize && !metadata_changes(patch)) {
+	if (!patchsize) {
 		static const char binhdr[] = "Binary files ";
 
 		if (sizeof(binhdr) - 1 < size - offset - hdrsize &&
@@ -899,9 +899,12 @@ static int parse_chunk(char *buffer, uns
 			    sizeof(binhdr)-1))
 			patch->is_binary = 1;
 
-		if (patch->is_binary && !apply && !check)
-			;
-		else
+		/* Empty patch cannot be applied if:
+		 * - it is a binary patch or
+		 * - metadata does not change and is not a binary patch.
+		 */
+		if ((apply || check) &&
+		    (patch->is_binary || !metadata_changes(patch)))
 			die("patch with only garbage at line %d", linenr);
 	}
 
diff --git a/t/t4103-apply-binary.sh b/t/t4103-apply-binary.sh
new file mode 100644
index 0000000..948d5b5
--- /dev/null
+++ b/t/t4103-apply-binary.sh
@@ -0,0 +1,78 @@
+#!/bin/sh
+#
+# Copyright (c) 2005 Junio C Hamano
+#
+
+test_description='git-apply handling binary patches
+
+'
+. ./test-lib.sh
+
+# setup
+
+cat >file1 <<EOF
+A quick brown fox jumps over the lazy dog.
+A tiny little penguin runs around in circles.
+There is a flag with Linux written on it.
+A slow black-and-white panda just sits there,
+munching on his bamboo.
+EOF
+cat file1 >file2
+cat file1 >file4
+
+git-update-index --add --remove file1 file2 file4
+git-commit -m 'Initial Version' 2>/dev/null
+
+git-checkout -b binary
+tr 'x' '\0' <file1 >file3
+cat file3 >file4
+git-add file2
+tr '\0' 'v' <file3 >file1
+rm -f file2
+git-update-index --add --remove file1 file2 file3 file4
+git-commit -m 'Second Version'
+
+git-diff-tree -p master binary >B.diff
+git-diff-tree -p -C master binary >C.diff
+
+test_expect_success 'stat binary diff -- should not fail.' \
+	'git-checkout master
+	 git-apply --stat --summary B.diff'
+
+test_expect_success 'stat binary diff (copy) -- should not fail.' \
+	'git-checkout master
+	 git-apply --stat --summary C.diff'
+
+test_expect_failure 'check binary diff -- should fail.' \
+	'git-checkout master
+	 git-apply --check B.diff'
+
+test_expect_failure 'check binary diff (copy) -- should fail.' \
+	'git-checkout master
+	 git-apply --check C.diff'
+
+# Now we start applying them.
+
+test_expect_failure 'apply binary diff -- should fail.' \
+	'git-checkout master
+	 git-apply B.diff'
+
+git-reset --hard
+
+test_expect_failure 'apply binary diff -- should fail.' \
+	'git-checkout master
+	 git-apply --index B.diff'
+
+git-reset --hard
+
+test_expect_failure 'apply binary diff (copy) -- should fail.' \
+	'git-checkout master
+	 git-apply C.diff'
+
+git-reset --hard
+
+test_expect_failure 'apply binary diff (copy) -- should fail.' \
+	'git-checkout master
+	 git-apply --index C.diff'
+
+test_done
---
0.99.9.GIT

master has some toys

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> Incidentally, for the last couple of days, I was working on
> adding a very limited binary file diff support to "diff piped to
> apply" pattern, and the result has been posted as "reworked
> rebase" patches.  It is very limited in the sense that the diff
> output does not attempt to be useful if the patch consumer does
> not have both pre- and post-image blob, but for the use of
> StGIT's internal patch replaying purposes that is not a concern,
> so you might be interested in taking a look.

Along with the git wrapper fixes and git-apply bugfix (it did
not fail when it saw unapplicable binary patches), and
cvsexportcommit fixes from Kevin Geiss, I have the "limited
binary patch support" on the master branch.  The reworked rebase
is still in proposed updates branch.

I'll be offline for a couple of hours chaffering my wife.

Re: master has some toys

[Alex Riesen]

On 11/17/05, Junio C Hamano <junkio@cox.net> wrote:
> Along with the git wrapper fixes and git-apply bugfix (it did

cygwin is completely broken. Still debugging, but it looks like the
old "windows can't unlink/rename open files" problem.

Re: master has some toys

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> cygwin is completely broken. Still debugging, but it looks like the
> old "windows can't unlink/rename open files" problem.

Ouch.  Sorry, and thanks for reporting.

Ideally I should get a cygwin environment myself, but for that
first I need to procure Windows box.  Or does cygwin run on
Wine, and if so is cygwin running on Wine a good enough
approximation of the real thing?

Re: master has some toys

[Alex Riesen]

On 11/17/05, Junio C Hamano <junkio@cox.net> wrote:
> > cygwin is completely broken. Still debugging, but it looks like the
> > old "windows can't unlink/rename open files" problem.
>
> Ouch.  Sorry, and thanks for reporting.

As it turned out, not the git.c is guilty, but the missing
NO_MMAP=YesPlease in Cygwin section. I had it for a long time and
accidentally removed by the recent pull. BTW, I couldn't find nowhere
on original branch. Was it never submitted?

> Ideally I should get a cygwin environment myself, but for that
> first I need to procure Windows box.  Or does cygwin run on
> Wine, and if so is cygwin running on Wine a good enough
> approximation of the real thing?

I don't know. One should try real hard to create as much junk as there is.

Re: master has some toys

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> As it turned out, not the git.c is guilty, but the missing
> NO_MMAP=YesPlease in Cygwin section. I had it for a long time and
> accidentally removed by the recent pull. BTW, I couldn't find nowhere
> on original branch. Was it never submitted?

Neither 'git-whatchanged Makefile' nor 'git-whatchanged
-SNO_MMAP Makefile' reports such on my end.  Do we need one?

Johannes said he tests on Cygwin as well, and I am sure there
are others with Cygin on the list.  Help us out here please?

Re: master has some toys

[John Benes]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Junio C Hamano wrote:
> Alex Riesen <raa.lkml@gmail.com> writes:
> 
>> As it turned out, not the git.c is guilty, but the missing
>> NO_MMAP=YesPlease in Cygwin section. I had it for a long time and
>> accidentally removed by the recent pull. BTW, I couldn't find nowhere
>> on original branch. Was it never submitted?
> 
> Neither 'git-whatchanged Makefile' nor 'git-whatchanged
> -SNO_MMAP Makefile' reports such on my end.  Do we need one?

The patch with the commented out option seems like it would work...

> Johannes said he tests on Cygwin as well, and I am sure there
> are others with Cygin on the list.  Help us out here please?


I was able to compile master and pu on Cygwin without NO_MMAP=YesPlease
in the Cygwin section.  However, the make test failed on the
binary-apply on both master and pu, output follows.

Commit ID's used for testing:
refs/heads/master	4e1da85d7d0480b6d9973317da4f7a5aa603fcb5
refs/heads/pu		3b4587eb3c549649af7e84659b4808003c34c2d3

make test barfing on master:
*** t4103-apply-binary.sh ***
*   ok 1: stat binary diff -- should not fail.
*   ok 2: stat binary diff (copy) -- should not fail.
*   ok 3: check binary diff -- should fail.
*   ok 4: check binary diff (copy) -- should fail.
*   ok 5: check incomplete binary diff with replacement -- should fail.
*   ok 6: check incomplete binary diff with replacement (copy) -- should
fail.
* FAIL 7: check binary diff with replacement.
        git-checkout master
                 git-apply --check --allow-binary-replacement BF.diff
* FAIL 8: check binary diff with replacement (copy).
        git-checkout master
                 git-apply --check --allow-binary-replacement CF.diff
*   ok 9: apply binary diff -- should fail.
*   ok 10: apply binary diff -- should fail.
*   ok 11: apply binary diff (copy) -- should fail.
*   ok 12: apply binary diff (copy) -- should fail.
*   ok 13: apply binary diff without replacement -- should fail.
*   ok 14: apply binary diff without replacement (copy) -- should fail.
* FAIL 15: apply binary diff.
        do_reset
                 git-apply --allow-binary-replacement --index BF.diff &&
                 test -z "$(git-diff --name-status binary)"
* FAIL 16: apply binary diff (copy).
        do_reset
                 git-apply --allow-binary-replacement --index CF.diff &&
                 test -z "$(git-diff --name-status binary)"
* failed 4 among 16 test(s)
make[1]: *** [t4103-apply-binary.sh] Error 1

make test barfing on pu:
*** t4103-apply-binary.sh ***
usage: git-diff-tree [--stdin] [-m] [-s] [-v] [--pretty] [-t] [-r]
[--root] [<co
mmon diff options>] <tree-ish> [<tree-ish>] [<path>...]
***SNIP***

usage: git-diff-tree [--stdin] [-m] [-s] [-v] [--pretty] [-t] [-r]
[--root] [<co
mmon diff options>] <tree-ish> [<tree-ish>] [<path>...]
***SNIP***

* FAIL 1: stat binary diff -- should not fail.
        git-checkout master
                 git-apply --stat --summary B.diff
* FAIL 2: stat binary diff (copy) -- should not fail.
        git-checkout master
                 git-apply --stat --summary C.diff
*   ok 3: check binary diff -- should fail.
*   ok 4: check binary diff (copy) -- should fail.
*   ok 5: check incomplete binary diff with replacement -- should fail.
*   ok 6: check incomplete binary diff with replacement (copy) -- should
fail.
* FAIL 7: check binary diff with replacement.
        git-checkout master
                 git-apply --check --allow-binary-replacement BF.diff
* FAIL 8: check binary diff with replacement (copy).
        git-checkout master
                 git-apply --check --allow-binary-replacement CF.diff
*   ok 9: apply binary diff -- should fail.
*   ok 10: apply binary diff -- should fail.
*   ok 11: apply binary diff (copy) -- should fail.
*   ok 12: apply binary diff (copy) -- should fail.
*   ok 13: apply binary diff without replacement -- should fail.
*   ok 14: apply binary diff without replacement (copy) -- should fail.
* FAIL 15: apply binary diff.
        do_reset
                 git-apply --allow-binary-replacement --index BF.diff &&
                 test -z "$(git-diff --name-status binary)"
* FAIL 16: apply binary diff (copy).
        do_reset
                 git-apply --allow-binary-replacement --index CF.diff &&
                 test -z "$(git-diff --name-status binary)"
* failed 6 among 16 test(s)
make[1]: *** [t4103-apply-binary.sh] Error 1

- --
John Benes
GPG Fingerprint: D519 25DB BB5C 38FC 9D02  02E7 596D BC50 F880 27FA
"It is not only the living who are killed in war." - Isaac Asimov
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iQIVAwUBQ30tE2F0oWcU9kCNAQKzRQ/8DEoZTRYDcU80mzddq8AS8ClctLx8CorY
OJYY/sTAivjot7R/bPUj97Ie+FaUKC0mpTOGZTT8KkSpRg2mbZ6YlTUoHAtf4iq3
F9vQs2qVJqyUJqbXcP+X68huIZF7vHZI47e9ExTO1RrxZCGxDp8JigfhmlS4CN4K
Dee1NSMnoWGklNEXvZMwkvhLerV+9Xs6UTGZ5AeRHRiHiLqK+I9eDNaULABHwAYO
JSsat0HDjKMKrgNNFo43TpRTm17gq7N83LOJJkxiNYnRPh8nGpIOgfNx8riWP5DB
tMw3FWvA9bv3tllvTFC4wx92Rgfs255cvXEunqfRTsMzGG7rhdg4UY92I0yB8k6b
g6nucu93VtwEkHZ1b9ZTBwz85ZuTImS/72pHbRPvOaeSuDArBI9i+Lwrb+z+0vUS
iMMuAxtWhKUFr87A6ljdu27IfE+pJaEgGEySGkquU3EexuNRMz62raveRsEpLSu8
iqbKoMeC1A7CtcbV3OBdktfFyM3rR8/G9OpjBNdkojo08BOvsoMCU45jLdXFIFtu
MGMajhGEu8EwAdItq6Qq0fXOfU6ph5oN4tg1pKt83QDkDFJyEJzOH1CMLh+qxXR3
m25wLSJcV8YbNfQVMumw2aFXn3ojwkP3kEheoTKexTFVkwGS0dU/j9kpQ8wj9Pqv
1ttxTdlX2HM=
=OthK
-----END PGP SIGNATURE-----

Re: master has some toys

[Johannes Schindelin]

Hi,

On Thu, 17 Nov 2005, John Benes wrote:

> I was able to compile master and pu on Cygwin without NO_MMAP=YesPlease
> in the Cygwin section.  However, the make test failed on the
> binary-apply on both master and pu, output follows.

Nobody doubts that you can *compile* it. The problem is the fixing of the 
mmap()ed regions after fork(). Since win32 is such a sane system, it does 
not provide mmap() or fork() out of the box. And under some very obscure 
circumstances, cygwin's emulation of mmap() and fork() fails.

BTW, it would be more helpful if you do not just tell *what* test fails, 
but *how*. For example, try to run "git-whatchanged -p" and send just 
the first page of the output (both stdout and stderr). I bet it says it 
has a problem fixing up mmap() after fork().

Hth,
Dscho

Re: master has some toys

[John Benes]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Johannes Schindelin wrote:
> BTW, it would be more helpful if you do not just tell *what* test fails, 
> but *how*. For example, try to run "git-whatchanged -p" and send just 
> the first page of the output (both stdout and stderr). I bet it says it 
> has a problem fixing up mmap() after fork().

I ran "git-whatchanged -p" with the previously mentioned master build
installed in /usr/bin.  Below is a copy of the command session.

Stdout was redirected to test2.txt, which is located at:
http://www.penguinlounge.1and2.org/videos/test2.txt.bz2

Twinkie@squirrel ~/git
$ git-whatchanged -p > ../test2.txt

Twinkie@squirrel ~/git
$ cd ..

Twinkie@squirrel ~
$ ls -la test2.txt
- -rw-r--r--  1 Twinkie None 9338905 Nov 17 21:23 test2.txt


nothing was displayed on stderr.


The make test error was shown on linux too. [1]
It reports as:
Twinkie@squirrel ~/git/t/trash
$ git-apply --check --allow-binary-replacement BF.diff
fatal: patch with only garbage at line 30

could it be an outdated diff? (2.8.7)

[1] Re: "make test" fails with current HEAD @ 9:23PM CST by A Large
Angry SCM

Anything else you want me to bash this Cygwin install against?

- --
John Benes
GPG Fingerprint: D519 25DB BB5C 38FC 9D02  02E7 596D BC50 F880 27FA
"It is not only the living who are killed in war." - Isaac Asimov
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iQIVAwUBQ31SF2F0oWcU9kCNAQKLUxAAtqOpSCsGSnNz+q9x8gzqaqjCK+mjjUkJ
YJFalTEeNO9/alp3mPGb4kXwhMahyugGqHZzr2Lp5JC0gDSsn5vqF2sxYtB9yvAg
1lg33awy5AI1LkIcbeiCvjkBI83UvIjOYLEs1YcIXKRDYJkcbo6yBN4uqkDZKAoS
O8L3q2r2/mCJQYlANy1ruhT0Al5xPwTFNv6JqIkm8PWsPE+es6ZwyFBXKi62CHu5
AWsc2nHDhPIx5YS1h7CIIQjiRh2qkf807IvXKZtY++o7yoHn49Rtu87VblZhkuH5
UHOSE0gDbT37MbIg6hoZc20kBZIq1s+6e+/FhrFAUrmnny9t7fPaU35gJmPPA63W
KajRKOwChmO6V5scTfgimxxzN4Jlnd+AM8FdxTWCfz6s4FLnnzukwX5T4PbWw6Kg
if8fekhKkPYCLQtAa8Q6Xe5GIZJ5ghVRfAnZWhc4p8OJj6gacwGCk/QX0Rxu6z3r
BF9e7RCyytwDw2Lw87NJFc7AYp4V4uINOAMMxPK0eidHOkkVVzu3eaoB8OIVzh5Q
oAwo+gPpd/PgFldJJu4i9e8bD8/J7cwZ0vpMkRDig9P1ZqFCj9t2+bAVzOgJPb80
4YG/UR4VwVuKyYDeLhCHNLYJBssNXCCkN4+UKuLcNsk6+pXzniPVAc2Wda5BMIj2
LV5oMRUCpKQ=
=NFuc
-----END PGP SIGNATURE-----

Re: master has some toys

[Junio C Hamano]

John Benes <smartcat99s@gmail.com> writes:

> Junio C Hamano wrote:
>
> I was able to compile master and pu on Cygwin without NO_MMAP=YesPlease
> in the Cygwin section.  However, the make test failed on the
> binary-apply on both master and pu, output follows.
>
> Commit ID's used for testing:
> refs/heads/master	4e1da85d7d0480b6d9973317da4f7a5aa603fcb5
> refs/heads/pu		3b4587eb3c549649af7e84659b4808003c34c2d3

Thanks.  But the test result look suspicious for pu.

	$ git-ls-tree 3b4587eb t | grep t4103

outputs empty, so what you tested does not seem to be that
commit.

Anyway, the master is more important at this point.

> make test barfing on master:
> *** t4103-apply-binary.sh ***
> * FAIL 7: check binary diff with replacement.
>         git-checkout master
>                  git-apply --check --allow-binary-replacement BF.diff
> * FAIL 8: check binary diff with replacement (copy).
>         git-checkout master
>                  git-apply --check --allow-binary-replacement CF.diff
> * FAIL 15: apply binary diff.
>         do_reset
>                  git-apply --allow-binary-replacement --index BF.diff &&
>                  test -z "$(git-diff --name-status binary)"
> * FAIL 16: apply binary diff (copy).
>         do_reset
>                  git-apply --allow-binary-replacement --index CF.diff &&
>                  test -z "$(git-diff --name-status binary)"
> * failed 4 among 16 test(s)
> make[1]: *** [t4103-apply-binary.sh] Error 1

So it fails on these binary diffs with full index tests.  Could
you try running it like this?

	$ cd t
        $ sh ./t4103-apply-binary.sh -i -v

If all things being equal, this will stop at the first failing
test "* FAIL 7: ", and you will have trash/ directory under t/.

	$ cd trash
	$ ls -l

I would first want to see if it was diff that failed or the
apply.  What does BF.diff contain?

Re: master has some toys

[A Large Angry SCM]

Junio C Hamano wrote:
> John Benes <smartcat99s@gmail.com> writes:
> 
>>Junio C Hamano wrote:
>>
>>I was able to compile master and pu on Cygwin without NO_MMAP=YesPlease
>>in the Cygwin section.  However, the make test failed on the
>>binary-apply on both master and pu, output follows.
>>
>>Commit ID's used for testing:
>>refs/heads/master	4e1da85d7d0480b6d9973317da4f7a5aa603fcb5
>>refs/heads/pu		3b4587eb3c549649af7e84659b4808003c34c2d3
> 
> Thanks.  But the test result look suspicious for pu.
> 
> 	$ git-ls-tree 3b4587eb t | grep t4103
> 
> outputs empty, so what you tested does not seem to be that
> commit.
> 
> Anyway, the master is more important at this point.
> 
>>make test barfing on master:
>>*** t4103-apply-binary.sh ***
>>* FAIL 7: check binary diff with replacement.
>>        git-checkout master
>>                 git-apply --check --allow-binary-replacement BF.diff
>>* FAIL 8: check binary diff with replacement (copy).
>>        git-checkout master
>>                 git-apply --check --allow-binary-replacement CF.diff
>>* FAIL 15: apply binary diff.
>>        do_reset
>>                 git-apply --allow-binary-replacement --index BF.diff &&
>>                 test -z "$(git-diff --name-status binary)"
>>* FAIL 16: apply binary diff (copy).
>>        do_reset
>>                 git-apply --allow-binary-replacement --index CF.diff &&
>>                 test -z "$(git-diff --name-status binary)"
>>* failed 4 among 16 test(s)
>>make[1]: *** [t4103-apply-binary.sh] Error 1
> 
> So it fails on these binary diffs with full index tests.  Could
> you try running it like this?
> 
> 	$ cd t
>         $ sh ./t4103-apply-binary.sh -i -v
> 
> If all things being equal, this will stop at the first failing
> test "* FAIL 7: ", and you will have trash/ directory under t/.
> 
> 	$ cd trash
> 	$ ls -l
> 
> I would first want to see if it was diff that failed or the
> apply.  What does BF.diff contain?
> 

OK, here's what I get:

 > sh ./t4103-apply-binary.sh -i -v
* expecting success: git-checkout master
          git-apply --stat --summary B.diff
  file1 |    4 ++--
  file2 |    5 -----
  file3 |    0
  file4 |    0
  4 files changed, 2 insertions(+), 7 deletions(-)
  delete mode 100644 file2
  create mode 100644 file3
*   ok 1: stat binary diff -- should not fail.
* expecting success: git-checkout master
          git-apply --stat --summary C.diff
  file1 |    4 ++--
  file2 |    5 -----
  file3 |    0
  file4 |    0
  4 files changed, 2 insertions(+), 7 deletions(-)
  delete mode 100644 file2
  copy file1 => file3 (70%)
*   ok 2: stat binary diff (copy) -- should not fail.
* expecting failure: git-checkout master
          git-apply --check B.diff
fatal: patch with only garbage at line 30
*   ok 3: check binary diff -- should fail.
* expecting failure: git-checkout master
          git-apply --check C.diff
fatal: patch with only garbage at line 32
*   ok 4: check binary diff (copy) -- should fail.
* expecting failure: git-checkout master
          git-apply --check --allow-binary-replacement B.diff
fatal: patch with only garbage at line 30
*   ok 5: check incomplete binary diff with replacement -- should fail.
* expecting failure: git-checkout master
          git-apply --check --allow-binary-replacement C.diff
fatal: patch with only garbage at line 32
*   ok 6: check incomplete binary diff with replacement (copy) -- should 
fail.
* expecting success: git-checkout master
          git-apply --check --allow-binary-replacement BF.diff
fatal: patch with only garbage at line 30
* FAIL 7: check binary diff with replacement.
         git-checkout master
                  git-apply --check --allow-binary-replacement BF.diff
 > cd trash
internet@Gojira:~/GIT/git/t/trash> ls -l
total 28
-rw-r--r--  1 internet internet  909 2005-11-17 19:47 B.diff
-rw-r--r--  1 internet internet 1173 2005-11-17 19:47 BF.diff
-rw-r--r--  1 internet internet  944 2005-11-17 19:47 C.diff
-rw-r--r--  1 internet internet 1208 2005-11-17 19:47 CF.diff
-rw-r--r--  1 internet internet  201 2005-11-17 19:47 file1
-rw-r--r--  1 internet internet  201 2005-11-17 19:47 file2
-rw-r--r--  1 internet internet  201 2005-11-17 19:47 file4
 > cat BF.diff
diff --git a/file1 b/file1
index 
edc575dec543a684da5007b43886ee32ecb381ae..af1eedd35be991f3ced320f7d927799c72cd8435 
100644
--- a/file1
+++ b/file1
@@ -1,5 +1,5 @@
-A quick brown fox jumps over the lazy dog.
+A quick brown fov jumps over the lazy dog.
  A tiny little penguin runs around in circles.
-There is a flag with Linux written on it.
+There is a flag with Linuv written on it.
  A slow black-and-white panda just sits there,
  munching on his bamboo.
diff --git a/file2 b/file2
deleted file mode 100644
index 
edc575dec543a684da5007b43886ee32ecb381ae..0000000000000000000000000000000000000000
--- a/file2
+++ /dev/null
@@ -1,5 +0,0 @@
-A quick brown fox jumps over the lazy dog.
-A tiny little penguin runs around in circles.
-There is a flag with Linux written on it.
-A slow black-and-white panda just sits there,
-munching on his bamboo.
diff --git a/file3 b/file3
new file mode 100644
index 
0000000000000000000000000000000000000000..adb07b7ad3fa2c63251b06d1d39cb90a85b860b4
Files /dev/null and b/file3 differ
diff --git a/file4 b/file4
index 
edc575dec543a684da5007b43886ee32ecb381ae..adb07b7ad3fa2c63251b06d1d39cb90a85b860b4 
100644
Files a/file4 and b/file4 differ

Re: master has some toys

[Junio C Hamano]

A Large Angry SCM <gitzilla@gmail.com> writes:

> Files /dev/null and b/file3 differ
> diff --git a/file4 b/file4
> index 
> edc575dec543a684da5007b43886ee32ecb381ae..adb07b7ad3fa2c63251b06d1d39cb90a85b860b4 
> 100644
> Files a/file4 and b/file4 differ

Thanks.  I've seen enough.  I expected diff (GNU diffutils 2.8.1
is what I have handy) output which says "Binary files a/foo and
b/foo differ".

Hmph.  Now I'd need to find a way to catch at least these two
cases...

[PATCH] Deal with binary diff output from (unknown version of) diff

[Junio C Hamano]

Some vintage of diff says just "Files X and Y differ\n", instead
of "Binary files X and Y differ\n", so catch both patterns.

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

    Junio C Hamano <junkio@cox.net> writes:

    >> Files /dev/null and b/file3 differ
    >> diff --git a/file4 b/file4
    >> index edc575d..adb07b7 100644
    >> Files a/file4 and b/file4 differ
    >
    > Thanks.  I've seen enough.  I expected diff (GNU diffutils 2.8.1
    > is what I have handy) output which says "Binary files a/foo and
    > b/foo differ".
    >
    > Hmph.  Now I'd need to find a way to catch at least these two
    > cases...

    Could you two try this patch please?

diff --git a/apply.c b/apply.c
index 129edb1..50be8f3 100644
--- a/apply.c
+++ b/apply.c
@@ -893,12 +893,24 @@ static int parse_chunk(char *buffer, uns
 	patchsize = parse_single_patch(buffer + offset + hdrsize, size - offset - hdrsize, patch);
 
 	if (!patchsize) {
-		static const char binhdr[] = "Binary files ";
-
-		if (sizeof(binhdr) - 1 < size - offset - hdrsize &&
-		    !memcmp(binhdr, buffer + hdrsize + offset,
-			    sizeof(binhdr)-1))
-			patch->is_binary = 1;
+		static const char *binhdr[] = {
+			"Binary files ",
+			"Files ",
+			NULL,
+		};
+		int i;
+		int hd = hdrsize + offset;
+		unsigned long llen = linelen(buffer + hd, size - hd);
+
+		if (!memcmp(" differ\n", buffer + hd + llen - 8, 8))
+			for (i = 0; binhdr[i]; i++) {
+				int len = strlen(binhdr[i]);
+				if (len < size - hd &&
+				    !memcmp(binhdr[i], buffer + hd, len)) {
+					patch->is_binary = 1;
+					break;
+				}
+			}
 
 		/* Empty patch cannot be applied if:
 		 * - it is a binary patch and we do not do binary_replace, or

Re: [PATCH] Deal with binary diff output from (unknown version of) diff

[A Large Angry SCM]

Passed here!


Junio C Hamano wrote:
> Some vintage of diff says just "Files X and Y differ\n", instead
> of "Binary files X and Y differ\n", so catch both patterns.
> 
> Signed-off-by: Junio C Hamano <junkio@cox.net>
> 
> ---
> 
>     Junio C Hamano <junkio@cox.net> writes:
> 
>     >> Files /dev/null and b/file3 differ
>     >> diff --git a/file4 b/file4
>     >> index edc575d..adb07b7 100644
>     >> Files a/file4 and b/file4 differ
>     >
>     > Thanks.  I've seen enough.  I expected diff (GNU diffutils 2.8.1
>     > is what I have handy) output which says "Binary files a/foo and
>     > b/foo differ".
>     >
>     > Hmph.  Now I'd need to find a way to catch at least these two
>     > cases...
> 
>     Could you two try this patch please?

[snip snip]

Re: master has some toys

[John Benes]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Junio C Hamano wrote:
> John Benes <smartcat99s AT gmail DOT com> writes:
>> Commit ID's used for testing:
>> refs/heads/master	4e1da85d7d0480b6d9973317da4f7a5aa603fcb5
>> refs/heads/pu		3b4587eb3c549649af7e84659b4808003c34c2d3
> 
> Thanks.  But the test result look suspicious for pu.

I verified that I was testing that commit, and the ls-tree results.
Must have been left over after switching from master...

> Anyway, the master is more important at this point.

> So it fails on these binary diffs with full index tests.  Could
> you try running it like this?
> 
> 	$ cd t
>         $ sh ./t4103-apply-binary.sh -i -v
> 
> If all things being equal, this will stop at the first failing
> test "* FAIL 7: ", and you will have trash/ directory under t/.
> 
> 	$ cd trash
> 	$ ls -l
> 
> I would first want to see if it was diff that failed or the
> apply.  What does BF.diff contain?
> 

Output from the commands requested:

Twinkie@squirrel ~/git/t
$ sh ./t4103-apply-binary.sh -i -v
sh ./t4103-apply-binary.sh -i -v
* expecting success: git-checkout master
         git-apply --stat --summary B.diff
 file1 |    4 ++--
 file2 |    5 -----
 file3 |    0
 file4 |    0
 4 files changed, 2 insertions(+), 7 deletions(-)
 delete mode 100644 file2
 create mode 100644 file3
*   ok 1: stat binary diff -- should not fail.
* expecting success: git-checkout master
         git-apply --stat --summary C.diff
 file1 |    4 ++--
 file2 |    5 -----
 file3 |    0
 file4 |    0
 4 files changed, 2 insertions(+), 7 deletions(-)
 delete mode 100644 file2
 copy file1 => file3 (70%)
*   ok 2: stat binary diff (copy) -- should not fail.
* expecting failure: git-checkout master
         git-apply --check B.diff
fatal: patch with only garbage at line 30
*   ok 3: check binary diff -- should fail.
* expecting failure: git-checkout master
         git-apply --check C.diff
fatal: patch with only garbage at line 32
*   ok 4: check binary diff (copy) -- should fail.
* expecting failure: git-checkout master
         git-apply --check --allow-binary-replacement B.diff
fatal: patch with only garbage at line 30
*   ok 5: check incomplete binary diff with replacement -- should fail.
* expecting failure: git-checkout master
         git-apply --check --allow-binary-replacement C.diff
fatal: patch with only garbage at line 32
*   ok 6: check incomplete binary diff with replacement (copy) -- should
fail.
* expecting success: git-checkout master
         git-apply --check --allow-binary-replacement BF.diff
fatal: patch with only garbage at line 30
* FAIL 7: check binary diff with replacement.
        git-checkout master
                 git-apply --check --allow-binary-replacement BF.diff

Twinkie@squirrel ~/git/t
$ cd trash
cd trash

Twinkie@squirrel ~/git/t/trash
$ ls -l
ls -l
total 19
- -rw-r--r--  1 Twinkie None  909 Nov 17 21:49 B.diff
- -rw-r--r--  1 Twinkie None 1173 Nov 17 21:49 BF.diff
- -rw-r--r--  1 Twinkie None  944 Nov 17 21:49 C.diff
- -rw-r--r--  1 Twinkie None 1208 Nov 17 21:49 CF.diff
- -rw-r--r--  1 Twinkie None  201 Nov 17 21:49 file1
- -rw-r--r--  1 Twinkie None  201 Nov 17 21:49 file2
- -rw-r--r--  1 Twinkie None  201 Nov 17 21:49 file4

Twinkie@squirrel ~/git/t/trash
$ cat BF.diff
cat BF.diff
diff --git a/file1 b/file1
index
edc575dec543a684da5007b43886ee32ecb381ae..af1eedd35be991f3ced320f7d927799c
72cd8435 100644
- --- a/file1
+++ b/file1
@@ -1,5 +1,5 @@
- -A quick brown fox jumps over the lazy dog.
+A quick brown fov jumps over the lazy dog.
 A tiny little penguin runs around in circles.
- -There is a flag with Linux written on it.
+There is a flag with Linuv written on it.
 A slow black-and-white panda just sits there,
 munching on his bamboo.
diff --git a/file2 b/file2
deleted file mode 100644
index
edc575dec543a684da5007b43886ee32ecb381ae..00000000000000000000000000000000
00000000
- --- a/file2
+++ /dev/null
@@ -1,5 +0,0 @@
- -A quick brown fox jumps over the lazy dog.
- -A tiny little penguin runs around in circles.
- -There is a flag with Linux written on it.
- -A slow black-and-white panda just sits there,
- -munching on his bamboo.
diff --git a/file3 b/file3
new file mode 100644
index
0000000000000000000000000000000000000000..adb07b7ad3fa2c63251b06d1d39cb90a
85b860b4
Files /dev/null and b/file3 differ
diff --git a/file4 b/file4
index
edc575dec543a684da5007b43886ee32ecb381ae..adb07b7ad3fa2c63251b06d1d39cb90a
85b860b4 100644
Files a/file4 and b/file4 differ

Twinkie@squirrel ~/git/t/trash
$ git-apply BF.diff
fatal: patch with only garbage at line 30

- --
John Benes
GPG Fingerprint: D519 25DB BB5C 38FC 9D02  02E7 596D BC50 F880 27FA
"It is not only the living who are killed in war." - Isaac Asimov
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iQIVAwUBQ31SGGF0oWcU9kCNAQJmchAA0L9fnTvMNwYhDHRVdPyF6XQYJJ7j3s6M
CtcjFvQZ0Zo2tp3ZZ8mqv9ANgPkV80HZFpWPwmlAouFQfNbjvEDlFxLejaKZ5TJj
q068iMk8ZmUUHKPUhONhCsi/toXHQuhA7RMWEwtE9EeugOOIC/++eI8qhKzBhJr3
REzLQN55lqrdhN/6ksrhJQU2VQ3AcAukgezrJy3j8CYOId3pVLYoyD75oxTrelnN
xSPritxM/zkdQZYwx/WeSFRivQQZgiSwm2nREJ5NY5MJ8X5SIZJ+bcHSnndNmUYv
e6YkZRPLXQBAyJpQHwmAQIqtYikPZ/Q6SNBoiQgA3Ws1SyzSaMcOo9R30Cg4LT4i
37EtQwjrGXzY6V/YlHbPqauPlUW6Sosc7fadNXHXkJJrdgyGSATZghh+XEZpNQ1G
2cS05y7/Xu9KnhL0GxbwLf9FZg14CndRh04NDtkdvwyE5rK9SCD5seW6HQOeBcuX
oHLPeA29IaZvHFUYTITXP5p2ZySAXrrFt7R532j6njeJvVCRzV7pPh7msHmAgdXL
Wz5Xv9ED43wz+q6JpXuWloYNyDKUUil2emXVA/MHwLW9ugfZmu9/OVo2ChXf+ZLH
8pSGxRmxTAVSf0GIiODPMWmGrm2zsKbNczCY1wATjiRaYvlmRk1kiXpXpubW5bw9
F1UDCLIFaJk=
=Uh0t
-----END PGP SIGNATURE-----

Re: master has some toys

[Junio C Hamano]

John Benes <smartcat99s@gmail.com> writes:

> diff --git a/file4 b/file4
> index
> edc575dec543a684da5007b43886ee32ecb381ae..adb07b7ad3fa2c63251b06d1d39cb90a
> 85b860b4 100644
> Files a/file4 and b/file4 differ

Thanks.  This is the same problem as what Large Angly SCM
reports.  What does your "diff --version" say?

Re: master has some toys

[John Benes]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Junio C Hamano wrote:
> Thanks.  This is the same problem as what Large Angly SCM
> reports.  What does your "diff --version" say?

Twinkie@squirrel ~
$ diff --version
diff (GNU diffutils) 2.8.7
Written by Paul Eggert, Mike Haertel, David Hayes,
Richard Stallman, and Len Tower.
[snip 2004 copyright notice]

Extra useful info about my cygwin enviroment:

Twinkie@squirrel ~
$ cygcheck -V
cygcheck version 1.74
System Checker for Cygwin
Copyright 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2005 Red Hat, Inc.
Compiled on Jul  2 2005

cygwin1.dll Version: 1.5.18

HTH!

- --
John Benes
GPG Fingerprint: D519 25DB BB5C 38FC 9D02  02E7 596D BC50 F880 27FA
"It is not only the living who are killed in war." - Isaac Asimov
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iQIVAwUBQ31aF2F0oWcU9kCNAQJrSRAAqDVQuCuF0PBn0ssGJA4sZ5fSa4S4hUju
yftaR9dDaWqpML4ieMcp5NXD1HEm6yzJbL9WhGuDQlpolo7ujbiI1sx7jP53Au+3
zm7iWojJVnRBirRAqsZpD2Ufriuzwnm6/rPJqe97RSkYIm29BcKsiSly67mFKEKJ
AL/MiAmH62h7HLOjSRR2o+3eGmvMlAesojoLdoTaCkic5l34WIfv4EzSSTSfwS+O
cn0K8MFqbuuApZTwECl5OdmAVbKGdC0PjgqgJQIEM39dWxKqqGrmSLvbs42AAEok
tOPTdB+4O2ECEIIYtjiqUAZ2ZDwVvxJwEyTEt60tzvbcfX0TwfXaO+JDxd9gmr8P
he/f05NT+IlqFrcXyXD8PVc+29ZBr7ocM1k+VIzqclGIjHMO6VjytEO3xppbtEDz
iAA5MFYsCM5fiJRndNYiH51WvVMjmmY1SO6LJtqGiwZhX3/couMy71JorXeqGgaW
Yjrkvs8q9AtvTG3Nq9BpmJCz0kXHbMmULchooaNtpDlPVVti9CA08vJ/9PssWRfD
MrkVkEijTpkw/lWClqW91aGWT9vlMeQSBVZf8Cr2Zg9DQNAFEE4LiVmlt10odZGw
cE4TlhaCk2Z2X1AeuDTimyN7hL6FMVweMsLgio1XNpk/NOCGkgvXDo7R76MfU8PS
dsT2ItOIm6w=
=o9sa
-----END PGP SIGNATURE-----

Re: master has some toys

[A Large Angry SCM]

Junio C Hamano wrote:
> John Benes <smartcat99s@gmail.com> writes:
> 
>>diff --git a/file4 b/file4
>>index
>>edc575dec543a684da5007b43886ee32ecb381ae..adb07b7ad3fa2c63251b06d1d39cb90a
>>85b860b4 100644
>>Files a/file4 and b/file4 differ
> 
> Thanks.  This is the same problem as what Large Angly SCM
> reports.  What does your "diff --version" say?


 > diff --version
diff (GNU diffutils) 2.8.7
Written by Paul Eggert, Mike Haertel, David Hayes,
Richard Stallman, and Len Tower.

Copyright (C) 2004 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
 >

Re: master has some toys

[Johannes Schindelin]

Hi,

On Thu, 17 Nov 2005, Alex Riesen wrote:

> As it turned out, not the git.c is guilty, but the missing
> NO_MMAP=YesPlease in Cygwin section. I had it for a long time and
> accidentally removed by the recent pull. BTW, I couldn't find nowhere
> on original branch. Was it never submitted?

It was not, because someone (Peter?) had success without NO_MMAP=YesPlease 
on cygwin. So it was decided to keep NO_MMAP an option which has to be 
provided by the user.

> > Ideally I should get a cygwin environment myself, but for that
> > first I need to procure Windows box.  Or does cygwin run on
> > Wine, and if so is cygwin running on Wine a good enough
> > approximation of the real thing?

It does not. The problem with mmap is a problem which stems from working 
around Windows not being POSIX. Also note that the problem goes poof in a 
gdb session. I tend to think it is a timing problem in conjunction with 
the (very conservative) Antivirus setting.

Ciao,
Dscho

Re: master has some toys

[Johannes Schindelin]

Hi,

On Thu, 17 Nov 2005, Alex Riesen wrote:

> On 11/17/05, Junio C Hamano <junkio@cox.net> wrote:
> > Along with the git wrapper fixes and git-apply bugfix (it did
> 
> cygwin is completely broken. Still debugging, but it looks like the
> old "windows can't unlink/rename open files" problem.

FWIW I had no problems on cygwin (NO_MMAP=YesPlease).

Ciao,
Dscho

Re: master has some toys

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi,
>
> On Thu, 17 Nov 2005, Alex Riesen wrote:
>
>> On 11/17/05, Junio C Hamano <junkio@cox.net> wrote:
>> > Along with the git wrapper fixes and git-apply bugfix (it did
>> 
>> cygwin is completely broken. Still debugging, but it looks like the
>> old "windows can't unlink/rename open files" problem.
>
> FWIW I had no problems on cygwin (NO_MMAP=YesPlease).

It appears we'd better have something like this in the main
Makefile, so people do not have to do it themselves everywhere?

---

diff --git a/Makefile b/Makefile
index 7ce62e8..93d51c6 100644
--- a/Makefile
+++ b/Makefile
@@ -213,6 +213,7 @@ endif
 ifeq ($(uname_O),Cygwin)
 	NO_STRCASESTR = YesPlease
 	NEEDS_LIBICONV = YesPlease
+	NO_MMAP = YesPlease
 	NO_IPV6 = YesPlease
 	X = .exe
 endif

Re: master has some toys

[Alex Riesen]

On 11/17/05, Junio C Hamano <junkio@cox.net> wrote:
> >> > Along with the git wrapper fixes and git-apply bugfix (it did
> >>
> >> cygwin is completely broken. Still debugging, but it looks like the
> >> old "windows can't unlink/rename open files" problem.
> >
> > FWIW I had no problems on cygwin (NO_MMAP=YesPlease).
>
> It appears we'd better have something like this in the main
> Makefile, so people do not have to do it themselves everywhere?

damn... Missed by 4 minutes... :)

Re: master has some toys

[Johannes Schindelin]

Hi,

On Thu, 17 Nov 2005, Junio C Hamano wrote:

> It appears we'd better have something like this in the main
> Makefile, so people do not have to do it themselves everywhere?

I'd like to wait to have a reaction from other people. I vividly remember 
my eyes falling out of my sockets when somebody reported success on cygwin 
without NO_MMAP. If there is *any* cygwin version which fixes it, we 
should rather make people upgrade, no?

Ciao,
Dscho

Re: master has some toys

[Alex Riesen]

On 11/17/05, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > It appears we'd better have something like this in the main
> > Makefile, so people do not have to do it themselves everywhere?
>
> I'd like to wait to have a reaction from other people. I vividly remember
> my eyes falling out of my sockets when somebody reported success on cygwin
> without NO_MMAP. If there is *any* cygwin version which fixes it, we
> should rather make people upgrade, no?

my eyes too. I used to compile Peters tree, and it never worked (w2k,
antivirus present, but self-disabled because of some lucky crash).
That is why the whole story started (and I started to look for
unclosed files and unmapped maps).

Re: master has some toys

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> I'd like to wait to have a reaction from other people. I vividly remember 
> my eyes falling out of my sockets when somebody reported success on cygwin 
> without NO_MMAP. If there is *any* cygwin version which fixes it, we 
> should rather make people upgrade, no?

I am not so sure about forcing people upgrade, but we may end up
deciding it is better not to have NO_MMAP as the default.  If
that turns out to be the case, I'd prefer to have something like
this instead:

diff --git a/Makefile b/Makefile
index 7ce62e8..215abf0 100644
--- a/Makefile
+++ b/Makefile
@@ -213,6 +213,10 @@ endif
 ifeq ($(uname_O),Cygwin)
 	NO_STRCASESTR = YesPlease
 	NEEDS_LIBICONV = YesPlease
+	# There are conflicting reports about this.
+	# On some boxes NO_MMAP is needed, and not so elsewhere.
+	# Try uncommenting this if you see things break -- YMMV.
+	# NO_MMAP = YesPlease
 	NO_IPV6 = YesPlease
 	X = .exe
 endif

Re: master has some toys

[Johannes Schindelin]

Hi,

On Thu, 17 Nov 2005, Junio C Hamano wrote:

> +	# There are conflicting reports about this.
> +	# On some boxes NO_MMAP is needed, and not so elsewhere.
> +	# Try uncommenting this if you see things break -- YMMV.
> +	# NO_MMAP = YesPlease

Sounds sensible. Maybe you want to output that to stderr or stdout?

Ciao,
Dscho

Re: master has some toys

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

I just had a small excitement finding out I did something right
and felt an urge to brag ;-).

> I am not so sure about forcing people upgrade, but we may end up
> deciding it is better not to have NO_MMAP as the default.  If
> that turns out to be the case, I'd prefer to have something like
> this instead:
>
> diff --git a/Makefile b/Makefile
> index 7ce62e8..215abf0 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -213,6 +213,10 @@ endif
>  ifeq ($(uname_O),Cygwin)
>  	NO_STRCASESTR = YesPlease
>  	NEEDS_LIBICONV = YesPlease
> +	# There are conflicting reports about this.
> +	# On some boxes NO_MMAP is needed, and not so elsewhere.
> +	# Try uncommenting this if you see things break -- YMMV.
> +	# NO_MMAP = YesPlease
>  	NO_IPV6 = YesPlease
>  	X = .exe
>  endif

I did the above patch on top of "pu", which contained the patch
from Pavel Roskin and sent it out.  Later I saved the message
from my mbox, went back to the "master" branch, whose Makefile
had the releveant part like this:

        ifeq ($(uname_O),Cygwin)
                NO_STRCASESTR = YesPlease
                NEEDS_LIBICONV = YesPlease
                NO_IPV6 = YesPlease
                X = .exe
                ALL_CFLAGS += -DUSE_SYMLINK_HEAD=0
        endif

Notice ALL_CFLAGS line?  The patch does not apply cleanly and
usual e-mail patch application tool would have barfed; git-apply
would not allow any fuzz, and patch would have dropped a .rej
file.

However, I usually run "git-am" with --3way option enabled when
applying the e-mailed patches.  After git-apply failed, it
noticed I am applying on top of a different blob, namely, the
Makefile from somewhere else (it reads the "index 7ce62e8"),
then fell back on 3-way merge and made a clean commit.  Happy.

Back to day-job.

Re: master has some toys

[timo]

On Thu, Nov 17, 2005 at 12:51:22PM +0100, Johannes Schindelin wrote:
> Hi,
> 
> On Thu, 17 Nov 2005, Junio C Hamano wrote:
> 
> > It appears we'd better have something like this in the main
> > Makefile, so people do not have to do it themselves everywhere?
> 
> I'd like to wait to have a reaction from other people. I vividly remember 
> my eyes falling out of my sockets when somebody reported success on cygwin 
> without NO_MMAP. If there is *any* cygwin version which fixes it, we 
> should rather make people upgrade, no?
> 

It is not in the official Cygwin distribution yet. Though I've started the
formalities, moving house as been taking all my spare time. So, the upgrade
worries would be for those people tracking the main git repo. As they are
almost certainly on this list, they should be aware of possible breakage.

I've noted some breakage with git-archimport, git-svnimport and git-cvsimport,
though i have not yet looked into it, some are due to the lack of necessary
tools under Cygwin. I'm re-jigging my distro script to emulate the new package
split and plan to distribute just the git 'core' stuff for the moment.

I was wondering if anyone has scripts that i could use to test the
svn/arch/CVS import/export for expected behavior?

Tim.
"However beautiful the strategy, you should occasionally look at the results."
-- Winston Churchill

Re: master has some toys

[Alex Riesen]

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

On 11/17/05, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > > Along with the git wrapper fixes and git-apply bugfix (it did
> >
> > cygwin is completely broken. Still debugging, but it looks like the
> > old "windows can't unlink/rename open files" problem.
>
> FWIW I had no problems on cygwin (NO_MMAP=YesPlease).
>

yes, that's what I had, too. Junio, how about the attached patch?

[-- Attachment #2: cygwin-has-no-mmap.patch --]
[-- Type: application/xxxxx, Size: 294 bytes --]