[RFC PATCH 0/9] Use a structure for object IDs.

[RFC PATCH 0/9] Use a structure for object IDs.

[brian m. carlson]

This is a preliminary RFC patch series to move all the relevant uses of
unsigned char [20] to struct object_id.  It should not be applied to any
branch yet.

The goal of this series to improve type-checking in the codebase and to
make it easier to move to a different hash function if the project
decides to do that.  This series does not convert all of the codebase,
but only parts.  I'm looking for feedback to see if there is consensus
that this is the right direction before investing a large amount of
time.

Certain parts of the code have to be converted before others to keep the
patch sizes small, maintainable, and bisectable, so functions and
structures that are used across the codebase (e.g. hashcmp and struct
object) will be converted later.  Conversion has been done in a roughly
alphabetical order by name of file.

The constants for raw and hex sizes of SHA-1 values are maintained.
These constants are used where the quantity is the size of an SHA-1
value, and sizeof(struct object_id) is used wherever memory is to be
allocated.  This is done to permit the struct to turn into a union later
if multiple hashes are supported.  I left the names at GIT_OID_RAWSZ and
GIT_OID_HEXSZ because that's what libgit2 uses and what Junio seemed to
prefer, but they can be changed later if there's a desire to do that.

I called the structure member "oid" because it was easily grepable and
distinct from the rest of the codebase.  It, too, can be changed if we
decide on a better name.  I specifically did not choose "sha1" since it
looks weird to have "sha1->sha1" and I didn't want to rename lots of
variables.

Comments?

brian m. carlson (9):
  Define a structure for object IDs.
  bisect.c: convert to use struct object_id
  archive.c: convert to use struct object_id
  zip: use GIT_OID_HEXSZ for trailers
  branch.c: convert to use struct object_id
  bulk-checkin.c: convert to use struct object_id
  bundle.c: convert leaf functions to struct object_id
  cache-tree: convert struct cache_tree to use object_id
  diff: convert struct combine_diff_path to object_id

 archive-zip.c          |  4 ++--
 archive.c              | 16 +++++++--------
 archive.h              |  1 +
 bisect.c               | 30 ++++++++++++++--------------
 branch.c               | 16 +++++++--------
 builtin/commit.c       |  2 +-
 builtin/fsck.c         |  4 ++--
 bulk-checkin.c         | 12 +++++------
 bundle.c               | 38 +++++++++++++++++------------------
 cache-tree.c           | 30 ++++++++++++++--------------
 cache-tree.h           |  3 ++-
 combine-diff.c         | 54 +++++++++++++++++++++++++-------------------------
 diff-lib.c             | 10 +++++-----
 diff.h                 |  5 +++--
 merge-recursive.c      |  2 +-
 object.h               | 13 +++++++++++-
 reachable.c            |  2 +-
 sequencer.c            |  2 +-
 test-dump-cache-tree.c |  4 ++--
 19 files changed, 131 insertions(+), 117 deletions(-)

-- 
2.0.0.rc0

[PATCH 1/9] Define a structure for object IDs.

[brian m. carlson]

Many places throughout the code use "unsigned char [20]" to store object IDs
(SHA-1 values).  This leads to lots of hardcoded numbers throughout the
codebase.  It also leads to confusion about the purposes of a buffer.

Introduce a structure for object IDs.  This allows us to obtain the benefits
of compile-time checking for misuse.  The structure is expected to remain
the same size and have the same alignment requirements on all known
platforms, compared to the array of unsigned char.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 object.h | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/object.h b/object.h
index 6e12f2c..6a9680d 100644
--- a/object.h
+++ b/object.h
@@ -1,6 +1,17 @@
 #ifndef OBJECT_H
 #define OBJECT_H
 
