Elijah Newren [off-list ref] writes:
On Fri, Aug 6, 2021 at 6:26 PM Junio C Hamano [off-list ref] wrote:
quoted
* jt/push-negotiation-fixes (2021-07-15) 3 commits
- fetch: die on invalid --negotiation-tip hash
- send-pack: fix push nego. when remote has refs
- send-pack: fix push.negotiate with remote helper
Bugfix for common ancestor negotiation recently introduced in "git
push" codepath.
Needs review.
I decided to take a look. It appears both Emily and Ævar have
reviewed previously. Ævar brought up some comments on v2 of patch 2,
but Jonathan responded to them all (and Peff chimed in as well), and
there has been no further response from Ævar leaving the impression to
me that Jonathan has replied to all feedback.
I'm not at all familiar with the protocol areas of the code, but I
tried to read over the patches too. I think it'd have been a bit
easier to understand for someone unfamiliar with this area if there
were separate patches that introduced test_expect_failure showing the
particular failures being fixed, followed by fixes in subsequent
patches. I also noticed a stray newline removal in patch 2. Those
are pretty minor issues, though, and I didn't spot anything
substantial.
I am mostly worried about the lack of response to comments on [2/3],
especially to Peff's "Wouldn't this also be a problem for
multi-round fetch?".
Thanks.
On Mon, Aug 9, 2021 at 10:17 AM Junio C Hamano [off-list ref] wrote:
Elijah Newren [off-list ref] writes:
quoted
On Fri, Aug 6, 2021 at 6:26 PM Junio C Hamano [off-list ref] wrote:
quoted
* jt/push-negotiation-fixes (2021-07-15) 3 commits
- fetch: die on invalid --negotiation-tip hash
- send-pack: fix push nego. when remote has refs
- send-pack: fix push.negotiate with remote helper
Bugfix for common ancestor negotiation recently introduced in "git
push" codepath.
Needs review.
I decided to take a look.
[...]
I am mostly worried about the lack of response to comments on [2/3],
especially to Peff's "Wouldn't this also be a problem for
multi-round fetch?".
Oh, I read Peff's comments[1] to mean that he thought Jonathan's
patches behaved no worse than the existing code in the face of
simultaneously running gc's, and thus that Ævar's concerns shouldn't
hold up Jonathan's patches. But, it's totally possible I
misunderstood his intent -- as I mentioned, I'm not at all familiar
with this area of the code. Maybe Peff or Jonathan can clarify.
[1] Handy link:
https://lore.kernel.org/git/YQA4bzFxdOHSQva7@coredump.intra.peff.net/
On Mon, Aug 09, 2021 at 11:39:57AM -0700, Elijah Newren wrote:
On Mon, Aug 9, 2021 at 10:17 AM Junio C Hamano [off-list ref] wrote:
quoted
Elijah Newren [off-list ref] writes:
quoted
On Fri, Aug 6, 2021 at 6:26 PM Junio C Hamano [off-list ref] wrote:
quoted
* jt/push-negotiation-fixes (2021-07-15) 3 commits
- fetch: die on invalid --negotiation-tip hash
- send-pack: fix push nego. when remote has refs
- send-pack: fix push.negotiate with remote helper
Bugfix for common ancestor negotiation recently introduced in "git
push" codepath.
Needs review.
I decided to take a look.
[...]
quoted
I am mostly worried about the lack of response to comments on [2/3],
especially to Peff's "Wouldn't this also be a problem for
multi-round fetch?".
Oh, I read Peff's comments[1] to mean that he thought Jonathan's
patches behaved no worse than the existing code in the face of
simultaneously running gc's, and thus that Ævar's concerns shouldn't
hold up Jonathan's patches. But, it's totally possible I
misunderstood his intent -- as I mentioned, I'm not at all familiar
with this area of the code. Maybe Peff or Jonathan can clarify.
Yep, that's what I meant. I think Jonathan's adjacent response to Ævar
agrees with that, too.
(I haven't actually thought hard about _other_ possible issues with push
negotiation at this point, though).
-Peff