On Tue, Sep 4, 2018 at 1:57 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> writes: > > > The three functions init_revisions(), diff_setup() and rerere() are > > prefixed temporarily with repo_ to avoid breaking other topics which > > add new call sites for these functions. This is a temporary > > measure. Once everything is merged, it will be reverted and the new > > call sites fixed. > > That's a sensible thing to do, but isn't it too late at 24/24 stage? > IOW, doesn't in-flight topics break if up to 23/24 of this series is > merged? Presumably you merge the tip of this series (which contains 24/24) with the other in-flight topics, that make new uses of init_revisions(revs, prefix), which 24/24 allows. Going on either parent side of such a merge will have working commits, that compile. So from that POV it doesn't matter when the #define is introduced. > IOW, the one that teaches "work in this repository" to rerere(int) > for example should have introduced > > repo_rerere(struct repository *, int); > #define rerere(x) repo_rerere(the_repository, x) > > in its own step, not this late in the series, no? That might make for a better presentation to reviewers now and here, but at some later date, we would want to 'git revert 24/24' after fixing all in-flights of today. And with that in mind it makes sense to separate out all these changes into this patch, as that allows us to have the revert rename back to easy names. You may be asking this question as it was done differently in the series sb/object-store-lookup. But there the #define's served a different purpose. That series used the #define to aid author&reviewer to know where dependencies to the_repository are and which function is done already. This series solely uses the #define after the main series is done to aid the maintainer to merge it which as you noted was one of the problems with that other approach. The downside of the series as it is presented here is that it is very easy to miss some hidden dependency, that leads to a bug only discovered later (e.g. some function still uses the_repository, when working on a submodule for example, but as that function is used in some corner case our test suite would not find it). So why don't we try this approach as well and see how well it goes over time? Thanks, Stefan
