Patchwork [6,of,6] largefiles: modernize how capabilities are added to the wire protocol

login
register
mail settings
Submitter Matt Harbison
Date Dec. 27, 2017, 8:27 a.m.
Message ID <f2e5631c99e6e2c7e9ba.1514363278@Envy>
Download mbox | patch
Permalink /patch/26471/
State Accepted
Headers show

Comments

Matt Harbison - Dec. 27, 2017, 8:27 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1514349649 18000
#      Tue Dec 26 23:40:49 2017 -0500
# Node ID f2e5631c99e6e2c7e9bac058185c24dd73a04e98
# Parent  25ecea2ec3d7844c9146bf878fc5093ab33b6e11
largefiles: modernize how capabilities are added to the wire protocol

See 982f13bef503, which came well after this code was originally written.
Yuya Nishihara - Dec. 28, 2017, 1:04 p.m.
On Wed, 27 Dec 2017 03:27:58 -0500, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1514349649 18000
> #      Tue Dec 26 23:40:49 2017 -0500
> # Node ID f2e5631c99e6e2c7e9bac058185c24dd73a04e98
> # Parent  25ecea2ec3d7844c9146bf878fc5093ab33b6e11
> largefiles: modernize how capabilities are added to the wire protocol

Looks good to me, so queued, thanks.

> diff --git a/hgext/largefiles/proto.py b/hgext/largefiles/proto.py
> --- a/hgext/largefiles/proto.py
> +++ b/hgext/largefiles/proto.py
> @@ -161,9 +161,11 @@
>      repo.__class__ = lfileswirerepository
>  
>  # advertise the largefiles=serve capability
> -def capabilities(repo, proto):
> -    '''Wrap server command to announce largefile server capability'''
> -    return capabilitiesorig(repo, proto) + ' largefiles=serve'
> +def _capabilities(orig, repo, proto):
> +    '''announce largefile server capability'''
> +    caps = orig(repo, proto)
> +    caps.append('largefiles=serve')
> +    return caps

Dropped "capabilitiesorig = None" in flight.
Matt Harbison - Jan. 3, 2018, 4:55 a.m.
On Thu, 28 Dec 2017 08:04:01 -0500, Yuya Nishihara <yuya@tcha.org> wrote:

> On Wed, 27 Dec 2017 03:27:58 -0500, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1514349649 18000
>> #      Tue Dec 26 23:40:49 2017 -0500
>> # Node ID f2e5631c99e6e2c7e9bac058185c24dd73a04e98
>> # Parent  25ecea2ec3d7844c9146bf878fc5093ab33b6e11
>> largefiles: modernize how capabilities are added to the wire protocol
>
> Looks good to me, so queued, thanks.

Any thoughts on how the client can let the server know it has the  
extension loaded, so I can close off the last hole in this series?
Yuya Nishihara - Jan. 5, 2018, 6:19 a.m.
On Tue, 02 Jan 2018 23:55:08 -0500, Matt Harbison wrote:
> On Thu, 28 Dec 2017 08:04:01 -0500, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Wed, 27 Dec 2017 03:27:58 -0500, Matt Harbison wrote:
> >> # HG changeset patch
> >> # User Matt Harbison <matt_harbison@yahoo.com>
> >> # Date 1514349649 18000
> >> #      Tue Dec 26 23:40:49 2017 -0500
> >> # Node ID f2e5631c99e6e2c7e9bac058185c24dd73a04e98
> >> # Parent  25ecea2ec3d7844c9146bf878fc5093ab33b6e11
> >> largefiles: modernize how capabilities are added to the wire protocol
> >
> > Looks good to me, so queued, thanks.
> 
> Any thoughts on how the client can let the server know it has the
> extension loaded, so I can close off the last hole in this series?

CC +indygreg, augie

