threads / patch / 26933

patch, 4 partswrite_idx_file should use an unsigned nr_objects parameter

Subject: [PATCH 1/4] write_idx_file should use an unsigned nr_objects parameter

## tl;dr

6 messages between Mar 31, 2011 and Apr 1, 2011. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Dan McGee· Mar 31, 2011, 01:24 UTC · lore

This follows the precedent set in the pack-objects code and being adjusted for in index-pack and unpack-objects.

Signed-off-by: Dan McGee <dpmcgee@gmail.com>
---
 pack-write.c |    6 +++---
 pack.h       |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)
Show changes to 2 files +4 −4

pack-write.c, pack.h

diff --git a/pack-write.c b/pack-write.c
index a905ca4..c2de03a 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -18,15 +18,15 @@ static int sha1_compare(const void *_a, const void *_b)
  * will be sorted by SHA1 on exit.
  */
 const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects,
-			   int nr_objects, unsigned char *sha1)
+			   uint32_t nr_objects, unsigned char *sha1)
 {
 	struct sha1file *f;
 	struct pack_idx_entry **sorted_by_sha, **list, **last;
 	off_t last_obj_offset = 0;
 	uint32_t array[256];
-	int i, fd;
+	uint32_t i, index_version;
+	int fd;
 	git_SHA_CTX ctx;
-	uint32_t index_version;
 
 	if (nr_objects) {
 		sorted_by_sha = objects;
diff --git a/pack.h b/pack.h
index bb27576..c588454 100644
--- a/pack.h
+++ b/pack.h
@@ -55,7 +55,7 @@ struct pack_idx_entry {
 	off_t offset;
 };
 
-extern const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects, int nr_objects, unsigned char *sha1);
+extern const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects, uint32_t nr_objects, unsigned char *sha1);
 extern int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offset, off_t len, unsigned int nr);
 extern int verify_pack_index(struct packed_git *);
 extern int verify_pack(struct packed_git *);

-- 
1.7.4.2
Dan McGee· Mar 31, 2011, 01:24 UTC · re: Dan McGee · lore

[PATCH 2/4] Use uint32_t for unpack-objects counters

This is a follow-up to the very old commit 7cadf491c6d7a2. The fixes are relatively simple (unsigned -> uint32_t) and we can also turn a static global (nr_objects) into a passed variable since it is used in very few places.

Signed-off-by: Dan McGee <dpmcgee@gmail.com>
---
 builtin/unpack-objects.c |   38 ++++++++++++++++++++------------------
 1 files changed, 20 insertions(+), 18 deletions(-)
