Re: Odd httpheader=1024 required for Phabricator
On 12/02/2020 17:48, Augie Fackler wrote: > >> Complex software products routinely depend on wrong assumptions. >> >> De-facto we have a situation that Phabricator requires a very old version of >> Mercurial, and thus makes Mercurial look bad. > > Last I knew phabricator was pinning to an ancient hg specifically because > they try to parse our wire protocol blindly without actually understanding > our format evolution policy (we detect new capabilities and then use a new > format), and then blamed /us/ when we changed a format they shouldn't have > been claiming (in the capabilities network call) to support. So I suspect > even if you fix this immediate issue, hg-on-phabricator will still be Very > Broken unless they've gotten their act together about filtering capabilities > to something their software can cope with. OK, that sheds further light on the overall situation. At least I've got answers quickly. I will find other workarounds: I am used to make "broken" things work smoothly, but I don't even have "fix" in my vocabulary. Funnily, someone on the Phabricator discourse forum has just proposed to avoid the "blame" terminology in their product: https://discourse.phabricator-community.org/t/replace-blame-with-annotate/3527 Makarius ___ Mercurial mailing list Mercurial@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial
Re: Odd httpheader=1024 required for Phabricator
> On Feb 12, 2020, at 02:24, Makarius wrote: > > On 12/02/2020 07:50, Augie Fackler wrote: >>> >>> A bisection over the hg repository yields the following relevant changeset: >>> >>> changeset: 30563:e118233172fe >>> user:Gregory Szorc >>> date:Mon Nov 28 20:46:42 2016 -0800 >>> files: mercurial/wireproto.py tests/test-ssh-bundle1.t >>> tests/test-ssh.t >>> description: >>> wireproto: only advertise HTTP-specific capabilities to HTTP peers (BC) >>> >>> What means "BC"? >> >> "breaking change" or "behavior change" depending who you ask. :) >> >>> Maybe Phabricator is a good reason to keep the full information? >> >> Not especially. They're doing it wrong assuming that the --stdio protocol is >> compatible with http, and we've told them that more than once and they've >> responded...negatively. If someone wanted to try and figure out a way to >> have phabricator do the right thing (invoke hg's WSGI application, probably >> via CGI?) that would probably help. I'd certainly be open to mentoring >> someone doing that work, but I can't justify spending time on it myself. >> >>> Do you think you can refine that for a future release of Mercurial? >> >> I'm not sure what you're asking for here... > > A minimal change to resolve the situation, e.g. by some option or config to > force http over stdio. Like I said up-thread, I'm happy to mentor someone through a `hg debugcgiweb` or something that would make it easier to use hg as a CGI script that would be appropriate for exposing to HTTP. I'm not going to say I'll do that work because I frankly don't have the time. > Complex software products routinely depend on wrong assumptions. > > De-facto we have a situation that Phabricator requires a very old version of > Mercurial, and thus makes Mercurial look bad. Last I knew phabricator was pinning to an ancient hg specifically because they try to parse our wire protocol blindly without actually understanding our format evolution policy (we detect new capabilities and then use a new format), and then blamed /us/ when we changed a format they shouldn't have been claiming (in the capabilities network call) to support. So I suspect even if you fix this immediate issue, hg-on-phabricator will still be Very Broken unless they've gotten their act together about filtering capabilities to something their software can cope with. But I'm happy to mentor an easier cgi endpoint for things if someone has the energy. It just won't be me that has that energy. :) > > >Makarius ___ Mercurial mailing list Mercurial@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial
Re: Odd httpheader=1024 required for Phabricator
On 12/02/2020 07:50, Augie Fackler wrote: >> >> A bisection over the hg repository yields the following relevant changeset: >> >> changeset: 30563:e118233172fe >> user:Gregory Szorc >> date:Mon Nov 28 20:46:42 2016 -0800 >> files: mercurial/wireproto.py tests/test-ssh-bundle1.t tests/test-ssh.t >> description: >> wireproto: only advertise HTTP-specific capabilities to HTTP peers (BC) >> >> What means "BC"? > > "breaking change" or "behavior change" depending who you ask. :) > >> Maybe Phabricator is a good reason to keep the full information? > > Not especially. They're doing it wrong assuming that the --stdio protocol is > compatible with http, and we've told them that more than once and they've > responded...negatively. If someone wanted to try and figure out a way to have > phabricator do the right thing (invoke hg's WSGI application, probably via > CGI?) that would probably help. I'd certainly be open to mentoring someone > doing that work, but I can't justify spending time on it myself. > >> Do you think you can refine that for a future release of Mercurial? > > I'm not sure what you're asking for here... A minimal change to resolve the situation, e.g. by some option or config to force http over stdio. Complex software products routinely depend on wrong assumptions. De-facto we have a situation that Phabricator requires a very old version of Mercurial, and thus makes Mercurial look bad. Makarius ___ Mercurial mailing list Mercurial@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial
Re: Odd httpheader=1024 required for Phabricator
> On Feb 11, 2020, at 16:32, Makarius wrote: > > On 11/02/2020 03:20, Augie Fackler wrote: >> I guess I'm not sure what's going on here. >> https://www.mercurial-scm.org/repo/hg/rev/5cda0ce05c42 is the revision that >> introduced that, but I'm not sure why you need to do anything /to >> phabricator/ unless it's trying (poorly) to pretend to be an hg server. Is >> it not just blindly proxying the hg protocol from your hg binary? > > Thanks. This hint has helped me to look in the right spots. > > Phabricator essentially starts a command-line process to learn about the > server capabilities like this: > > echo capabilities | hg -R .../test-repo serve --stdio > > It uses the result for its own http communication. In the output above, there > used to be httpheader=1024 until 4.0.2, but it has disappeared in 4.1. > > > See also the Phabricator sources: > > https://github.com/phacility/phabricator/blob/master/src/applications/diffusion/controller/DiffusionServeController.php#L781 > > https://github.com/phacility/phabricator/blob/master/src/applications/diffusion/controller/DiffusionServeController.php#L838 > > https://github.com/phacility/phabricator/blob/master/src/applications/diffusion/protocol/DiffusionMercurialWireProtocol.php#L107 > > The latter contains further comments about slightly odd censorship of the > capabilities. This is also the place where the workaround is inserted, see > again https://isabelle-dev.sketis.net/T8 > > > A bisection over the hg repository yields the following relevant changeset: > > changeset: 30563:e118233172fe > user:Gregory Szorc > date:Mon Nov 28 20:46:42 2016 -0800 > files: mercurial/wireproto.py tests/test-ssh-bundle1.t tests/test-ssh.t > description: > wireproto: only advertise HTTP-specific capabilities to HTTP peers (BC) > > Previously, the capabilities list was protocol agnostic and we > advertised the same capabilities list to all clients, regardless of > transport protocol. > > A few capabilities are specific to HTTP. I see no good reason why we > should advertise them to SSH clients. So this patch limits their > advertisement to HTTP clients. > > This patch is BC, but SSH clients shouldn't be using the removed > capabilities so there should be no impact. > > > What means "BC"? "breaking change" or "behavior change" depending who you ask. :) > Maybe Phabricator is a good reason to keep the full information? Not especially. They're doing it wrong assuming that the --stdio protocol is compatible with http, and we've told them that more than once and they've responded...negatively. If someone wanted to try and figure out a way to have phabricator do the right thing (invoke hg's WSGI application, probably via CGI?) that would probably help. I'd certainly be open to mentoring someone doing that work, but I can't justify spending time on it myself. > Do you think you can refine that for a future release of Mercurial? I'm not sure what you're asking for here... > > In contrast, it is probably difficult to get a patch accepted by the > Phabricator project, because they are only using rather old hg 2.8.2 for their > main installation, and 2.6.2, 3.5.1 in their tests: > https://github.com/phacility/phabricator/blob/master/src/applications/diffusion/protocol/__tests__/DiffusionMercurialWireProtocolTests.php > > > Makarius ___ Mercurial mailing list Mercurial@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial
Re: Odd httpheader=1024 required for Phabricator
> On Feb 11, 2020, at 4:49 PM, Emile Snyder wrote: > > > >> On Tue, Feb 11, 2020 at 1:34 PM Makarius wrote: >> ... >> This patch is BC, but SSH clients shouldn't be using the removed >> capabilities so there should be no impact. >> >> >> What means "BC"? > > I suspect "backwards compatible"? Breaking Change? ___ Mercurial mailing list Mercurial@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial
Re: Odd httpheader=1024 required for Phabricator
On Tue, Feb 11, 2020 at 1:34 PM Makarius wrote: > ... > This patch is BC, but SSH clients shouldn't be using the removed > capabilities so there should be no impact. > > > What means "BC"? > I suspect "backwards compatible"? ___ Mercurial mailing list Mercurial@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial
Re: Odd httpheader=1024 required for Phabricator
On 11/02/2020 03:20, Augie Fackler wrote: > I guess I'm not sure what's going on here. > https://www.mercurial-scm.org/repo/hg/rev/5cda0ce05c42 is the revision that > introduced that, but I'm not sure why you need to do anything /to > phabricator/ unless it's trying (poorly) to pretend to be an hg server. Is it > not just blindly proxying the hg protocol from your hg binary? Thanks. This hint has helped me to look in the right spots. Phabricator essentially starts a command-line process to learn about the server capabilities like this: echo capabilities | hg -R .../test-repo serve --stdio It uses the result for its own http communication. In the output above, there used to be httpheader=1024 until 4.0.2, but it has disappeared in 4.1. See also the Phabricator sources: https://github.com/phacility/phabricator/blob/master/src/applications/diffusion/controller/DiffusionServeController.php#L781 https://github.com/phacility/phabricator/blob/master/src/applications/diffusion/controller/DiffusionServeController.php#L838 https://github.com/phacility/phabricator/blob/master/src/applications/diffusion/protocol/DiffusionMercurialWireProtocol.php#L107 The latter contains further comments about slightly odd censorship of the capabilities. This is also the place where the workaround is inserted, see again https://isabelle-dev.sketis.net/T8 A bisection over the hg repository yields the following relevant changeset: changeset: 30563:e118233172fe user:Gregory Szorc date:Mon Nov 28 20:46:42 2016 -0800 files: mercurial/wireproto.py tests/test-ssh-bundle1.t tests/test-ssh.t description: wireproto: only advertise HTTP-specific capabilities to HTTP peers (BC) Previously, the capabilities list was protocol agnostic and we advertised the same capabilities list to all clients, regardless of transport protocol. A few capabilities are specific to HTTP. I see no good reason why we should advertise them to SSH clients. So this patch limits their advertisement to HTTP clients. This patch is BC, but SSH clients shouldn't be using the removed capabilities so there should be no impact. What means "BC"? Maybe Phabricator is a good reason to keep the full information? Do you think you can refine that for a future release of Mercurial? In contrast, it is probably difficult to get a patch accepted by the Phabricator project, because they are only using rather old hg 2.8.2 for their main installation, and 2.6.2, 3.5.1 in their tests: https://github.com/phacility/phabricator/blob/master/src/applications/diffusion/protocol/__tests__/DiffusionMercurialWireProtocolTests.php Makarius ___ Mercurial mailing list Mercurial@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial
Re: Odd httpheader=1024 required for Phabricator
I guess I'm not sure what's going on here. https://www.mercurial-scm.org/repo/hg/rev/5cda0ce05c42 is the revision that introduced that, but I'm not sure why you need to do anything /to phabricator/ unless it's trying (poorly) to pretend to be an hg server. Is it not just blindly proxying the hg protocol from your hg binary? (We've seen problems with phabricator being too-clever-by-half in the past and breaking us...) > On Feb 10, 2020, at 14:16, Makarius wrote: > > Dear Mercurial experts, > > working on Phabrictor hosting for Mercurial, I have come across an odd problem > with https URLs. In contrast to the old hg 2.8.2 of > https://admin.phacility.com more recent hg versions require a slightly odd > "httpheader=1024" for the capabilities of the command server, otherwise > Phabricator is unable to serve hosted repositories via HTTP. > > See also my own ticket https://isabelle-dev.sketis.net/T8 for more details. > > > Can anyone shed some light on this patch and/or point to the precise hg > version where this was introduced? > > [The motivation of this post is to have a better explanation of the patch that > I've found on the net, and eventually submit a well-founded request to get it > accepted by the Phabricator project.] > > >Makarius > ___ > Mercurial mailing list > Mercurial@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial ___ Mercurial mailing list Mercurial@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial
