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

From: Kyle J. McKay <mackyle@gmail.com>
Date: Mar 11, 2015, 02:38 UTC
Message-ID: <82A92572-98E5-4911-87A6-DF5377579436@gmail.com>
In-Reply-To: <1425770645-628957-1-git-send-email-sandals@crustytoothpaste.net>

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

Show 5 quoted lines

> 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.

Show 9 quoted lines

>  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:

Show 22 quoted lines

> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
>> 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