Patchwork [3,of,4,V2] serve: make the URL the same for `hg serve` and `hg serve -S`

login
register
mail settings
Submitter Matt Harbison
Date Feb. 16, 2017, 9:41 p.m.
Message ID <38babd487181374325f3.1487281270@Envy>
Download mbox | patch
Permalink /patch/18574/
State Changes Requested
Delegated to: Yuya Nishihara
Headers show

Comments

Matt Harbison - Feb. 16, 2017, 9:41 p.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1486877030 18000
#      Sun Feb 12 00:23:50 2017 -0500
# Node ID 38babd487181374325f3d27c5bc081dadf31b9b9
# Parent  27a4bc77e8b8e50abc76c387f117082e5853c47e
serve: make the URL the same for `hg serve` and `hg serve -S`

It's perfectly workable to serve up the parent repo without the -S for push and
pull, as long as there are no subrepo changes are in play.  Therefore, having a
different URL for the main repo based on the presence of this option seems like
it would get annoying.

The first attempt at this was simply to trim the outer repo name from the paths
when populating 'webconf', so that the root repository was actually hosted at
'/'.  That worked fine for `hg` operations, but had bad effects on the html,
such as making the 'changeset' link on the left side simply 'rev/tip'.  (The
source html shows the link was '//rev/tip', since {repo} was subbed in as an
empty string.)  There were also issues rendering the subrepo pages that ended
with 'tip', including missing icons.

By forwarding the `hg` request for the outer repo, all of the actual
repositories can remain in their normal places.  The forward needs to be
permanent, so that the actual address is stored in the hgrc file.  This is
crucial, because the .hgsub source paths are evaluated relative to the parent
repo's source.  This avoids having to figure out how to put in forwarding logic
for each subrepository as well.
Yuya Nishihara - Feb. 20, 2017, 2:38 p.m.
On Thu, 16 Feb 2017 16:41:10 -0500, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1486877030 18000
> #      Sun Feb 12 00:23:50 2017 -0500
> # Node ID 38babd487181374325f3d27c5bc081dadf31b9b9
> # Parent  27a4bc77e8b8e50abc76c387f117082e5853c47e
> serve: make the URL the same for `hg serve` and `hg serve -S`

>              elif not virtual:
> +                if self._rootrepo:
> +                    # Redirect '/' to the main repo when -S is given.
> +                    path = '/' + self._rootrepo
> +                    if self.prefix:
> +                        path = '/' + self.prefix + path
> +                    req.headers.append(('Location', path))
> +                    html = '<html>Moved <a href="' + path + '">here</a>.</html>'
> +                    req.respond(HTTP_MOVED_PERMANENTLY, "text/html", body=html)
> +                    return []

Suppose "hg serve" is used for temporarily serving random repositories, 301
seems too strong.
Matt Harbison - Feb. 21, 2017, 3:25 a.m.
On Mon, 20 Feb 2017 09:38:13 -0500, Yuya Nishihara <yuya@tcha.org> wrote:

> On Thu, 16 Feb 2017 16:41:10 -0500, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1486877030 18000
>> #      Sun Feb 12 00:23:50 2017 -0500
>> # Node ID 38babd487181374325f3d27c5bc081dadf31b9b9
>> # Parent  27a4bc77e8b8e50abc76c387f117082e5853c47e
>> serve: make the URL the same for `hg serve` and `hg serve -S`
>
>>              elif not virtual:
>> +                if self._rootrepo:
>> +                    # Redirect '/' to the main repo when -S is given.
>> +                    path = '/' + self._rootrepo
>> +                    if self.prefix:
>> +                        path = '/' + self.prefix + path
>> +                    req.headers.append(('Location', path))
>> +                    html = '<html>Moved <a href="' + path +  
>> '">here</a>.</html>'
>> +                    req.respond(HTTP_MOVED_PERMANENTLY, "text/html",  
>> body=html)
>> +                    return []
>
> Suppose "hg serve" is used for temporarily serving random repositories,  
> 301
> seems too strong.

The temporary serve is the case that I'm interested in.  It may not be  
clear from this snippet, but the redirect is only reachable when using `hg  
serve -S`, not random repos listed in a --web-conf file.

I guess I didn't think it was too strong, because the repo never is at  
'/'.  Once the `hg serve` process is killed, everything is gone.   
Everything goes away eventually.  But until then, it's a permanent  
redirect.

The temporary/permanent distinction is important, because there needs to  
be a trigger to update paths.default in hgrc without doing so for  
temporary redirects.  The updated hgrc path is important, because the  
subrepo is cloned relative to the parent repo's paths.default value.   
(Keeping both the sub=sub and sub=../sub style paths working in the two  
new tests were one of the many headaches with this.)

