[RFC] Don't expect verify_pack() callers to set pack_size

[RFC] Don't expect verify_pack() callers to set pack_size

[Mike Hommey]

Since use_pack() will end up populating pack_size if it is not already set,
we can just adapt the code in verify_packfile() such that it doesn't require
pack_size to be set beforehand.

This allows callers not to have to set pack_size themselves, and we can thus
revert changes from 1c23d794.
---

 I'd like some feedback on this change in pack-check.c, that help refactoring
 more efficiently the code in http-*.c.

 http-push.c   |    3 ---
 http-walker.c |    1 -
 pack-check.c  |    8 +++++---
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/http-push.c b/http-push.c
index b2b410d..be54ed3 100644
--- a/http-push.c
+++ b/http-push.c
@@ -770,14 +770,11 @@ static void finish_request(struct transfer_request *request)
 				request->url, curl_errorstr);
 			remote->can_update_info_refs = 0;
 		} else {
-			off_t pack_size = ftell(request->local_stream);
-
 			fclose(request->local_stream);
 			request->local_stream = NULL;
 			if (!move_temp_to_file(request->tmpfile,
 					       request->filename)) {
 				target = (struct packed_git *)request->userData;
-				target->pack_size = pack_size;
 				lst = &remote->packs;
 				while (*lst != target)
 					lst = &((*lst)->next);
diff --git a/http-walker.c b/http-walker.c
index 2c37868..29f9728 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -783,7 +783,6 @@ static int fetch_pack(struct walker *walker, struct alt_base *repo, unsigned cha
 		return error("Unable to start request");
 	}
 
-	target->pack_size = ftell(packfile);
 	fclose(packfile);
 
 	ret = move_temp_to_file(tmpfile, filename);
diff --git a/pack-check.c b/pack-check.c
index d7dd62b..e7f0126 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -25,7 +25,7 @@ static int verify_packfile(struct packed_git *p,
 	const unsigned char *index_base = p->index_data;
 	SHA_CTX ctx;
 	unsigned char sha1[20];
-	off_t offset = 0, pack_sig = p->pack_size - 20;
+	off_t offset = 0, pack_sig = 0;
 	uint32_t nr_objects, i;
 	int err;
 	struct idx_entry *entries;
@@ -37,14 +37,16 @@ static int verify_packfile(struct packed_git *p,
 	 */
 
 	SHA1_Init(&ctx);
-	while (offset < pack_sig) {
+	do {
 		unsigned int remaining;
 		unsigned char *in = use_pack(p, w_curs, offset, &remaining);
 		offset += remaining;
+		if (pack_sig == 0)
+			pack_sig = p->pack_size - 20;
 		if (offset > pack_sig)
 			remaining -= (unsigned int)(offset - pack_sig);
 		SHA1_Update(&ctx, in, remaining);
-	}
+	} while (offset < pack_sig);
 	SHA1_Final(sha1, &ctx);
 	if (hashcmp(sha1, use_pack(p, w_curs, pack_sig, NULL)))
 		return error("Packfile %s SHA1 mismatch with itself",
-- 
1.5.4.rc5.12.gb8680-dirty

Re: [RFC] Don't expect verify_pack() callers to set pack_size

[Shawn O. Pearce]

Mike Hommey <mh@glandium.org> wrote:
> Since use_pack() will end up populating pack_size if it is not already set,
> we can just adapt the code in verify_packfile() such that it doesn't require
> pack_size to be set beforehand.
> 
> This allows callers not to have to set pack_size themselves, and we can thus
> revert changes from 1c23d794.

That's a good improvement in code clarity.  When I implemented
1c23 I wasn't sure what was initialized in the struct packed_git
that the HTTP code was slugging around in request->userData, and
it was hard to track down.  1c23 was a quick fix to get HTTP fetch
working again when sp/mmap merged into next.

I'm guessing you have figured out it is not actually open at this
point, which makes sense as we did fclose() calls to close out
the writer.  So sha1_file.c needs to open a new file descriptor.
use_pack() will force it to be opened and the open packfile function
does update the pack_size if we don't have it yet.

Acked-by: Shawn O. Pearce <spearce@spearce.org>

> diff --git a/pack-check.c b/pack-check.c
> index d7dd62b..e7f0126 100644
> --- a/pack-check.c
> +++ b/pack-check.c
> @@ -37,14 +37,16 @@ static int verify_packfile(struct packed_git *p,
>  	 */
>  
>  	SHA1_Init(&ctx);
> -	while (offset < pack_sig) {
> +	do {
>  		unsigned int remaining;
>  		unsigned char *in = use_pack(p, w_curs, offset, &remaining);
>  		offset += remaining;
> +		if (pack_sig == 0)
> +			pack_sig = p->pack_size - 20;

fyi, we prefer "if (!pack_sig)" in Git for equality with 0 tests.

-- 
Shawn.