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