Since nothing other than hg clients should see the redirect, do you have a  
specific concern?  (I haven't paid much attention to chg to know if it  
would somehow cache the URL and get directed wrong when `hg serve` is  
killed and something else is served.  But the aggressive caching done by  
web browsers is why I limited the redirect as narrowly as possible.)
Yuya Nishihara - Feb. 21, 2017, 2:32 p.m.
On Mon, 20 Feb 2017 22:25:21 -0500, Matt Harbison wrote:
> On Mon, 20 Feb 2017 09:38:13 -0500, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Thu, 16 Feb 2017 16:41:10 -0500, Matt Harbison wrote:
> >> # HG changeset patch
> >> # User Matt Harbison <matt_harbison@yahoo.com>
> >> # Date 1486877030 18000
> >> #      Sun Feb 12 00:23:50 2017 -0500
> >> # Node ID 38babd487181374325f3d27c5bc081dadf31b9b9
> >> # Parent  27a4bc77e8b8e50abc76c387f117082e5853c47e
> >> serve: make the URL the same for `hg serve` and `hg serve -S`
> >
> >>              elif not virtual:
> >> +                if self._rootrepo:
> >> +                    # Redirect '/' to the main repo when -S is given.
> >> +                    path = '/' + self._rootrepo
> >> +                    if self.prefix:
> >> +                        path = '/' + self.prefix + path
> >> +                    req.headers.append(('Location', path))
> >> +                    html = '<html>Moved <a href="' + path +  
> >> '">here</a>.</html>'
> >> +                    req.respond(HTTP_MOVED_PERMANENTLY, "text/html",  
> >> body=html)
> >> +                    return []
> >
> > Suppose "hg serve" is used for temporarily serving random repositories,  
> > 301
> > seems too strong.
> 
> The temporary serve is the case that I'm interested in.  It may not be  
> clear from this snippet, but the redirect is only reachable when using `hg  
> serve -S`, not random repos listed in a --web-conf file.
> 
> I guess I didn't think it was too strong, because the repo never is at  
> '/'.  Once the `hg serve` process is killed, everything is gone.   
> Everything goes away eventually.  But until then, it's a permanent  
> redirect.
> 
> The temporary/permanent distinction is important, because there needs to  
> be a trigger to update paths.default in hgrc without doing so for  
> temporary redirects.  The updated hgrc path is important, because the  
> subrepo is cloned relative to the parent repo's paths.default value.   
> (Keeping both the sub=sub and sub=../sub style paths working in the two  
> new tests were one of the many headaches with this.)
> 
> Since nothing other than hg clients should see the redirect, do you have a  
> specific concern?  (I haven't paid much attention to chg to know if it  
> would somehow cache the URL and get directed wrong when `hg serve` is  
> killed and something else is served.  But the aggressive caching done by  
> web browsers is why I limited the redirect as narrowly as possible.)

Hmm, if no web browser is involved, it would be okay as hg client wouldn't have
a permanent URL cache. I don't like the idea of using redirection to compensate
the problem of hgweb, though.

Patch

diff --git a/mercurial/hgweb/__init__.py b/mercurial/hgweb/__init__.py
--- a/mercurial/hgweb/__init__.py
+++ b/mercurial/hgweb/__init__.py
@@ -85,9 +85,13 @@ 
     def run(self):
         self.httpd.serve_forever()
 
-def createapp(baseui, repo, webconf):
+def createapp(baseui, repo, webconf, subrepos=False):
     if webconf:
