Patchwork changegroup: don't run changegroup hooks if nodes are gone

login
register
mail settings
Submitter Durham Goode
Date Oct. 7, 2013, 9:17 p.m.
Message ID <572bf59dd5950cefc2c0.1381180649@dev350.prn1.facebook.com>
Download mbox | patch
Permalink /patch/2737/
State Accepted
Commit fc3fbca350853591c4d78bb7a15059533afdfa6f
Headers show

Comments

Durham Goode - Oct. 7, 2013, 9:17 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1381171501 25200
#      Mon Oct 07 11:45:01 2013 -0700
# Node ID 572bf59dd5950cefc2c00a20fbfee9fdf65a8176
# Parent  b3de50b0c7aa464fdf73ece8c4e5ee8af59b4242
changegroup: don't run changegroup hooks if nodes are gone

The changegroup hook runs when the repo lock is released, but it's possible that
multiple transactions have happened during that single lock and therefore the
commits the hook was for may be gone. This is the case in the shelve extension
where it adds a commit and strips it in the same lock but different
transactions (which results in warning messages during unshelve on hgsubversion
repos).

A real fix would be to attach the hook to the transaction instead, but that
might have unknown consequences. Since we're this close to code-freeze, this fix
just prevents the hook from running if the commit disappeared.
Durham Goode - Oct. 9, 2013, 4:32 p.m.
On 10/7/13 2:17 PM, "Durham Goode" <durham@fb.com> wrote:

># HG changeset patch
># User Durham Goode <durham@fb.com>
># Date 1381171501 25200
>#      Mon Oct 07 11:45:01 2013 -0700
># Node ID 572bf59dd5950cefc2c00a20fbfee9fdf65a8176
># Parent  b3de50b0c7aa464fdf73ece8c4e5ee8af59b4242
>changegroup: don't run changegroup hooks if nodes are gone
>
>The changegroup hook runs when the repo lock is released, but it's
>possible that
>multiple transactions have happened during that single lock and therefore
>the
>commits the hook was for may be gone. This is the case in the shelve
>extension
>where it adds a commit and strips it in the same lock but different
>transactions (which results in warning messages during unshelve on
>hgsubversion
>repos).
>
>A real fix would be to attach the hook to the transaction instead, but
>that
>might have unknown consequences. Since we're this close to code-freeze,
>this fix
>just prevents the hook from running if the commit disappeared.

Ping. This patch is blocking us from deploying shelve at Facebook, so it'd
be nice to get it in so we can get some real user feedback/bugs before the
2.8 release.
Matt Mackall - Oct. 9, 2013, 7:11 p.m.
On Mon, 2013-10-07 at 14:17 -0700, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1381171501 25200
> #      Mon Oct 07 11:45:01 2013 -0700
> # Node ID 572bf59dd5950cefc2c00a20fbfee9fdf65a8176
> # Parent  b3de50b0c7aa464fdf73ece8c4e5ee8af59b4242
> changegroup: don't run changegroup hooks if nodes are gone

Queued for default, thanks.

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -2229,6 +2229,12 @@ 
                     # In other case we can safely update cache on disk.
                     branchmap.updatecache(self.filtered('served'))
                 def runhooks():
+                    # These hooks run when the lock releases, not when the
+                    # transaction closes. So it's possible for the changelog
+                    # to have changed since we last saw it.
+                    if clstart >= len(self):
+                        return
+
                     # forcefully update the on-disk branch cache
                     self.ui.debug("updating the branch cache\n")
                     self.hook("changegroup", node=hex(cl.node(clstart)),