Patchwork [v2,STABLE] amend: fix amending rename commit with obsolescence markers (issue4405)

login
register
mail settings
Submitter Ryan McElroy
Date Oct. 20, 2014, 4:53 p.m.
Message ID <d4c47f740c3e1e51789e.1413824022@devbig105.prn2.facebook.com>
Download mbox | patch
Permalink /patch/6431/
State Changes Requested
Headers show

Comments

Ryan McElroy - Oct. 20, 2014, 4:53 p.m.
# HG changeset patch
# User Ryan McElroy <rmcelroy@fb.com>
# Date 1413466506 25200
#      Thu Oct 16 06:35:06 2014 -0700
# Node ID d4c47f740c3e1e51789ee976de7dcaa6974e815a
# Parent  f10019d2ee0afa2cc629a5e5409803e14fb40697
amend: fix amending rename commit with obsolescence markers (issue4405)

This addresses the bug described in issue4405: when obsolescence markers are
enabled, amending a commit with a file move can lead to the copy information
being lost.
Augie Fackler - Oct. 22, 2014, 2:43 a.m.
On Mon, Oct 20, 2014 at 09:53:42AM -0700, Ryan McElroy wrote:
> # HG changeset patch
> # User Ryan McElroy <rmcelroy@fb.com>
> # Date 1413466506 25200
> #      Thu Oct 16 06:35:06 2014 -0700
> # Node ID d4c47f740c3e1e51789ee976de7dcaa6974e815a
> # Parent  f10019d2ee0afa2cc629a5e5409803e14fb40697
> amend: fix amending rename commit with obsolescence markers (issue4405)

marmoute told me to expect a v3 of this

>
> This addresses the bug described in issue4405: when obsolescence markers are
> enabled, amending a commit with a file move can lead to the copy information
> being lost.
>
> diff --git a/mercurial/copies.py b/mercurial/copies.py
> --- a/mercurial/copies.py
> +++ b/mercurial/copies.py
> @@ -19,7 +19,12 @@
>      return f[:s]
>
>  def _findlimit(repo, a, b):
> -    """Find the earliest revision that's an ancestor of a or b but not both,
> +    """
> +    Find the last revision that needs to be checked to ensure that a full
> +    transitive closure for file copies can be properly calculated.
> +    Generally, this means finding the earliest revision number that's an
> +    ancestor of a or b but not both, except when a or b is a direct descendent
> +    of the other, in which case we can return the minimum revnum of a and b.
>      None if no such revision exists.
>      """
>      # basic idea:
> @@ -73,7 +78,32 @@
>
>      if not hascommonancestor:
>          return None
> -    return limit
> +
> +    # Consider the following flow (see test-commit-amend.t under issue4405):
> +    # 1/ File 'a' committed
> +    # 2/ File renamed from 'a' to 'b' in a new commit
> +    # 3/ Commit messaged updated with amend
> +    # 4/ File renamed from 'b' to 'c' and commit (2) amended
> +    #
> +    # When obsolescence markers are enabled, this would create a graph like:
> +    # @  4 mv b c
> +    # |
> +    # | x  3 temporary amend commit for 60e352014901
> +    # | |
> +    # | x  2 change message
> +    # |/
> +    # | x  1 mv a b
> +    # |/
> +    # o  0 add a
> +    #
> +    # During the second amend (step 4), limit is calculated to be 3, which does
> +    # not look back far enough to reconstruct the full transitive closure of the
> +    # renames.
> +    #
> +    # In this case, a is rev 4 and b is rev 0; taking the minimum of these three
> +    # ensures that we always look back far enough when we have obsolescence
> +    # markers enabled.
> +    return min(limit, a, b)
>
>  def _chain(src, dst, a, b):
>      '''chain two sets of copies a->b'''
> diff --git a/tests/test-commit-amend.t b/tests/test-commit-amend.t
> --- a/tests/test-commit-amend.t
> +++ b/tests/test-commit-amend.t
> @@ -854,3 +854,38 @@
>    HG: changed foo
>    $ hg parents --template "{desc}\n"
>    editor should be invoked
> +
> +Check for issue4405
> +  $ cat > obs.py << EOF
> +  > import mercurial.obsolete
> +  > mercurial.obsolete._enabled = True
> +  > EOF
> +  $ cat >> $HGRCPATH <<EOF
> +  > [extensions]
> +  > EOF
> +  $ echo "obs=${TESTTMP}/obs.py" >> $HGRCPATH
> +  $ hg init repo
> +  $ cd repo
> +  $ echo a > a
> +  $ hg add a
> +  $ hg commit -m a
> +  $ hg mv a b
> +  $ hg commit -m 'mv a b'
> +  $ hg st -C --change .
> +  A b
> +    a
> +  R a
> +  $ hg commit --amend -m 'change commit message'
> +  $ hg st -C --change .
> +  A b
> +    a
> +  R a
> +  $ hg mv b c
> +  $ hg commit --amend -m 'mv b c'
> +  $ hg st -C --change .
> +  A c
> +    a
> +  R a
> +  $ rm ${TESTTMP}/obs.py
> +  $ rm $HGRCPATH
> +  $ cd ..
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/copies.py b/mercurial/copies.py
--- a/mercurial/copies.py
+++ b/mercurial/copies.py
@@ -19,7 +19,12 @@ 
     return f[:s]
 
 def _findlimit(repo, a, b):
-    """Find the earliest revision that's an ancestor of a or b but not both,
+    """
+    Find the last revision that needs to be checked to ensure that a full
+    transitive closure for file copies can be properly calculated.
+    Generally, this means finding the earliest revision number that's an
+    ancestor of a or b but not both, except when a or b is a direct descendent
+    of the other, in which case we can return the minimum revnum of a and b.
     None if no such revision exists.
     """
     # basic idea:
