Patchwork [stable,treemanifest] changegroup: fix treemanifest exchange code (issue5061)

login
register
mail settings
Submitter Augie Fackler
Date Jan. 27, 2016, 3:24 p.m.
Message ID <7f7e25855eac3dafb388.1453908288@augie-macbookair2.roam.corp.google.com>
Download mbox | patch
Permalink /patch/12895/
State Accepted
Commit ca8d2b73155d30d7402391cb48ab6727cd33f9e7
Headers show

Comments

Augie Fackler - Jan. 27, 2016, 3:24 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1453908265 18000
#      Wed Jan 27 10:24:25 2016 -0500
# Branch stable
# Node ID 7f7e25855eac3dafb388d63fb0b7f566334ff732
# Parent  4511e8dac4c798e5fed91e629aa9802b01c2b6c3
changegroup: fix treemanifest exchange code (issue5061)

There were two mistakes: one was accidental reuse of the fclnode
variable from the loop gathering file nodes, and the other (masked by
that bug) was not correctly handling deleted directories. Both cases
are now fixed and the test passes.
Martin von Zweigbergk - Jan. 27, 2016, 5:35 p.m.
On Wed, Jan 27, 2016 at 7:25 AM Augie Fackler <raf@durin42.com> wrote:

> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1453908265 18000
> #      Wed Jan 27 10:24:25 2016 -0500
> # Branch stable
> # Node ID 7f7e25855eac3dafb388d63fb0b7f566334ff732
> # Parent  4511e8dac4c798e5fed91e629aa9802b01c2b6c3
> changegroup: fix treemanifest exchange code (issue5061)
>
> There were two mistakes: one was accidental reuse of the fclnode
> variable from the loop gathering file nodes, and the other (masked by
> that bug) was not correctly handling deleted directories. Both cases
> are now fixed and the test passes.
>

Nice, that takes "hg clone --pull" on a test repo I had a little further.
It still fails a little later, but this seems better than before, thanks.


>
> diff --git a/mercurial/changegroup.py b/mercurial/changegroup.py
> --- a/mercurial/changegroup.py
> +++ b/mercurial/changegroup.py
> @@ -778,12 +778,15 @@ class cg1packer(object):
>                  if 'treemanifest' in repo.requirements:
>                      submfs = {'/': mdata}
>                      for dn, bn in _moddirs(mfchangedfiles[x]):
> -                        submf = submfs[dn]
> -                        submf = submf._dirs[bn]
> +                        try:
> +                            submf = submfs[dn]
> +                            submf = submf._dirs[bn]
> +                        except KeyError:
> +                            continue # deleted directory, so nothing to
> send
>                          submfs[submf.dir()] = submf
>                          tmfclnodes = tmfnodes.setdefault(submf.dir(), {})
> -                        tmfclnodes.setdefault(submf._node, clnode)
> -                        if clrevorder[clnode] < clrevorder[fclnode]:
> +                        tmfclnode = tmfclnodes.setdefault(submf._node,
> clnode)
> +                        if clrevorder[clnode] < clrevorder[tmfclnode]:
>                              tmfclnodes[n] = clnode
>                  return clnode
>
> diff --git a/tests/test-treemanifest.t b/tests/test-treemanifest.t
> --- a/tests/test-treemanifest.t
> +++ b/tests/test-treemanifest.t
> @@ -332,6 +332,21 @@ Pushing from treemanifest repo to an emp
>    $ grep treemanifest empty-repo/.hg/requires
>    treemanifest
>
> +Pushing to an empty repo makes that a treemanifest repo
>

This seems like it's taken from the test case I recently added, but it
doesn't seem accurate for your test. What is really the point of this test
case? I think it's pretty much the same thing as the test case further down
using "deepclone", but it just happens to hit some other conditions in the
lookup function. Also, you could probably switch to "hg clone --pull"
instead if you prefer. Either way, can I ask for at least a new
description? Either as a resend or just a text for me to paste.


