Re: [PATCH] drop support for "experimental" loose objects

Jeff King [off-list ref] writes:

On Sun, Nov 24, 2013 at 03:44:44AM -0500, Jeff King wrote:

quoted

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 suspect the processing cost would be
noticeable.

Curious, I tested this. It is noticeable. Here's the best-of-five
timings for the patch below when running a --batch cat-file on every
object in my git.git repo, using the patch below:

  [before]
  real    0m12.941s
  user    0m12.700s
  sys     0m0.244s

  [after]
  real    0m15.800s
  user    0m15.472s
  sys     0m0.344s

So it's about 20% slower. I don't know what the right tradeoff is. It's
cool to check the data each time we look at it, but it does carry a
performance penalty.

check_sha1_signature() opens the object again and streams the data.
Essentially the read side is doing twice the work with that patch,
isn't it?

I wonder if we want to extend the stream_blob_to_fd() API to
optionally allow the caller to ask to validate that the returned
data is consistent with the object name the caller asked the data
for.  Something along the lines of the attached weatherbaloon patch?

 builtin/fsck.c |  3 ++-
 entry.c        |  2 +-
 streaming.c    | 29 ++++++++++++++++++++++++++++-
 streaming.h    |  4 +++-
 4 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 97ce678..f42ed9c 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c

@@ -237,7 +237,8 @@ static void check_unreachable_object(struct object *obj)
 			if (!(f = fopen(filename, "w")))
 				die_errno("Could not open '%s'", filename);
 			if (obj->type == OBJ_BLOB) {
-				if (stream_blob_to_fd(fileno(f), obj->sha1, NULL, 1))
+				if (stream_blob_to_fd(fileno(f), obj->sha1, NULL,
+						      STREAMING_OUTPUT_CAN_SEEK))
 					die_errno("Could not write '%s'", filename);
 			} else
 				fprintf(f, "%s\n", sha1_to_hex(obj->sha1));

diff --git a/entry.c b/entry.c
index 7b7aa81..b3bc827 100644
--- a/entry.c
+++ b/entry.c