-        return hgwebdir_mod.hgwebdir(webconf, baseui=baseui)
+        rootrepo = None
+        if subrepos:
+            rootrepo = repo.wvfs.basename(repo.root)
+
+        return hgwebdir_mod.hgwebdir(webconf, baseui=baseui, rootrepo=rootrepo)
     else:
         if not repo:
             raise error.RepoError(_("there is no Mercurial repository"
diff --git a/mercurial/hgweb/common.py b/mercurial/hgweb/common.py
--- a/mercurial/hgweb/common.py
+++ b/mercurial/hgweb/common.py
@@ -23,6 +23,7 @@ 
 httpserver = util.httpserver
 
 HTTP_OK = 200
+HTTP_MOVED_PERMANENTLY = 301
 HTTP_NOT_MODIFIED = 304
 HTTP_BAD_REQUEST = 400
 HTTP_UNAUTHORIZED = 401
diff --git a/mercurial/hgweb/hgwebdir_mod.py b/mercurial/hgweb/hgwebdir_mod.py
--- a/mercurial/hgweb/hgwebdir_mod.py
+++ b/mercurial/hgweb/hgwebdir_mod.py
@@ -16,6 +16,7 @@ 
 
 from .common import (
     ErrorResponse,
+    HTTP_MOVED_PERMANENTLY,
     HTTP_NOT_FOUND,
     HTTP_OK,
     HTTP_SERVER_ERROR,
@@ -115,12 +116,13 @@ 
 
     Instances are typically used as WSGI applications.
     """
-    def __init__(self, conf, baseui=None):
+    def __init__(self, conf, baseui=None, rootrepo=None):
         self.conf = conf
         self.baseui = baseui
         self.ui = None
         self.lastrefresh = 0
         self.motd = None
+        self._rootrepo = rootrepo
         self.refresh()
 
     def refresh(self):
@@ -255,6 +257,15 @@ 
 
             # top-level index
             elif not virtual:
+                if self._rootrepo:
+                    # Redirect '/' to the main repo when -S is given.
+                    path = '/' + self._rootrepo
+                    if self.prefix:
+                        path = '/' + self.prefix + path
+                    req.headers.append(('Location', path))
+                    html = '<html>Moved <a href="' + path + '">here</a>.</html>'
+                    req.respond(HTTP_MOVED_PERMANENTLY, "text/html", body=html)
+                    return []
                 req.respond(HTTP_OK, ctype)
                 return self.makeindex(req, tmpl)
 
diff --git a/mercurial/server.py b/mercurial/server.py
--- a/mercurial/server.py
+++ b/mercurial/server.py
@@ -166,7 +166,7 @@ 
         for u in alluis:
             u.setconfig("web", o, val, 'serve')
 
-    app = hgweb.createapp(baseui, repo, webconf)
+    app = hgweb.createapp(baseui, repo, webconf, opts.get('subrepos'))
     return hgweb.httpservice(servui, app, opts)
 
 def createservice(ui, repo, opts):
diff --git a/tests/test-subrepo-deep-nested-change.t b/tests/test-subrepo-deep-nested-change.t
--- a/tests/test-subrepo-deep-nested-change.t
+++ b/tests/test-subrepo-deep-nested-change.t
@@ -84,7 +84,8 @@ 
   adding sub2/ = $TESTTMP/main/sub1/sub2 (glob)
   $ cat hg1.pid >> $DAEMON_PIDS
 
-  $ hg clone http://user@localhost:$HGPORT/main httpclone --config progress.disable=True
+  $ hg clone http://user@localhost:$HGPORT httpclone --config progress.disable=True
+  real URL is http://localhost:$HGPORT/main
   requesting all changes
   adding changesets
   adding manifests
@@ -111,7 +112,10 @@ 
   http://user@localhost:$HGPORT/sub1
 
   $ cat access.log
+  * "GET /?cmd=capabilities HTTP/1.1" 301 - (glob)
+  * "GET /main HTTP/1.1" 200 - (glob)
   * "GET /main?cmd=capabilities HTTP/1.1" 200 - (glob)
+  * "GET /main?cmd=between HTTP/1.1" 200 - * (glob)
   * "GET /main?cmd=batch HTTP/1.1" 200 - * (glob)
   * "GET /main?cmd=getbundle HTTP/1.1" 200 - * (glob)
   * "GET /sub1?cmd=capabilities HTTP/1.1" 200 - (glob)
diff --git a/tests/test-subrepo-recursion.t b/tests/test-subrepo-recursion.t
--- a/tests/test-subrepo-recursion.t
+++ b/tests/test-subrepo-recursion.t
@@ -263,7 +263,8 @@ 
   adding repo/foo/bar/ = $TESTTMP/repo/foo/bar (glob)
   $ cat hg1.pid >> $DAEMON_PIDS
 
-  $ hg clone http://user:pass@localhost:$HGPORT/repo clone  --config progress.disable=True
+  $ hg clone http://user:pass@localhost:$HGPORT clone  --config progress.disable=True
+  real URL is http://localhost:$HGPORT/repo
   requesting all changes
   adding changesets
   adding manifests
@@ -295,7 +296,10 @@ 
   z3
 
   $ cat access.log
+  * "GET /?cmd=capabilities HTTP/1.1" 301 - (glob)
+  * "GET /repo HTTP/1.1" 200 - (glob)
   * "GET /repo?cmd=capabilities HTTP/1.1" 200 - (glob)
+  * "GET /repo?cmd=between HTTP/1.1" 200 - * (glob)
   * "GET /repo?cmd=batch HTTP/1.1" 200 - * (glob)
   * "GET /repo?cmd=getbundle HTTP/1.1" 200 - * (glob)
   * "GET /repo/foo?cmd=capabilities HTTP/1.1" 200 - (glob)