Patchwork [1,of,3,STABLE] manifest: fix a bug where working copy file 'add' mark was buggy

# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1417042456 28800
#      Wed Nov 26 14:54:16 2014 -0800
# Branch stable
# Node ID 82ede43751eae4b870b598326d30105960ce0e10
# Parent  7f8d27e1f862819b6f13e368c949aa824de5ae45
manifest: fix a bug where working copy file 'add' mark was buggy

Because the same dictionary was used to (1) get node from parent and (2) store
annotated version, we could end up with buggy values. For example with a chain
of rename:

  $ hg mv b c
  $ hg mv a b

The value from 'b' would be updated as "<old-a>a", then the value of c would be
updated as "<old-b>a'. With the current dictionary sharing this ends up with:

    '<new-c>' == '<old-a>aa'

This value is double-wrong as we should use '<old-b>' and a single 'a'.

We now use a read only value for look up. The 'test-rename.t' test is impacted
because such chainly added file is suddenly detected as such.

On Wed Nov 26 2014 at 3:31:27 PM Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1417042456 28800
> #      Wed Nov 26 14:54:16 2014 -0800
> # Branch stable
> # Node ID 82ede43751eae4b870b598326d30105960ce0e10
> # Parent  7f8d27e1f862819b6f13e368c949aa824de5ae45
> manifest: fix a bug where working copy file 'add' mark was buggy
>
> Because the same dictionary was used to (1) get node from parent and (2)
> store
> annotated version, we could end up with buggy values. For example with a
> chain
> of rename:
>
>   $ hg mv b c
>   $ hg mv a b
>
> The value from 'b' would be updated as "<old-a>a", then the value of c
> would be
> updated as "<old-b>a'. With the current dictionary sharing this ends up
> with:
>
>     '<new-c>' == '<old-a>aa'
>
> This value is double-wrong as we should use '<old-b>' and a single 'a'.
>
> We now use a read only value for look up. The 'test-rename.t' test is
> impacted
> because such chainly added file is suddenly detected as such.
>
> diff --git a/mercurial/context.py b/mercurial/context.py
> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -1061,19 +1061,20 @@ class committablectx(basectx):
>
>      @propertycache
>      def _manifest(self):
>          """generate a manifest corresponding to the values in
> self._status"""
>
> -        man = self._parents[0].manifest().copy()
> +        man1 = self._parents[0].manifest()
> +        man = man1.copy()
>          if len(self._parents) > 1:
>              man2 = self.p2().manifest()
>              def getman(f):
> -                if f in man:
> -                    return man
> +                if f in man1:
> +                    return man1
>                  return man2
>          else:
> -            getman = lambda f: man
> +            getman = lambda f: man1
>
>          copied = self._repo.dirstate.copies()
>          ff = self._flagfunc
>          for i, l in (("a", self._status.added), ("m",
> self._status.modified)):
>              for f in l:
> diff --git a/tests/test-rename.t b/tests/test-rename.t
> --- a/tests/test-rename.t
> +++ b/tests/test-rename.t
> @@ -582,11 +582,12 @@ overwriting with renames (issue1959)
>    rename to d1/a
>    diff --git a/d1/a b/d1/c
>    copy from d1/a
>    copy to d1/c
>    $ hg update -C
> -  2 files updated, 0 files merged, 1 files removed, 0 files unresolved
> +  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  $ rm d1/c # update 'forget' file because it was just added.
>

I'm not sure what you mean here. Is the "update" just a left-over? Or does
it mean that we should update the code somehow? Or should I read it
something like "The file's update action is 'forget'"? Either way, I think
it should be clarified.

On 11/26/2014 03:53 PM, Martin von Zweigbergk wrote:
> I'm not sure what you mean here. Is the "update" just a left-over? Or
> does it mean that we should update the code somehow? Or should I read it
> something like "The file's update action is 'forget'"? Either way, I
> think it should be clarified.

The file update action is "forget". I'll clarify this.