@@ -127,7 +127,7 @@ static int streaming_write_entry(const struct cache_entry *ce, char *path,
 	if (fd < 0)
 		return -1;
 
-	result |= stream_blob_to_fd(fd, ce->sha1, filter, 1);
+	result |= stream_blob_to_fd(fd, ce->sha1, filter, STREAMING_OUTPUT_CAN_SEEK);
 	*fstat_done = fstat_output(fd, state, statbuf);
 	result |= close(fd);
 

diff --git a/streaming.c b/streaming.c
index debe904..50599df 100644
--- a/streaming.c
+++ b/streaming.c

@@ -2,6 +2,7 @@
  * Copyright (c) 2011, Google Inc.
  */
 #include "cache.h"
+#include "object.h"
 #include "streaming.h"
 
 enum input_source {

@@ -496,19 +497,33 @@ static open_method_decl(incore)
  ****************************************************************/
 
 int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *filter,
-		      int can_seek)
+		      unsigned flags)
 {
 	struct git_istream *st;
 	enum object_type type;
 	unsigned long sz;
 	ssize_t kept = 0;
 	int result = -1;
+	int can_seek = flags & STREAMING_OUTPUT_CAN_SEEK;
+
+	int want_verify = flags & STREAMING_VERIFY_OBJECT_NAME;
+	git_SHA_CTX ctx;
 
 	st = open_istream(sha1, &type, &sz, filter);
 	if (!st)
 		return result;
 	if (type != OBJ_BLOB)
 		goto close_and_exit;
+
+	if (want_verify) {
+		char hdr[32];
+		int hdrlen;
+
+		git_SHA1_Init(&ctx);
+		hdrlen = sprintf(hdr, "%s %lu", typename(type), sz) + 1;
+		git_SHA1_Update(&ctx, hdr, hdrlen);
+	}
+
 	for (;;) {
 		char buf[1024 * 16];
 		ssize_t wrote, holeto;

@@ -518,6 +533,10 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *f
 			goto close_and_exit;
 		if (!readlen)
 			break;
+
+		if (want_verify)
+			git_SHA1_Update(&ctx, buf, readlen);
+
 		if (can_seek && sizeof(buf) == readlen) {
 			for (holeto = 0; holeto < readlen; holeto++)
 				if (buf[holeto])

@@ -542,6 +561,14 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *f
 		goto close_and_exit;
 	result = 0;
 
+	if (want_verify) {
+		unsigned char verify[20];
+
+		git_SHA1_Final(verify, &ctx);
+		if (hashcmp(verify, lookup_replace_object(sha1)))
+			result = -1;
+	}
+
  close_and_exit:
 	close_istream(st);
 	return result;

diff --git a/streaming.h b/streaming.h
index 1d05c2a..68fe3a4 100644
--- a/streaming.h
+++ b/streaming.h

@@ -12,6 +12,8 @@ extern struct git_istream *open_istream(const unsigned char *, enum object_type
 extern int close_istream(struct git_istream *);
 extern ssize_t read_istream(struct git_istream *, void *, size_t);
 
-extern int stream_blob_to_fd(int fd, const unsigned char *, struct stream_filter *, int can_seek);
+#define STREAMING_OUTPUT_CAN_SEEK 01
+#define STREAMING_VERIFY_OBJECT_NAME 02
+extern int stream_blob_to_fd(int fd, const unsigned char *, struct stream_filter *, unsigned flags);
 
 #endif /* STREAMING_H */

On Mon, Nov 25, 2013 at 10:35:19AM -0800, Junio C Hamano wrote:

quoted

 	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)
+			die("object %s sha1 mismatch", sha1_to_hex(sha1));

check_sha1_signature() opens the object again and streams the data.
Essentially the read side is doing twice the work with that patch,
isn't it?

Yes. I considered that, but I also got ~20% slow-down when just doing
commits/trees, which are in-core and can re-hash the same buffer. So
since my with-blobs numbers backed that up, I didn't think too much
further on it.

But there is something curious about the numbers I posted. It takes 12s
without the check, and 15s with the check. So the extra hashing adds 3s.
But if we are reading each blob twice, and we would expect blob reading
to be a significant chunk of that 12s, then shouldn't we expect much
more than 3s increase?

The answer must be that either we are not streaming as much as I think,
or re-reading the data is much cheaper than I expect. And I think it is
the latter.

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 never get access to
it outside of stream_blob_to_fd, and it is discarded. However, we do
keep a copy in the delta base cache. When we immediately ask to unpack
the exact same entry for check_sha1_signature, we can pull the copy
straight out of the cache without having to re-inflate the object.

After applying the patch below on top of yours, my numbers remain the
same:

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index b2ca775..e3ff677 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c

@@ -197,7 +197,7 @@ static void print_object_or_die(int fd, const unsigned char *sha1,
 				enum object_type type, unsigned long size)
 {
 	if (type == OBJ_BLOB) {
-		if (stream_blob_to_fd(fd, sha1, NULL, 0) < 0)
+		if (stream_blob_to_fd(fd, sha1, NULL, STREAMING_VERIFY_OBJECT_NAME) < 0)
 			die("unable to stream %s to stdout", sha1_to_hex(sha1));
 	}
 	else {

@@ -208,6 +208,8 @@ static void print_object_or_die(int fd, const unsigned char *sha1,
 		contents = read_sha1_file(sha1, &rtype, &rsize);
 		if (!contents)
 			die("object %s disappeared", sha1_to_hex(sha1));
+		if (check_sha1_signature(sha1, contents, rsize, typename(rtype)) < 0)
+			die("object %s sha1 mismatch", sha1_to_hex(sha1));
 		if (rtype != type)
 			die("object %s changed type!?", sha1_to_hex(sha1));
 		if (rsize != size)

I wonder if we want to extend the stream_blob_to_fd() API to
optionally allow the caller to ask to validate that the returned
data is consistent with the object name the caller asked the data
for.  Something along the lines of the attached weatherbaloon patch?

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 alternative, and not parse_object. So we are not verifying the
sha1 in either case (we may want to change that, of course, but that is
a bigger decision than just trying to bring streaming and non-streaming
code-paths into parity).

I also wondered if parse_object itself had problems with double-reading
or failing to verify. But its use goes the opposite direction; it wants
to verify the sha1 of the blob object, but it knows that it does not
actually need the data. So it streams (as of 090ea12) to check the
signature, but then discards each buffer-full after hashing it.

-Peff