I think there were some discussion about advertising client's capability,
but I don't remember.
Augie Fackler - Jan. 6, 2018, 8:09 p.m.
> On Jan 5, 2018, at 1:19 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> On Tue, 02 Jan 2018 23:55:08 -0500, Matt Harbison wrote:
>> On Thu, 28 Dec 2017 08:04:01 -0500, Yuya Nishihara <yuya@tcha.org> wrote:
>>> On Wed, 27 Dec 2017 03:27:58 -0500, Matt Harbison wrote:
>>>> # HG changeset patch
>>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>>> # Date 1514349649 18000
>>>> #      Tue Dec 26 23:40:49 2017 -0500
>>>> # Node ID f2e5631c99e6e2c7e9bac058185c24dd73a04e98
>>>> # Parent  25ecea2ec3d7844c9146bf878fc5093ab33b6e11
>>>> largefiles: modernize how capabilities are added to the wire protocol
>>> 
>>> Looks good to me, so queued, thanks.
>> 
>> Any thoughts on how the client can let the server know it has the
>> extension loaded, so I can close off the last hole in this series?
> 
> CC +indygreg, augie
> 
> I think there were some discussion about advertising client's capability,
> but I don't remember.

OOC, why do we want the client to be advertising that it groks largefiles to the server? I have a guess, but I’d like that explained before I start trying to figure out what we need to support...
Matt Harbison - Jan. 6, 2018, 8:56 p.m.
> On Jan 6, 2018, at 3:09 PM, Augie Fackler <raf@durin42.com> wrote:
> 
> 
>> On Jan 5, 2018, at 1:19 AM, Yuya Nishihara <yuya@tcha.org> wrote:
>> 
>> On Tue, 02 Jan 2018 23:55:08 -0500, Matt Harbison wrote:
>>>> On Thu, 28 Dec 2017 08:04:01 -0500, Yuya Nishihara <yuya@tcha.org> wrote:
>>>>> On Wed, 27 Dec 2017 03:27:58 -0500, Matt Harbison wrote:
>>>>> # HG changeset patch
>>>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>>>> # Date 1514349649 18000
>>>>> #      Tue Dec 26 23:40:49 2017 -0500
>>>>> # Node ID f2e5631c99e6e2c7e9bac058185c24dd73a04e98
>>>>> # Parent  25ecea2ec3d7844c9146bf878fc5093ab33b6e11
>>>>> largefiles: modernize how capabilities are added to the wire protocol
>>>> 
>>>> Looks good to me, so queued, thanks.
>>> 
>>> Any thoughts on how the client can let the server know it has the
>>> extension loaded, so I can close off the last hole in this series?
>> 
>> CC +indygreg, augie
>> 
>> I think there were some discussion about advertising client's capability,
>> but I don't remember.
> 
> OOC, why do we want the client to be advertising that it groks largefiles to the server? I have a guess, but I’d like that explained before I start trying to figure out what we need to support...

I’m not in front of a computer right now, but see this.  I think it is related to choking on the external flag that it doesn’t understand without the extension.

https://www.mercurial-scm.org/repo/hg-all/file/fa865878a849/tests/test-lfs-serve.t#l189
Matt Harbison - Jan. 7, 2018, 12:42 a.m.
On Sat, 06 Jan 2018 15:56:43 -0500, Matt Harbison <mharbison72@gmail.com>  
wrote:

>
>
>> On Jan 6, 2018, at 3:09 PM, Augie Fackler <raf@durin42.com> wrote:
>>
>>
>>> On Jan 5, 2018, at 1:19 AM, Yuya Nishihara <yuya@tcha.org> wrote:
>>>
>>> On Tue, 02 Jan 2018 23:55:08 -0500, Matt Harbison wrote:
>>>>> On Thu, 28 Dec 2017 08:04:01 -0500, Yuya Nishihara <yuya@tcha.org>  
>>>>> wrote:
>>>>>> On Wed, 27 Dec 2017 03:27:58 -0500, Matt Harbison wrote:
>>>>>> # HG changeset patch
>>>>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>>>>> # Date 1514349649 18000
>>>>>> #      Tue Dec 26 23:40:49 2017 -0500
>>>>>> # Node ID f2e5631c99e6e2c7e9bac058185c24dd73a04e98
>>>>>> # Parent  25ecea2ec3d7844c9146bf878fc5093ab33b6e11
>>>>>> largefiles: modernize how capabilities are added to the wire  
>>>>>> protocol
>>>>>
>>>>> Looks good to me, so queued, thanks.
>>>>
>>>> Any thoughts on how the client can let the server know it has the
>>>> extension loaded, so I can close off the last hole in this series?
>>>
>>> CC +indygreg, augie
>>>
>>> I think there were some discussion about advertising client's  
>>> capability,
>>> but I don't remember.
>>
>> OOC, why do we want the client to be advertising that it groks  
>> largefiles to the server? I have a guess, but I’d like that explained  
>> before I start trying to figure out what we need to support...
>
> I’m not in front of a computer right now, but see this.  I think it is  
> related to choking on the external flag that it doesn’t understand  
> without the extension.
>
> https://www.mercurial-scm.org/repo/hg-all/file/fa865878a849/tests/test-lfs-serve.t#l189

