Message ID | cover.1684178576.git.me@ttaylorr.com (mailing list archive) |
---|---|
Headers | show |
Series | refs: implement jump lists for packed backend | expand |
On Mon, May 15, 2023 at 03:23:07PM -0400, Taylor Blau wrote: > Here is a reroll of my series to implement jump (née skip) lists for the > packed refs backend. > > Not a ton has changed since last time, but some notable things that have > changed include: > > - Renaming "skip lists" to "jump lists" to clarify that this > implementation does not use the skip list data structure. > - Patch reorganization, splitting out `find_reference_location_end()` > more sensibly, rewording patch messages, etc. > - Addresses feedback from Junio and Patrick Steinhardt's helpful > reviews. > > As usual, a range-diff is included below for convenience. > > Given that we are expecting -rc0 today, we should aim to not let review > of this topic direct our attention away from testing the release > candidates. We can get more serious about it on the other side of 2.41. > > Thanks in advance for another look. I didn't have many comments in this round. Personally though I'd split up this patch series into two in order to land the individual parts faster, where the first part introduces `git for-each-ref --exclude` and the second part introduces the jump list for the packed-refs backend. Each of these have merit on their own, and especially the first part should require less discussion. Furthermore, by splitting it up the review becomes easier to manage as 16 patches does require quite a long attention span to handle. Anyway, this is just a suggestion from my sided, please feel free to ignore. Patrick
On Tue, Jun 06, 2023 at 09:01:10AM +0200, Patrick Steinhardt wrote: > On Mon, May 15, 2023 at 03:23:07PM -0400, Taylor Blau wrote: > > Here is a reroll of my series to implement jump (née skip) lists for the > > packed refs backend. > > > > Not a ton has changed since last time, but some notable things that have > > changed include: > > > > - Renaming "skip lists" to "jump lists" to clarify that this > > implementation does not use the skip list data structure. > > - Patch reorganization, splitting out `find_reference_location_end()` > > more sensibly, rewording patch messages, etc. > > - Addresses feedback from Junio and Patrick Steinhardt's helpful > > reviews. > > > > As usual, a range-diff is included below for convenience. > > > > Given that we are expecting -rc0 today, we should aim to not let review > > of this topic direct our attention away from testing the release > > candidates. We can get more serious about it on the other side of 2.41. > > > > Thanks in advance for another look. > > I didn't have many comments in this round. Personally though I'd split > up this patch series into two in order to land the individual parts > faster, where the first part introduces `git for-each-ref --exclude` and > the second part introduces the jump list for the packed-refs backend. Thanks for reviewing, and for the good suggestion. I think that splitting this series in two could be worthwhile, but I'm not sure I want to make this change for v4. You could imagine splitting the series there, where the first half would implement the naive version of `for-each-ref --exclude`, and the second half would implement jump lists, and make upload- and receive-pack take advantage of it. But I think that makes the first half trivial and leaves all of the complexity in this series to the latter half. I suppose that makes it easier to graduate the first six or so patches earlier, but they aren't really all that useful without the remaining patches. Another split might be: - the naive implementation of `for-each-ref --exclude` - jump lists, and using them within `for-each-ref` - teaching upload- and receive-pack to use the jump list But juggling this series as three sub-topics feels like it would be burdensome to the maintainer ;-). So I think I'd rather leave it as-is, unless you feel strongly. Thanks, Taylor