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

On 04/23/2015 01:24 AM, brian m. carlson wrote:

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.