Patchwork [STABLE] hgweb: use introrev() for finding parents (issue4506)

login
register
mail settings
Submitter Anton Shestakov
Date Feb. 19, 2015, 1:41 p.m.
Message ID <c5c1ffca678395337f8e.1424353306@neuro>
Download mbox | patch
Permalink /patch/7812/
State Accepted
Commit 75f94dcf76fdfeaebeaaea279ca5b88e3bc8a20b
Headers show

Comments

Anton Shestakov - Feb. 19, 2015, 1:41 p.m.
# HG changeset patch
# User Anton Shestakov <engored@ya.ru>
# Date 1424345526 -28800
#      Thu Feb 19 19:32:06 2015 +0800
# Branch stable
# Node ID c5c1ffca678395337f8ef415423f990a9397d4ef
# Parent  942a5a34b2d0611ab284380fbe45b9bb1897af98
hgweb: use introrev() for finding parents (issue4506)

The issue is titled "filtered revision 'XXX' (not in 'served' subset)" and that
is the error message you sometimes get when trying to look at a file (/file or
/annotate) in hgweb. For example:

http://hg.intevation.org/mercurial/crew/file/90cf454edd70/mercurial/cmdutil.py

This happens when a parent revision for a file is hidden, thus it is
not 'served' and isn't accessible in hgweb by default. When hgweb tries to
access such changeset, it produces the error and HTTP status code 404.

Another detail is that the parents() function, that is used in multiple places
in hgweb, sometimes returned changesets that were obsoleted by the current
changeset for the file. For example, when using rebase with evolve and rebasing
a divergent changeset that introduces a file on top of current branch. Or
grafting a change and making the new grafted changeset obsolete the source
(shown in the test case). The result is the same - the obsoleted changeset was
mistakingly returned from parents(), even though it's not a parent and the only
link to the new changeset is an obsoletion marker (and rebase/graft metadata?
not sure it matters).

The problem is fixed by using introrev() instead of linkrev() for finding
parents. This prevents parents() function from returning unrelated obsolete
changesets.

The test case prepares a separate repo because (afaict) all other test cases
never reuse file names, so there are no files that were changed in multiple
changesets. So no previously available files have obsolete changesets in their
history.
Augie Fackler - Feb. 19, 2015, 9:14 p.m.
On Thu, Feb 19, 2015 at 09:41:46PM +0800, Anton Shestakov wrote:
> # HG changeset patch
> # User Anton Shestakov <engored@ya.ru>
> # Date 1424345526 -28800
> #      Thu Feb 19 19:32:06 2015 +0800
> # Branch stable
> # Node ID c5c1ffca678395337f8ef415423f990a9397d4ef
> # Parent  942a5a34b2d0611ab284380fbe45b9bb1897af98
> hgweb: use introrev() for finding parents (issue4506)

This patch LGTM, but I want marmoute's eyes on it before I push it.