> +
> +  $ hg --config experimental.treemanifest=1 init clone
> +  $ grep treemanifest clone/.hg/requires
> +  treemanifest
> +  $ hg push -R repo clone
> +  pushing to clone
> +  searching for changes
> +  adding changesets
> +  adding manifests
> +  adding file changes
> +  added 11 changesets with 15 changes to 10 files (+3 heads)
> +  $ grep treemanifest clone/.hg/requires
> +  treemanifest
> +
>  Create deeper repo with tree manifests.
>
>    $ hg --config experimental.treemanifest=True init deeprepo
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
>
Augie Fackler - Jan. 27, 2016, 9:49 p.m.
> On Jan 27, 2016, at 12:35, Martin von Zweigbergk <martinvonz@google.com> wrote:
> 
> 
> 
> On Wed, Jan 27, 2016 at 7:25 AM Augie Fackler <raf@durin42.com <mailto:raf@durin42.com>> wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com <mailto:augie@google.com>>
> # Date 1453908265 18000
> #      Wed Jan 27 10:24:25 2016 -0500
> # Branch stable
> # Node ID 7f7e25855eac3dafb388d63fb0b7f566334ff732
> # Parent  4511e8dac4c798e5fed91e629aa9802b01c2b6c3
> changegroup: fix treemanifest exchange code (issue5061)
> 
> There were two mistakes: one was accidental reuse of the fclnode
> variable from the loop gathering file nodes, and the other (masked by
> that bug) was not correctly handling deleted directories. Both cases
> are now fixed and the test passes.
> 
> Nice, that takes "hg clone --pull" on a test repo I had a little further. It still fails a little later, but this seems better than before, thanks.
>  
> 
> diff --git a/mercurial/changegroup.py b/mercurial/changegroup.py
> --- a/mercurial/changegroup.py
> +++ b/mercurial/changegroup.py
> @@ -778,12 +778,15 @@ class cg1packer(object):
>                  if 'treemanifest' in repo.requirements:
>                      submfs = {'/': mdata}
>                      for dn, bn in _moddirs(mfchangedfiles[x]):
> -                        submf = submfs[dn]
> -                        submf = submf._dirs[bn]
> +                        try:
> +                            submf = submfs[dn]
> +                            submf = submf._dirs[bn]
> +                        except KeyError:
> +                            continue # deleted directory, so nothing to send
>                          submfs[submf.dir()] = submf
>                          tmfclnodes = tmfnodes.setdefault(submf.dir(), {})
> -                        tmfclnodes.setdefault(submf._node, clnode)
> -                        if clrevorder[clnode] < clrevorder[fclnode]:
> +                        tmfclnode = tmfclnodes.setdefault(submf._node, clnode)
> +                        if clrevorder[clnode] < clrevorder[tmfclnode]:
>                              tmfclnodes[n] = clnode
>                  return clnode
> 
> diff --git a/tests/test-treemanifest.t b/tests/test-treemanifest.t
> --- a/tests/test-treemanifest.t
> +++ b/tests/test-treemanifest.t
> @@ -332,6 +332,21 @@ Pushing from treemanifest repo to an emp
>    $ grep treemanifest empty-repo/.hg/requires
>    treemanifest
> 
> +Pushing to an empty repo makes that a treemanifest repo
> 
> This seems like it's taken from the test case I recently added, but it doesn't seem accurate for your test. What is really the point of this test case? I think it's pretty much the same thing as the test case further down using "deepclone", but it just happens to hit some other conditions in the lookup function. Also, you could probably switch to "hg clone --pull" instead if you prefer. Either way, can I ask for at least a new description? Either as a resend or just a text for me to paste.

Maybe make this "Pushing to an empty repo works."? I'm not really sure how much more detail I can work into it, since that's really all it's doing.

