Subject: Re: [PATCH v3 18/44] refs: move transaction functions into common code

From: David Turner <[email-masked]>
Date: Tue, 13 Oct 2015 18:21:41 -0400

On Tue, 2015-10-13 at 13:29 +0200, Michael Haggerty wrote:
> I reviewed the patches up to here pretty carefully, and aside from the
> comments I already sent, they look good.
> 
> I like the new approach where the ref_transaction-building code is
> shared across backends.
> 
> It seems to me that a good breaking point for the first batch of patches
> would be here, just before you start creating the VTABLE in commit
> [19/44]. The patches before this point all have to do with moving code
> around and a little bit of light refactoring. They cause a lot of code
> churn that will soon conflict with other people's work (e.g., [1]), but
> I think they are pretty uncontroversial.
> 
> After this you start making a few important design decisions that
> *could* be controversial. Therefore, by making a cut, you can maximize
> the chance that the earlier patches can be merged to master relatively
> quickly, after which the cross section for future conflicts will be much
> smaller.
> 
> Ideally, you would include a few later patches in the "pre-VTABLE" patch
> series. It looks like the following patches also mostly have to do with
> moving code around, and would fit logically with the "pre-VTABLE" changes:
> 
> [24] refs.c: move refname_is_safe to the common code
> [25] refs.h: document make refname_is_safe and add it to header
> [26] refs.c: move copy_msg to the common code
> [27] refs.c: move peel_object to the common code
> [28] refs.c: move should_autocreate_reflog to common code
> [32] initdb: move safe_create_dir into common code
> [36] refs: make files_log_ref_write functions public
> [37] refs: break out a ref conflict check
> 
> I tried rebasing those commits on top of your patch 18 and it wasn't too
> bad. The result is on branch "refs-backend-pre-vtable" on my GitHub repo
> [2], including my suggested changes to those eight patches (which
> therefore became seven because I squashed the first two together).

Thanks.  I started from that, and made the changes that you suggested 
in reviews that were not yet in there.

I also added Jeff's extension patch, since it seems uncontroversial to
me, and since we'll need it sooner or later anyway.

I put the result on my github at:
https://github.com/dturner-tw/git
on the refs-backend-pre-vtable branch