>
> The issue is titled "filtered revision 'XXX' (not in 'served' subset)" and that
> is the error message you sometimes get when trying to look at a file (/file or
> /annotate) in hgweb. For example:
>
> http://hg.intevation.org/mercurial/crew/file/90cf454edd70/mercurial/cmdutil.py
>
> This happens when a parent revision for a file is hidden, thus it is
> not 'served' and isn't accessible in hgweb by default. When hgweb tries to
> access such changeset, it produces the error and HTTP status code 404.
>
> Another detail is that the parents() function, that is used in multiple places
> in hgweb, sometimes returned changesets that were obsoleted by the current
> changeset for the file. For example, when using rebase with evolve and rebasing
> a divergent changeset that introduces a file on top of current branch. Or
> grafting a change and making the new grafted changeset obsolete the source
> (shown in the test case). The result is the same - the obsoleted changeset was
> mistakingly returned from parents(), even though it's not a parent and the only
> link to the new changeset is an obsoletion marker (and rebase/graft metadata?
> not sure it matters).
>
> The problem is fixed by using introrev() instead of linkrev() for finding
> parents. This prevents parents() function from returning unrelated obsolete
> changesets.
>
> The test case prepares a separate repo because (afaict) all other test cases
> never reuse file names, so there are no files that were changed in multiple
> changesets. So no previously available files have obsolete changesets in their
> history.
>
> diff --git a/mercurial/hgweb/webutil.py b/mercurial/hgweb/webutil.py
> --- a/mercurial/hgweb/webutil.py
> +++ b/mercurial/hgweb/webutil.py
> @@ -139,8 +139,8 @@ def _siblings(siblings=[], hiderev=None)
>
>  def parents(ctx, hide=None):
>      if (isinstance(ctx, context.basefilectx) and
> -        ctx.changectx().rev() != ctx.linkrev()):
> -        return _siblings([ctx._repo[ctx.linkrev()]], hide)
> +        ctx.changectx().rev() != ctx.introrev()):
> +        return _siblings([ctx._repo[ctx.introrev()]], hide)
>      return _siblings(ctx.parents(), hide)
>
>  def children(ctx, hide=None):
> diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t
> --- a/tests/test-obsolete.t
> +++ b/tests/test-obsolete.t
> @@ -753,3 +753,42 @@ Test that removing a local tag does not
>    $ hg tags
>    visible                            0:193e9254ce7e
>    tip                                0:193e9254ce7e
> +
> +#if serve
> +
> +Test issue 4506
> +
> +  $ cd ..
> +  $ hg init repo-issue4506
> +  $ cd repo-issue4506
> +  $ echo "0" > foo
> +  $ hg add foo
> +  $ hg ci -m "0"
> +
> +  $ hg up null
> +  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
> +  $ echo "1" > bar
> +  $ hg add bar
> +  $ hg ci -m "1"
> +  created new head
> +  $ hg up 0
> +  1 files updated, 0 files merged, 1 files removed, 0 files unresolved
> +  $ hg graft 1
> +  grafting 1:9fe6fab2acbd "1" (tip)
> +
> +  $ hg debugobsolete `hg log -r1 -T'{node}'` `hg log -r2 -T'{node}'`
> +
> +  $ hg serve -n test -p $HGPORT -d --pid-file=hg.pid -A access.log -E errors.log
> +  $ cat hg.pid >> $DAEMON_PIDS
> +
> +  $ "$TESTDIR/get-with-headers.py" --headeronly localhost:$HGPORT 'rev/1'
> +  404 Not Found
> +  [1]
> +  $ "$TESTDIR/get-with-headers.py" --headeronly localhost:$HGPORT 'file/tip/bar'
> +  200 Script output follows
> +  $ "$TESTDIR/get-with-headers.py" --headeronly localhost:$HGPORT 'annotate/tip/bar'
> +  200 Script output follows
> +
> +  $ "$TESTDIR/killdaemons.py" $DAEMON_PIDS
> +
> +#endif
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Feb. 20, 2015, 4:37 p.m.
On 02/19/2015 01:41 PM, Anton Shestakov wrote:
> # HG changeset patch
> # User Anton Shestakov <engored@ya.ru>
> # Date 1424345526 -28800
> #      Thu Feb 19 19:32:06 2015 +0800
> # Branch stable
> # Node ID c5c1ffca678395337f8ef415423f990a9397d4ef
> # Parent  942a5a34b2d0611ab284380fbe45b9bb1897af98
> hgweb: use introrev() for finding parents (issue4506)

Overall direction is good, But I've a couple of feedback on the code.

> diff --git a/mercurial/hgweb/webutil.py b/mercurial/hgweb/webutil.py
> --- a/mercurial/hgweb/webutil.py
> +++ b/mercurial/hgweb/webutil.py
> @@ -139,8 +139,8 @@ def _siblings(siblings=[], hiderev=None)
>
>   def parents(ctx, hide=None):
>       if (isinstance(ctx, context.basefilectx) and
> -        ctx.changectx().rev() != ctx.linkrev()):
> -        return _siblings([ctx._repo[ctx.linkrev()]], hide)
> +        ctx.changectx().rev() != ctx.introrev()):
> +        return _siblings([ctx._repo[ctx.introrev()]], hide)

ctx.introrev() can lead to some actual computation (in opposition to 
ctx.linkrev()). So you should call it only once and store the result 
into a temporary variable.

(This is the only "blocking" feedback)