@@ -73,7 +78,32 @@ 
 
     if not hascommonancestor:
         return None
-    return limit
+
+    # Consider the following flow (see test-commit-amend.t under issue4405):
+    # 1/ File 'a' committed
+    # 2/ File renamed from 'a' to 'b' in a new commit
+    # 3/ Commit messaged updated with amend
+    # 4/ File renamed from 'b' to 'c' and commit (2) amended
+    #
+    # When obsolescence markers are enabled, this would create a graph like:
+    # @  4 mv b c
+    # |
+    # | x  3 temporary amend commit for 60e352014901
+    # | |
+    # | x  2 change message
+    # |/
+    # | x  1 mv a b
+    # |/
+    # o  0 add a
+    #
+    # During the second amend (step 4), limit is calculated to be 3, which does
+    # not look back far enough to reconstruct the full transitive closure of the
+    # renames.
+    #
+    # In this case, a is rev 4 and b is rev 0; taking the minimum of these three
+    # ensures that we always look back far enough when we have obsolescence
+    # markers enabled.
+    return min(limit, a, b)
 
 def _chain(src, dst, a, b):
     '''chain two sets of copies a->b'''
diff --git a/tests/test-commit-amend.t b/tests/test-commit-amend.t
--- a/tests/test-commit-amend.t
+++ b/tests/test-commit-amend.t
@@ -854,3 +854,38 @@ 
   HG: changed foo
   $ hg parents --template "{desc}\n"
   editor should be invoked
+
+Check for issue4405
+  $ cat > obs.py << EOF
+  > import mercurial.obsolete
+  > mercurial.obsolete._enabled = True
+  > EOF
+  $ cat >> $HGRCPATH <<EOF
+  > [extensions]
+  > EOF
+  $ echo "obs=${TESTTMP}/obs.py" >> $HGRCPATH
+  $ hg init repo
+  $ cd repo
+  $ echo a > a
+  $ hg add a
+  $ hg commit -m a
+  $ hg mv a b
+  $ hg commit -m 'mv a b'
+  $ hg st -C --change .
+  A b
+    a
+  R a
+  $ hg commit --amend -m 'change commit message'
+  $ hg st -C --change .
+  A b
+    a
+  R a
+  $ hg mv b c
+  $ hg commit --amend -m 'mv b c'
+  $ hg st -C --change .
+  A c
+    a
+  R a
+  $ rm ${TESTTMP}/obs.py
+  $ rm $HGRCPATH
+  $ cd ..