Re: [PATCH v2 00/16] Convert parts of refs.c to struct object_id

[Michael Haggerty <mhagger@alum.mit.edu>]

On 04/23/2015 01:24 AM, brian m. carlson wrote:
> This is a conversion of parts of refs.c to use struct object_id.
> 
> refs.c, and the for_each_ref series of functions explicitly, is the
> source for many instances of object IDs in the codebase.  Therefore, it
> makes sense to convert this series of functions to provide a basis for
> further conversions.
> 
> This series is essentially just for_each_ref and friends, the callbacks,
> and callers.  Other parts of refs.c will be converted in a later series,
> so as to keep the number of patches to a reasonable size.
> 
> There should be no functional change from this patch series.

I wanted to review your patches, but wasn't really sure how to go about
it in a way that would make me confident in the result. In a way these
refactoring patch series are easier to implement than to review.

...so that's what I did. I reimplemented your changes "from scratch" [1]
and then checked how my result differed from yours. My conclusion is
that the final result of your patches looks good, though there are some
other changes in the neighborhood that could sensibly be added.


However, I reached the destination via a different route and I thought
I'd describe it in case you are interested, and as the technique might
be useful for future "object_id" refactorings. My patch series is
available on my GitHub account at

    https://github.com/mhagger/git branch oid-refs-adapter

My starting point was to change each_ref_fn to take a "const object_id
*" parameter instead of "const unsigned char *". This change requires
all call sites of all of the for_each_ref functions to be modified,
because they currently pass callback functions that match the old signature.

So I kept the old typedef (the one that takes "const unsigned char *")
but renamed it to each_ref_sha1_fn. And I added an adapter that allows
such functions to be wrapped then passed to the new for_each_ref
functions. It looks like this:

    typedef int each_ref_sha1_fn(const char *refname,
    			         const unsigned char *sha1, int flags, void *cb_data);

    struct each_ref_fn_sha1_adapter {
	    each_ref_sha1_fn *original_fn;
	    void *original_cb_data;
    };

    extern int each_ref_fn_adapter(const char *refname,
			           const struct object_id *oid, int flags, void *cb_data);

Each callsite has to be changed, but the changes are quite
straightforward. At a callsite that would have called

    for_each_ref(my_function, &my_data)

you wrap my_function and my_data in an each_ref_fn_sha1_adapter and call
for_each_ref using each_ref_fn_adapter as the callback:

    struct each_ref_fn_sha1_adapter wrapped_my_function =
            {my_function, &my_data};

    for_each_ref(each_ref_fn_adapter, &wrapped_my_function);

The function each_ref_fn_adapter extracts the SHA-1 out of the oid and
calls my_function, passing it &my_data as extra data.

This patch is thus giant but very straightforward.

After that, there is one patch for each callsite, rewriting it to use
for_each_ref natively (which usually entails modifying my_function to
take an object_id parameter then undoing the wrapper). These patches
involve a little bit of thought, but not too much. And the results are
very bisectable because each patch makes a single small change. I also
suspect it might be easier to rebase and/or merge my patch series, for
the same reason.

The end result was very similar to yours, so I am confident that the net
result of your patch series is correct. But the remaining differences in
the end results are also interesting. I made a few more changes in the
neighborhood of the patches, not to mention a few formatting
improvements in code that I touched. If you compare the tip of my
branch, above, to the tip of yours (I uploaded that to my repo too, as
branch "bc-oid-refs"), it may give you some ideas for other code that
can be changed to object_id.

Yours,
Michael

[1] Obviously I glanced at your patches while I was working to make sure
that I was headed in the same direction as you, and to minimize
gratuitous differences between our versions.

-- 
Michael Haggerty
mhagger@alum.mit.edu