Re: [PATCH v2 00/20] Separate `ref_cache` into a separate module

[Michael Haggerty]

On 04/07/2017 01:53 PM, Duy Nguyen wrote:
> On Wed, Apr 5, 2017 at 9:03 PM, Duy Nguyen  wrote:
>> On Sat, Apr 1, 2017 at 12:16 PM, Michael Haggerty  
>> wrote:
>>> Duy, have you looked over my patch series? Since you've been working in
>>> the area, your feedback would be very welcome, if you have the time for it.
>>
>> You probably have guessed my answer based on my lack of response :)
>> Tomorrow is holiday though, will have a look. But I doubt I would find
>> anything given that both you and Jeff are involved in this.
> 
> Overall, very nice. I made a comment here and there, but they're more
> questions for my education than actual code review ) At least there's
> another set of eyes on this series now.

Thanks for your review! I'll submit v3 shortly to address your feedback.

Michael

Re: [PATCH v2 00/20] Separate `ref_cache` into a separate module

[Duy Nguyen]

On Wed, Apr 5, 2017 at 9:03 PM, Duy Nguyen  wrote:
> On Sat, Apr 1, 2017 at 12:16 PM, Michael Haggerty  
> wrote:
>> Duy, have you looked over my patch series? Since you've been working in
>> the area, your feedback would be very welcome, if you have the time for it.
>
> You probably have guessed my answer based on my lack of response :)
> Tomorrow is holiday though, will have a look. But I doubt I would find
> anything given that both you and Jeff are involved in this.

Overall, very nice. I made a comment here and there, but they're more
questions for my education than actual code review ) At least there's
another set of eyes on this series now.
-- 
Duy

Re: [PATCH v2 00/20] Separate `ref_cache` into a separate module

[Duy Nguyen]

On Sat, Apr 1, 2017 at 12:16 PM, Michael Haggerty  wrote:
> Duy, have you looked over my patch series? Since you've been working in
> the area, your feedback would be very welcome, if you have the time for it.

You probably have guessed my answer based on my lack of response :)
Tomorrow is holiday though, will have a look. But I doubt I would find
anything given that both you and Jeff are involved in this.

On Sun, Apr 2, 2017 at 4:26 AM, Ramsay Jones
 wrote:
> I sent Duy some comments on that branch (an unused function and
> a 'plain integer used as NULL pointer' warning).

I still remember :) Or at least I vaguely remember something I need to
fix from you, and made sure comments in all my re-rolls are addressed.
nd/prune-in-worktree should be the next one, soon.
-- 
Duy

Re: [PATCH v2 00/20] Separate `ref_cache` into a separate module

[Junio C Hamano]

Ramsay Jones  writes:

>> In that sense, Michael's series and Duy's later two series are
>> "tangled" (i.e. shares some common commits that are still not in
>> 'master').  If nd/files-backend-git-dir that is shared among them is
>> ever rebased, all of them need to be rebased on top of it
>> consistently.
>> 
>> But if either of nd/prune-in-worktree and nd/worktree-kill-parse-ref
>> needs to be rerolled, that can be done independently from Michael's
>> series, as long as nd/files-backend-git-dir is solid and unchanging.
>
> I suspected as much (hence the 'maybe not now' comment), but I noticed
> that all four branches were merged into 'jch'. So, it seemed possible
> to me that they were all being considered for merging into next.

Yes they were (and still are, but I do not expect they will be
moving to 'next' while I am offline ;-).  What you can do to help is
to review and say things like "nd/files-backend-git-dir and
Michael's one on top of it looked good, no opinion on others",
"the others have this and that issues that need a reroll", etc.

Thanks.

Re: [PATCH v2 00/20] Separate `ref_cache` into a separate module

[Ramsay Jones]

On 02/04/17 04:38, Junio C Hamano wrote:
> Ramsay Jones  writes:
> 
>>> I am getting the impression that the files-backend thing as well as
>>> this topic are ready for 'next'.  Please stop me if I missed something
>>> in these topics (especially the other one) that needs updating
>>> before that happens.
>>
>> Hmm, are these branches 'tangled' with nd/prune-in-worktree?
>> (I think they were at one point, but maybe not now?).
> 
> Michael's mh/separate-ref-cache builds directly on top of
> nd/files-backend-git-dir topic.
> 
> nd/prune-in-worktree builds directly on top of
> nd/worktree-kill-parse-ref, which in turn builds directly on top of
> nd/files-backend-git-dir.
> 
> In that sense, Michael's series and Duy's later two series are
> "tangled" (i.e. shares some common commits that are still not in
> 'master').  If nd/files-backend-git-dir that is shared among them is
> ever rebased, all of them need to be rebased on top of it
> consistently.
> 
> But if either of nd/prune-in-worktree and nd/worktree-kill-parse-ref
> needs to be rerolled, that can be done independently from Michael's
> series, as long as nd/files-backend-git-dir is solid and unchanging.

I suspected as much (hence the 'maybe not now' comment), but I noticed
that all four branches were merged into 'jch'. So, it seemed possible
to me that they were all being considered for merging into next.

Thanks!

ATB,
Ramsay Jones

Re: [PATCH v2 00/20] Separate `ref_cache` into a separate module

