{"thread":{"id":"26654","subject":"[BUG] git cat-file does not terminate","startedAt":"2011-03-04T13:04:00Z","lastAt":"2011-03-09T21:43:23Z","messageCount":12,"participants":["Robert Wruck","Peter Baumann","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"162765","messageId":"4D70E340.3050309@tweerlei.de","threadId":"26654","inReplyTo":null,"subject":"[BUG] git cat-file does not terminate","fromName":"Robert Wruck","fromEmail":"wruck@tweerlei.de","sentAt":"2011-03-04T13:04:00Z","receivedAt":"2011-03-04T13:04:00Z","isPatch":false,"sender":{"key":"wruck@tweerlei.de","avatar":null},"body":"Hi,\n\nthis is some strange behaviour of cat-file:\nOn a certain file, `git cat-file blob <objectname>` writes an endless \nstream repeating the first 4096 byte of the original file.\ncat-file -s and cat-file -t produce correct results.\n\nEven stranger: This only happens with cygwin-git (1.7.4.1).\nmsysgit (same machine, same repository): works\nlinux-git (same machine, same repository): works\n\nEven more strange: This only happens with cygwin on a particular machine \n(recent cygwin1.dll 1.7.8) under WinXP/32bit. On another machine, recent \ncygwin, Windows7/64bit it works...\n\nDebugging a bit, I found that the following happens:\nIn xwrite (wrapper.c), write() is called with the total file size - in \nmy case about 87 MB. This call returns -1 and EAGAIN but nevertheless \nwrites 4096 byte to the output fd. I don't think that's expected \nbehaviour...\n\nI \"fixed\" it by limiting each write to 64k (thus looping in \nwrite_in_full) but maybe somebody knows about that cygwin behaviour?\n\nThis seems to be the cause of the dreaded \"No newline found after blob\" \nwhen running `git svn clone` under cygwin on a repository with large files.\n\nYou could argue that this is a cygwin bug but maybe limiting each write \nto a maximum size is a simple workaround.\n\n\n-Robert\n"},{"id":"162769","messageId":"20110304154014.GE24660@m62s10.vlinux.de","threadId":"26654","inReplyTo":"4D70E340.3050309@tweerlei.de","subject":"Re: [BUG] git cat-file does not terminate","fromName":"Peter Baumann","fromEmail":"waste.manager@gmx.de","sentAt":"2011-03-04T15:40:14Z","receivedAt":"2011-03-04T15:40:14Z","isPatch":false,"sender":{"key":"waste.manager@gmx.de","avatar":null},"body":"On Fri, Mar 04, 2011 at 02:04:00PM +0100, Robert Wruck wrote:\n> Hi,\n> \n> this is some strange behaviour of cat-file:\n> On a certain file, `git cat-file blob <objectname>` writes an\n> endless stream repeating the first 4096 byte of the original file.\n> cat-file -s and cat-file -t produce correct results.\n> \n> Even stranger: This only happens with cygwin-git (1.7.4.1).\n> msysgit (same machine, same repository): works\n> linux-git (same machine, same repository): works\n> \n> Even more strange: This only happens with cygwin on a particular\n> machine (recent cygwin1.dll 1.7.8) under WinXP/32bit. On another\n> machine, recent cygwin, Windows7/64bit it works...\n> \n> Debugging a bit, I found that the following happens:\n> In xwrite (wrapper.c), write() is called with the total file size -\n> in my case about 87 MB. This call returns -1 and EAGAIN but\n> nevertheless writes 4096 byte to the output fd. I don't think that's\n> expected behaviour...\n> \n> I \"fixed\" it by limiting each write to 64k (thus looping in\n> write_in_full) but maybe somebody knows about that cygwin behaviour?\n> \n> This seems to be the cause of the dreaded \"No newline found after\n> blob\" when running `git svn clone` under cygwin on a repository with\n> large files.\n> \n> You could argue that this is a cygwin bug but maybe limiting each\n> write to a maximum size is a simple workaround.\n> \nMaybe you could post a patch, so everyone can see the technical implications\nand discuss the fix?\n\n-Peter\n"},{"id":"162771","messageId":"20110304160047.GA9662@sigill.intra.peff.net","threadId":"26654","inReplyTo":"20110304154014.GE24660@m62s10.vlinux.de","subject":"Re: [BUG] git cat-file does not terminate","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-04T16:00:47Z","receivedAt":"2011-03-04T16:00:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 04, 2011 at 04:40:14PM +0100, Peter Baumann wrote:\n\n> > I \"fixed\" it by limiting each write to 64k (thus looping in\n> > write_in_full) but maybe somebody knows about that cygwin behaviour?\n> > \n> > This seems to be the cause of the dreaded \"No newline found after\n> > blob\" when running `git svn clone` under cygwin on a repository with\n> > large files.\n> > \n> > You could argue that this is a cygwin bug but maybe limiting each\n> > write to a maximum size is a simple workaround.\n> > \n> Maybe you could post a patch, so everyone can see the technical implications\n> and discuss the fix?\n\nIt would probably look like the patch below, though it really feels like\nthe right solution is to fix the cygwin bug.\n\n-Peff\n\n---\ndiff --git a/Makefile b/Makefile\nindex 4c31d1a..e7d3285 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -167,6 +167,9 @@ all::\n # Define NO_ST_BLOCKS_IN_STRUCT_STAT if your platform does not have st_blocks\n # field that counts the on-disk footprint in 512-byte blocks.\n #\n+# Define MAX_WRITE_SIZE to N if your platform has unpredictable results for\n+# write() calls larger than N (e.g., cygwin).\n+#\n # Define ASCIIDOC7 if you want to format documentation with AsciiDoc 7\n #\n # Define DOCBOOK_XSL_172 if you want to format man pages with DocBook XSL v1.72\n@@ -928,6 +931,7 @@ ifeq ($(uname_O),Cygwin)\n \tNO_FAST_WORKING_DIRECTORY = UnfortunatelyYes\n \tNO_TRUSTABLE_FILEMODE = UnfortunatelyYes\n \tNO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease\n+\tMAX_WRITE_SIZE=65536\n \t# There are conflicting reports about this.\n \t# On some boxes NO_MMAP is needed, and not so elsewhere.\n \t# Try commenting this out if you suspect MMAP is more efficient\n@@ -1495,6 +1499,10 @@ ifdef NO_POSIX_GOODIES\n \tBASIC_CFLAGS += -DNO_POSIX_GOODIES\n endif\n \n+ifdef MAX_WRITE_SIZE\n+\tBASIC_CFLAGS += -DMAX_WRITE_SIZE=$(MAX_WRITE_SIZE)\n+endif\n+\n ifdef BLK_SHA1\n \tSHA1_HEADER = \"block-sha1/sha1.h\"\n \tLIB_OBJS += block-sha1/sha1.o\ndiff --git a/wrapper.c b/wrapper.c\nindex 056e9d6..a7a2437 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -133,6 +133,10 @@ ssize_t xread(int fd, void *buf, size_t len)\n ssize_t xwrite(int fd, const void *buf, size_t len)\n {\n \tssize_t nr;\n+#ifdef MAX_WRITE_SIZE\n+\tif (len > MAX_WRITE_SIZE)\n+\t\tlen = MAX_WRITE_SIZE;\n+#endif\n \twhile (1) {\n \t\tnr = write(fd, buf, len);\n \t\tif ((nr < 0) && (errno == EAGAIN || errno == EINTR))\n"},{"id":"162775","messageId":"7vzkpa7qmp.fsf@alter.siamese.dyndns.org","threadId":"26654","inReplyTo":"20110304160047.GA9662@sigill.intra.peff.net","subject":"Re: [BUG] git cat-file does not terminate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-04T17:16:30Z","receivedAt":"2011-03-04T17:16:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> It would probably look like the patch below, though it really feels like\n> the right solution is to fix the cygwin bug.\n\nThanks for a quick analysis and fix, folks.\n\nWe would need to assume certain things that the platform would give its\nusers are reliable, and system calls are fundamental part; otherwise we\nwould end up tying our hands behind our back saying \"we cannot use this\nand that as these are unreliable in certain places\" and litter our\ncodebase with full of ifdefs and workarounds.\n\nIf this were merely of a breakage in a test release or a nightly build, I\nwould be happier to see us ignore this, but the problematic one is widely\nin the wild, a workaround would be necessary, and more importantly, if it\nis harder for a casual end-user to tell if the platform is affected (I am\nassuming that most releases of Cygwin is without this bug, and most users\nwho build git themselves wouldn't bother reading README or Makefile even\nif we said a MAX_WRITE_SIZE definition is necessary only for this and that\nversion), I would rather see a change that covers the problem a bit more\nwidely than necessary.\n\nHow prevalent is the problematic cygwin1.dll 1.7.8?  Also for how long did\nthis bug exist, in other words, if we were to make a table of problematic\nversions, would we have only just a handful entries in it?  Also can we at\nruntime find out what version we are running?\n\nThe reason I am asking these questions is because I think, assuming that\nthis would affect many unsuspecting Cygwin users, the best fix would be to\nadd a hook in the compat/ layer that decides if MAX_WRITE_SIZE workaround\nis necessary at runtime, and do something like this:\n\n\tssize_t xwrite(int fd, const void *buf, size_t len)\n        {\n        \tssize_t nr;\n                static size_t max_write_size = platform_max_write_size();\n\n                if (max_write_size && max_write_size < len)\n                \tlen = max_write_size;\n\t\t...\n\t}\n\nAnd then we would have in git-compat-util.h something like:\n\n\t#define platform_max_write_size() 0\n\non sane platforms, so that the fix will be optimized away by the compiler.\n\nBy the way, does the same version of Cygwin have similar issue on the read\nside?\n"},{"id":"162778","messageId":"4D712189.7090105@tweerlei.de","threadId":"26654","inReplyTo":"7vzkpa7qmp.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] git cat-file does not terminate","fromName":"Robert Wruck","fromEmail":"wruck@tweerlei.de","sentAt":"2011-03-04T17:29:45Z","receivedAt":"2011-03-04T17:29:45Z","isPatch":false,"sender":{"key":"wruck@tweerlei.de","avatar":null},"body":"> By the way, does the same version of Cygwin have similar issue on the read\n> side?\n\nActually I've written a small test program on the problematic cygwin \nmachine that seems to show that the EAGAIN is somehow related to pipes \n(e.g. stdout & stuff) and does not occur when fd refers to a file.\nI will test this for read() as well but I don't know enough cygwin \ninternals to tell what other versions may be affected.\nIt might even be related to 32/64 bit or different Windows versions \nsince I didn't test more constellations.\n\n-Robert\n"},{"id":"162783","messageId":"4D712EB9.3080802@tweerlei.de","threadId":"26654","inReplyTo":"7vzkpa7qmp.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] git cat-file does not terminate","fromName":"Robert Wruck","fromEmail":"wruck@tweerlei.de","sentAt":"2011-03-04T18:26:01Z","receivedAt":"2011-03-04T18:26:01Z","isPatch":false,"sender":{"key":"wruck@tweerlei.de","avatar":null},"body":"> By the way, does the same version of Cygwin have similar issue on the read\n> side?\n\nHere some quick results:\n\nread(fd, buffer, 90000000)\nReturns total file size for a file and 65536 (errno=0) for pipes (cat \nfile | readtest). When repeating the read, the whole file is read until \nread() returns 0. No problem here on any cygwin I tested.\n\nwrite(fd, buffer, 90000000)\nReturns 90000000 for a file.\nReturns 90000000 for redirection (writetest > outfile)\nReturns 90000000 for pipes (writetest | cat > outfile) on sane cygwins \nand -1 / EAGAIN on the machine with the original problem.\n"},{"id":"163021","messageId":"20110308211423.GB4594@sigill.intra.peff.net","threadId":"26654","inReplyTo":"7vzkpa7qmp.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] git cat-file does not terminate","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-08T21:14:23Z","receivedAt":"2011-03-08T21:14:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 04, 2011 at 09:16:30AM -0800, Junio C Hamano wrote:\n\n> How prevalent is the problematic cygwin1.dll 1.7.8?  Also for how long did\n> this bug exist, in other words, if we were to make a table of problematic\n> versions, would we have only just a handful entries in it?  Also can we at\n> runtime find out what version we are running?\n> \n> The reason I am asking these questions is because I think, assuming that\n> this would affect many unsuspecting Cygwin users, the best fix would be to\n> add a hook in the compat/ layer that decides if MAX_WRITE_SIZE workaround\n> is necessary at runtime, and do something like this:\n> \n> \tssize_t xwrite(int fd, const void *buf, size_t len)\n>         {\n>         \tssize_t nr;\n>                 static size_t max_write_size = platform_max_write_size();\n> \n>                 if (max_write_size && max_write_size < len)\n>                 \tlen = max_write_size;\n> \t\t...\n> \t}\n\nHow are we doing the runtime test for platform max write?\n\nIf I read the original bug report correctly, the problem was that write\nwould actually write some bytes _and_ return -1, which is terrible. We\ncan detect \"seems to be returning -1 over and over\", but we can't handle\na misbehavior like writing and claiming not to have done so.\n\nSo I think the test needs to be \"is our version of cygwin in the broken\nlist\" and not \"let's try a few different writes and see what works\".\n\nBut it is still not clear to me how many versions have this bug. I think\nthe next stop is to show the cygwin developers a clear test-case and see\nwhether it's already fixed, and which versions show the behavior. They\nshould be able to get that information much more easily than us. I\nreally don't want to get involved in bisecting bugs in cygwin (according\nto cygwin.com, it's kept in CVS. Blech).\n\nRobert, can you try (or have you already tried) submitting a bug report\nto Cygwin?\n\n-Peff\n"},{"id":"163040","messageId":"7vwrk9cjib.fsf@alter.siamese.dyndns.org","threadId":"26654","inReplyTo":"20110308211423.GB4594@sigill.intra.peff.net","subject":"Re: [BUG] git cat-file does not terminate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-08T22:52:44Z","receivedAt":"2011-03-08T22:52:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Mar 04, 2011 at 09:16:30AM -0800, Junio C Hamano wrote:\n>\n>> How prevalent is the problematic cygwin1.dll 1.7.8?  Also for how long did\n>> this bug exist, in other words, if we were to make a table of problematic\n>> versions, would we have only just a handful entries in it?  Also can we at\n>> runtime find out what version we are running?\n>> \n>> The reason I am asking these questions is because I think, assuming that\n>> ...\n> How are we doing the runtime test for platform max write?\n\nI asked (1) if we can find out at runtime if we are on which version of\ncygwin1.dll, and (2) if we can have a small list of \"bad\" versions of\ncygwin1.dll.  If both are true, the runtime test should be trivial, no?\n\n> So I think the test needs to be \"is our version of cygwin in the broken\n> list\" and not \"let's try a few different writes and see what works\".\n\nYes, that is exactly what I had in mind.\n"},{"id":"163059","messageId":"4D779385.3070602@tweerlei.de","threadId":"26654","inReplyTo":"7vwrk9cjib.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] git cat-file does not terminate","fromName":"Robert Wruck","fromEmail":"wruck@tweerlei.de","sentAt":"2011-03-09T14:49:41Z","receivedAt":"2011-03-09T14:49:41Z","isPatch":false,"sender":{"key":"wruck@tweerlei.de","avatar":null},"body":"> I asked (1) if we can find out at runtime if we are on which version of\n> cygwin1.dll, and (2) if we can have a small list of \"bad\" versions of\n> cygwin1.dll.  If both are true, the runtime test should be trivial, no?\n\nCurrently I don't know of a programmatic way to get the cygwin version \nexcept `cygcheck -c cygwin` or `uname -r` but these utilities seem to \nknow where to find it. I'll take a look at the source.\n\nUnfortunately, the same cygwin version works on most platforms except \nWinXP, so its rather a platform issue and I fear that in this case all \ncygwin versions up to a currently unknown fixed version will be subject.\nDepending on the machine, the \"limit\" at which write() fails seems to \nvary as well. In my initial report, it was about 80MB, on another \nmachine it was around 200MB...\n\nI submitted a bug report to cygwin over the weekend and tried to debug \nwhat's going on in cygwin1.dll but haven't gone very far yet.\n\n-Robert\n"},{"id":"163071","messageId":"7vzkp49jk3.fsf@alter.siamese.dyndns.org","threadId":"26654","inReplyTo":"4D779385.3070602@tweerlei.de","subject":"Re: [BUG] git cat-file does not terminate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-09T19:32:12Z","receivedAt":"2011-03-09T19:32:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robert Wruck <wruck@tweerlei.de> writes:\n\n>> I asked (1) if we can find out at runtime if we are on which version of\n>> cygwin1.dll, and (2) if we can have a small list of \"bad\" versions of\n>> cygwin1.dll.  If both are true, the runtime test should be trivial, no?\n>\n> Currently I don't know of a programmatic way to get the cygwin version\n> except `cygcheck -c cygwin` or `uname -r` but these utilities seem to\n> know where to find it. I'll take a look at the source.\n\nThanks; it is not very critical so don't spend too much effort trying to\nfind a way to do so at runtime.  We can always use the approach Jeff's\nMakefile patch took to make it safe (and potentially slow) by default on\nall Cygwin while still allowing people on an unaffected version to turn\nthe workaround off while building.\n"},{"id":"163073","messageId":"4D77DA52.7050701@tweerlei.de","threadId":"26654","inReplyTo":"7vzkp49jk3.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] git cat-file does not terminate","fromName":"Robert Wruck","fromEmail":"wruck@tweerlei.de","sentAt":"2011-03-09T19:51:46Z","receivedAt":"2011-03-09T19:51:46Z","isPatch":false,"sender":{"key":"wruck@tweerlei.de","avatar":null},"body":"> Thanks; it is not very critical so don't spend too much effort trying to\n> find a way to do so at runtime.  We can always use the approach Jeff's\n> Makefile patch took to make it safe (and potentially slow) by default on\n> all Cygwin while still allowing people on an unaffected version to turn\n> the workaround off while building.\n\nThe write() issue will presumably be fixed in the next cygwin release. I \nalready have a version that works on WinXP where it used to fail.\n\n-Robert\n"},{"id":"163090","messageId":"20110309214323.GB4400@sigill.intra.peff.net","threadId":"26654","inReplyTo":"4D77DA52.7050701@tweerlei.de","subject":"Re: [BUG] git cat-file does not terminate","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-09T21:43:23Z","receivedAt":"2011-03-09T21:43:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 09, 2011 at 08:51:46PM +0100, Robert Wruck wrote:\n\n> >Thanks; it is not very critical so don't spend too much effort trying to\n> >find a way to do so at runtime.  We can always use the approach Jeff's\n> >Makefile patch took to make it safe (and potentially slow) by default on\n> >all Cygwin while still allowing people on an unaffected version to turn\n> >the workaround off while building.\n> \n> The write() issue will presumably be fixed in the next cygwin\n> release. I already have a version that works on WinXP where it used\n> to fail.\n\nCool. Does our Makefile know the cygwin version number via \"uname -r\"?\nWe could at least tweak the build-time patch to turn it on for the right\nversions (though we should perhaps wait for the fixed version to be\nreleased so we know what it is. :) ).\n\nPersonally I wouldn't bother much with run-time detection. But maybe\ncygwin people tend to download binary packages and run them on top of\narbitrary versions cygwin? I don't know what's normal.\n\n-Peff\n"}]}