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" <sandals@xxxxxxxxxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html