+/*
+ * The length in bytes and in hex digits of an object name (SHA-1 value).
+ * These are the same names used by libgit2.
+ */
+#define GIT_OID_RAWSZ 20
+#define GIT_OID_HEXSZ 40
+
+struct object_id {
+	unsigned char oid[GIT_OID_RAWSZ];
+};
+
 struct object_list {
 	struct object *item;
 	struct object_list *next;
@@ -49,7 +60,7 @@ struct object {
 	unsigned used : 1;
 	unsigned type : TYPE_BITS;
 	unsigned flags : FLAG_BITS;
-	unsigned char sha1[20];
+	unsigned char sha1[GIT_OID_RAWSZ];
 };
 
 extern const char *typename(unsigned int type);
-- 
2.0.0.rc0

[PATCH 2/9] bisect.c: convert to use struct object_id

[brian m. carlson]

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 bisect.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/bisect.c b/bisect.c
index d6e851d..fe53214 100644
--- a/bisect.c
+++ b/bisect.c
@@ -15,7 +15,7 @@
 static struct sha1_array good_revs;
 static struct sha1_array skipped_revs;
 
-static unsigned char *current_bad_sha1;
+static struct object_id *current_bad_sha1;
 
 static const char *argv_checkout[] = {"checkout", "-q", NULL, "--", NULL};
 static const char *argv_show_branch[] = {"show-branch", NULL, NULL};
@@ -403,8 +403,8 @@ static int register_ref(const char *refname, const unsigned char *sha1,
 			int flags, void *cb_data)
 {
 	if (!strcmp(refname, "bad")) {
-		current_bad_sha1 = xmalloc(20);
-		hashcpy(current_bad_sha1, sha1);
+		current_bad_sha1 = xmalloc(sizeof(*current_bad_sha1));
+		hashcpy(current_bad_sha1->oid, sha1);
 	} else if (starts_with(refname, "good-")) {
 		sha1_array_append(&good_revs, sha1);
 	} else if (starts_with(refname, "skip-")) {
@@ -563,7 +563,7 @@ static struct commit_list *skip_away(struct commit_list *list, int count)
 
 	for (i = 0; cur; cur = cur->next, i++) {
 		if (i == index) {
-			if (hashcmp(cur->item->object.sha1, current_bad_sha1))
+			if (hashcmp(cur->item->object.sha1, current_bad_sha1->oid))
 				return cur;
 			if (previous)
 				return previous;
@@ -606,7 +606,7 @@ static void bisect_rev_setup(struct rev_info *revs, const char *prefix,
 
 	/* rev_argv.argv[0] will be ignored by setup_revisions */
 	argv_array_push(&rev_argv, "bisect_rev_setup");
-	argv_array_pushf(&rev_argv, bad_format, sha1_to_hex(current_bad_sha1));
+	argv_array_pushf(&rev_argv, bad_format, sha1_to_hex(current_bad_sha1->oid));
 	for (i = 0; i < good_revs.nr; i++)
 		argv_array_pushf(&rev_argv, good_format,
 				 sha1_to_hex(good_revs.sha1[i]));
@@ -627,7 +627,7 @@ static void bisect_common(struct rev_info *revs)
 }
 
 static void exit_if_skipped_commits(struct commit_list *tried,
-				    const unsigned char *bad)
+				    const struct object_id *bad)
 {
 	if (!tried)
 		return;
@@ -636,12 +636,12 @@ static void exit_if_skipped_commits(struct commit_list *tried,
 	       "The first bad commit could be any of:\n");
 	print_commit_list(tried, "%s\n", "%s\n");
 	if (bad)
-		printf("%s\n", sha1_to_hex(bad));
+		printf("%s\n", sha1_to_hex(bad->oid));
 	printf("We cannot bisect more!\n");
 	exit(2);
 }
 
-static int is_expected_rev(const unsigned char *sha1)
+static int is_expected_rev(const struct object_id *sha1)
 {
 	const char *filename = git_path("BISECT_EXPECTED_REV");
 	struct stat st;
@@ -657,7 +657,7 @@ static int is_expected_rev(const unsigned char *sha1)
 		return 0;
 
 	if (strbuf_getline(&str, fp, '\n') != EOF)
-		res = !strcmp(str.buf, sha1_to_hex(sha1));
+		res = !strcmp(str.buf, sha1_to_hex(sha1->oid));
 
 	strbuf_release(&str);
 	fclose(fp);
@@ -718,7 +718,7 @@ static struct commit **get_bad_and_good_commits(int *rev_nr)
 	struct commit **rev = xmalloc(len * sizeof(*rev));
 	int i, n = 0;
 
-	rev[n++] = get_commit_reference(current_bad_sha1);
+	rev[n++] = get_commit_reference(current_bad_sha1->oid);
 	for (i = 0; i < good_revs.nr; i++)
 		rev[n++] = get_commit_reference(good_revs.sha1[i]);
 	*rev_nr = n;
@@ -729,7 +729,7 @@ static struct commit **get_bad_and_good_commits(int *rev_nr)
 static void handle_bad_merge_base(void)
 {
 	if (is_expected_rev(current_bad_sha1)) {
-		char *bad_hex = sha1_to_hex(current_bad_sha1);
+		char *bad_hex = sha1_to_hex(current_bad_sha1->oid);
 		char *good_hex = join_sha1_array_hex(&good_revs, ' ');
 
 		fprintf(stderr, "The merge base %s is bad.\n"
@@ -749,7 +749,7 @@ static void handle_bad_merge_base(void)
 static void handle_skipped_merge_base(const unsigned char *mb)
 {
 	char *mb_hex = sha1_to_hex(mb);
-	char *bad_hex = sha1_to_hex(current_bad_sha1);
+	char *bad_hex = sha1_to_hex(current_bad_sha1->oid);
 	char *good_hex = join_sha1_array_hex(&good_revs, ' ');
 
 	warning("the merge base between %s and [%s] "
@@ -780,7 +780,7 @@ static void check_merge_bases(int no_checkout)
 
 	for (; result; result = result->next) {
 		const unsigned char *mb = result->item->object.sha1;
-		if (!hashcmp(mb, current_bad_sha1)) {
+		if (!hashcmp(mb, current_bad_sha1->oid)) {
 			handle_bad_merge_base();
 		} else if (0 <= sha1_array_lookup(&good_revs, mb)) {
 			continue;
@@ -926,7 +926,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
 		exit_if_skipped_commits(tried, NULL);
 
 		printf("%s was both good and bad\n",
-		       sha1_to_hex(current_bad_sha1));
+		       sha1_to_hex(current_bad_sha1->oid));
 		exit(1);
 	}
 
@@ -939,7 +939,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
 	bisect_rev = revs.commits->item->object.sha1;
 	memcpy(bisect_rev_hex, sha1_to_hex(bisect_rev), 41);
 
-	if (!hashcmp(bisect_rev, current_bad_sha1)) {
+	if (!hashcmp(bisect_rev, current_bad_sha1->oid)) {
 		exit_if_skipped_commits(tried, current_bad_sha1);
 		printf("%s is the first bad commit\n", bisect_rev_hex);
 		show_diff_tree(prefix, revs.commits->item);
-- 
2.0.0.rc0

[PATCH 3/9] archive.c: convert to use struct object_id

[brian m. carlson]

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 archive.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/archive.c b/archive.c
index 3fc0fb2..dba148a 100644
--- a/archive.c
+++ b/archive.c
@@ -255,7 +255,7 @@ static void parse_treeish_arg(const char **argv,
 	time_t archive_time;
 	struct tree *tree;
 	const struct commit *commit;
-	unsigned char sha1[20];
+	struct object_id sha1;
 
 	/* Remotes are only allowed to fetch actual refs */
 	if (remote && !remote_allow_unreachable) {
@@ -263,15 +263,15 @@ static void parse_treeish_arg(const char **argv,
 		const char *colon = strchrnul(name, ':');
 		int refnamelen = colon - name;
 
-		if (!dwim_ref(name, refnamelen, sha1, &ref))
+		if (!dwim_ref(name, refnamelen, sha1.oid, &ref))
 			die("no such ref: %.*s", refnamelen, name);
 		free(ref);
 	}
 
-	if (get_sha1(name, sha1))
+	if (get_sha1(name, sha1.oid))
 		die("Not a valid object name");
 
-	commit = lookup_commit_reference_gently(sha1, 1);
+	commit = lookup_commit_reference_gently(sha1.oid, 1);
 	if (commit) {
 		commit_sha1 = commit->object.sha1;
 		archive_time = commit->date;
@@ -280,21 +280,21 @@ static void parse_treeish_arg(const char **argv,
 		archive_time = time(NULL);
 	}
 
-	tree = parse_tree_indirect(sha1);
+	tree = parse_tree_indirect(sha1.oid);
 	if (tree == NULL)
 		die("not a tree object");
 
 	if (prefix) {
-		unsigned char tree_sha1[20];
+		struct object_id tree_sha1;
 		unsigned int mode;
 		int err;
 
 		err = get_tree_entry(tree->object.sha1, prefix,
-				     tree_sha1, &mode);
+				     tree_sha1.oid, &mode);
 		if (err || !S_ISDIR(mode))
 			die("current working directory is untracked");
 
-		tree = parse_tree_indirect(tree_sha1);
+		tree = parse_tree_indirect(tree_sha1.oid);
 	}
 	ar_args->tree = tree;
 	ar_args->commit_sha1 = commit_sha1;
-- 
2.0.0.rc0

[PATCH 4/9] zip: use GIT_OID_HEXSZ for trailers

[brian m. carlson]

The object.h header is included in archive.h for this constant.  It will be
used by other parts of the archiving code in the future.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 archive-zip.c | 4 ++--
 archive.h     | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/archive-zip.c b/archive-zip.c
index 4bde019..5b9fe42 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -427,12 +427,12 @@ static void write_zip_trailer(const unsigned char *sha1)
 	copy_le16(trailer.entries, zip_dir_entries);
 	copy_le32(trailer.size, zip_dir_offset);
 	copy_le32(trailer.offset, zip_offset);
-	copy_le16(trailer.comment_length, sha1 ? 40 : 0);
+	copy_le16(trailer.comment_length, sha1 ? GIT_OID_HEXSZ : 0);
 
 	write_or_die(1, zip_dir, zip_dir_offset);
 	write_or_die(1, &trailer, ZIP_DIR_TRAILER_SIZE);
 	if (sha1)
-		write_or_die(1, sha1_to_hex(sha1), 40);
+		write_or_die(1, sha1_to_hex(sha1), GIT_OID_HEXSZ);
 }
 
 static void dos_time(time_t *time, int *dos_date, int *dos_time)
diff --git a/archive.h b/archive.h
index 4a791e1..fd21408 100644
--- a/archive.h
+++ b/archive.h
@@ -2,6 +2,7 @@
 #define ARCHIVE_H
 
 #include "pathspec.h"
+#include "object.h"
 
 struct archiver_args {
 	const char *base;
-- 
2.0.0.rc0

[PATCH 5/9] branch.c: convert to use struct object_id

[brian m. carlson]

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 branch.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/branch.c b/branch.c
index 660097b..8dc0d49 100644
--- a/branch.c
+++ b/branch.c
@@ -184,9 +184,9 @@ int validate_new_branchname(const char *name, struct strbuf *ref,
 
 	if (!attr_only) {
 		const char *head;
-		unsigned char sha1[20];
+		struct object_id sha1;
 
-		head = resolve_ref_unsafe("HEAD", sha1, 0, NULL);
+		head = resolve_ref_unsafe("HEAD", sha1.oid, 0, NULL);
 		if (!is_bare_repository() && head && !strcmp(head, ref->buf))
 			die(_("Cannot force update the current branch."));
 	}
@@ -228,7 +228,7 @@ void create_branch(const char *head,
 {
 	struct ref_lock *lock = NULL;
 	struct commit *commit;
-	unsigned char sha1[20];
+	struct object_id sha1;
 	char *real_ref, msg[PATH_MAX + 20];
 	struct strbuf ref = STRBUF_INIT;
 	int forcing = 0;
@@ -248,7 +248,7 @@ void create_branch(const char *head,
 	}
 
 	real_ref = NULL;
-	if (get_sha1(start_name, sha1)) {
+	if (get_sha1(start_name, sha1.oid)) {
 		if (explicit_tracking) {
 			if (advice_set_upstream_failure) {
 				error(_(upstream_missing), start_name);
@@ -260,7 +260,7 @@ void create_branch(const char *head,
 		die(_("Not a valid object name: '%s'."), start_name);
 	}
 
-	switch (dwim_ref(start_name, strlen(start_name), sha1, &real_ref)) {
+	switch (dwim_ref(start_name, strlen(start_name), sha1.oid, &real_ref)) {
 	case 0:
 		/* Not branching from any existing branch */
 		if (explicit_tracking)
@@ -281,9 +281,9 @@ void create_branch(const char *head,
 		break;
 	}
 
-	if ((commit = lookup_commit_reference(sha1)) == NULL)
+	if ((commit = lookup_commit_reference(sha1.oid)) == NULL)
 		die(_("Not a valid branch point: '%s'."), start_name);
-	hashcpy(sha1, commit->object.sha1);
+	hashcpy(sha1.oid, commit->object.sha1);
 
 	if (!dont_change_ref) {
 		lock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL);
@@ -305,7 +305,7 @@ void create_branch(const char *head,
 		setup_tracking(ref.buf + 11, real_ref, track, quiet);
 
 	if (!dont_change_ref)
-		if (write_ref_sha1(lock, sha1, msg) < 0)
+		if (write_ref_sha1(lock, sha1.oid, msg) < 0)
 			die_errno(_("Failed to write ref"));
 
 	strbuf_release(&ref);
-- 
2.0.0.rc0

[PATCH 6/9] bulk-checkin.c: convert to use struct object_id

[brian m. carlson]

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 bulk-checkin.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/bulk-checkin.c b/bulk-checkin.c
index 98e651c..92c7b5e 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -23,7 +23,7 @@ static struct bulk_checkin_state {
 
 static void finish_bulk_checkin(struct bulk_checkin_state *state)
 {
-	unsigned char sha1[20];
+	struct object_id sha1;
 	struct strbuf packname = STRBUF_INIT;
 	int i;
 
@@ -35,11 +35,11 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)
 		unlink(state->pack_tmp_name);
 		goto clear_exit;
 	} else if (state->nr_written == 1) {
-		sha1close(state->f, sha1, CSUM_FSYNC);
+		sha1close(state->f, sha1.oid, CSUM_FSYNC);
 	} else {
-		int fd = sha1close(state->f, sha1, 0);
-		fixup_pack_header_footer(fd, sha1, state->pack_tmp_name,
-					 state->nr_written, sha1,
+		int fd = sha1close(state->f, sha1.oid, 0);
+		fixup_pack_header_footer(fd, sha1.oid, state->pack_tmp_name,
+					 state->nr_written, sha1.oid,
 					 state->offset);
 		close(fd);
 	}
@@ -47,7 +47,7 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)
 	strbuf_addf(&packname, "%s/pack/pack-", get_object_directory());
 	finish_tmp_packfile(&packname, state->pack_tmp_name,
 			    state->written, state->nr_written,
-			    &state->pack_idx_opts, sha1);
+			    &state->pack_idx_opts, sha1.oid);
 	for (i = 0; i < state->nr_written; i++)
 		free(state->written[i]);
 
-- 
2.0.0.rc0

[PATCH 7/9] bundle.c: convert leaf functions to struct object_id

[brian m. carlson]

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 bundle.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/bundle.c b/bundle.c
index 1222952..798ba28 100644
--- a/bundle.c
+++ b/bundle.c
@@ -11,11 +11,11 @@
 
 static const char bundle_signature[] = "# v2 git bundle\n";
 
-static void add_to_ref_list(const unsigned char *sha1, const char *name,
+static void add_to_ref_list(const struct object_id *sha1, const char *name,
 		struct ref_list *list)
 {
 	ALLOC_GROW(list->list, list->nr + 1, list->alloc);
-	hashcpy(list->list[list->nr].sha1, sha1);
+	hashcpy(list->list[list->nr].sha1, sha1->oid);
 	list->list[list->nr].name = xstrdup(name);
 	list->nr++;
 }
@@ -39,7 +39,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
 	/* The bundle header ends with an empty line */
 	while (!strbuf_getwholeline_fd(&buf, fd, '\n') &&
 	       buf.len && buf.buf[0] != '\n') {
-		unsigned char sha1[20];
+		struct object_id sha1;
 		int is_prereq = 0;
 
 		if (*buf.buf == '-') {
@@ -53,9 +53,9 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
 		 * Prerequisites have object name that is optionally
 		 * followed by SP and subject line.
 		 */
-		if (get_sha1_hex(buf.buf, sha1) ||
-		    (buf.len > 40 && !isspace(buf.buf[40])) ||
-		    (!is_prereq && buf.len <= 40)) {
+		if (get_sha1_hex(buf.buf, sha1.oid) ||
+		    (buf.len > GIT_OID_HEXSZ && !isspace(buf.buf[GIT_OID_HEXSZ])) ||
+		    (!is_prereq && buf.len <= GIT_OID_HEXSZ)) {
 			if (report_path)
 				error(_("unrecognized header: %s%s (%d)"),
 				      (is_prereq ? "-" : ""), buf.buf, (int)buf.len);
@@ -63,9 +63,9 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
 			break;
 		} else {
 			if (is_prereq)
-				add_to_ref_list(sha1, "", &header->prerequisites);
+				add_to_ref_list(&sha1, "", &header->prerequisites);
 			else
-				add_to_ref_list(sha1, buf.buf + 41, &header->references);
+				add_to_ref_list(&sha1, buf.buf + 41, &header->references);
 		}
 	}
 
@@ -274,16 +274,16 @@ int create_bundle(struct bundle_header *header, const char *path,
 		return -1;
 	rls_fout = xfdopen(rls.out, "r");
 	while (strbuf_getwholeline(&buf, rls_fout, '\n') != EOF) {
-		unsigned char sha1[20];
+		struct object_id sha1;
 		if (buf.len > 0 && buf.buf[0] == '-') {
 			write_or_die(bundle_fd, buf.buf, buf.len);
-			if (!get_sha1_hex(buf.buf + 1, sha1)) {
-				struct object *object = parse_object_or_die(sha1, buf.buf);
+			if (!get_sha1_hex(buf.buf + 1, sha1.oid)) {
+				struct object *object = parse_object_or_die(sha1.oid, buf.buf);
 				object->flags |= UNINTERESTING;
 				add_pending_object(&revs, object, buf.buf);
 			}
-		} else if (!get_sha1_hex(buf.buf, sha1)) {
-			struct object *object = parse_object_or_die(sha1, buf.buf);
+		} else if (!get_sha1_hex(buf.buf, sha1.oid)) {
+			struct object *object = parse_object_or_die(sha1.oid, buf.buf);
 			object->flags |= SHOWN;
 		}
 	}
@@ -302,16 +302,16 @@ int create_bundle(struct bundle_header *header, const char *path,
 
 	for (i = 0; i < revs.pending.nr; i++) {
 		struct object_array_entry *e = revs.pending.objects + i;
-		unsigned char sha1[20];
+		struct object_id sha1;
 		char *ref;
 		const char *display_ref;
 		int flag;
 
 		if (e->item->flags & UNINTERESTING)
 			continue;
-		if (dwim_ref(e->name, strlen(e->name), sha1, &ref) != 1)
+		if (dwim_ref(e->name, strlen(e->name), sha1.oid, &ref) != 1)
 			continue;
-		if (read_ref_full(e->name, sha1, 1, &flag))
+		if (read_ref_full(e->name, sha1.oid, 1, &flag))
 			flag = 0;
 		display_ref = (flag & REF_ISSYMREF) ? e->name : ref;
 
@@ -342,13 +342,13 @@ int create_bundle(struct bundle_header *header, const char *path,
 		 * commit that is referenced by the tag, and not the tag
 		 * itself.
 		 */
-		if (hashcmp(sha1, e->item->sha1)) {
+		if (hashcmp(sha1.oid, e->item->sha1)) {
 			/*
 			 * Is this the positive end of a range expressed
 			 * in terms of a tag (e.g. v2.0 from the range
 			 * "v1.0..v2.0")?
 			 */
-			struct commit *one = lookup_commit_reference(sha1);
+			struct commit *one = lookup_commit_reference(sha1.oid);
 			struct object *obj;
 
 			if (e->item == &(one->object)) {
@@ -360,7 +360,7 @@ int create_bundle(struct bundle_header *header, const char *path,
 				 * end up triggering "empty bundle"
 				 * error.
 				 */
-				obj = parse_object_or_die(sha1, e->name);
+				obj = parse_object_or_die(sha1.oid, e->name);
 				obj->flags |= SHOWN;
 				add_pending_object(&revs, obj, e->name);
 			}
-- 
2.0.0.rc0

[PATCH 8/9] cache-tree: convert struct cache_tree to use object_id

[brian m. carlson]

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/commit.c       |  2 +-
 builtin/fsck.c         |  4 ++--
 cache-tree.c           | 30 +++++++++++++++---------------
 cache-tree.h           |  3 ++-
 merge-recursive.c      |  2 +-
 reachable.c            |  2 +-
 sequencer.c            |  2 +-
 test-dump-cache-tree.c |  4 ++--
 8 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 9cfef6c..639f843 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1659,7 +1659,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		append_merge_tag_headers(parents, &tail);
 	}
 
-	if (commit_tree_extended(&sb, active_cache_tree->sha1, parents, sha1,
+	if (commit_tree_extended(&sb, active_cache_tree->sha1.oid, parents, sha1,
 				 author_ident.buf, sign_commit, extra)) {
 		rollback_index_files();
 		die(_("failed to write commit object"));
diff --git a/builtin/fsck.c b/builtin/fsck.c
index fc150c8..6854c81 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -587,10 +587,10 @@ static int fsck_cache_tree(struct cache_tree *it)
 		fprintf(stderr, "Checking cache tree\n");
 
 	if (0 <= it->entry_count) {
-		struct object *obj = parse_object(it->sha1);
+		struct object *obj = parse_object(it->sha1.oid);
 		if (!obj) {
 			error("%s: invalid sha1 pointer in cache-tree",
-			      sha1_to_hex(it->sha1));
+			      sha1_to_hex(it->sha1.oid));
 			return 1;
 		}
 		obj->used = 1;
diff --git a/cache-tree.c b/cache-tree.c
index 7fa524a..b7b2d06 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -219,7 +219,7 @@ int cache_tree_fully_valid(struct cache_tree *it)
 	int i;
 	if (!it)
 		return 0;
-	if (it->entry_count < 0 || !has_sha1_file(it->sha1))
+	if (it->entry_count < 0 || !has_sha1_file(it->sha1.oid))
 		return 0;
 	for (i = 0; i < it->subtree_nr; i++) {
 		if (!cache_tree_fully_valid(it->down[i]->cache_tree))
@@ -244,7 +244,7 @@ static int update_one(struct cache_tree *it,
 
 	*skip_count = 0;
 
-	if (0 <= it->entry_count && has_sha1_file(it->sha1))
+	if (0 <= it->entry_count && has_sha1_file(it->sha1.oid))
 		return it->entry_count;
 
 	/*
@@ -311,7 +311,7 @@ static int update_one(struct cache_tree *it,
 		struct cache_tree_sub *sub;
 		const char *path, *slash;
 		int pathlen, entlen;
-		const unsigned char *sha1;
+		const struct object_id *sha1;
 		unsigned mode;
 
 		path = ce->name;
@@ -327,21 +327,21 @@ static int update_one(struct cache_tree *it,
 				die("cache-tree.c: '%.*s' in '%s' not found",
 				    entlen, path + baselen, path);
 			i += sub->count;
-			sha1 = sub->cache_tree->sha1;
+			sha1 = &sub->cache_tree->sha1;
 			mode = S_IFDIR;
 			if (sub->cache_tree->entry_count < 0)
 				to_invalidate = 1;
 		}
 		else {
-			sha1 = ce->sha1;
+			sha1 = (struct object_id *)ce->sha1;
 			mode = ce->ce_mode;
 			entlen = pathlen - baselen;
 			i++;
 		}
-		if (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1)) {
+		if (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1->oid)) {
 			strbuf_release(&buffer);
 			return error("invalid object %06o %s for '%.*s'",
-				mode, sha1_to_hex(sha1), entlen+baselen, path);
+				mode, sha1_to_hex(sha1->oid), entlen+baselen, path);
 		}
 
 		/*
@@ -375,8 +375,8 @@ static int update_one(struct cache_tree *it,
 	}
 
 	if (dryrun)
-		hash_sha1_file(buffer.buf, buffer.len, tree_type, it->sha1);
-	else if (write_sha1_file(buffer.buf, buffer.len, tree_type, it->sha1)) {
+		hash_sha1_file(buffer.buf, buffer.len, tree_type, it->sha1.oid);
+	else if (write_sha1_file(buffer.buf, buffer.len, tree_type, it->sha1.oid)) {
 		strbuf_release(&buffer);
 		return -1;
 	}
@@ -432,7 +432,7 @@ static void write_one(struct strbuf *buffer, struct cache_tree *it,
 #endif
 
 	if (0 <= it->entry_count) {
-		strbuf_add(buffer, it->sha1, 20);
+		strbuf_add(buffer, it->sha1.oid, GIT_OID_RAWSZ);
 	}
 	for (i = 0; i < it->subtree_nr; i++) {
 		struct cache_tree_sub *down = it->down[i];
@@ -489,7 +489,7 @@ static struct cache_tree *read_one(const char **buffer, unsigned long *size_p)
 	if (0 <= it->entry_count) {
 		if (size < 20)
 			goto free_return;
-		hashcpy(it->sha1, (const unsigned char*)buf);
+		hashcpy(it->sha1.oid, (const unsigned char*)buf);
 		buf += 20;
 		size -= 20;
 	}
@@ -612,10 +612,10 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)
 			cache_tree_find(active_cache_tree, prefix);
 		if (!subtree)
 			return WRITE_TREE_PREFIX_ERROR;
-		hashcpy(sha1, subtree->sha1);
+		hashcpy(sha1, subtree->sha1.oid);
 	}
 	else
-		hashcpy(sha1, active_cache_tree->sha1);
+		hashcpy(sha1, active_cache_tree->sha1.oid);
 
 	if (0 <= newfd)
 		rollback_lock_file(lock_file);
@@ -629,7 +629,7 @@ static void prime_cache_tree_rec(struct cache_tree *it, struct tree *tree)
 	struct name_entry entry;
 	int cnt;
 
-	hashcpy(it->sha1, tree->object.sha1);
+	hashcpy(it->sha1.oid, tree->object.sha1);
 	init_tree_desc(&desc, tree->buffer, tree->size);
 	cnt = 0;
 	while (tree_entry(&desc, &entry)) {
@@ -683,7 +683,7 @@ int cache_tree_matches_traversal(struct cache_tree *root,
 
 	it = find_cache_tree_from_traversal(root, info);
 	it = cache_tree_find(it, ent->path);
-	if (it && it->entry_count > 0 && !hashcmp(ent->sha1, it->sha1))
+	if (it && it->entry_count > 0 && !hashcmp(ent->sha1, it->sha1.oid))
 		return it->entry_count;
 	return 0;
 }
diff --git a/cache-tree.h b/cache-tree.h
index f1923ad..a65231e 100644
--- a/cache-tree.h
+++ b/cache-tree.h
@@ -3,6 +3,7 @@
 
 #include "tree.h"
 #include "tree-walk.h"
+#include "object.h"
 
 struct cache_tree;
 struct cache_tree_sub {
@@ -15,7 +16,7 @@ struct cache_tree_sub {
 
 struct cache_tree {
 	int entry_count; /* negative means "invalid" */
-	unsigned char sha1[20];
+	struct object_id sha1;
 	int subtree_nr;
 	int subtree_alloc;
 	struct cache_tree_sub **down;
diff --git a/merge-recursive.c b/merge-recursive.c
index 4177092..7db772d 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -270,7 +270,7 @@ struct tree *write_tree_from_memory(struct merge_options *o)
 			      active_nr, 0) < 0)
 		die(_("error building trees"));
 
-	result = lookup_tree(active_cache_tree->sha1);
+	result = lookup_tree(active_cache_tree->sha1.oid);
 
 	return result;
 }
diff --git a/reachable.c b/reachable.c
index 654a8c5..464c5ef 100644
--- a/reachable.c
+++ b/reachable.c
@@ -177,7 +177,7 @@ static void add_cache_tree(struct cache_tree *it, struct rev_info *revs)
 	int i;
 
 	if (it->entry_count >= 0)
-		add_one_tree(it->sha1, revs);
+		add_one_tree(it->sha1.oid, revs);
 	for (i = 0; i < it->subtree_nr; i++)
 		add_cache_tree(it->down[i]->cache_tree, revs);
 }
diff --git a/sequencer.c b/sequencer.c
index bde5f04..fe48518 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -377,7 +377,7 @@ static int is_index_unchanged(void)
 				      active_nr, 0))
 			return error(_("Unable to update cache tree\n"));
 
-	return !hashcmp(active_cache_tree->sha1, head_commit->tree->object.sha1);
+	return !hashcmp(active_cache_tree->sha1.oid, head_commit->tree->object.sha1);
 }
 
 /*
diff --git a/test-dump-cache-tree.c b/test-dump-cache-tree.c
index 47eab97..9d97908 100644
--- a/test-dump-cache-tree.c
+++ b/test-dump-cache-tree.c
@@ -10,7 +10,7 @@ static void dump_one(struct cache_tree *it, const char *pfx, const char *x)
 		       "invalid", x, pfx, it->subtree_nr);
 	else
 		printf("%s %s%s (%d entries, %d subtrees)\n",
-		       sha1_to_hex(it->sha1), x, pfx,
+		       sha1_to_hex(it->sha1.oid), x, pfx,
 		       it->entry_count, it->subtree_nr);
 }
 
@@ -33,7 +33,7 @@ static int dump_cache_tree(struct cache_tree *it,
 	}
 	else {
 		dump_one(it, pfx, "");
-		if (hashcmp(it->sha1, ref->sha1) ||
+		if (hashcmp(it->sha1.oid, ref->sha1.oid) ||
 		    ref->entry_count != it->entry_count ||
 		    ref->subtree_nr != it->subtree_nr) {
 			dump_one(ref, pfx, "#(ref) ");
-- 
2.0.0.rc0

[PATCH 9/9] diff: convert struct combine_diff_path to object_id

[brian m. carlson]

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 combine-diff.c | 54 +++++++++++++++++++++++++++---------------------------
 diff-lib.c     | 10 +++++-----
 diff.h         |  5 +++--
 3 files changed, 35 insertions(+), 34 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index 24ca7e2..f97eb3a 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -34,9 +34,9 @@ static struct combine_diff_path *intersect_paths(struct combine_diff_path *curr,
 			memset(p->parent, 0,
 			       sizeof(p->parent[0]) * num_parent);
 
-			hashcpy(p->sha1, q->queue[i]->two->sha1);
+			hashcpy(p->sha1.oid, q->queue[i]->two->sha1);
 			p->mode = q->queue[i]->two->mode;
-			hashcpy(p->parent[n].sha1, q->queue[i]->one->sha1);
+			hashcpy(p->parent[n].sha1.oid, q->queue[i]->one->sha1);
 			p->parent[n].mode = q->queue[i]->one->mode;
 			p->parent[n].status = q->queue[i]->status;
 			*tail = p;
@@ -67,7 +67,7 @@ static struct combine_diff_path *intersect_paths(struct combine_diff_path *curr,
 			continue;
 		}
 
-		hashcpy(p->parent[n].sha1, q->queue[i]->one->sha1);
+		hashcpy(p->parent[n].sha1.oid, q->queue[i]->one->sha1);
 		p->parent[n].mode = q->queue[i]->one->mode;
 		p->parent[n].status = q->queue[i]->status;
 
@@ -274,7 +274,7 @@ static struct lline *coalesce_lines(struct lline *base, int *lenbase,
 	return base;
 }
 
-static char *grab_blob(const unsigned char *sha1, unsigned int mode,
+static char *grab_blob(const struct object_id *sha1, unsigned int mode,
 		       unsigned long *size, struct userdiff_driver *textconv,
 		       const char *path)
 {
@@ -284,20 +284,20 @@ static char *grab_blob(const unsigned char *sha1, unsigned int mode,
 	if (S_ISGITLINK(mode)) {
 		blob = xmalloc(100);
 		*size = snprintf(blob, 100,
-				 "Subproject commit %s\n", sha1_to_hex(sha1));
-	} else if (is_null_sha1(sha1)) {
+				 "Subproject commit %s\n", sha1_to_hex(sha1->oid));
+	} else if (is_null_sha1(sha1->oid)) {
 		/* deleted blob */
 		*size = 0;
 		return xcalloc(1, 1);
 	} else if (textconv) {
 		struct diff_filespec *df = alloc_filespec(path);
-		fill_filespec(df, sha1, 1, mode);
+		fill_filespec(df, sha1->oid, 1, mode);
 		*size = fill_textconv(textconv, df, &blob);
 		free_filespec(df);
 	} else {
-		blob = read_sha1_file(sha1, &type, size);
+		blob = read_sha1_file(sha1->oid, &type, size);
 		if (type != OBJ_BLOB)
-			die("object '%s' is not a blob!", sha1_to_hex(sha1));
+			die("object '%s' is not a blob!", sha1_to_hex(sha1->oid));
 	}
 	return blob;
 }
@@ -379,7 +379,7 @@ static void consume_line(void *state_, char *line, unsigned long len)
 	}
 }
 
-static void combine_diff(const unsigned char *parent, unsigned int mode,
+static void combine_diff(const struct object_id *parent, unsigned int mode,
 			 mmfile_t *result_file,
 			 struct sline *sline, unsigned int cnt, int n,
 			 int num_parent, int result_deleted,
@@ -904,11 +904,11 @@ static void show_combined_header(struct combine_diff_path *elem,
 			 "", elem->path, line_prefix, c_meta, c_reset);
 	printf("%s%sindex ", line_prefix, c_meta);
 	for (i = 0; i < num_parent; i++) {
-		abb = find_unique_abbrev(elem->parent[i].sha1,
+		abb = find_unique_abbrev(elem->parent[i].sha1.oid,
 					 abbrev);
 		printf("%s%s", i ? "," : "", abb);
 	}
-	abb = find_unique_abbrev(elem->sha1, abbrev);
+	abb = find_unique_abbrev(elem->sha1.oid, abbrev);
 	printf("..%s%s\n", abb, c_reset);
 
 	if (mode_differs) {
@@ -981,7 +981,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 
 	/* Read the result of merge first */
 	if (!working_tree_file)
-		result = grab_blob(elem->sha1, elem->mode, &result_size,
+		result = grab_blob(&elem->sha1, elem->mode, &result_size,
 				   textconv, elem->path);
 	else {
 		/* Used by diff-tree to read from the working tree */
@@ -1003,12 +1003,12 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			result = strbuf_detach(&buf, NULL);
 			elem->mode = canon_mode(st.st_mode);
 		} else if (S_ISDIR(st.st_mode)) {
-			unsigned char sha1[20];
-			if (resolve_gitlink_ref(elem->path, "HEAD", sha1) < 0)
-				result = grab_blob(elem->sha1, elem->mode,
+			struct object_id sha1;
+			if (resolve_gitlink_ref(elem->path, "HEAD", sha1.oid) < 0)
+				result = grab_blob(&elem->sha1, elem->mode,
 						   &result_size, NULL, NULL);
 			else
-				result = grab_blob(sha1, elem->mode,
+				result = grab_blob(&sha1, elem->mode,
 						   &result_size, NULL, NULL);
 		} else if (textconv) {
 			struct diff_filespec *df = alloc_filespec(elem->path);
@@ -1080,7 +1080,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 		for (i = 0; !is_binary && i < num_parent; i++) {
 			char *buf;
 			unsigned long size;
-			buf = grab_blob(elem->parent[i].sha1,
+			buf = grab_blob(&elem->parent[i].sha1,
 					elem->parent[i].mode,
 					&size, NULL, NULL);
 			if (buffer_is_binary(buf, size))
@@ -1129,14 +1129,14 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 	for (i = 0; i < num_parent; i++) {
 		int j;
 		for (j = 0; j < i; j++) {
-			if (!hashcmp(elem->parent[i].sha1,
-				     elem->parent[j].sha1)) {
+			if (!hashcmp(elem->parent[i].sha1.oid,
+				     elem->parent[j].sha1.oid)) {
 				reuse_combine_diff(sline, cnt, i, j);
 				break;
 			}
 		}
 		if (i <= j)
-			combine_diff(elem->parent[i].sha1,
+			combine_diff(&elem->parent[i].sha1,
 				     elem->parent[i].mode,
 				     &result_file, sline,
 				     cnt, i, num_parent, result_deleted,
@@ -1196,9 +1196,9 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
 
 		/* Show sha1's */
 		for (i = 0; i < num_parent; i++)
-			printf(" %s", diff_unique_abbrev(p->parent[i].sha1,
+			printf(" %s", diff_unique_abbrev(p->parent[i].sha1.oid,
 							 opt->abbrev));
-		printf(" %s ", diff_unique_abbrev(p->sha1, opt->abbrev));
+		printf(" %s ", diff_unique_abbrev(p->sha1.oid, opt->abbrev));
 	}
 
 	if (opt->output_format & (DIFF_FORMAT_RAW | DIFF_FORMAT_NAME_STATUS)) {
@@ -1261,16 +1261,16 @@ static struct diff_filepair *combined_pair(struct combine_diff_path *p,
 	for (i = 0; i < num_parent; i++) {
 		pair->one[i].path = p->path;
 		pair->one[i].mode = p->parent[i].mode;
-		hashcpy(pair->one[i].sha1, p->parent[i].sha1);
-		pair->one[i].sha1_valid = !is_null_sha1(p->parent[i].sha1);
+		hashcpy(pair->one[i].sha1, p->parent[i].sha1.oid);
+		pair->one[i].sha1_valid = !is_null_sha1(p->parent[i].sha1.oid);
 		pair->one[i].has_more_entries = 1;
 	}
 	pair->one[num_parent - 1].has_more_entries = 0;
 
 	pair->two->path = p->path;
 	pair->two->mode = p->mode;
-	hashcpy(pair->two->sha1, p->sha1);
-	pair->two->sha1_valid = !is_null_sha1(p->sha1);
+	hashcpy(pair->two->sha1, p->sha1.oid);
+	pair->two->sha1_valid = !is_null_sha1(p->sha1.oid);
 	return pair;
 }
 
diff --git a/diff-lib.c b/diff-lib.c
index 0448729..4b74a02 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -124,7 +124,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			dpath->next = NULL;
 			memcpy(dpath->path, ce->name, path_len);
 			dpath->path[path_len] = '\0';
-			hashclr(dpath->sha1);
+			hashclr(dpath->sha1.oid);
 			memset(&(dpath->parent[0]), 0,
 			       sizeof(struct combine_diff_parent)*5);
 
@@ -154,7 +154,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 				if (2 <= stage) {
 					int mode = nce->ce_mode;
 					num_compare_stages++;
-					hashcpy(dpath->parent[stage-2].sha1, nce->sha1);
+					hashcpy(dpath->parent[stage-2].sha1.oid, nce->sha1);
 					dpath->parent[stage-2].mode = ce_mode_from_stat(nce, mode);
 					dpath->parent[stage-2].status =
 						DIFF_STATUS_MODIFIED;
@@ -326,14 +326,14 @@ static int show_modified(struct rev_info *revs,
 		memcpy(p->path, new->name, pathlen);
 		p->path[pathlen] = 0;
 		p->mode = mode;
-		hashclr(p->sha1);
+		hashclr(p->sha1.oid);
 		memset(p->parent, 0, 2 * sizeof(struct combine_diff_parent));
 		p->parent[0].status = DIFF_STATUS_MODIFIED;
 		p->parent[0].mode = new->ce_mode;
-		hashcpy(p->parent[0].sha1, new->sha1);
+		hashcpy(p->parent[0].sha1.oid, new->sha1);
 		p->parent[1].status = DIFF_STATUS_MODIFIED;
 		p->parent[1].mode = old->ce_mode;
-		hashcpy(p->parent[1].sha1, old->sha1);
+		hashcpy(p->parent[1].sha1.oid, old->sha1);
 		show_combined_diff(p, 2, revs->dense_combined_merges, revs);
 		free(p);
 		return 0;
diff --git a/diff.h b/diff.h
index a24a767..38bb1ed 100644
--- a/diff.h
+++ b/diff.h
@@ -6,6 +6,7 @@
 
 #include "tree-walk.h"
 #include "pathspec.h"
+#include "object.h"
 
 struct rev_info;
 struct diff_options;
@@ -200,11 +201,11 @@ struct combine_diff_path {
 	struct combine_diff_path *next;
 	char *path;
 	unsigned int mode;
-	unsigned char sha1[20];
+	struct object_id sha1;
 	struct combine_diff_parent {
 		char status;
 		unsigned int mode;
-		unsigned char sha1[20];
+		struct object_id sha1;
 	} parent[FLEX_ARRAY];
 };
 #define combine_diff_path_size(n, l) \
-- 
2.0.0.rc0

Re: [RFC PATCH 0/9] Use a structure for object IDs.

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1197 bytes --]

On Sat, May 03, 2014 at 08:12:13PM +0000, brian m. carlson wrote:
> This is a preliminary RFC patch series to move all the relevant uses of
> unsigned char [20] to struct object_id.  It should not be applied to any
> branch yet.
> 
> The goal of this series to improve type-checking in the codebase and to
> make it easier to move to a different hash function if the project
> decides to do that.  This series does not convert all of the codebase,
> but only parts.  I'm looking for feedback to see if there is consensus
> that this is the right direction before investing a large amount of
> time.

I would like to point out that to get something with as few calls as
hashclr converted to use struct object_id requires an insane amount of
work, because often major parts of several files have to be converted
first.  So the list should be aware that this will likely be an
extensive series, although it is bisectable, so it could theoretically
be done in batches.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/9] Define a structure for object IDs.

[Michael Haggerty]

On 05/03/2014 10:12 PM, brian m. carlson wrote:
> Many places throughout the code use "unsigned char [20]" to store object IDs
> (SHA-1 values).  This leads to lots of hardcoded numbers throughout the
> codebase.  It also leads to confusion about the purposes of a buffer.
> 
> Introduce a structure for object IDs.  This allows us to obtain the benefits
> of compile-time checking for misuse.  The structure is expected to remain
> the same size and have the same alignment requirements on all known
> platforms, compared to the array of unsigned char.

Please clarify whether you plan to rely on all platforms having "the
same size and alignment constraints" for correctness, or whether that
observation of the status quo is only meant to reassure us that this
change won't cause memory to be wasted on padding.

If the former then I would feel very uncomfortable about the change.
Otherwise I think it will be a nice improvement in code clarity (and I
admire your ambition in taking on this project!)

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [RFC PATCH 0/9] Use a structure for object IDs.

[Michael Haggerty]

On 05/03/2014 10:12 PM, brian m. carlson wrote:
> This is a preliminary RFC patch series to move all the relevant uses of
> unsigned char [20] to struct object_id.  It should not be applied to any
> branch yet.
> 
> The goal of this series to improve type-checking in the codebase and to
> make it easier to move to a different hash function if the project
> decides to do that.  This series does not convert all of the codebase,
> but only parts.  I'm looking for feedback to see if there is consensus
> that this is the right direction before investing a large amount of
> time.
> 
> Certain parts of the code have to be converted before others to keep the
> patch sizes small, maintainable, and bisectable, so functions and
> structures that are used across the codebase (e.g. hashcmp and struct
> object) will be converted later.  Conversion has been done in a roughly
> alphabetical order by name of file.
> 
> The constants for raw and hex sizes of SHA-1 values are maintained.
> These constants are used where the quantity is the size of an SHA-1
> value, and sizeof(struct object_id) is used wherever memory is to be
> allocated.  This is done to permit the struct to turn into a union later
> if multiple hashes are supported.  I left the names at GIT_OID_RAWSZ and
> GIT_OID_HEXSZ because that's what libgit2 uses and what Junio seemed to
> prefer, but they can be changed later if there's a desire to do that.
> 
> I called the structure member "oid" because it was easily grepable and
> distinct from the rest of the codebase.  It, too, can be changed if we
> decide on a better name.  I specifically did not choose "sha1" since it
> looks weird to have "sha1->sha1" and I didn't want to rename lots of
> variables.

That means that we will have sha1->oid all over the place, right?
That's unfortunate, because it is exactly backwards from what we would
want in a hypothetical future where OIDs are not necessarily SHA-1s.  In
that future we would certainly have to support SHA-1s in parallel with
the new hash.  So (in that hypothetical future) we will probably want
these expressions to look like oid->sha1, to allow, say, a second struct
or union field oid->sha256 [1].

If that future would come to pass, then we would also want to have
distinct constants like GIT_SHA1_RAWSZ and GIT_SHA256_RAWSZ rather than
the generically-named GIT_OID_RAWSZ.

I think that this patch series will improve the code clarity and type
safety independent of thoughts about supporting different hash
algorithms, so I'm not objecting to your naming decision.  But *if* such
support is part of your long-term hope, then you might ease the future
transition by choosing different names now.

(Maybe renaming local variables "sha1 -> oid" might be a handy way of
making clear which code has been converted to the new style.)

Just to be clear, the above are just some random thoughts for your
consideration, but feel free to disregard them.

In any case, it sure will be a lot of code churn.  If you succeed in
this project, then "git blame" will probably consider you the author of
about 2/3 of git :-)

Michael

[1] I'm certainly not advocating that we want to support a different
hash, let alone that that hash should be SHA-256; these examples are
just for illustration.

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [RFC PATCH 0/9] Use a structure for object IDs.

[Johannes Sixt]

Am 04.05.2014 08:35, schrieb Michael Haggerty:
> On 05/03/2014 10:12 PM, brian m. carlson wrote:
>> I specifically did not choose "sha1" since it
>> looks weird to have "sha1->sha1" and I didn't want to rename lots of
>> variables.
>
> That means that we will have sha1->oid all over the place, right?

Only during the transition period. When all functions that currently take 
unsigned char[20] are converted to struct object_id *, this additional 
dereferences go away again.

-- Hannes

Re: [PATCH 1/9] Define a structure for object IDs.

[Johannes Sixt]

Am 04.05.2014 08:07, schrieb Michael Haggerty:
> On 05/03/2014 10:12 PM, brian m. carlson wrote:
>> Introduce a structure for object IDs.  This allows us to obtain the benefits
>> of compile-time checking for misuse.  The structure is expected to remain
>> the same size and have the same alignment requirements on all known
>> platforms, compared to the array of unsigned char.
>
> Please clarify whether you plan to rely on all platforms having "the
> same size and alignment constraints" for correctness, or whether that
> observation of the status quo is only meant to reassure us that this
> change won't cause memory to be wasted on padding.

I think that a compiler that has different size and alignment requirements 
for the proposed struct object_id and an unsigned char[20] would, strictly 
speaking, not be a "C" compiler.

-- Hannes

Re: [PATCH 1/9] Define a structure for object IDs.

[David Kastrup]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 04.05.2014 08:07, schrieb Michael Haggerty:
>> On 05/03/2014 10:12 PM, brian m. carlson wrote:
>>> Introduce a structure for object IDs.  This allows us to obtain the benefits
>>> of compile-time checking for misuse.  The structure is expected to remain
>>> the same size and have the same alignment requirements on all known
>>> platforms, compared to the array of unsigned char.
>>
>> Please clarify whether you plan to rely on all platforms having "the
>> same size and alignment constraints" for correctness, or whether that
>> observation of the status quo is only meant to reassure us that this
>> change won't cause memory to be wasted on padding.
>
> I think that a compiler that has different size and alignment
> requirements for the proposed struct object_id and an unsigned
> char[20] would, strictly speaking, not be a "C" compiler.

Huh?  How so?  There is no warranty as far as I know that a structure
with only a single member has the same size and alignment requirements
as the single member would have.  There is also no guarantee as far as I
know that anything but element dereference is a valid means of
converting access to a struct to access to a sole element.

-- 
David Kastrup

Re: [PATCH 1/9] Define a structure for object IDs.

[Andreas Schwab]

Johannes Sixt <j6t@kdbg.org> writes:

> I think that a compiler that has different size and alignment requirements
> for the proposed struct object_id and an unsigned char[20] would, strictly
> speaking, not be a "C" compiler.

Unlike arrays, a struct can have arbitrary internal padding.  It is
perfectly compliant (and even reasonable) to make struct object_id
require 8 byte alignment, adding 4 bytes of padding at the end.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 1/9] Define a structure for object IDs.

[Duy Nguyen]

On Sun, May 4, 2014 at 1:07 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 05/03/2014 10:12 PM, brian m. carlson wrote:
>> Many places throughout the code use "unsigned char [20]" to store object IDs
>> (SHA-1 values).  This leads to lots of hardcoded numbers throughout the
>> codebase.  It also leads to confusion about the purposes of a buffer.
>>
>> Introduce a structure for object IDs.  This allows us to obtain the benefits
>> of compile-time checking for misuse.  The structure is expected to remain
>> the same size and have the same alignment requirements on all known
>> platforms, compared to the array of unsigned char.
>
> Please clarify whether you plan to rely on all platforms having "the
> same size and alignment constraints" for correctness, or whether that
> observation of the status quo is only meant to reassure us that this
> change won't cause memory to be wasted on padding.

It's not just about wasted padding. Some structs, like
ondisk_cache_entry, reflect on-disk format. Padding means breakage.
But I don't think this will be a big issue because we can detect if
padding happens, abort to force the user to complain, and deal with it
on case-by-case basis.
-- 
Duy

Re: [PATCH 1/9] Define a structure for object IDs.

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 948 bytes --]

On Sun, May 04, 2014 at 08:07:26AM +0200, Michael Haggerty wrote:
> Please clarify whether you plan to rely on all platforms having "the
> same size and alignment constraints" for correctness, or whether that
> observation of the status quo is only meant to reassure us that this
> change won't cause memory to be wasted on padding.

I plan to write the code portably.  My statement was basically that I
don't expect this to result in more memory being used.  I don't even
plan to write the code assuming that offsetof(struct object_id, oid) is
0.

I have owned SPARC systems, and I have experienced plenty of aggravation
with code that makes unportable alignment assumptions.  I don't want to
make that mistake myself.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/9] Define a structure for object IDs.

[Andreas Schwab]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> I don't even plan to write the code assuming that offsetof(struct
> object_id, oid) is 0.

This is guaranteed by the C standard, though.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 1/9] Define a structure for object IDs.

[David Kastrup]

Andreas Schwab <schwab@linux-m68k.org> writes:

> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
>> I don't even plan to write the code assuming that offsetof(struct
>> object_id, oid) is 0.
>
> This is guaranteed by the C standard, though.

Any reference?

-- 
David Kastrup

Re: [PATCH 1/9] Define a structure for object IDs.

[Andreas Schwab]

David Kastrup <dak@gnu.org> writes:

> Andreas Schwab <schwab@linux-m68k.org> writes:
>
>> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>>
>>> I don't even plan to write the code assuming that offsetof(struct
>>> object_id, oid) is 0.
>>
>> This is guaranteed by the C standard, though.
>
> Any reference?

§6.7.2.1#15

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 1/9] Define a structure for object IDs.

[David Kastrup]

Andreas Schwab <schwab@linux-m68k.org> writes:

> David Kastrup <dak@gnu.org> writes:
>
>> Andreas Schwab <schwab@linux-m68k.org> writes:
>>
>>> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>>>
>>>> I don't even plan to write the code assuming that offsetof(struct
>>>> object_id, oid) is 0.
>>>
>>> This is guaranteed by the C standard, though.
>>
>> Any reference?
>
> §6.7.2.1#15

More like #13.  I am pretty sure, however, that this has not always been
the case.

-- 
David Kastrup

Re: [RFC PATCH 0/9] Use a structure for object IDs.

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2408 bytes --]

On Sun, May 04, 2014 at 08:35:00AM +0200, Michael Haggerty wrote:
> On 05/03/2014 10:12 PM, brian m. carlson wrote:
> > I called the structure member "oid" because it was easily grepable and
> > distinct from the rest of the codebase.  It, too, can be changed if we
> > decide on a better name.  I specifically did not choose "sha1" since it
> > looks weird to have "sha1->sha1" and I didn't want to rename lots of
> > variables.
> 
> That means that we will have sha1->oid all over the place, right?
> That's unfortunate, because it is exactly backwards from what we would
> want in a hypothetical future where OIDs are not necessarily SHA-1s.  In
> that future we would certainly have to support SHA-1s in parallel with
> the new hash.  So (in that hypothetical future) we will probably want
> these expressions to look like oid->sha1, to allow, say, a second struct
> or union field oid->sha256 [1].

As Johannes pointed out, only during the transition period.

> If that future would come to pass, then we would also want to have
> distinct constants like GIT_SHA1_RAWSZ and GIT_SHA256_RAWSZ rather than
> the generically-named GIT_OID_RAWSZ.

You have a point.  I'll make the change.

> I think that this patch series will improve the code clarity and type
> safety independent of thoughts about supporting different hash
> algorithms, so I'm not objecting to your naming decision.  But *if* such
> support is part of your long-term hope, then you might ease the future
> transition by choosing different names now.

It is an eventual goal, but without this series, it's not even worth
discussing since it's too hard to implement.  Even if that doesn't
happen, my hope is that we'll at least improve the safety of the code
and hopefully avoid a bug or two out of it.

> (Maybe renaming local variables "sha1 -> oid" might be a handy way of
> making clear which code has been converted to the new style.)

This is a good idea as well.  I'll walk through the patches and fix
that.

> Just to be clear, the above are just some random thoughts for your
> consideration, but feel free to disregard them.

I appreciate the well-thought-out response.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/9] Define a structure for object IDs.

[Andreas Schwab]

David Kastrup <dak@gnu.org> writes:

> Andreas Schwab <schwab@linux-m68k.org> writes:
>
>> David Kastrup <dak@gnu.org> writes:
>>
>>> Andreas Schwab <schwab@linux-m68k.org> writes:
>>>
>>>> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>>>>
>>>>> I don't even plan to write the code assuming that offsetof(struct
>>>>> object_id, oid) is 0.
>>>>
>>>> This is guaranteed by the C standard, though.
>>>
>>> Any reference?
>>
>> §6.7.2.1#15
>
> More like #13.

I don't know what you mean, but this is a C11 reference.

> I am pretty sure, however, that this has not always been the case.

You are wrong.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 1/9] Define a structure for object IDs.

[Johannes Sixt]

Am 04.05.2014 12:55, schrieb Andreas Schwab:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>> I think that a compiler that has different size and alignment requirements
>> for the proposed struct object_id and an unsigned char[20] would, strictly
>> speaking, not be a "C" compiler.
> 
> Unlike arrays, a struct can have arbitrary internal padding.  It is
> perfectly compliant (and even reasonable) to make struct object_id
> require 8 byte alignment, adding 4 bytes of padding at the end.

Isn't internal padding only allowed between members to achieve correct
alignment of later members, and at the end only sufficient padding so
that members are aligned correctly when the struct is part of an array?
The former would not be the case because there is only one member, and
the latter is not the case because a char or array of char does not have
alignment requirement?

-- Hannes

Re: [PATCH 1/9] Define a structure for object IDs.

[Andreas Schwab]

Johannes Sixt <j6t@kdbg.org> writes:

> Isn't internal padding only allowed between members to achieve correct
> alignment of later members, and at the end only sufficient padding so
> that members are aligned correctly when the struct is part of an array?

The standard allows arbitrary internal padding, it doesn't have to be
minimal.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 1/9] Define a structure for object IDs.

[David Kastrup]

Andreas Schwab <schwab@linux-m68k.org> writes:

> Johannes Sixt <j6t@kdbg.org> writes:
>
>> Isn't internal padding only allowed between members to achieve correct
>> alignment of later members, and at the end only sufficient padding so
>> that members are aligned correctly when the struct is part of an array?
>
> The standard allows arbitrary internal padding, it doesn't have to be
> minimal.

What the standard does guarantee is that a pointer to a struct can be
cast to a pointer to its first member and vice versa.  It does not as
far as I can see guarantee that a pointer to something of the same type
of its first member can be converted to a pointer to a struct even if
the struct only contains a member of such type.

-- 
David Kastrup

Re: [PATCH 1/9] Define a structure for object IDs.

[Andreas Schwab]

David Kastrup <dak@gnu.org> writes:

> It does not as far as I can see guarantee that a pointer to something
> of the same type of its first member can be converted to a pointer to
> a struct even if the struct only contains a member of such type.

This sentence doesn't make any sense.  If you have an object of struct
type then any pointer to the first member of the object can only be a
pointer to the one and same object.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 1/9] Define a structure for object IDs.

[James Denholm]

On 5 May 2014 19:23:06 GMT+10:00, Andreas Schwab <schwab@linux-m68k.org> wrote:
>David Kastrup <dak@gnu.org> writes:
>
>> It does not as far as I can see guarantee that a pointer to something
>> of the same type of its first member can be converted to a pointer to
>> a struct even if the struct only contains a member of such type.
>
>This sentence doesn't make any sense.  If you have an object of struct
>type then any pointer to the first member of the object can only be a
>pointer to the one and same object.

I think what David means is that a pointer to a wrapper
can be derefed into its internal, sure, but an object of
that internal type can't necessarily pretend to be a
wrapper.

That said, obviously I'm not David, so I could be wrong.
That's what I got from his statement, though.

Regards,
James Denholm.

Re: [PATCH 1/9] Define a structure for object IDs.

[David Kastrup]

Andreas Schwab <schwab@linux-m68k.org> writes:

> David Kastrup <dak@gnu.org> writes:
>
>> It does not as far as I can see guarantee that a pointer to something
>> of the same type of its first member can be converted to a pointer to
>> a struct even if the struct only contains a member of such type.
>
> This sentence doesn't make any sense.

I disagree.

> If you have an object of struct type

Your premise is _not_ assumed in my statement.  My premise was "a
pointer to something of the same type of [the struct's] first member".
That does quite explicitly _not_ state that an object of struct type is
in existence.

> then any pointer to the first member of the object can only be a
> pointer to the one and same object.

The case we are talking about is basically passing a pointer to some
actual bonafide toplevel unsigned char [20] object to a routine that
expects a pointer to a struct _only_ containing one such
unsigned char [20] element.

This is the situation we have to deal with if a caller has not been
converted to using such a struct, but the called function does.

More seriously, this is the situation we have to deal with when our SHA1
is actually embedded in some header or whatever else that is actually
available only inside of a larger byte buffer.

In that case, the standard does not permit us converting the address
where that SHA1 is into a pointer to struct.  It may well be that this
will fall under the "let's ignore the standard and write for "sensible"
compilers/architectures" dictum, but if it doesn't, it might be
necessary to first copy the data to a struct before passing it to
routines expecting a pointer to struct.

-- 
David Kastrup

Re: [PATCH 1/9] Define a structure for object IDs.

[Michael Haggerty]

On 05/05/2014 11:50 AM, David Kastrup wrote:
> The case we are talking about is basically passing a pointer to some
> actual bonafide toplevel unsigned char [20] object to a routine that
> expects a pointer to a struct _only_ containing one such
> unsigned char [20] element.
> 
> This is the situation we have to deal with if a caller has not been
> converted to using such a struct, but the called function does.

If the rewrite is done by first changing data structures and then
changing functions in caller -> callee order then (1) the deltas can be
pretty small, and (2) such illegal casting should be unnecessary.

> More seriously, this is the situation we have to deal with when our SHA1
> is actually embedded in some header or whatever else that is actually
> available only inside of a larger byte buffer.
> 
> In that case, the standard does not permit us converting the address
> where that SHA1 is into a pointer to struct.  It may well be that this
> will fall under the "let's ignore the standard and write for "sensible"
> compilers/architectures" dictum, but if it doesn't, it might be
> necessary to first copy the data to a struct before passing it to
> routines expecting a pointer to struct.

This sounds dangerous even for a "sensible" compiler.  For example, I
can imagine that a sensible compiler might make the assumption that a
sha1 field that it knows was obtained from oid->sha1 is word-aligned,
and generate optimized code based on that assumption, even though it
otherwise wouldn't have had trouble working with unaligned (unsigned
char *) pointers.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH 1/9] Define a structure for object IDs.

[Andreas Schwab]

David Kastrup <dak@gnu.org> writes:

> Your premise is _not_ assumed in my statement.  My premise was "a
> pointer to something of the same type of [the struct's] first member".
> That does quite explicitly _not_ state that an object of struct type is
> in existence.

So you are not taking about struct object_id, and it's irrelevant to
this thread.

This thread is about objects of type struct object_id, and their address
is always the same as the address of its first member.  Nothing else is
relevant.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH 1/9] Define a structure for object IDs.

[David Kastrup]

Andreas Schwab <schwab@linux-m68k.org> writes:

> David Kastrup <dak@gnu.org> writes:
>
>> Your premise is _not_ assumed in my statement.  My premise was "a
>> pointer to something of the same type of [the struct's] first member".
>> That does quite explicitly _not_ state that an object of struct type is
>> in existence.
>
> So you are not taking about struct object_id, and it's irrelevant to
> this thread.
>
> This thread is about objects of type struct object_id, and their address
> is always the same as the address of its first member.  Nothing else is
> relevant.

Have it your way.  I am too old for selective quotation games.

-- 
David Kastrup

Re: [PATCH 1/9] Define a structure for object IDs.

[Felipe Contreras]

Andreas Schwab wrote:
> This thread is about objects of type struct object_id, and their
> address is always the same as the address of its first member.
> Nothing else is relevant.

Indeed. I suggest you ingore that guy, he will only derail the
discussion.

-- 
Felipe Contreras

Re: [PATCH 7/9] bundle.c: convert leaf functions to struct object_id

[Michael Haggerty]

On 05/03/2014 10:12 PM, brian m. carlson wrote:
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  bundle.c | 38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/bundle.c b/bundle.c
> index 1222952..798ba28 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -11,11 +11,11 @@
>  
>  static const char bundle_signature[] = "# v2 git bundle\n";
>  
> -static void add_to_ref_list(const unsigned char *sha1, const char *name,
> +static void add_to_ref_list(const struct object_id *sha1, const char *name,
>  		struct ref_list *list)
>  {
>  	ALLOC_GROW(list->list, list->nr + 1, list->alloc);
> -	hashcpy(list->list[list->nr].sha1, sha1);
> +	hashcpy(list->list[list->nr].sha1, sha1->oid);
>  	list->list[list->nr].name = xstrdup(name);
>  	list->nr++;
>  }
> @@ -39,7 +39,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
>  	/* The bundle header ends with an empty line */
>  	while (!strbuf_getwholeline_fd(&buf, fd, '\n') &&
>  	       buf.len && buf.buf[0] != '\n') {
> -		unsigned char sha1[20];
> +		struct object_id sha1;
>  		int is_prereq = 0;
>  
>  		if (*buf.buf == '-') {
> @@ -53,9 +53,9 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
>  		 * Prerequisites have object name that is optionally
>  		 * followed by SP and subject line.
>  		 */
> -		if (get_sha1_hex(buf.buf, sha1) ||
> -		    (buf.len > 40 && !isspace(buf.buf[40])) ||
> -		    (!is_prereq && buf.len <= 40)) {
> +		if (get_sha1_hex(buf.buf, sha1.oid) ||
> +		    (buf.len > GIT_OID_HEXSZ && !isspace(buf.buf[GIT_OID_HEXSZ])) ||
> +		    (!is_prereq && buf.len <= GIT_OID_HEXSZ)) {
>  			if (report_path)
>  				error(_("unrecognized header: %s%s (%d)"),
>  				      (is_prereq ? "-" : ""), buf.buf, (int)buf.len);
> @@ -63,9 +63,9 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
>  			break;
>  		} else {
>  			if (is_prereq)
> -				add_to_ref_list(sha1, "", &header->prerequisites);
> +				add_to_ref_list(&sha1, "", &header->prerequisites);
>  			else
> -				add_to_ref_list(sha1, buf.buf + 41, &header->references);
> +				add_to_ref_list(&sha1, buf.buf + 41, &header->references);

I think that 41 here is GIT_OID_HEXSZ + 1.

> [...]

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH 8/9] cache-tree: convert struct cache_tree to use object_id

[Michael Haggerty]

On 05/03/2014 10:12 PM, brian m. carlson wrote:
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  builtin/commit.c       |  2 +-
>  builtin/fsck.c         |  4 ++--
>  cache-tree.c           | 30 +++++++++++++++---------------
>  cache-tree.h           |  3 ++-
>  merge-recursive.c      |  2 +-
>  reachable.c            |  2 +-
>  sequencer.c            |  2 +-
>  test-dump-cache-tree.c |  4 ++--
>  8 files changed, 25 insertions(+), 24 deletions(-)
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 9cfef6c..639f843 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1659,7 +1659,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  		append_merge_tag_headers(parents, &tail);
>  	}
>  
> -	if (commit_tree_extended(&sb, active_cache_tree->sha1, parents, sha1,
> +	if (commit_tree_extended(&sb, active_cache_tree->sha1.oid, parents, sha1,
>  				 author_ident.buf, sign_commit, extra)) {
>  		rollback_index_files();
>  		die(_("failed to write commit object"));
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index fc150c8..6854c81 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -587,10 +587,10 @@ static int fsck_cache_tree(struct cache_tree *it)
>  		fprintf(stderr, "Checking cache tree\n");
>  
>  	if (0 <= it->entry_count) {
> -		struct object *obj = parse_object(it->sha1);
> +		struct object *obj = parse_object(it->sha1.oid);
>  		if (!obj) {
>  			error("%s: invalid sha1 pointer in cache-tree",
> -			      sha1_to_hex(it->sha1));
> +			      sha1_to_hex(it->sha1.oid));
>  			return 1;
>  		}
>  		obj->used = 1;
> diff --git a/cache-tree.c b/cache-tree.c
> index 7fa524a..b7b2d06 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -219,7 +219,7 @@ int cache_tree_fully_valid(struct cache_tree *it)
>  	int i;
>  	if (!it)
>  		return 0;
> -	if (it->entry_count < 0 || !has_sha1_file(it->sha1))
> +	if (it->entry_count < 0 || !has_sha1_file(it->sha1.oid))
>  		return 0;
>  	for (i = 0; i < it->subtree_nr; i++) {
>  		if (!cache_tree_fully_valid(it->down[i]->cache_tree))
> @@ -244,7 +244,7 @@ static int update_one(struct cache_tree *it,
>  
>  	*skip_count = 0;
>  
> -	if (0 <= it->entry_count && has_sha1_file(it->sha1))
> +	if (0 <= it->entry_count && has_sha1_file(it->sha1.oid))
>  		return it->entry_count;
>  
>  	/*
> @@ -311,7 +311,7 @@ static int update_one(struct cache_tree *it,
>  		struct cache_tree_sub *sub;
>  		const char *path, *slash;
>  		int pathlen, entlen;
> -		const unsigned char *sha1;
> +		const struct object_id *sha1;
>  		unsigned mode;
>  
>  		path = ce->name;
> @@ -327,21 +327,21 @@ static int update_one(struct cache_tree *it,
>  				die("cache-tree.c: '%.*s' in '%s' not found",
>  				    entlen, path + baselen, path);
>  			i += sub->count;
> -			sha1 = sub->cache_tree->sha1;
> +			sha1 = &sub->cache_tree->sha1;
>  			mode = S_IFDIR;
>  			if (sub->cache_tree->entry_count < 0)
>  				to_invalidate = 1;
>  		}
>  		else {
> -			sha1 = ce->sha1;
> +			sha1 = (struct object_id *)ce->sha1;

This topic was discussed on the mailing list in the abstract.  Here is a
concrete example.

This cast is undefined, because you can't make the assumption that
cache_entry::sha1 has the same alignment and padding as (struct object_id).

I think the transition will be more tractable if you rewrite the data
structures *first*; in this case changing cache_entry::sha1 to be
(struct object_id) *before* rewriting code that works with it.

> [...]

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH 8/9] cache-tree: convert struct cache_tree to use object_id

[Michael Haggerty]

On 05/03/2014 10:12 PM, brian m. carlson wrote:
> [...]
> diff --git a/cache-tree.c b/cache-tree.c
> index 7fa524a..b7b2d06 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c

In this file I also found a couple other "20" that could be converted to
GIT_OID_RAWSZ:

Around line 369:
		strbuf_add(&buffer, sha1, 20);

And around line 490 (three instances):
		if (size < 20)
			goto free_return;
		hashcpy(it->sha1, (const unsigned char*)buf);
		buf += 20;
		size -= 20;

I guess a search for "\<[24][0-9]\>" will find most (but not all!) of
the literal constants that are derived from GIT_OID_RAWSZ and GIT_OID_HEXSZ.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH 9/9] diff: convert struct combine_diff_path to object_id

[Michael Haggerty]

On 05/03/2014 10:12 PM, brian m. carlson wrote:
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  combine-diff.c | 54 +++++++++++++++++++++++++++---------------------------
>  diff-lib.c     | 10 +++++-----
>  diff.h         |  5 +++--
>  3 files changed, 35 insertions(+), 34 deletions(-)
> 
> diff --git a/combine-diff.c b/combine-diff.c
> index 24ca7e2..f97eb3a 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> [...]

This file also has two literal "40" constants in it that are probably
GIT_OID_HEXSZ.

FWIW, I glanced over all of the patches in this series (though without
systematically looking for other literal constants that should be
derived from GIT_OID_RAWSZ and GIT_OID_HEXSZ) and, aside from the
problems that I already noted, they looked OK to me.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/