To put a little meat on this:

1) A requirement should always be added when a commit adding lfs files  
appears in a repo.  Otherwise cryptic errors occur.  Pulling from a remote  
server is the last hole I'm aware of.

2) The error referenced above is because client and server can't agree on  
a common changegroup.  LFS forces only changegroup3.  See the error log at  
the bottom of the test.  (Aside: you previously mentioned aiming to take  
the experimental shrink wrap off LFS after the next cycle.  But IDK if we  
want a non-experimental thing depending on something experimental, and IDK  
how far from being finalize changegroup3 is.)

3) If changegroup3 is manually enabled, then the server error no longer  
happens on that clone, but rather the client blows up:

   $ hg clone -q http://localhost:$HGPORT $TESTTMP/client4_clone --config  
experimental.changegroup3=True
   transaction abort!
   rollback completed
   abort: missing processor for flag '0x2000'!
   [255]

The check in the other direction is in fa865878a849, along with some  
comments on how largefiles already does this.  (Largefiles doesn't have  
these flags, so I think it is just to ensure the extension is loaded, so  
you don't see the standins.)
Yuya Nishihara - Jan. 7, 2018, 6:28 a.m.
On Sat, 06 Jan 2018 19:42:35 -0500, Matt Harbison wrote:
> On Sat, 06 Jan 2018 15:56:43 -0500, Matt Harbison <mharbison72@gmail.com>  
> wrote:
> >> On Jan 6, 2018, at 3:09 PM, Augie Fackler <raf@durin42.com> wrote:
> >>> On Jan 5, 2018, at 1:19 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> >>> On Tue, 02 Jan 2018 23:55:08 -0500, Matt Harbison wrote:
> >>>>> On Thu, 28 Dec 2017 08:04:01 -0500, Yuya Nishihara <yuya@tcha.org>  
> >>>>> wrote:
> >>>>>> On Wed, 27 Dec 2017 03:27:58 -0500, Matt Harbison wrote:
> >>>>>> # HG changeset patch
> >>>>>> # User Matt Harbison <matt_harbison@yahoo.com>
> >>>>>> # Date 1514349649 18000
> >>>>>> #      Tue Dec 26 23:40:49 2017 -0500
> >>>>>> # Node ID f2e5631c99e6e2c7e9bac058185c24dd73a04e98
> >>>>>> # Parent  25ecea2ec3d7844c9146bf878fc5093ab33b6e11
> >>>>>> largefiles: modernize how capabilities are added to the wire  
> >>>>>> protocol
> >>>>>
> >>>>> Looks good to me, so queued, thanks.
> >>>>
> >>>> Any thoughts on how the client can let the server know it has the
> >>>> extension loaded, so I can close off the last hole in this series?
> >>>
> >>> CC +indygreg, augie
> >>>
> >>> I think there were some discussion about advertising client's  
> >>> capability,
> >>> but I don't remember.
> >>
> >> OOC, why do we want the client to be advertising that it groks  
> >> largefiles to the server? I have a guess, but I’d like that explained  
> >> before I start trying to figure out what we need to support...
> >
> > I’m not in front of a computer right now, but see this.  I think it is  
> > related to choking on the external flag that it doesn’t understand  
> > without the extension.
> >
> > https://www.mercurial-scm.org/repo/hg-all/file/fa865878a849/tests/test-lfs-serve.t#l189
> 
> To put a little meat on this:
> 
> 1) A requirement should always be added when a commit adding lfs files  
> appears in a repo.  Otherwise cryptic errors occur.  Pulling from a remote  
> server is the last hole I'm aware of.
> 
> 2) The error referenced above is because client and server can't agree on  
> a common changegroup.  LFS forces only changegroup3.  See the error log at  
> the bottom of the test.  (Aside: you previously mentioned aiming to take  
> the experimental shrink wrap off LFS after the next cycle.  But IDK if we  
> want a non-experimental thing depending on something experimental, and IDK  
> how far from being finalize changegroup3 is.)

