On Fri, Sep 11, 2015 at 7:10 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > On Fri, Sep 11, 2015 at 5:52 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Thanks for bringing this up. I haven't found a sufficient block of > time yet to review these patches, however, I had the same thought upon > a quick initial read, and is how I was hoping to see the patch series > constructed based upon my earlier two reviews suggesting refactoring > the existing branch.c functions into a new get_worktrees(). There are > at least a couple important reasons for taking this approach. > > First, it keeps the "blame" trail intact, the full context of which > can be helpful to someone trying to understand why the code is the way > it is. The current approach of having get_worktree_list() materialize > out of thin air (even though it may have been patterned after existing > code) doesn't give the same benefit. > > Second, it's easier for reviewers to understand and check the code for > correctness if it mutates to the final form in small increments than > it is to have get_worktrees() come into existence fully matured. > > A final minor comment: Since all three branch.c functions, > die_if_checked_out(), find_shared_symref(), and find_linked_symref(), > ought to be moved to top-level worktree.c, I'd probably have patch 1 > do the relocation (with no functional changes), and subsequent patches > refactor the moved code into a general purpose get_worktrees(). The > final patch would then implement "git worktrees list". I like the way this history works out, and I have reworked the history to follow this idea. The only thing I didn't do was move the die_if_checked_out() function from branch.c, as I feel that this function is more of a branch feature than a worktree feature. -- 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