[Junio C Hamano]

Ramsay Jones  writes:

>> I am getting the impression that the files-backend thing as well as
>> this topic are ready for 'next'.  Please stop me if I missed something
>> in these topics (especially the other one) that needs updating
>> before that happens.
>
> Hmm, are these branches 'tangled' with nd/prune-in-worktree?
> (I think they were at one point, but maybe not now?).

Michael's mh/separate-ref-cache builds directly on top of
nd/files-backend-git-dir topic.

nd/prune-in-worktree builds directly on top of
nd/worktree-kill-parse-ref, which in turn builds directly on top of
nd/files-backend-git-dir.

In that sense, Michael's series and Duy's later two series are
"tangled" (i.e. shares some common commits that are still not in
'master').  If nd/files-backend-git-dir that is shared among them is
ever rebased, all of them need to be rebased on top of it
consistently.

But if either of nd/prune-in-worktree and nd/worktree-kill-parse-ref
needs to be rerolled, that can be done independently from Michael's
series, as long as nd/files-backend-git-dir is solid and unchanging.

Re: [PATCH v2 00/20] Separate `ref_cache` into a separate module

[Ramsay Jones]

On 31/03/17 17:01, Junio C Hamano wrote:
> Michael Haggerty  writes:
> 
>> This version literally only contains a few commit message changes and
>> one minor comment changes relative to v1. The code is identical. I
>> wasn't sure whether it is even worth sending this patch series to the
>> ML again; Junio, if you'd prefer I just send a link to a published
>> branch in such cases, please let me know.
> 
> The review on the list is not about letting me pick it up, but is
> about giving reviewing contributors a chance to comment.  I think
> for a series this important ;-) it is good that you are giving it
> multiple exposures so that people who were offline the last time can
> have a chance to look at it, even if the update is minimum.
> 
>> This patch series is also available from my GitHub fork [2] as branch
>> "separate-ref-cache". These patches depend on Duy's
>> nd/files-backend-git-dir branch.
> 
> I am getting the impression that the files-backend thing as well as
> this topic are ready for 'next'.  Please stop me if I missed something
> in these topics (especially the other one) that needs updating
> before that happens.

Hmm, are these branches 'tangled' with nd/prune-in-worktree?
(I think they were at one point, but maybe not now?).

I sent Duy some comments on that branch (an unused function and
a 'plain integer used as NULL pointer' warning).

ATB,
Ramsay Jones

Re: [PATCH v2 00/20] Separate `ref_cache` into a separate module

[Jeff King]

On Fri, Mar 31, 2017 at 04:10:58PM +0200, Michael Haggerty wrote:

> * I added a long blurb about the removal of an internal consistency
>   check in the commit message for
> 
>   [18/20] commit_packed_refs(): use reference iteration

Thanks, the explanation there makes sense.

I think this version addresses all of my comments.

-Peff

Re: [PATCH v2 00/20] Separate `ref_cache` into a separate module

[Michael Haggerty]

On 03/31/2017 06:01 PM, Junio C Hamano wrote:
> Michael Haggerty  writes:
> 
>> This version literally only contains a few commit message changes and
>> one minor comment changes relative to v1. The code is identical. I
>> wasn't sure whether it is even worth sending this patch series to the
>> ML again; Junio, if you'd prefer I just send a link to a published
>> branch in such cases, please let me know.
> 
> The review on the list is not about letting me pick it up, but is
> about giving reviewing contributors a chance to comment.  I think
> for a series this important ;-) it is good that you are giving it
> multiple exposures so that people who were offline the last time can
> have a chance to look at it, even if the update is minimum.
> 
>> This patch series is also available from my GitHub fork [2] as branch
>> "separate-ref-cache". These patches depend on Duy's
>> nd/files-backend-git-dir branch.
> 
> I am getting the impression that the files-backend thing as well as
> this topic are ready for 'next'.  Please stop me if I missed something
> in these topics (especially the other one) that needs updating
> before that happens.

I don't know of any remaining problems with these two patch series
(aside from a couple of cosmetic problems that I just pointed out about
v7 of nd/files-backend-git-dir). I'm pretty confident about both of them.

Duy, have you looked over my patch series? Since you've been working in
the area, your feedback would be very welcome, if you have the time for it.

Michael

Re: [PATCH v2 00/20] Separate `ref_cache` into a separate module

[Junio C Hamano]

Michael Haggerty  writes:

> This version literally only contains a few commit message changes and
> one minor comment changes relative to v1. The code is identical. I
> wasn't sure whether it is even worth sending this patch series to the
> ML again; Junio, if you'd prefer I just send a link to a published
> branch in such cases, please let me know.

The review on the list is not about letting me pick it up, but is
about giving reviewing contributors a chance to comment.  I think
for a series this important ;-) it is good that you are giving it
multiple exposures so that people who were offline the last time can
have a chance to look at it, even if the update is minimum.

> This patch series is also available from my GitHub fork [2] as branch
> "separate-ref-cache". These patches depend on Duy's
> nd/files-backend-git-dir branch.

I am getting the impression that the files-backend thing as well as
this topic are ready for 'next'.  Please stop me if I missed something
in these topics (especially the other one) that needs updating
before that happens.

Thanks.