Isn't that a server issue which should fail gracefully if no common changegroup
version found?

> 3) If changegroup3 is manually enabled, then the server error no longer  
> happens on that clone, but rather the client blows up:
> 
>    $ hg clone -q http://localhost:$HGPORT $TESTTMP/client4_clone --config  
> experimental.changegroup3=True
>    transaction abort!
>    rollback completed
>    abort: missing processor for flag '0x2000'!
>    [255]

The error message can be made less cryptic as the core knows 0x2000 means
REVIDX_EXTSTORED.
Matt Harbison - Jan. 7, 2018, 7:08 a.m.
On Sun, 07 Jan 2018 01:28:22 -0500, Yuya Nishihara <yuya@tcha.org> wrote:

> On Sat, 06 Jan 2018 19:42:35 -0500, Matt Harbison wrote:
>> On Sat, 06 Jan 2018 15:56:43 -0500, Matt Harbison  
>> <mharbison72@gmail.com>
>> wrote:
>> >> On Jan 6, 2018, at 3:09 PM, Augie Fackler <raf@durin42.com> wrote:
>> >>> On Jan 5, 2018, at 1:19 AM, Yuya Nishihara <yuya@tcha.org> wrote:
>> >>> On Tue, 02 Jan 2018 23:55:08 -0500, Matt Harbison wrote:
>> >>>>> On Thu, 28 Dec 2017 08:04:01 -0500, Yuya Nishihara <yuya@tcha.org>
>> >>>>> wrote:
>> >>>>>> On Wed, 27 Dec 2017 03:27:58 -0500, Matt Harbison wrote:
>> >>>>>> # HG changeset patch
>> >>>>>> # User Matt Harbison <matt_harbison@yahoo.com>
>> >>>>>> # Date 1514349649 18000
>> >>>>>> #      Tue Dec 26 23:40:49 2017 -0500
>> >>>>>> # Node ID f2e5631c99e6e2c7e9bac058185c24dd73a04e98
>> >>>>>> # Parent  25ecea2ec3d7844c9146bf878fc5093ab33b6e11
>> >>>>>> largefiles: modernize how capabilities are added to the wire
>> >>>>>> protocol
>> >>>>>
>> >>>>> Looks good to me, so queued, thanks.
>> >>>>
>> >>>> Any thoughts on how the client can let the server know it has the
>> >>>> extension loaded, so I can close off the last hole in this series?
>> >>>
>> >>> CC +indygreg, augie
>> >>>
>> >>> I think there were some discussion about advertising client's
>> >>> capability,
>> >>> but I don't remember.
>> >>
>> >> OOC, why do we want the client to be advertising that it groks
>> >> largefiles to the server? I have a guess, but I’d like that explained
>> >> before I start trying to figure out what we need to support...
>> >
>> > I’m not in front of a computer right now, but see this.  I think it is
>> > related to choking on the external flag that it doesn’t understand
>> > without the extension.
>> >
>> >  
>> https://www.mercurial-scm.org/repo/hg-all/file/fa865878a849/tests/test-lfs-serve.t#l189
>>
>> To put a little meat on this:
>>
>> 1) A requirement should always be added when a commit adding lfs files
>> appears in a repo.  Otherwise cryptic errors occur.  Pulling from a  
>> remote
>> server is the last hole I'm aware of.
>>
>> 2) The error referenced above is because client and server can't agree  
>> on
>> a common changegroup.  LFS forces only changegroup3.  See the error log  
>> at
>> the bottom of the test.  (Aside: you previously mentioned aiming to take
>> the experimental shrink wrap off LFS after the next cycle.  But IDK if  
>> we
>> want a non-experimental thing depending on something experimental, and  
>> IDK
>> how far from being finalize changegroup3 is.)
>
> Isn't that a server issue which should fail gracefully if no common  
> changegroup
> version found?

