Re: [PATCH v2 00/10] Use a structure for object IDs.

On Mar 7, 2015, at 15:23, brian m. carlson wrote:

This is a patch series to convert some of the relevant uses of  
unsigned
char [20] to struct object_id.

brian m. carlson (10):

All patches applied for me (to master) and all tests pass.

Tested-by: Kyle J. McKay

 Define a structure for object IDs.

Comments in reply to the patch.

 Define utility functions for object IDs.
 bisect.c: convert leaf functions to use struct object_id
 archive.c: convert to use struct object_id
 zip: use GIT_SHA1_HEXSZ for trailers
 bulk-checkin.c: convert to use struct object_id
 diff: convert struct combine_diff_path to object_id
 commit: convert parts to struct object_id
 patch-id: convert to use struct object_id
 apply: convert threeway_stage to object_id

These all look good, the conversions are simple and easy to follow.


On Mar 7, 2015, at 23:43, Junio C Hamano wrote:

"brian m. carlson" [off-list ref] writes:

quoted

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. struct object)  
will
be converted later.  Conversion has been done in a somewhat haphazard
manner by converting modules with leaf functions and less commonly  
used
structs first.

That "leaf-first" approach sounds very sensible.

In the medium term, I wonder if the changes can progress faster and
in a less error prone way if you first used GIT_SHA1_RAWSZ in places
that cannot be immediately converted to the struct yet.  That way,
we will be easily tell by "git grep GIT_SHA1_RAWSZ" how many more
places need treatment.  I do not know if that is all that useful
offhand, though.  Those places need to be touched in the second pass
to use the struct again, after the "s/\[20\]/[GIT_SHA1_RAWSZ]/"
first pass.

I definitely noticed the leaf-first approach as I was looking through  
the patches where, for example (03/10), this prototype was left  
unchanged:

static int register_ref(const char *refname, const unsigned char *sha1,
                         int flags, void *cb_data)

but its contents got the update leaving it half converted.  As  
mentioned above this makes the patches more manageable, maintainable  
and bisectable.  However, these functions could be converted to take a  
typedef (a quick grep of 'CodingGuidelines' does not mention typedef)  
and perhaps, as Junio mentions above, help the changes progress faster  
by making it easier to find the affected code (e.g. changing or  
removing the typedef would make the compiler find them for you).

For example, if we added this to object.h:

     typedef unsigned char sha1raw_t[GIT_SHA1_RAWSZ];

then the above prototype could be immediately converted to (and this  
does compile and pass all the tests):

static int register_ref(const char *refname, const sha1raw_t sha,
                         int flags, void *cb_data)

So that together with Junio's suggestion above (and perhaps also a  
sha1hex_t type) would help mark everything in the first pass that  
needs to be touched again in the second pass.  (I'm just throwing out  
some typedef names as an example, there may be more preferable names  
to "sha1raw_t" and "sha1hex_t", but those names would end up being  
replaced eventually anyway.)

-Kyle