Patchwork adjustlinkrev: search ancestors from oldest to newest

login
register
mail settings
Submitter Martin von Zweigbergk
Date Jan. 15, 2015, 12:02 a.m.
Message ID <5b82ace74f36afe0ec6d.1421280156@martinvonz.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/7461/
State Rejected
Headers show

Comments

Martin von Zweigbergk - Jan. 15, 2015, 12:02 a.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1421273570 28800
#      Wed Jan 14 14:12:50 2015 -0800
# Node ID 5b82ace74f36afe0ec6d44d17489b844724cd708
# Parent  b2358bc1407c19007b0e7852262f61d5fe7a8f83
adjustlinkrev: search ancestors from oldest to newest

When searching for the oldest ancestor that introduced a file's node
id, we search in the order we get from changelog.ancestors(), which is
in descending order (newest to oldest). For file modifications, it is
safe to search backwards like this, but for when the file has been
temporarily removed, the same nodeid can appear twice. Make sure we
search from oldest to newest so we get the right result in this case
too.
Pierre-Yves David - Jan. 15, 2015, 1:41 a.m.
On 01/14/2015 04:02 PM, Martin von Zweigbergk wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1421273570 28800
> #      Wed Jan 14 14:12:50 2015 -0800
> # Node ID 5b82ace74f36afe0ec6d44d17489b844724cd708
> # Parent  b2358bc1407c19007b0e7852262f61d5fe7a8f83
> adjustlinkrev: search ancestors from oldest to newest
>
> When searching for the oldest ancestor that introduced a file's node
> id, we search in the order we get from changelog.ancestors(), which is
> in descending order (newest to oldest). For file modifications, it is
> safe to search backwards like this, but for when the file has been
> temporarily removed, the same nodeid can appear twice. Make sure we
> search from oldest to newest so we get the right result in this case
> too.

The recent linkrev change aims at getting the situation better. The code 
still struggle with multiple introduction of the same content. Take the 
oldest over the newest is kind of arbitrary and does not significantly 
improves the situation.

What we needs now to real tracking of all file revision introduction 
somewhere on disk, such adjustement is probably not a win.

(this is a gentle rejection of this patch unless you can make a better 
case for it)

> diff -r b2358bc1407c -r 5b82ace74f36 mercurial/context.py
> --- a/mercurial/context.py	Tue Jan 13 15:08:55 2015 -0500
> +++ b/mercurial/context.py	Wed Jan 14 14:12:50 2015 -0800
> @@ -23,7 +23,7 @@
>   _newnode = '!' * 21
>
>   def _adjustlinkrev(repo, path, filelog, fnode, srcrev, inclusive=False):
> -    """return the first ancestor of <srcrev> introducting <fnode>
> +    """return the oldest ancestor of <srcrev> introducting <fnode>
>
>       If the linkrev of the file revision does not point to an ancestor of
>       srcrev, we'll walk down the ancestors until we find one introducing this
> @@ -42,7 +42,8 @@
>       fr = filelog.rev(fnode)
>       lkr = filelog.linkrev(fr)
>       # check if this linkrev is an ancestor of srcrev
> -    anc = cl.ancestors([srcrev], lkr, inclusive=inclusive)
> +    anc = list(cl.ancestors([srcrev], lkr, inclusive=inclusive))
> +    anc.reverse()

Turning a smart object into a lis: meh.

>       if lkr not in anc:

And making a membership testing in that list: nope, nope, nope.
Martin von Zweigbergk - Jan. 15, 2015, 5:11 a.m.
On Wed Jan 14 2015 at 5:41:20 PM Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 01/14/2015 04:02 PM, Martin von Zweigbergk wrote:
> > # HG changeset patch
> > # User Martin von Zweigbergk <martinvonz@google.com>
> > # Date 1421273570 28800
> > #      Wed Jan 14 14:12:50 2015 -0800
> > # Node ID 5b82ace74f36afe0ec6d44d17489b844724cd708
> > # Parent  b2358bc1407c19007b0e7852262f61d5fe7a8f83
> > adjustlinkrev: search ancestors from oldest to newest
> >
> > When searching for the oldest ancestor that introduced a file's node
> > id, we search in the order we get from changelog.ancestors(), which is
> > in descending order (newest to oldest). For file modifications, it is
> > safe to search backwards like this, but for when the file has been
> > temporarily removed, the same nodeid can appear twice. Make sure we
> > search from oldest to newest so we get the right result in this case
> > too.
>
> The recent linkrev change aims at getting the situation better. The code
> still struggle with multiple introduction of the same content. Take the
> oldest over the newest is kind of arbitrary and does not significantly
> improves the situation.
>
> What we needs now to real tracking of all file revision introduction
> somewhere on disk, such adjustement is probably not a win.
>
> (this is a gentle rejection of this patch unless you can make a better
> case for it)
>