Maybe.  The wire protocol stuff is pretty opaque to me.  When two sides  
can't talk to each other, IDK how one tells the other that though.

I wasn't too worried about this, because presumably we would enable it by  
default soon-ish.

>> 3) If changegroup3 is manually enabled, then the server error no longer
>> happens on that clone, but rather the client blows up:
>>
>>    $ hg clone -q http://localhost:$HGPORT $TESTTMP/client4_clone  
>> --config
>> experimental.changegroup3=True
>>    transaction abort!
>>    rollback completed
>>    abort: missing processor for flag '0x2000'!
>>    [255]
>
> The error message can be made less cryptic as the core knows 0x2000 means
> REVIDX_EXTSTORED.

I'd be fine with mentioning 'enable lfs' from here if we can sort out the  
changegroup3 issue.  Are there any cache issues to worry about with the  
rollback?  (I keep seeing that strip isn't cache friendly, so I assume  
rollback isn't either, unless they are part of the transaction.   
Presumably knowing capabilities up front would fail fast.)
Matt Harbison - Jan. 7, 2018, 6:18 p.m.
On Sun, 07 Jan 2018 02:08:58 -0500, Matt Harbison <mharbison72@gmail.com>  
wrote:

> On Sun, 07 Jan 2018 01:28:22 -0500, Yuya Nishihara <yuya@tcha.org> wrote:
>
>> On Sat, 06 Jan 2018 19:42:35 -0500, Matt Harbison wrote:
>>> On Sat, 06 Jan 2018 15:56:43 -0500, Matt Harbison  
>>> <mharbison72@gmail.com>
>>> wrote:

>>> 3) If changegroup3 is manually enabled, then the server error no longer
>>> happens on that clone, but rather the client blows up:
>>>
>>>    $ hg clone -q http://localhost:$HGPORT $TESTTMP/client4_clone  
>>> --config
>>> experimental.changegroup3=True
>>>    transaction abort!
>>>    rollback completed
>>>    abort: missing processor for flag '0x2000'!
>>>    [255]
>>
>> The error message can be made less cryptic as the core knows 0x2000  
>> means
>> REVIDX_EXTSTORED.
>
> I'd be fine with mentioning 'enable lfs' from here if we can sort out  
> the changegroup3 issue.  Are there any cache issues to worry about with  
> the rollback?  (I keep seeing that strip isn't cache friendly, so I  
> assume rollback isn't either, unless they are part of the transaction.   
> Presumably knowing capabilities up front would fail fast.)

On second thought, maybe not.  Existing clients won't be able to handle  
this.  Granted, they won't be able to do LFS period.  But largefiles is  
able to tell the client "please enable the largefiles extension" no matter  
the client version, which seems more user friendly.

I guess the amount of work determines whether or not it's worth it.  I  
assume some other extensions could use this too.  (e.g. does a clone of a  
sparse/narrow clone require sparse/narrow to be enabled?)
Augie Fackler - Jan. 15, 2018, 4:20 p.m.
> On Jan 7, 2018, at 1:18 PM, Matt Harbison <mharbison72@gmail.com> wrote:
> 
> On Sun, 07 Jan 2018 02:08:58 -0500, Matt Harbison <mharbison72@gmail.com> wrote:
> 
>> On Sun, 07 Jan 2018 01:28:22 -0500, Yuya Nishihara <yuya@tcha.org> wrote:
>> 
>>> On Sat, 06 Jan 2018 19:42:35 -0500, Matt Harbison wrote:
>>>> On Sat, 06 Jan 2018 15:56:43 -0500, Matt Harbison <mharbison72@gmail.com>
>>>> wrote:
> 
>>>> 3) If changegroup3 is manually enabled, then the server error no longer
>>>> happens on that clone, but rather the client blows up:
>>>> 
>>>>   $ hg clone -q http://localhost:$HGPORT $TESTTMP/client4_clone --config
>>>> experimental.changegroup3=True
>>>>   transaction abort!
>>>>   rollback completed
>>>>   abort: missing processor for flag '0x2000'!
>>>>   [255]
>>> 
>>> The error message can be made less cryptic as the core knows 0x2000 means
>>> REVIDX_EXTSTORED.
>> 
>> I'd be fine with mentioning 'enable lfs' from here if we can sort out the changegroup3 issue.  Are there any cache issues to worry about with the rollback?  (I keep seeing that strip isn't cache friendly, so I assume rollback isn't either, unless they are part of the transaction.  Presumably knowing capabilities up front would fail fast.)
> 
> On second thought, maybe not.  Existing clients won't be able to handle this.  Granted, they won't be able to do LFS period.  But largefiles is able to tell the client "please enable the largefiles extension" no matter the client version, which seems more user friendly.

