Jonathan Tan [off-list ref] writes:
For what it's worth, here's v7 with advertise/allow/ignore and by
default, advertise. I think that some server operators will have use for
this feature, and people who want to disable it for whatever reason can
still do so. The main disadvantage is complexity - the server knob that
server administrators will need to control (but between a simpler
allow/ignore knob and a more complicated advertise/allow/ignore knob, I
think we might as well go for the slightly more complex one) and
complexity in the code (but now that is constrained to one function and
a few global variables).
As you can see from the range-diff, not much has changed from v6.
I've also included Junio's suggestion of tightening the promise made by
the server (when the client says "unborn").
This looks reasonable overall, especially with the feature turned on
by default, we'd hopefully get reasonable exposure from the get-go.
Let's mark the topic to be merged to 'next' soonish, unless people
object.
Thanks.
On Fri, Feb 05 2021, Junio C Hamano wrote:
Jonathan Tan [off-list ref] writes:
quoted
For what it's worth, here's v7 with advertise/allow/ignore and by
default, advertise. I think that some server operators will have use for
this feature, and people who want to disable it for whatever reason can
still do so. The main disadvantage is complexity - the server knob that
server administrators will need to control (but between a simpler
allow/ignore knob and a more complicated advertise/allow/ignore knob, I
think we might as well go for the slightly more complex one) and
complexity in the code (but now that is constrained to one function and
a few global variables).
As you can see from the range-diff, not much has changed from v6.
I've also included Junio's suggestion of tightening the promise made by
the server (when the client says "unborn").
This looks reasonable overall, especially with the feature turned on
by default, we'd hopefully get reasonable exposure from the get-go.
Let's mark the topic to be merged to 'next' soonish, unless people
object.
FWIW I'm still unclear on re [1] whether Jonathan thinks the semantics
of this shouldn't be documented for <reasons>, or whether he just
doesn't want to submit the patch and I should, or something else.
I still think this "remote as config source" without actually explaining
that it is is a very glaring hole in the docs[2].
And in [3] I noted that we introduced the word "branches" into
protocol-v2.txt for the first time without defining what it means
(presumably just refs/heads/*, but then let's say so...). There was a
reply promising clarification, but I note that v7 still just says
"branches".
To be clear I'm perfectly fine with a note in a CL to the effect of "had
X feedback on last version, Ævar said Y and Z both of which I think are
stupid ideas, so I'm not doing them" :)
The only thing I mind is being left hanging to the effect of not knowing
if a diff you proposed is considered bad by the primary author, or if I
should just submit it myself as a patch on top or whatever.
It also saves other people following along with reviews time, since they
can read later cover letters and get a brief summary of some
side-discussion in an earlier round without diving into it themselves.
Me too honestly, sometimes I come back to these threads and completely
forgot what I had to say in earlier rounds, and when I try to find out
it's hit-and-miss whether I agree with much/any of it :)
1. https://lore.kernel.org/git/87h7n3p363.fsf@evledraar.gmail.com/
2. https://lore.kernel.org/git/878s8apthr.fsf@evledraar.gmail.com/
3. https://lore.kernel.org/git/87k0rzp3qx.fsf@evledraar.gmail.com/
On Thu, Feb 04, 2021 at 09:25:57PM -0800, Junio C Hamano wrote:
Jonathan Tan [off-list ref] writes:
quoted
For what it's worth, here's v7 with advertise/allow/ignore and by
default, advertise. I think that some server operators will have use for
this feature, and people who want to disable it for whatever reason can
still do so. The main disadvantage is complexity - the server knob that
server administrators will need to control (but between a simpler
allow/ignore knob and a more complicated advertise/allow/ignore knob, I
think we might as well go for the slightly more complex one) and
complexity in the code (but now that is constrained to one function and
a few global variables).
As you can see from the range-diff, not much has changed from v6.
I've also included Junio's suggestion of tightening the promise made by
the server (when the client says "unborn").
This looks reasonable overall, especially with the feature turned on
by default, we'd hopefully get reasonable exposure from the get-go.
Let's mark the topic to be merged to 'next' soonish, unless people
object.
No objection here. I sent a few comments in response to patch 1; the doc
fix and the leak are probably worth addressing before it hits next. I
couldn't help express my thoughts on the protocol wording, but it may be
best to ignore me. ;)
Thanks for working on this, Jonathan. I think it's a very useful
feature.
-Peff