>  
> +
> +  $ hg --config experimental.treemanifest=1 init clone
> +  $ grep treemanifest clone/.hg/requires
> +  treemanifest
> +  $ hg push -R repo clone
> +  pushing to clone
> +  searching for changes
> +  adding changesets
> +  adding manifests
> +  adding file changes
> +  added 11 changesets with 15 changes to 10 files (+3 heads)
> +  $ grep treemanifest clone/.hg/requires
> +  treemanifest
> +
>  Create deeper repo with tree manifests.
> 
>    $ hg --config experimental.treemanifest=True init deeprepo
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com <mailto:Mercurial-devel@selenic.com>
> https://selenic.com/mailman/listinfo/mercurial-devel <https://selenic.com/mailman/listinfo/mercurial-devel>
Martin von Zweigbergk - Jan. 27, 2016, 10:01 p.m.
On Wed, Jan 27, 2016 at 1:49 PM, Augie Fackler <raf@durin42.com> wrote:
>
> On Jan 27, 2016, at 12:35, Martin von Zweigbergk <martinvonz@google.com>
> wrote:
>
>
>
> On Wed, Jan 27, 2016 at 7:25 AM Augie Fackler <raf@durin42.com> wrote:
>>
>> +Pushing to an empty repo makes that a treemanifest repo
>
>
> This seems like it's taken from the test case I recently added, but it
> doesn't seem accurate for your test. What is really the point of this test
> case? I think it's pretty much the same thing as the test case further down
> using "deepclone", but it just happens to hit some other conditions in the
> lookup function. Also, you could probably switch to "hg clone --pull"
> instead if you prefer. Either way, can I ask for at least a new description?
> Either as a resend or just a text for me to paste.
>
>
> Maybe make this "Pushing to an empty repo works."? I'm not really sure how
> much more detail I can work into it, since that's really all it's doing.

Sure, that's at least not incorrect :-) Pushed to the clowncopter, thanks.

Patch

diff --git a/mercurial/changegroup.py b/mercurial/changegroup.py
--- a/mercurial/changegroup.py
+++ b/mercurial/changegroup.py
@@ -778,12 +778,15 @@  class cg1packer(object):
                 if 'treemanifest' in repo.requirements:
                     submfs = {'/': mdata}
                     for dn, bn in _moddirs(mfchangedfiles[x]):
-                        submf = submfs[dn]
-                        submf = submf._dirs[bn]
+                        try:
+                            submf = submfs[dn]
+                            submf = submf._dirs[bn]
+                        except KeyError:
+                            continue # deleted directory, so nothing to send
                         submfs[submf.dir()] = submf
                         tmfclnodes = tmfnodes.setdefault(submf.dir(), {})
-                        tmfclnodes.setdefault(submf._node, clnode)
-                        if clrevorder[clnode] < clrevorder[fclnode]:
+                        tmfclnode = tmfclnodes.setdefault(submf._node, clnode)
+                        if clrevorder[clnode] < clrevorder[tmfclnode]:
                             tmfclnodes[n] = clnode
                 return clnode
 
diff --git a/tests/test-treemanifest.t b/tests/test-treemanifest.t
--- a/tests/test-treemanifest.t
+++ b/tests/test-treemanifest.t
@@ -332,6 +332,21 @@  Pushing from treemanifest repo to an emp
   $ grep treemanifest empty-repo/.hg/requires
   treemanifest
 
+Pushing to an empty repo makes that a treemanifest repo
+
+  $ hg --config experimental.treemanifest=1 init clone
+  $ grep treemanifest clone/.hg/requires
+  treemanifest
+  $ hg push -R repo clone
+  pushing to clone
+  searching for changes
+  adding changesets
+  adding manifests
+  adding file changes
+  added 11 changesets with 15 changes to 10 files (+3 heads)
+  $ grep treemanifest clone/.hg/requires
+  treemanifest
+
 Create deeper repo with tree manifests.
 
   $ hg --config experimental.treemanifest=True init deeprepo