I think if we can come up with a kludge to enable lfs during clone, that’s the best choice. Otherwise you end up throwing away all the clone progress and force a re-clone after changesets and manifests were already transferred, which is a pretty lousy user experience.

> 
> I guess the amount of work determines whether or not it's worth it.  I assume some other extensions could use this too.  (e.g. does a clone of a sparse/narrow clone require sparse/narrow to be enabled?)

Yeah, it does. I think there’s some amount of sanity here, we just need to reason carefully about it for security reasons. For now, I’d be totally fine with a whitelist of extensions to allow clone to force to on, and some bundle part to request that (or just automatically do it for lfs if we can figure out how without the overhead of an extra part.)

AF
Matt Harbison - Jan. 15, 2018, 5:21 p.m.
> On Jan 15, 2018, at 11:20 AM, Augie Fackler <raf@durin42.com> wrote:
> 
> 
>> On Jan 7, 2018, at 1:18 PM, Matt Harbison <mharbison72@gmail.com> wrote:
>> 
>> On Sun, 07 Jan 2018 02:08:58 -0500, Matt Harbison <mharbison72@gmail.com> wrote:
>> 
>>>> On Sun, 07 Jan 2018 01:28:22 -0500, Yuya Nishihara <yuya@tcha.org> wrote:
>>>> 
>>>>> On Sat, 06 Jan 2018 19:42:35 -0500, Matt Harbison wrote:
>>>>> On Sat, 06 Jan 2018 15:56:43 -0500, Matt Harbison <mharbison72@gmail.com>
>>>>> wrote:
>> 
>>>>> 3) If changegroup3 is manually enabled, then the server error no longer
>>>>> happens on that clone, but rather the client blows up:
>>>>> 
>>>>>  $ hg clone -q http://localhost:$HGPORT $TESTTMP/client4_clone --config
>>>>> experimental.changegroup3=True
>>>>>  transaction abort!
>>>>>  rollback completed
>>>>>  abort: missing processor for flag '0x2000'!
>>>>>  [255]
>>>> 
>>>> The error message can be made less cryptic as the core knows 0x2000 means
>>>> REVIDX_EXTSTORED.
>>> 
>>> I'd be fine with mentioning 'enable lfs' from here if we can sort out the changegroup3 issue.  Are there any cache issues to worry about with the rollback?  (I keep seeing that strip isn't cache friendly, so I assume rollback isn't either, unless they are part of the transaction.  Presumably knowing capabilities up front would fail fast.)
>> 
>> On second thought, maybe not.  Existing clients won't be able to handle this.  Granted, they won't be able to do LFS period.  But largefiles is able to tell the client "please enable the largefiles extension" no matter the client version, which seems more user friendly.
> 
> I think if we can come up with a kludge to enable lfs during clone, that’s the best choice. Otherwise you end up throwing away all the clone progress and force a re-clone after changesets and manifests were already transferred, which is a pretty lousy user experience.

I guess there are two cases here then. I wasn’t thinking of clone separately, but that does sound like an improvement.

The case I’m thinking of is an existing non-LFS repo, Alice enables the extension on the server and pushes LFS content. Somehow Bob needs to know when he pulls into his existing repo. It doesn’t sounds like a clone kludge helps with this?