>       return _siblings(ctx.parents(), hide)
>
>   def children(ctx, hide=None):
> diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t
> --- a/tests/test-obsolete.t
> +++ b/tests/test-obsolete.t
> @@ -753,3 +753,42 @@ Test that removing a local tag does not
>     $ hg tags
>     visible                            0:193e9254ce7e
>     tip                                0:193e9254ce7e
> +
> +#if serve
> +
> +Test issue 4506

Small nits, we can probably use a bit more verbose title (the reference 
to the issue is good, but some actual description of the text is better).

> +  $ cd ..
> +  $ hg init repo-issue4506
> +  $ cd repo-issue4506
> +  $ echo "0" > foo
> +  $ hg add foo
> +  $ hg ci -m "0"

Other optional nits: Using bare number as commit name is a good way to 
get confused with revision number, so I advise for a something a bit 
more version (eg: "content-0")
Pierre-Yves David - Feb. 20, 2015, 4:42 p.m.
On 02/20/2015 04:37 PM, Pierre-Yves David wrote:
> On 02/19/2015 01:41 PM, Anton Shestakov wrote:
>> # HG changeset patch
>> # User Anton Shestakov <engored@ya.ru>
>> # Date 1424345526 -28800
>> #      Thu Feb 19 19:32:06 2015 +0800
>> # Branch stable
>> # Node ID c5c1ffca678395337f8ef415423f990a9397d4ef
>> # Parent  942a5a34b2d0611ab284380fbe45b9bb1897af98
>> hgweb: use introrev() for finding parents (issue4506)
>
> Overall direction is good, But I've a couple of feedback on the code.

Eventually got bored of reading email when my train was in the middle of 
burgundy. I fixed the blocking point locally and got the result pushed 
to the clowncopter.

Thanks !

Patch

diff --git a/mercurial/hgweb/webutil.py b/mercurial/hgweb/webutil.py
--- a/mercurial/hgweb/webutil.py
+++ b/mercurial/hgweb/webutil.py
@@ -139,8 +139,8 @@  def _siblings(siblings=[], hiderev=None)
 
 def parents(ctx, hide=None):
     if (isinstance(ctx, context.basefilectx) and
-        ctx.changectx().rev() != ctx.linkrev()):
-        return _siblings([ctx._repo[ctx.linkrev()]], hide)
+        ctx.changectx().rev() != ctx.introrev()):
+        return _siblings([ctx._repo[ctx.introrev()]], hide)
     return _siblings(ctx.parents(), hide)
 
 def children(ctx, hide=None):
diff --git a/tests/test-obsolete.t b/tests/test-obsolete.t
--- a/tests/test-obsolete.t
+++ b/tests/test-obsolete.t
@@ -753,3 +753,42 @@  Test that removing a local tag does not 
   $ hg tags
   visible                            0:193e9254ce7e
   tip                                0:193e9254ce7e
+
+#if serve
+
+Test issue 4506
+
+  $ cd ..
+  $ hg init repo-issue4506
+  $ cd repo-issue4506
+  $ echo "0" > foo
+  $ hg add foo
+  $ hg ci -m "0"
+
+  $ hg up null
+  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  $ echo "1" > bar
+  $ hg add bar
+  $ hg ci -m "1"
+  created new head
+  $ hg up 0
+  1 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  $ hg graft 1
+  grafting 1:9fe6fab2acbd "1" (tip)
+
+  $ hg debugobsolete `hg log -r1 -T'{node}'` `hg log -r2 -T'{node}'`
+
+  $ hg serve -n test -p $HGPORT -d --pid-file=hg.pid -A access.log -E errors.log
+  $ cat hg.pid >> $DAEMON_PIDS
+
+  $ "$TESTDIR/get-with-headers.py" --headeronly localhost:$HGPORT 'rev/1'
+  404 Not Found
+  [1]
+  $ "$TESTDIR/get-with-headers.py" --headeronly localhost:$HGPORT 'file/tip/bar'
+  200 Script output follows
+  $ "$TESTDIR/get-with-headers.py" --headeronly localhost:$HGPORT 'annotate/tip/bar'
+  200 Script output follows
+
+  $ "$TESTDIR/killdaemons.py" $DAEMON_PIDS
+
+#endif