Patchwork [2,of,2] copies: clean up _related logic

login
register
mail settings
Submitter Gábor Stefanik
Date April 4, 2018, 1:35 p.m.
Message ID <084ee003f2f3cb4d5112.1522848934@GSTEFANIK.NavnGo.local>
Download mbox | patch
Permalink /patch/30285/
State New
Headers show

Comments

Gábor Stefanik - April 4, 2018, 1:35 p.m.
# HG changeset patch
# User Gábor Stefanik <gabor.stefanik@nng.com>
# Date 1522848882 -7200
#      Wed Apr 04 15:34:42 2018 +0200
# Node ID 084ee003f2f3cb4d51129c4f1bb63e1ff4db14d0
# Parent  d72ca973100a1f1a4451a7d1efdc3e43ebc2912e
copies: clean up _related logic

The limit parameter was never actually used, since the only way the 4th case
could be reached was if f1r and f2r converged. The new code makes this clear,
and additionally reduces the conditional block to just 3 cases.

________________________________
 This message, including its attachments, is confidential and the property of NNG Llc. For more information please read NNG's email policy here:
https://www.nng.com/email-policy/
By responding to this email you accept the email policy.
Yuya Nishihara - April 5, 2018, 12:57 p.m.
On Wed, 04 Apr 2018 15:35:34 +0200, Gábor Stefanik wrote:
> # HG changeset patch
> # User Gábor Stefanik <gabor.stefanik@nng.com>
> # Date 1522848882 -7200
> #      Wed Apr 04 15:34:42 2018 +0200
> # Node ID 084ee003f2f3cb4d51129c4f1bb63e1ff4db14d0
> # Parent  d72ca973100a1f1a4451a7d1efdc3e43ebc2912e
> copies: clean up _related logic
> 
> The limit parameter was never actually used, since the only way the 4th case
> could be reached was if f1r and f2r converged. The new code makes this clear,
> and additionally reduces the conditional block to just 3 cases.

Yeah. I suspect the limit should be tested first, but doing that breaks
some tests. So, perhaps we have to handle the case of f.linkrev() < anc.rev()
anyway.

> diff -r d72ca973100a -r 084ee003f2f3 mercurial/copies.py
> --- a/mercurial/copies.py       Wed Apr 04 15:28:09 2018 +0200
> +++ b/mercurial/copies.py       Wed Apr 04 15:34:42 2018 +0200
> @@ -737,7 +737,7 @@
>      except StopIteration:
>          raise
> 
> -def _related(f1, f2, limit):
> +def _related(f1, f2):

There's one more caller of _related().

> @@ -764,10 +764,8 @@
>                  f1, g1 = _loose_next(g1)
>              elif f2r > f1r:
>                  f1, g1 = _loose_next(g1)
> -            elif f1 == f2:
> -                return f1 # a match
> -            elif f1r == f2r or f1r < limit or f2r < limit:
> -                return False # copy no longer relevant
> +            else: # f1 and f2 point to files in the same linkrev
> +                return f1 == f2 # true if they point to the same file
Gábor Stefanik - April 5, 2018, 3:47 p.m.
> -----Original Message-----

> From: Yuya Nishihara [mailto:youjah@gmail.com] On Behalf Of Yuya

> Nishihara

> Sent: Thursday, April 5, 2018 2:57 PM

> To: Gábor STEFANIK <Gabor.STEFANIK@nng.com>

> Cc: mercurial-devel@mercurial-scm.org

> Subject: Re: [PATCH 2 of 2] copies: clean up _related logic

>

> On Wed, 04 Apr 2018 15:35:34 +0200, Gábor Stefanik wrote:

> > # HG changeset patch

> > # User Gábor Stefanik <gabor.stefanik@nng.com> # Date 1522848882 -7200

> > #      Wed Apr 04 15:34:42 2018 +0200

> > # Node ID 084ee003f2f3cb4d51129c4f1bb63e1ff4db14d0

> > # Parent  d72ca973100a1f1a4451a7d1efdc3e43ebc2912e

> > copies: clean up _related logic

> >

> > The limit parameter was never actually used, since the only way the

> > 4th case could be reached was if f1r and f2r converged. The new code

> > makes this clear, and additionally reduces the conditional block to just 3

> cases.

>

> Yeah. I suspect the limit should be tested first, but doing that breaks some

> tests. So, perhaps we have to handle the case of f.linkrev() < anc.rev()

> anyway.


We do need to care for relatedness behind anc.rev(), since anc may well be a revision that doesn't modify f at all. In that case, the file revision contained in anc will have a linkrev before anc (since linkrev points to the changelog revision in which the file revision in question was introduced, not the last one where it's present). The limit logic really doesn't make sense here.

>

> > diff -r d72ca973100a -r 084ee003f2f3 mercurial/copies.py

> > --- a/mercurial/copies.py       Wed Apr 04 15:28:09 2018 +0200

> > +++ b/mercurial/copies.py       Wed Apr 04 15:34:42 2018 +0200

> > @@ -737,7 +737,7 @@

> >      except StopIteration:

> >          raise

> >

> > -def _related(f1, f2, limit):

> > +def _related(f1, f2):

>

> There's one more caller of _related().


Right, in heuristicscopytracing.

>

> > @@ -764,10 +764,8 @@

> >                  f1, g1 = _loose_next(g1)

> >              elif f2r > f1r:

> >                  f1, g1 = _loose_next(g1)

> > -            elif f1 == f2:

> > -                return f1 # a match

> > -            elif f1r == f2r or f1r < limit or f2r < limit:

> > -                return False # copy no longer relevant

> > +            else: # f1 and f2 point to files in the same linkrev

> > +                return f1 == f2 # true if they point to the same file

________________________________
 This message, including its attachments, is confidential and the property of NNG Llc. For more information please read NNG's email policy here:
https://www.nng.com/email-policy/
By responding to this email you accept the email policy.

Patch

diff -r d72ca973100a -r 084ee003f2f3 mercurial/copies.py

--- a/mercurial/copies.py       Wed Apr 04 15:28:09 2018 +0200

+++ b/mercurial/copies.py       Wed Apr 04 15:34:42 2018 +0200

@@ -737,7 +737,7 @@ 

     except StopIteration:
         raise

-def _related(f1, f2, limit):

+def _related(f1, f2):

     """return True if f1 and f2 filectx have a common ancestor

     Walk back to common ancestor to see if the two files originate
@@ -764,10 +764,8 @@ 

                 f1, g1 = _loose_next(g1)
             elif f2r > f1r:
                 f1, g1 = _loose_next(g1)
-            elif f1 == f2:

-                return f1 # a match

-            elif f1r == f2r or f1r < limit or f2r < limit:

-                return False # copy no longer relevant

+            else: # f1 and f2 point to files in the same linkrev

+                return f1 == f2 # true if they point to the same file

     except StopIteration:
         return False

@@ -835,7 +833,7 @@ 

         c2 = getdstfctx(of, mdst[of])
         # c2 might be a plain new file on added on destination side that is
         # unrelated to the droids we are looking for.
-        cr = _related(oc, c2, tca.rev())

+        cr = _related(oc, c2)

         if cr and (of == f or of == c2.path()): # non-divergent
             if backwards:
                 data['copy'][of] = f