Re: [PATCH v2 01/16] refs: convert struct ref_entry to use struct object_id
On Wed, Apr 22, 2015 at 05:52:09PM -0700, Stefan Beller wrote: > 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)? This is true. However, I'll clean those up with a future patch series when we get to that point. I'll need to pass through the documentation as well to make it accurate and consistent, and I'll want to discuss the words we want to use before I make those changes. > That said I'll focus on the type checking part in this review > and not annotate the SHA1s I find any more. ;) Please do comment on any hardcoded 20s or 40s (or quantities based off those), as I do want to fix those up. I want to fix up any hardcoded assumptions we may have about the hash that don't involve text or documentation at this point, if only for maintainability reasons. -- 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 signature.asc Description: Digital signature
Re: [PATCH v2 01/16] refs: convert struct ref_entry to use struct object_id
On Wed, Apr 22, 2015 at 4:24 PM, brian m. carlson
wrote:
> Signed-off-by: brian m. carlson
> ---
> 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. ;)
>
> 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.v
[PATCH v2 01/16] refs: convert struct ref_entry to use struct object_id
Signed-off-by: brian m. carlson
---
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);
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
