Re: [PATCH v2 01/16] refs: convert struct ref_entry to use struct object_id
From: Stefan Beller <hidden>
Date: 2016-06-15 23:04:31
On Wed, Apr 22, 2015 at 4:24 PM, brian m. carlson [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Signed-off-by: brian m. carlson <redacted> --- refs.c | 44 ++++++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 22 deletions(-)diff --git a/refs.c b/refs.c index 81a455b..522d15d 100644 --- a/refs.c +++ b/refs.c@@ -156,7 +156,7 @@ struct ref_value { * null. If REF_ISSYMREF, then this is the name of the object * referred to by the last reference in the symlink chain. */ - unsigned char sha1[20]; + struct object_id oid; /* * If REF_KNOWS_PEELED, then this field holds the peeled value@@ -164,7 +164,7 @@ struct ref_value { * be peelable. See the documentation for peel_ref() for an * exact definition of "peelable". */ - unsigned char peeled[20]; + struct object_id peeled; }; struct ref_cache;@@ -346,8 +346,8 @@ static struct ref_entry *create_ref_entry(const char *refname, die("Reference has invalid format: '%s'", refname); len = strlen(refname) + 1; ref = xmalloc(sizeof(struct ref_entry) + len); - hashcpy(ref->u.value.sha1, sha1); - hashclr(ref->u.value.peeled); + hashcpy(ref->u.value.oid.hash, sha1); + oidclr(&ref->u.value.peeled); memcpy(ref->name, refname, len); ref->flag = flag; return ref;@@ -621,7 +621,7 @@ static int is_dup_ref(const struct ref_entry *ref1, const struct ref_entry *ref2 /* This is impossible by construction */ die("Reference directory conflict: %s", ref1->name); - if (hashcmp(ref1->u.value.sha1, ref2->u.value.sha1)) + if (oidcmp(&ref1->u.value.oid, &ref2->u.value.oid)) die("Duplicated ref, and SHA1s don't match: %s", ref1->name);
So you're switching the code for a possible future In an earlier series/cover letter you wrote
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've dropped some of the patches from earlier (which no longer apply) and added others.
Which yields the question if you also want to take care of the error message (It may not be a SHA1 any more but some $HASHFUNCTION)? That said I'll focus on the type checking part in this review and not annotate the SHA1s I find any more. ;)
quoted hunk ↗ jump to hunk
warning("Duplicated ref: %s", ref1->name);@@ -669,7 +669,7 @@ static int ref_resolves_to_object(struct ref_entry *entry) { if (entry->flag & REF_ISBROKEN) return 0; - if (!has_sha1_file(entry->u.value.sha1)) { + if (!has_sha1_file(entry->u.value.oid.hash)) { error("%s does not point to a valid object!", entry->name); return 0; }@@ -717,7 +717,7 @@ static int do_one_ref(struct ref_entry *entry, void *cb_data) /* Store the old value, in case this is a recursive call: */ old_current_ref = current_ref; current_ref = entry; - retval = data->fn(entry->name + data->trim, entry->u.value.sha1, + retval = data->fn(entry->name + data->trim, entry->u.value.oid.hash, entry->flag, data->cb_data); current_ref = old_current_ref; return retval;@@ -1193,7 +1193,7 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir) line.len == PEELED_LINE_LENGTH && line.buf[PEELED_LINE_LENGTH - 1] == '\n' && !get_sha1_hex(line.buf + 1, sha1)) { - hashcpy(last->u.value.peeled, sha1); + hashcpy(last->u.value.peeled.hash, sha1); /* * Regardless of what the file header said, * we definitely know the value of *this*@@ -1374,7 +1374,7 @@ static int resolve_gitlink_packed_ref(struct ref_cache *refs, if (ref == NULL) return -1; - hashcpy(sha1, ref->u.value.sha1); + hashcpy(sha1, ref->u.value.oid.hash); return 0; }@@ -1461,7 +1461,7 @@ static int resolve_missing_loose_ref(const char *refname, */ entry = get_packed_ref(refname); if (entry) { - hashcpy(sha1, entry->u.value.sha1); + hashcpy(sha1, entry->u.value.oid.hash); if (flags) *flags |= REF_ISPACKED; return 0;@@ -1771,9 +1771,9 @@ static enum peel_status peel_entry(struct ref_entry *entry, int repeel) if (entry->flag & REF_KNOWS_PEELED) { if (repeel) { entry->flag &= ~REF_KNOWS_PEELED; - hashclr(entry->u.value.peeled); + oidclr(&entry->u.value.peeled); } else { - return is_null_sha1(entry->u.value.peeled) ? + return is_null_oid(&entry->u.value.peeled) ? PEEL_NON_TAG : PEEL_PEELED; } }@@ -1782,7 +1782,7 @@ static enum peel_status peel_entry(struct ref_entry *entry, int repeel) if (entry->flag & REF_ISSYMREF) return PEEL_IS_SYMREF; - status = peel_object(entry->u.value.sha1, entry->u.value.peeled); + status = peel_object(entry->u.value.oid.hash, entry->u.value.peeled.hash); if (status == PEEL_PEELED || status == PEEL_NON_TAG) entry->flag |= REF_KNOWS_PEELED; return status;@@ -1797,7 +1797,7 @@ int peel_ref(const char *refname, unsigned char *sha1) || !strcmp(current_ref->name, refname))) { if (peel_entry(current_ref, 0)) return -1; - hashcpy(sha1, current_ref->u.value.peeled); + hashcpy(sha1, current_ref->u.value.peeled.hash); return 0; }@@ -1817,7 +1817,7 @@ int peel_ref(const char *refname, unsigned char *sha1) if (r) { if (peel_entry(r, 0)) return -1; - hashcpy(sha1, r->u.value.peeled); + hashcpy(sha1, r->u.value.peeled.hash); return 0; } }@@ -2422,9 +2422,9 @@ static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data) if (peel_status != PEEL_PEELED && peel_status != PEEL_NON_TAG) error("internal error: %s is not a valid packed reference!", entry->name); - write_packed_entry(cb_data, entry->name, entry->u.value.sha1, + write_packed_entry(cb_data, entry->name, entry->u.value.oid.hash, peel_status == PEEL_PEELED ? - entry->u.value.peeled : NULL); + entry->u.value.peeled.hash : NULL); return 0; }@@ -2531,24 +2531,24 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data) peel_status = peel_entry(entry, 1); if (peel_status != PEEL_PEELED && peel_status != PEEL_NON_TAG) die("internal error peeling reference %s (%s)", - entry->name, sha1_to_hex(entry->u.value.sha1)); + entry->name, oid_to_hex(&entry->u.value.oid)); packed_entry = find_ref(cb->packed_refs, entry->name); if (packed_entry) { /* Overwrite existing packed entry with info from loose entry */ packed_entry->flag = REF_ISPACKED | REF_KNOWS_PEELED; - hashcpy(packed_entry->u.value.sha1, entry->u.value.sha1); + oidcpy(&packed_entry->u.value.oid, &entry->u.value.oid); } else { - packed_entry = create_ref_entry(entry->name, entry->u.value.sha1, + packed_entry = create_ref_entry(entry->name, entry->u.value.oid.hash, REF_ISPACKED | REF_KNOWS_PEELED, 0); add_ref(cb->packed_refs, packed_entry); } - hashcpy(packed_entry->u.value.peeled, entry->u.value.peeled); + oidcpy(&packed_entry->u.value.peeled, &entry->u.value.peeled); /* Schedule the loose reference for pruning if requested. */ if ((cb->flags & PACK_REFS_PRUNE)) { int namelen = strlen(entry->name) + 1; struct ref_to_prune *n = xcalloc(1, sizeof(*n) + namelen); - hashcpy(n->sha1, entry->u.value.sha1); + hashcpy(n->sha1, entry->u.value.oid.hash); strcpy(n->name, entry->name); n->next = cb->ref_to_prune; cb->ref_to_prune = n; --2.3.5
The patch is Reviewed-by: Stefan Beller <redacted>
-- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html