threads / patch / 36577

patch, 9 partsDefine a structure for object IDs.

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

## tl;dr

39 messages between May 3, 2014 and May 6, 2014. Diffs are folded; open one to read it.

replies: 38people: 8as markdown or json

brian m. carlson· May 3, 2014, 20:12 UTC · lore

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

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
brian m. carlson· May 3, 2014, 20:12 UTC · re: brian m. carlson · lore

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(-)
Show changes to object.h +12 −1
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
Michael Haggerty· May 4, 2014, 06:07 UTC · re: brian m. carlson · lore

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

On 05/03/2014 10:12 PM, brian m. carlson wrote:
Show 8 quoted lines
> 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/
Johannes Sixt· May 4, 2014, 09:21 UTC · re: Michael Haggerty · lore

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

Am 04.05.2014 08:07, schrieb Michael Haggerty:
Show 10 quoted lines
> 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
David Kastrup· May 4, 2014, 09:43 UTC · re: Johannes Sixt · lore

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

Johannes Sixt <j6t@kdbg.org> writes:
Show 15 quoted lines
> 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
Andreas Schwab· May 4, 2014, 10:55 UTC · re: Johannes Sixt · lore

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

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."
Johannes Sixt· May 4, 2014, 20:18 UTC · re: Andreas Schwab · lore

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

Am 04.05.2014 12:55, schrieb Andreas Schwab:
Show 9 quoted lines
> 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
Andreas Schwab· May 4, 2014, 21:31 UTC · re: Johannes Sixt · lore

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

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."
David Kastrup· May 5, 2014, 05:19 UTC · re: Andreas Schwab · lore

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

Andreas Schwab <schwab@linux-m68k.org> writes:
Show 8 quoted lines
> 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
Andreas Schwab· May 5, 2014, 09:23 UTC · re: David Kastrup · lore

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

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."
James Denholm· May 5, 2014, 09:33 UTC · re: Andreas Schwab · lore

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

On 5 May 2014 19:23:06 GMT+10:00, Andreas Schwab <schwab@linux-m68k.org> wrote:
Show 9 quoted lines
>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.

David Kastrup· May 5, 2014, 09:50 UTC · re: Andreas Schwab · lore

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

Andreas Schwab <schwab@linux-m68k.org> writes:
Show 7 quoted lines
> 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
Michael Haggerty· May 5, 2014, 10:52 UTC · re: David Kastrup · lore

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

On 05/05/2014 11:50 AM, David Kastrup wrote:
Show 7 quoted lines
> 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.

Show 10 quoted lines
> 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/
Andreas Schwab· May 5, 2014, 11:05 UTC · re: David Kastrup · lore

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

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."
David Kastrup· May 5, 2014, 11:23 UTC · re: Andreas Schwab · lore

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

Andreas Schwab <schwab@linux-m68k.org> writes:
Show 13 quoted lines
> 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
Felipe Contreras· May 5, 2014, 18:16 UTC · re: Andreas Schwab · lore

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

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
Duy Nguyen· May 4, 2014, 12:29 UTC · re: Michael Haggerty · lore

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

On Sun, May 4, 2014 at 1:07 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
Show 14 quoted lines
> 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
brian m. carlson· May 4, 2014, 16:07 UTC · re: Michael Haggerty · lore

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

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
Andreas Schwab· May 4, 2014, 16:48 UTC · re: brian m. carlson · lore

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

"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."
David Kastrup· May 4, 2014, 17:07 UTC · re: Andreas Schwab · lore

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

Andreas Schwab <schwab@linux-m68k.org> writes:
Show 6 quoted lines
> "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
Andreas Schwab· May 4, 2014, 17:24 UTC · re: David Kastrup · lore

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

David Kastrup <dak@gnu.org> writes:
Show 10 quoted lines
> 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."
David Kastrup· May 4, 2014, 17:44 UTC · re: Andreas Schwab · lore

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

Andreas Schwab <schwab@linux-m68k.org> writes:
Show 14 quoted lines
> 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
Andreas Schwab· May 4, 2014, 18:01 UTC · re: David Kastrup · lore

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

David Kastrup <dak@gnu.org> writes:
Show 18 quoted lines
> 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."
brian m. carlson· May 3, 2014, 20:12 UTC · re: brian m. carlson · lore

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

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 bisect.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)
Show changes to bisect.c +15 −15
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
brian m. carlson· May 3, 2014, 20:12 UTC · re: brian m. carlson · lore

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

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 archive.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
Show changes to archive.c +8 −8
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
brian m. carlson· May 3, 2014, 20:12 UTC · re: brian m. carlson · lore

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

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(-)
Show changes to 2 files +3 −2

archive-zip.c, archive.h

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
brian m. carlson· May 3, 2014, 20:12 UTC · re: brian m. carlson · lore

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

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 branch.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
Show changes to branch.c +8 −8
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
brian m. carlson· May 3, 2014, 20:12 UTC · re: brian m. carlson · lore

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

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 bulk-checkin.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
Show changes to bulk-checkin.c +6 −6
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
brian m. carlson· May 3, 2014, 20:12 UTC · re: brian m. carlson · lore

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

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 bundle.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)
Show changes to bundle.c +19 −19
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
Michael Haggerty· May 6, 2014, 14:42 UTC · re: brian m. carlson · lore

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

On 05/03/2014 10:12 PM, brian m. carlson wrote:
Show 54 quoted lines
> 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/
brian m. carlson· May 3, 2014, 20:12 UTC · re: brian m. carlson · lore

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

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(-)
Show changes to 8 files +25 −24

builtin/commit.c, builtin/fsck.c, cache-tree.c, cache-tree.h, merge-recursive.c, reachable.c, sequencer.c, test-dump-cache-tree.c

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
Michael Haggerty· May 6, 2014, 14:53 UTC · re: brian m. carlson · lore

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

On 05/03/2014 10:12 PM, brian m. carlson wrote:
Show 86 quoted lines
> 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/
Michael Haggerty· May 6, 2014, 15:02 UTC · re: brian m. carlson · lore

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

On 05/03/2014 10:12 PM, brian m. carlson wrote:
Show 5 quoted lines
> [...]
> 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/
brian m. carlson· May 3, 2014, 20:12 UTC · re: brian m. carlson · lore

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

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(-)
Show changes to 3 files +35 −34

combine-diff.c, diff-lib.c, diff.h

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
Michael Haggerty· May 6, 2014, 15:08 UTC · re: brian m. carlson · lore

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

On 05/03/2014 10:12 PM, brian m. carlson wrote:
Show 12 quoted lines
> 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/
brian m. carlson· May 3, 2014, 22:49 UTC · re: brian m. carlson · lore

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

On Sat, May 03, 2014 at 08:12:13PM +0000, brian m. carlson wrote:
Show 10 quoted lines
> 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
Michael Haggerty· May 4, 2014, 06:35 UTC · re: brian m. carlson · lore

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

On 05/03/2014 10:12 PM, brian m. carlson wrote:
Show 30 quoted lines
> 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/
Johannes Sixt· May 4, 2014, 09:19 UTC · re: Michael Haggerty · lore

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

Am 04.05.2014 08:35, schrieb Michael Haggerty:
Show 6 quoted lines
> 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
brian m. carlson· May 4, 2014, 17:54 UTC · re: Michael Haggerty · lore

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

On Sun, May 04, 2014 at 08:35:00AM +0200, Michael Haggerty wrote:
Show 14 quoted lines
> 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.
Show 5 quoted lines
> 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

← back to recent threads