Re: [PATCH] drop support for experimental loose objects

2013-11-27 Thread Jeff King
On Mon, Nov 25, 2013 at 10:35:19AM -0800, Junio C Hamano wrote: if (type == OBJ_BLOB) { if (stream_blob_to_fd(fd, sha1, NULL, 0) 0) die(unable to stream %s to stdout, sha1_to_hex(sha1)); + if (check_sha1_signature(sha1, NULL, 0, NULL) 0) +

Re: [PATCH] drop support for experimental loose objects

2013-11-27 Thread Junio C Hamano
Jeff King p...@peff.net writes: The vast majority of blobs in git.git will be stored as packed deltas. That means the streaming code will fall back to doing the regular in-core access. We _could_ therefore use that in-core copy to do our sha1 check rather than streaming; but of course we

Re: [PATCH] drop support for experimental loose objects

2013-11-27 Thread Jeff King
On Wed, Nov 27, 2013 at 10:57:14AM -0800, Junio C Hamano wrote: Yes, I think it is a reasonable addition to the streaming API. However, I do not think there are any callsites which would currently want it. All of the current users of stream_blob_to_fd use read_sha1_file as their

Re: [PATCH] drop support for experimental loose objects

2013-11-25 Thread Junio C Hamano
Jeff King p...@peff.net writes: On Sun, Nov 24, 2013 at 03:44:44AM -0500, Jeff King wrote: In any code path where we call parse_object, we double-check that the result matches the sha1 we asked for. But low-level commands like cat-file just call read_sha1_file directly, and do not have such

Re: [PATCH] drop support for experimental loose objects

2013-11-24 Thread Jeff King
On Fri, Nov 22, 2013 at 01:28:59PM -0400, Joey Hess wrote: Hrm. For --batch, I'd think we would open the whole object and notice the corruption, even with the current code. But for --batch-check, we use sha1_object_info, and for an experimental object, we do not need to de-zlib the object

Re: [PATCH] drop support for experimental loose objects

2013-11-24 Thread Jeff King
On Sun, Nov 24, 2013 at 03:44:44AM -0500, Jeff King wrote: In any code path where we call parse_object, we double-check that the result matches the sha1 we asked for. But low-level commands like cat-file just call read_sha1_file directly, and do not have such a check. We could add it, but I

Re: [PATCH] drop support for experimental loose objects

2013-11-22 Thread Jeff King
On Thu, Nov 21, 2013 at 09:19:25PM +0100, Christian Couder wrote: Yeah, I think it might report wrong size in case of replaced objects for example. I looked at that following Junio's comment about the sha1_object_info() API, which, unlike read_sha1_file() API, does not interact with the

Re: [PATCH] drop support for experimental loose objects

2013-11-22 Thread Christian Couder
On Fri, Nov 22, 2013 at 10:58 AM, Jeff King p...@peff.net wrote: On Thu, Nov 21, 2013 at 09:19:25PM +0100, Christian Couder wrote: Yeah, I think it might report wrong size in case of replaced objects for example. I looked at that following Junio's comment about the sha1_object_info() API,

Re: [PATCH] drop support for experimental loose objects

2013-11-22 Thread Jeff King
On Fri, Nov 22, 2013 at 12:04:01PM +0100, Christian Couder wrote: In sha1_file.c, there is: void *read_sha1_file_extended(const unsigned char *sha1, enum object_type *type, unsigned long *size,

Re: [PATCH] drop support for experimental loose objects

2013-11-22 Thread Christian Couder
On Fri, Nov 22, 2013 at 12:24 PM, Jeff King p...@peff.net wrote: On Fri, Nov 22, 2013 at 12:04:01PM +0100, Christian Couder wrote: In sha1_file.c, there is: void *read_sha1_file_extended(const unsigned char *sha1, enum object_type *type,

Re: [PATCH] drop support for experimental loose objects

2013-11-22 Thread Jeff King
On Fri, Nov 22, 2013 at 03:23:31PM +0100, Christian Couder wrote: The only site which calls read_sha1_file_extended directly and does not pass the REPLACE flag is in streaming.c. And that looks to be a case of (2), since we resolve the replacement at the start in open_istream(). Yeah,

Re: [PATCH] drop support for experimental loose objects

2013-11-22 Thread Junio C Hamano
Jeff King p...@peff.net writes: I guess we would need to audit all the sha1_object_info callers. Yup; I agree that was the conclusion of Christian's thread. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo

Re: [PATCH] drop support for experimental loose objects

2013-11-22 Thread Joey Hess
Jeff King wrote: BTW, I've also seen git cat-file --batch report wrong sizes for objects, Hrm. For --batch, I'd think we would open the whole object and notice the corruption, even with the current code. But for --batch-check, we use sha1_object_info, and for an experimental object, we do

Re: [PATCH] drop support for experimental loose objects

2013-11-22 Thread Jonathan Nieder
Jeff King wrote: sha1_file.c| 74 - Yay! t/t1013-loose-object-format.sh | 66 -- Hmm, not all of these tests are about the experimental format. Do we really want to remove them all? Thanks,

Re: [PATCH] drop support for experimental loose objects

2013-11-22 Thread Jeff King
On Fri, Nov 22, 2013 at 04:24:05PM -0800, Jonathan Nieder wrote: t/t1013-loose-object-format.sh | 66 -- Hmm, not all of these tests are about the experimental format. Do we really want to remove them all? I think so. They were not all testing the

Re: [PATCH] drop support for experimental loose objects

2013-11-22 Thread Jonathan Nieder
Jeff King wrote: On Fri, Nov 22, 2013 at 04:24:05PM -0800, Jonathan Nieder wrote: t/t1013-loose-object-format.sh | 66 -- Hmm, not all of these tests are about the experimental format. Do we really want to remove them all? I think so. They were not all

[PATCH] drop support for experimental loose objects

2013-11-21 Thread Jeff King
On Wed, Nov 20, 2013 at 06:28:06PM -0400, Joey Hess wrote: Jeff King wrote: As for your specific corruption, I can't make heads or tails of it. It is not a single-bit error. Oh, I should have mentioned that I am generating corrupt git repositories mechanically for testing. I think in

Re: [PATCH] drop support for experimental loose objects

2013-11-21 Thread Jeff King
On Thu, Nov 21, 2013 at 06:41:58AM -0500, Jeff King wrote: The test objects removed are all binary. Git seems to guess a few as non-binary, though, because they don't contain any NULs, and includes gross binary bytes in the patch below. In theory the mail's transfer encoding will take care of

Re: [PATCH] drop support for experimental loose objects

2013-11-21 Thread Jeff King
On Thu, Nov 21, 2013 at 07:43:03PM +0700, Duy Nguyen wrote: - if (experimental_loose_object(map)) { Perhaps keep this.. - /* -* The old experimental format we no longer produce; -* we can still read it. -*/ -

Re: [PATCH] drop support for experimental loose objects

2013-11-21 Thread Junio C Hamano
Jeff King p...@peff.net writes: We could try to improve the heuristic to err on the side of normal objects in the face of corruption, but there is really little point. The experimental format is long-dead, and was never enabled by default to begin with. We can instead simply remove it. The

Re: [PATCH] drop support for experimental loose objects

2013-11-21 Thread Christian Couder
On Thu, Nov 21, 2013 at 5:04 PM, Joey Hess j...@kitenet.net wrote: BTW, I've also seen git cat-file --batch report wrong sizes for objects, sometimes without crashing. This is particularly problimatic because if the object size is wrong, it's very hard to detect the actual end of the object

Re: [PATCH] drop support for experimental loose objects

2013-11-21 Thread Duy Nguyen
On Thu, Nov 21, 2013 at 6:48 PM, Jeff King p...@peff.net wrote: @@ -1514,14 +1469,6 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf, int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz) { -

Re: [PATCH] drop support for experimental loose objects

2013-11-21 Thread Keshav Kini
Duy Nguyen pclo...@gmail.com writes: On Thu, Nov 21, 2013 at 6:48 PM, Jeff King p...@peff.net wrote: @@ -1514,14 +1469,6 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf, int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void

Re: [PATCH] drop support for experimental loose objects

2013-11-21 Thread Jeff King
On Thu, Nov 21, 2013 at 12:04:26PM -0400, Joey Hess wrote: Jeff King wrote: This latter behavior is much worse for two reasons. One, git reports an allocation error when the real error is corruption. And two, the program dies unconditionally, so you cannot even run fsck (which would

Re: [PATCH] drop support for experimental loose objects

2013-11-21 Thread Joey Hess
Jeff King wrote: This latter behavior is much worse for two reasons. One, git reports an allocation error when the real error is corruption. And two, the program dies unconditionally, so you cannot even run fsck (which would otherwise ignore the broken object and keep going). BTW, I've also