>> 
>> I guess the amount of work determines whether or not it's worth it.  I assume some other extensions could use this too.  (e.g. does a clone of a sparse/narrow clone require sparse/narrow to be enabled?)
> 
> Yeah, it does. I think there’s some amount of sanity here, we just need to reason carefully about it for security reasons. For now, I’d be totally fine with a whitelist of extensions to allow clone to force to on, and some bundle part to request that (or just automatically do it for lfs if we can figure out how without the overhead of an extra part.)

Definitely gonna need someone with protocol knowledge to do that, and my priority before the freeze is landing the .hglfs file support. What sort of rules are in place for experimental stuff, the freeze, and obvious UX improvements like clear error messaging?  I thought some stuff was taken recently (maybe for terse status) that wouldn’t usually fly during a freeze.
Augie Fackler - Jan. 15, 2018, 5:33 p.m.
> On Jan 15, 2018, at 12:21 PM, Matt Harbison <mharbison72@gmail.com> wrote:
> 
>> 
>> On Jan 15, 2018, at 11:20 AM, Augie Fackler <raf@durin42.com> wrote:
>> 
>> 
>>> On Jan 7, 2018, at 1:18 PM, Matt Harbison <mharbison72@gmail.com> wrote:
>>> 
>>> On Sun, 07 Jan 2018 02:08:58 -0500, Matt Harbison <mharbison72@gmail.com> wrote:
>>> 
>>>>> On Sun, 07 Jan 2018 01:28:22 -0500, Yuya Nishihara <yuya@tcha.org> wrote:
>>>>> 
>>>>>> On Sat, 06 Jan 2018 19:42:35 -0500, Matt Harbison wrote:
>>>>>> On Sat, 06 Jan 2018 15:56:43 -0500, Matt Harbison <mharbison72@gmail.com>
>>>>>> wrote:
>>> 
>>>>>> 3) If changegroup3 is manually enabled, then the server error no longer
>>>>>> happens on that clone, but rather the client blows up:
>>>>>> 
>>>>>> $ hg clone -q http://localhost:$HGPORT $TESTTMP/client4_clone --config
>>>>>> experimental.changegroup3=True
>>>>>> transaction abort!
>>>>>> rollback completed
>>>>>> abort: missing processor for flag '0x2000'!
>>>>>> [255]
>>>>> 
>>>>> The error message can be made less cryptic as the core knows 0x2000 means
>>>>> REVIDX_EXTSTORED.
>>>> 
>>>> I'd be fine with mentioning 'enable lfs' from here if we can sort out the changegroup3 issue.  Are there any cache issues to worry about with the rollback?  (I keep seeing that strip isn't cache friendly, so I assume rollback isn't either, unless they are part of the transaction.  Presumably knowing capabilities up front would fail fast.)
>>> 
>>> On second thought, maybe not.  Existing clients won't be able to handle this.  Granted, they won't be able to do LFS period.  But largefiles is able to tell the client "please enable the largefiles extension" no matter the client version, which seems more user friendly.
>> 
>> I think if we can come up with a kludge to enable lfs during clone, that’s the best choice. Otherwise you end up throwing away all the clone progress and force a re-clone after changesets and manifests were already transferred, which is a pretty lousy user experience.
> 
> I guess there are two cases here then. I wasn’t thinking of clone separately, but that does sound like an improvement.
> 
> The case I’m thinking of is an existing non-LFS repo, Alice enables the extension on the server and pushes LFS content. Somehow Bob needs to know when he pulls into his existing repo. It doesn’t sounds like a clone kludge helps with this?
> 
>>> 
>>> I guess the amount of work determines whether or not it's worth it.  I assume some other extensions could use this too.  (e.g. does a clone of a sparse/narrow clone require sparse/narrow to be enabled?)
>> 
>> Yeah, it does. I think there’s some amount of sanity here, we just need to reason carefully about it for security reasons. For now, I’d be totally fine with a whitelist of extensions to allow clone to force to on, and some bundle part to request that (or just automatically do it for lfs if we can figure out how without the overhead of an extra part.)
> 
> Definitely gonna need someone with protocol knowledge to do that, and my priority before the freeze is landing the .hglfs file support. What sort of rules are in place for experimental stuff, the freeze, and obvious UX improvements like clear error messaging?  I thought some stuff was taken recently (maybe for terse status) that wouldn’t usually fly during a freeze.