Show changes to builtin/unpack-objects.c +20 −18
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index f63973c..a9e681e 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -124,7 +124,7 @@ static void *get_data(unsigned long size)
 
 struct delta_info {
 	unsigned char base_sha1[20];
-	unsigned nr;
+	uint32_t nr;
 	off_t base_offset;
 	unsigned long size;
 	void *delta;
@@ -133,7 +133,7 @@ struct delta_info {
 
 static struct delta_info *delta_list;
 
-static void add_delta_to_list(unsigned nr, unsigned const char *base_sha1,
+static void add_delta_to_list(uint32_t nr, unsigned const char *base_sha1,
 			      off_t base_offset,
 			      void *delta, unsigned long size)
 {
@@ -158,7 +158,6 @@ struct obj_info {
 #define FLAG_WRITTEN (1u<<21)
 
 static struct obj_info *obj_list;
-static unsigned nr_objects;
 
 /*
  * Called only from check_object() after it verified this object
@@ -206,16 +205,16 @@ static int check_object(struct object *obj, int type, void *data)
 	return 0;
 }
 
-static void write_rest(void)
+static void write_rest(uint32_t nr_objects)
 {
-	unsigned i;
+	uint32_t i;
 	for (i = 0; i < nr_objects; i++) {
 		if (obj_list[i].obj)
 			check_object(obj_list[i].obj, OBJ_ANY, NULL);
 	}
 }
 
-static void added_object(unsigned nr, enum object_type type,
+static void added_object(uint32_t nr, enum object_type type,
 			 void *data, unsigned long size);
 
 /*
@@ -223,7 +222,7 @@ static void added_object(unsigned nr, enum object_type type,
  * of it.  Under --strict, this buffers structured objects in-core,
  * to be checked at the end.
  */
-static void write_object(unsigned nr, enum object_type type,
+static void write_object(uint32_t nr, enum object_type type,
 			 void *buf, unsigned long size)
 {
 	if (!strict) {
@@ -259,7 +258,7 @@ static void write_object(unsigned nr, enum object_type type,
 	}
 }
 
-static void resolve_delta(unsigned nr, enum object_type type,
+static void resolve_delta(uint32_t nr, enum object_type type,
 			  void *base, unsigned long base_size,
 			  void *delta, unsigned long delta_size)
 {
@@ -279,7 +278,7 @@ static void resolve_delta(unsigned nr, enum object_type type,
  * We now know the contents of an object (which is nr-th in the pack);
  * resolve all the deltified objects that are based on it.
  */
-static void added_object(unsigned nr, enum object_type type,
+static void added_object(uint32_t nr, enum object_type type,
 			 void *data, unsigned long size)
 {
 	struct delta_info **p = &delta_list;
@@ -300,7 +299,7 @@ static void added_object(unsigned nr, enum object_type type,
 }
 
 static void unpack_non_delta_entry(enum object_type type, unsigned long size,
-				   unsigned nr)
+				   uint32_t nr)
 {
 	void *buf = get_data(size);
 
@@ -310,7 +309,7 @@ static void unpack_non_delta_entry(enum object_type type, unsigned long size,
 		free(buf);
 }
 
-static int resolve_against_held(unsigned nr, const unsigned char *base,
+static int resolve_against_held(uint32_t nr, const unsigned char *base,
 				void *delta_data, unsigned long delta_size)
 {
 	struct object *obj;
@@ -327,7 +326,7 @@ static int resolve_against_held(unsigned nr, const unsigned char *base,
 }
 
 static void unpack_delta_entry(enum object_type type, unsigned long delta_size,
-			       unsigned nr)
+			       uint32_t nr)
 {
 	void *delta_data, *base;
 	unsigned long base_size;
@@ -356,7 +355,7 @@ static void unpack_delta_entry(enum object_type type, unsigned long delta_size,
 		unsigned base_found = 0;
 		unsigned char *pack, c;
 		off_t base_offset;
-		unsigned lo, mid, hi;
+		uint32_t lo, mid, hi;
 
 		pack = fill(1);
 		c = *pack;
@@ -421,7 +420,7 @@ static void unpack_delta_entry(enum object_type type, unsigned long delta_size,
 	free(base);
 }
 
-static void unpack_one(unsigned nr)
+static void unpack_one(uint32_t nr)
 {
 	unsigned shift;
 	unsigned char *pack;
@@ -464,9 +463,9 @@ static void unpack_one(unsigned nr)
 	}
 }
 
-static void unpack_all(void)
+static uint32_t unpack_all(void)
 {
-	int i;
+	uint32_t i, nr_objects;
 	struct progress *progress = NULL;
 	struct pack_header *hdr = fill(sizeof(struct pack_header));
 
@@ -490,11 +489,14 @@ static void unpack_all(void)
 
 	if (delta_list)
 		die("unresolved deltas left after unpacking");
+
+	return nr_objects;
 }
 
 int cmd_unpack_objects(int argc, const char **argv, const char *prefix)
 {
 	int i;
+	uint32_t nr_objects;
 	unsigned char sha1[20];
 
 	read_replace_refs = 0;
@@ -545,11 +547,11 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix)
 		usage(unpack_usage);
 	}
 	git_SHA1_Init(&ctx);
-	unpack_all();
+	nr_objects = unpack_all();
 	git_SHA1_Update(&ctx, buffer, offset);
 	git_SHA1_Final(sha1, &ctx);
 	if (strict)
-		write_rest();
+		write_rest(nr_objects);
 	if (hashcmp(fill(20), sha1))
 		die("final sha1 did not match");
 	use(20);

-- 
1.7.4.2
Dan McGee· Mar 31, 2011, 01:24 UTC · re: Dan McGee · lore

[PATCH 3/4] Use uint32_t for index-pack counters

This is a bit more complex than the fixes needed for unpack-objects. A few very old methods were refactored to eliminate the need for negative value returns or usage.

Signed-off-by: Dan McGee <dpmcgee@gmail.com>
---
 builtin/index-pack.c |   89 ++++++++++++++++++++++++-------------------------
 1 files changed, 44 insertions(+), 45 deletions(-)
Show changes to builtin/index-pack.c +44 −45
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 5a67c81..38e61db 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -45,16 +45,16 @@ struct base_data {
 
 struct delta_entry {
 	union delta_base base;
-	int obj_no;
+	uint32_t obj_no;
 };
 
 static struct object_entry *objects;
 static struct delta_entry *deltas;
 static struct base_data *base_cache;
 static size_t base_cache_used;
-static int nr_objects;
-static int nr_deltas;
-static int nr_resolved_deltas;
+static uint32_t nr_objects;
+static uint32_t nr_deltas;
+static uint32_t nr_resolved_deltas;
 
 static int from_stdin;
 static int strict;
@@ -389,42 +389,40 @@ static void *get_data_from_pack(struct object_entry *obj)
 	return data;
 }
 
-static int find_delta(const union delta_base *base)
-{
-	int first = 0, last = nr_deltas;
-
-        while (first < last) {
-                int next = (first + last) / 2;
-                struct delta_entry *delta = &deltas[next];
-                int cmp;
-
-                cmp = memcmp(base, &delta->base, UNION_BASE_SZ);
-                if (!cmp)
-                        return next;
-                if (cmp < 0) {
-                        last = next;
-                        continue;
-                }
-                first = next+1;
-        }
-        return -first-1;
-}
-
 static void find_delta_children(const union delta_base *base,
-				int *first_index, int *last_index)
+				uint32_t *first_index, uint32_t *last_index)
 {
-	int first = find_delta(base);
-	int last = first;
-	int end = nr_deltas - 1;
+	uint32_t first = 0, last = nr_deltas, next;
 
-	if (first < 0) {
-		*first_index = 0;
-		*last_index = -1;
+	while (first < last) {
+		int cmp;
+		struct delta_entry *delta;
+
+		next = (first + last) / 2;
+		delta = &deltas[next];
+
+		cmp = memcmp(base, &delta->base, UNION_BASE_SZ);
+		if (!cmp)
+			break;
+		if (cmp < 0) {
+			last = next;
+			continue;
+		}
+		first = next + 1;
+	}
+
+	if (first >= last) {
+		/* return non-sensical first/last to show we found nothing */
+		*first_index = 1;
+		*last_index = 0;
 		return;
 	}
+
+	last = first = next;
+
 	while (first > 0 && !memcmp(&deltas[first - 1].base, base, UNION_BASE_SZ))
 		--first;
-	while (last < end && !memcmp(&deltas[last + 1].base, base, UNION_BASE_SZ))
+	while (last < (nr_deltas - 1) && !memcmp(&deltas[last + 1].base, base, UNION_BASE_SZ))
 		++last;
 	*first_index = first;
 	*last_index = last;
@@ -531,7 +529,7 @@ static void resolve_delta(struct object_entry *delta_obj,
 static void find_unresolved_deltas(struct base_data *base,
 				   struct base_data *prev_base)
 {
-	int i, ref_first, ref_last, ofs_first, ofs_last;
+	uint32_t i, ref_first, ref_last, ofs_first, ofs_last;
 
 	/*
 	 * This is a recursive function. Those brackets should help reducing
@@ -548,7 +546,7 @@ static void find_unresolved_deltas(struct base_data *base,
 		find_delta_children(&base_spec, &ofs_first, &ofs_last);
 	}
 
-	if (ref_last == -1 && ofs_last == -1) {
+	if (ref_last < ref_first && ofs_last < ofs_first) {
 		free(base->data);
 		return;
 	}
@@ -560,7 +558,7 @@ static void find_unresolved_deltas(struct base_data *base,
 		if (child->real_type == OBJ_REF_DELTA) {
 			struct base_data result;
 			resolve_delta(child, base, &result);
-			if (i == ref_last && ofs_last == -1)
+			if (i == ref_last && ofs_last < ofs_first)
 				free_base_data(base);
 			find_unresolved_deltas(&result, base);
 		}
@@ -590,7 +588,7 @@ static int compare_delta_entry(const void *a, const void *b)
 /* Parse all objects and return the pack content SHA1 hash */
 static void parse_pack_objects(unsigned char *sha1)
 {
-	int i;
+	uint32_t i;
 	struct delta_entry *delta = deltas;
 	struct stat st;
 
@@ -615,7 +613,7 @@ static void parse_pack_objects(unsigned char *sha1)
 		} else
 			sha1_object(data, obj->size, obj->type, obj->idx.sha1);
 		free(data);
-		display_progress(progress, i+1);
+		display_progress(progress, i + 1);
 	}
 	objects[i].idx.offset = consumed_bytes;
 	stop_progress(&progress);
@@ -726,10 +724,10 @@ static int delta_pos_compare(const void *_a, const void *_b)
 	return a->obj_no - b->obj_no;
 }
 
-static void fix_unresolved_deltas(struct sha1file *f, int nr_unresolved)
+static void fix_unresolved_deltas(struct sha1file *f, uint32_t nr_unresolved)
 {
 	struct delta_entry **sorted_by_pos;
-	int i, n = 0;
+	uint32_t i, n = 0;
 
 	/*
 	 * Since many unresolved deltas may well be themselves base objects
@@ -979,16 +977,17 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 			struct sha1file *f;
 			unsigned char read_sha1[20], tail_sha1[20];
 			char msg[48];
-			int nr_unresolved = nr_deltas - nr_resolved_deltas;
-			int nr_objects_initial = nr_objects;
-			if (nr_unresolved <= 0)
+			uint32_t nr_unresolved, nr_objects_initial;
+			if (nr_resolved_deltas > nr_deltas)
 				die("confusion beyond insanity");
+			nr_unresolved = nr_deltas - nr_resolved_deltas;
+			nr_objects_initial = nr_objects;
 			objects = xrealloc(objects,
 					   (nr_objects + nr_unresolved + 1)
 					   * sizeof(*objects));
 			f = sha1fd(output_fd, curr_pack);
 			fix_unresolved_deltas(f, nr_unresolved);
-			sprintf(msg, "completed with %d local objects",
+			sprintf(msg, "completed with %u local objects",
 				nr_objects - nr_objects_initial);
 			stop_progress_msg(&progress, msg);
 			sha1close(f, tail_sha1, 0);
@@ -1001,7 +1000,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 				    "(disk corruption?)", curr_pack);
 		}
 		if (nr_deltas != nr_resolved_deltas)
-			die("pack has %d unresolved deltas",
+			die("pack has %u unresolved deltas",
 			    nr_deltas - nr_resolved_deltas);
 	}
 	free(deltas);

-- 
1.7.4.2
Dan McGee· Mar 31, 2011, 01:24 UTC · re: Dan McGee · lore

[PATCH 4/4] Unify pack header checking between index-pack and unpack-objects

Both of these builtins share a lot of common code; taking steps to bring them closer together before a likely future commit to unify some of the shared code.

Signed-off-by: Dan McGee <dpmcgee@gmail.com>
---
 builtin/unpack-objects.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
Show changes to builtin/unpack-objects.c +3 −3
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index a9e681e..a0dde7f 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -471,10 +471,10 @@ static uint32_t unpack_all(void)
 
 	nr_objects = ntohl(hdr->hdr_entries);
 
-	if (ntohl(hdr->hdr_signature) != PACK_SIGNATURE)
-		die("bad pack file");
+	if (hdr->hdr_signature != htonl(PACK_SIGNATURE))
+		die("pack signature mismatch");
 	if (!pack_version_ok(hdr->hdr_version))
-		die("unknown pack file version %"PRIu32,
+		die("pack version %"PRIu32" unsupported",
 			ntohl(hdr->hdr_version));
 	use(sizeof(struct pack_header));
 

-- 
1.7.4.2
Junio C Hamano· Apr 1, 2011, 18:28 UTC · re: Dan McGee · lore

Re: [PATCH 1/4] write_idx_file should use an unsigned nr_objects parameter

Dan McGee <dpmcgee@gmail.com> writes:
> This follows the precedent set in the pack-objects code and being
> adjusted for in index-pack and unpack-objects.

Eh, why? The use of a fixed-width type in the existing code is mostly to make sure that the on-disk result will fit within the on-disk field. The variables like iteration counter "i" we use in write_idx_file() need to be at least as wide but there is no reason to forbid the compiler from using the natural interger type as long as it is more suiable on the platform, no?

Dan McGee· Apr 1, 2011, 18:36 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/4] write_idx_file should use an unsigned nr_objects parameter

On Fri, Apr 1, 2011 at 1:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> Dan McGee <dpmcgee@gmail.com> writes:
>
>> This follows the precedent set in the pack-objects code and being
>> adjusted for in index-pack and unpack-objects.
>
> Eh, why?  The use of a fixed-width type in the existing code is mostly to
> make sure that the on-disk result will fit within the on-disk field.  The
> variables like iteration counter "i" we use in write_idx_file() need to be
> at least as wide but there is no reason to forbid the compiler from using
> the natural interger type as long as it is more suiable on the platform,
> no?

Hmm. when making this patch, I thought it was required due to the other ones in this sequence or I was seeing compile errors. Reverting it seems to prove otherwise, as things compile just fine...

Either way, it should at least be an unsigned parameter of at least int length, no? The current parameter is signed, so 'unsigned' might make sense here.

-Dan

← back to recent threads