Patchwork [2,of,3,STABLE] changegroup: call 'prechangegroup' hook before setting up write delay

login
register
mail settings
Submitter Pierre-Yves David
Date Nov. 6, 2015, 6:07 p.m.
Message ID <b720bb662a3e17cdfac3.1446833220@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/11306/
State Accepted
Headers show

Comments

Pierre-Yves David - Nov. 6, 2015, 6:07 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1446832749 18000
#      Fri Nov 06 12:59:09 2015 -0500
# Branch stable
# Node ID b720bb662a3e17cdfac3cc5c90fdd895673ec864
# Parent  03916de16fe8f511a48b860bf8b7956d79ea8ef9
# Available At http://hg.netv6.net/marmoute-wip/mercurial/
#              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r b720bb662a3e
changegroup: call 'prechangegroup' hook before setting up write delay

The 'prechangegroup' interfere with 'delayupdate' logic because it trigger the
one time call of 'changelog._writepending' (see issure4934). There is no reason
not to call that hook before setting up 'delayupdate' so we move the call a bit
earlier to avoid interference.
Durham Goode - Nov. 9, 2015, 7:13 p.m.
On 11/6/15 10:07 AM, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1446832749 18000
> #      Fri Nov 06 12:59:09 2015 -0500
> # Branch stable
> # Node ID b720bb662a3e17cdfac3cc5c90fdd895673ec864
> # Parent  03916de16fe8f511a48b860bf8b7956d79ea8ef9
> # Available At https://urldefense.proofpoint.com/v2/url?u=http-3A__hg.netv6.net_marmoute-2Dwip_mercurial_&d=CwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=rnYZCBGjsYVaHYznTww8TS18sR-RU_3WQT5S-Y9gSzU&s=Od6nMDVqvebNfgQ5PgZF3Ns5ryRRfpZIGbSJOM3sQxU&e=
> #              hg pull https://urldefense.proofpoint.com/v2/url?u=http-3A__hg.netv6.net_marmoute-2Dwip_mercurial_&d=CwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=rnYZCBGjsYVaHYznTww8TS18sR-RU_3WQT5S-Y9gSzU&s=Od6nMDVqvebNfgQ5PgZF3Ns5ryRRfpZIGbSJOM3sQxU&e=  -r b720bb662a3e
> changegroup: call 'prechangegroup' hook before setting up write delay
>
> The 'prechangegroup' interfere with 'delayupdate' logic because it trigger the
> one time call of 'changelog._writepending' (see issure4934). There is no reason
> not to call that hook before setting up 'delayupdate' so we move the call a bit
> earlier to avoid interference.
>
> diff --git a/mercurial/changegroup.py b/mercurial/changegroup.py
> --- a/mercurial/changegroup.py
> +++ b/mercurial/changegroup.py
> @@ -327,17 +327,17 @@ class cg1unpacker(object):
>               # carries source information. In this case we use the top
>               # level data. We overwrite the argument because we need to use
>               # the top level value (if they exist) in this function.
>               srctype = tr.hookargs.setdefault('source', srctype)
>               url = tr.hookargs.setdefault('url', url)
> +            repo.hook('prechangegroup', throw=True, **tr.hookargs)
>   
> -            # write changelog data to temp files so concurrent readers will not see
> -            # inconsistent view
> +            # write changelog data to temp files so concurrent readers will not
> +            # see inconsistent view
>               cl = repo.changelog
>               cl.delayupdate(tr)
>               oldheads = cl.heads()
> -            repo.hook('prechangegroup', throw=True, **tr.hookargs)
>   
I think relying on no hooks being called between delayupdate and the 
actual changelog writes is subtle, brittle, and not a good fix for this. 
If you want a better fix than what I had, I think we should break the 
writepending subscription logic out of delayupdate and put it after the 
changelog write.

Patch

diff --git a/mercurial/changegroup.py b/mercurial/changegroup.py
--- a/mercurial/changegroup.py
+++ b/mercurial/changegroup.py
@@ -327,17 +327,17 @@  class cg1unpacker(object):
             # carries source information. In this case we use the top
             # level data. We overwrite the argument because we need to use
             # the top level value (if they exist) in this function.
             srctype = tr.hookargs.setdefault('source', srctype)
             url = tr.hookargs.setdefault('url', url)
+            repo.hook('prechangegroup', throw=True, **tr.hookargs)
 
-            # write changelog data to temp files so concurrent readers will not see
-            # inconsistent view
+            # write changelog data to temp files so concurrent readers will not
+            # see inconsistent view
             cl = repo.changelog
             cl.delayupdate(tr)
             oldheads = cl.heads()
-            repo.hook('prechangegroup', throw=True, **tr.hookargs)
 
             trp = weakref.proxy(tr)
             # pull off the changeset group
             repo.ui.status(_("adding changesets\n"))
             clstart = len(cl)