We’re generally more relaxed for things that only matter for experimental code, and UX improvements can happen if they’re important enough.

Ugh, I forgot the freeze. We might need to do this UX improvement in the next cycle as part of making lfs not experimental...
Matt Harbison - Jan. 16, 2018, 4:59 a.m.
On Mon, 15 Jan 2018 12:33:19 -0500, Augie Fackler <raf@durin42.com> wrote:

> We’re generally more relaxed for things that only matter for  
> experimental code, and UX improvements can happen if they’re important  
> enough.
>
> Ugh, I forgot the freeze. We might need to do this UX improvement in the  
> next cycle as part of making lfs not experimental...

Any idea when the official cutoff is?  I've been assuming the 18th,  
hopefully late evening -0500.  In addition to the stuff I have in flight,  
I need to tweak the User-Agent again, and default to no lfs workers to  
avoid the upload error.  The latter two are probably important to get into  
the -rc installer to avoid a bad first impression.

I'm fine holding off on that UX improvement.  I'm sure we will hit it, but  
it's manageable.
Augie Fackler - Jan. 16, 2018, 2:42 p.m.
> On Jan 15, 2018, at 23:59, Matt Harbison <mharbison72@gmail.com> wrote:
> 
> On Mon, 15 Jan 2018 12:33:19 -0500, Augie Fackler <raf@durin42.com> wrote:
> 
>> We’re generally more relaxed for things that only matter for experimental code, and UX improvements can happen if they’re important enough.
>> 
>> Ugh, I forgot the freeze. We might need to do this UX improvement in the next cycle as part of making lfs not experimental...
> 
> Any idea when the official cutoff is?  I've been assuming the 18th, hopefully late evening -0500.  In addition to the stuff I have in flight, I need to tweak the User-Agent again, and default to no lfs workers to avoid the upload error.  The latter two are probably important to get into the -rc installer to avoid a bad first impression.

Late on the 18th or first thing on the 19th seems likely as a time when one of us will cut the RC, which starts the freeze. I won't do it before EOD on the 18th.

> I'm fine holding off on that UX improvement.  I'm sure we will hit it, but it's manageable.

Patch

diff --git a/hgext/largefiles/proto.py b/hgext/largefiles/proto.py
--- a/hgext/largefiles/proto.py
+++ b/hgext/largefiles/proto.py
@@ -161,9 +161,11 @@ 
     repo.__class__ = lfileswirerepository
 
 # advertise the largefiles=serve capability
-def capabilities(repo, proto):
-    '''Wrap server command to announce largefile server capability'''
-    return capabilitiesorig(repo, proto) + ' largefiles=serve'
+def _capabilities(orig, repo, proto):
+    '''announce largefile server capability'''
+    caps = orig(repo, proto)
+    caps.append('largefiles=serve')
+    return caps
 
 def heads(repo, proto):
     '''Wrap server command - largefile capable clients will know to call
diff --git a/hgext/largefiles/uisetup.py b/hgext/largefiles/uisetup.py
--- a/hgext/largefiles/uisetup.py
+++ b/hgext/largefiles/uisetup.py
@@ -166,7 +166,6 @@ 
     wireproto.commands['statlfile'] = (proto.statlfile, 'sha')
 
     # ... and wrap some existing ones
-    wireproto.commands['capabilities'] = (proto.capabilities, '')
     wireproto.commands['heads'] = (proto.heads, '')
     wireproto.commands['lheads'] = (wireproto.heads, '')
 
@@ -178,10 +177,7 @@ 
 
     extensions.wrapfunction(webcommands, 'decodepath', overrides.decodepath)
 
-    # the hello wireproto command uses wireproto.capabilities, so it won't see
-    # our largefiles capability unless we replace the actual function as well.
-    proto.capabilitiesorig = wireproto.capabilities
-    wireproto.capabilities = proto.capabilities
+    extensions.wrapfunction(wireproto, '_capabilities', proto._capabilities)
 
     # can't do this in reposetup because it needs to have happened before
     # wirerepo.__init__ is called