That makes sense. I'm fine with dropping the patch.


> > diff -r b2358bc1407c -r 5b82ace74f36 mercurial/context.py
> > --- a/mercurial/context.py    Tue Jan 13 15:08:55 2015 -0500
> > +++ b/mercurial/context.py    Wed Jan 14 14:12:50 2015 -0800
> > @@ -23,7 +23,7 @@
> >   _newnode = '!' * 21
> >
> >   def _adjustlinkrev(repo, path, filelog, fnode, srcrev,
> inclusive=False):
> > -    """return the first ancestor of <srcrev> introducting <fnode>
> > +    """return the oldest ancestor of <srcrev> introducting <fnode>
> >
> >       If the linkrev of the file revision does not point to an ancestor
> of
> >       srcrev, we'll walk down the ancestors until we find one
> introducing this
> > @@ -42,7 +42,8 @@
> >       fr = filelog.rev(fnode)
> >       lkr = filelog.linkrev(fr)
> >       # check if this linkrev is an ancestor of srcrev
> > -    anc = cl.ancestors([srcrev], lkr, inclusive=inclusive)
> > +    anc = list(cl.ancestors([srcrev], lkr, inclusive=inclusive))
> > +    anc.reverse()
>
> Turning a smart object into a lis: meh.
>
> >       if lkr not in anc:
>
> And making a membership testing in that list: nope, nope, nope.
>

I agree. I viewed the revset as just an iterator, but it's clearly more
than that.


>
> --
> Pierre-Yves David
>

Patch

diff -r b2358bc1407c -r 5b82ace74f36 mercurial/context.py
--- a/mercurial/context.py	Tue Jan 13 15:08:55 2015 -0500
+++ b/mercurial/context.py	Wed Jan 14 14:12:50 2015 -0800
@@ -23,7 +23,7 @@ 
 _newnode = '!' * 21
 
 def _adjustlinkrev(repo, path, filelog, fnode, srcrev, inclusive=False):
-    """return the first ancestor of <srcrev> introducting <fnode>
+    """return the oldest ancestor of <srcrev> introducting <fnode>
 
     If the linkrev of the file revision does not point to an ancestor of
     srcrev, we'll walk down the ancestors until we find one introducing this
@@ -42,7 +42,8 @@ 
     fr = filelog.rev(fnode)
     lkr = filelog.linkrev(fr)
     # check if this linkrev is an ancestor of srcrev
-    anc = cl.ancestors([srcrev], lkr, inclusive=inclusive)
+    anc = list(cl.ancestors([srcrev], lkr, inclusive=inclusive))
+    anc.reverse()
     if lkr not in anc:
         for a in anc:
             ac = cl.read(a) # get changeset data (we avoid object creation).
diff -r b2358bc1407c -r 5b82ace74f36 tests/test-log.t
--- a/tests/test-log.t	Tue Jan 13 15:08:55 2015 -0500
+++ b/tests/test-log.t	Wed Jan 14 14:12:50 2015 -0800
@@ -1842,3 +1842,21 @@ 
   |  date:        Thu Jan 01 00:00:00 1970 +0000
   |  summary:     1
   |
+
+  $ hg up -q 3
+  $ hg ci --amend -m 'new message'
+  $ hg rm b
+  $ hg ci -m 'remove b'
+  $ hg revert -r 5 b
+  $ hg ci -m 'recover b'
+
+The same filerev now exists in hidden rev 3, as well as in visible rev 5 and 7,
+so 5 is the first visible and should be displayed.
+
+  $ hg log b
+  changeset:   5:71488aa303fd
+  parent:      0:f7b1eb17ad24
+  user:        test
+  date:        Thu Jan 01 00:00:00 1970 +0000
+  summary:     new message
+