Patchwork [3,of,5] repair: create transaction for bundle1 unbundling earlier

login
register
mail settings
Submitter via Mercurial-devel
Date June 18, 2017, 7:02 a.m.
Message ID <1a23893eb7f7ea55df88.1497769346@martinvonz.svl.corp.google.com>
Download mbox | patch
Permalink /patch/21476/
State Accepted
Headers show

Comments

via Mercurial-devel - June 18, 2017, 7:02 a.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1497593354 25200
#      Thu Jun 15 23:09:14 2017 -0700
# Node ID 1a23893eb7f7ea55df8835bd6208f8427fa36d13
# Parent  b6b0928d0128d2b424b40bf4c4d2a59acc20f9ac
repair: create transaction for bundle1 unbundling earlier

See earlier patch for motivation.
Yuya Nishihara - June 19, 2017, 2:39 p.m.
On Sun, 18 Jun 2017 00:02:26 -0700, Martin von Zweigbergk via Mercurial-devel wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1497593354 25200
> #      Thu Jun 15 23:09:14 2017 -0700
> # Node ID 1a23893eb7f7ea55df8835bd6208f8427fa36d13
> # Parent  b6b0928d0128d2b424b40bf4c4d2a59acc20f9ac
> repair: create transaction for bundle1 unbundling earlier
> 
> See earlier patch for motivation.
> 
> diff --git a/mercurial/repair.py b/mercurial/repair.py
> --- a/mercurial/repair.py
> +++ b/mercurial/repair.py
> @@ -202,13 +202,15 @@
>              if not repo.ui.verbose:
>                  # silence internal shuffling chatter
>                  repo.ui.pushbuffer()
> +            tmpbundleurl = 'bundle:' + vfs.join(tmpbundlefile)
>              if isinstance(gen, bundle2.unbundle20):
>                  with repo.transaction('strip') as tr:
>                      bundle2.applybundle(repo, gen, tr, source='strip',
> -                                        url='bundle:' + vfs.join(tmpbundlefile))
> +                                        url=tmpbundleurl)
>              else:
> -                gen.apply(repo, 'strip', 'bundle:' + vfs.join(tmpbundlefile),
> -                          True)
> +                txnname = "strip\n%s" % util.hidepassword(tmpbundleurl)
> +                with repo.lock(), repo.transaction(txnname):

I'm not sure if this repo.lock() is intentional or a copy-paste error.

FWIW, repair.strip() says "does not take any lock on the repo", but we already
have one for bookmark processing.

Other than that, the series looks good to me.
via Mercurial-devel - June 19, 2017, 4:23 p.m.
On Mon, Jun 19, 2017 at 7:39 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> On Sun, 18 Jun 2017 00:02:26 -0700, Martin von Zweigbergk via Mercurial-devel wrote:
>> # HG changeset patch
>> # User Martin von Zweigbergk <martinvonz@google.com>
>> # Date 1497593354 25200
>> #      Thu Jun 15 23:09:14 2017 -0700
>> # Node ID 1a23893eb7f7ea55df8835bd6208f8427fa36d13
>> # Parent  b6b0928d0128d2b424b40bf4c4d2a59acc20f9ac
>> repair: create transaction for bundle1 unbundling earlier
>>
>> See earlier patch for motivation.
>>
>> diff --git a/mercurial/repair.py b/mercurial/repair.py
>> --- a/mercurial/repair.py
>> +++ b/mercurial/repair.py
>> @@ -202,13 +202,15 @@
>>              if not repo.ui.verbose:
>>                  # silence internal shuffling chatter
>>                  repo.ui.pushbuffer()
>> +            tmpbundleurl = 'bundle:' + vfs.join(tmpbundlefile)
>>              if isinstance(gen, bundle2.unbundle20):
>>                  with repo.transaction('strip') as tr:
>>                      bundle2.applybundle(repo, gen, tr, source='strip',
>> -                                        url='bundle:' + vfs.join(tmpbundlefile))
>> +                                        url=tmpbundleurl)
>>              else:
>> -                gen.apply(repo, 'strip', 'bundle:' + vfs.join(tmpbundlefile),
>> -                          True)
>> +                txnname = "strip\n%s" % util.hidepassword(tmpbundleurl)
>> +                with repo.lock(), repo.transaction(txnname):
>
> I'm not sure if this repo.lock() is intentional or a copy-paste error.

I don't think that was intentional. Good catch. Will you fix in flight
it want me to send a v2?

>
> FWIW, repair.strip() says "does not take any lock on the repo", but we already
> have one for bookmark processing.

Hmm, good point about that comment. I wonder if that's even correct.
If it is, why don't we see the 'transaction requires locking'
exception from localrepo.transaction()? I'll look into it. I suspect
it will simply result in a patch that removes the comment.

>
> Other than that, the series looks good to me.
Yuya Nishihara - June 20, 2017, 11:56 a.m.
On Mon, 19 Jun 2017 09:23:35 -0700, Martin von Zweigbergk wrote:
> On Mon, Jun 19, 2017 at 7:39 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Sun, 18 Jun 2017 00:02:26 -0700, Martin von Zweigbergk via Mercurial-devel wrote:
> >> # HG changeset patch
> >> # User Martin von Zweigbergk <martinvonz@google.com>
> >> # Date 1497593354 25200
> >> #      Thu Jun 15 23:09:14 2017 -0700
> >> # Node ID 1a23893eb7f7ea55df8835bd6208f8427fa36d13
> >> # Parent  b6b0928d0128d2b424b40bf4c4d2a59acc20f9ac
> >> repair: create transaction for bundle1 unbundling earlier
> >>
> >> See earlier patch for motivation.
> >>
> >> diff --git a/mercurial/repair.py b/mercurial/repair.py
> >> --- a/mercurial/repair.py
> >> +++ b/mercurial/repair.py
> >> @@ -202,13 +202,15 @@
> >>              if not repo.ui.verbose:
> >>                  # silence internal shuffling chatter
> >>                  repo.ui.pushbuffer()
> >> +            tmpbundleurl = 'bundle:' + vfs.join(tmpbundlefile)
> >>              if isinstance(gen, bundle2.unbundle20):
> >>                  with repo.transaction('strip') as tr:
> >>                      bundle2.applybundle(repo, gen, tr, source='strip',
> >> -                                        url='bundle:' + vfs.join(tmpbundlefile))
> >> +                                        url=tmpbundleurl)
> >>              else:
> >> -                gen.apply(repo, 'strip', 'bundle:' + vfs.join(tmpbundlefile),
> >> -                          True)
> >> +                txnname = "strip\n%s" % util.hidepassword(tmpbundleurl)
> >> +                with repo.lock(), repo.transaction(txnname):
> >
> > I'm not sure if this repo.lock() is intentional or a copy-paste error.
> 
> I don't think that was intentional. Good catch. Will you fix in flight
> it want me to send a v2?

Okay, dropped repo.lock() and queued, thanks.

Patch

diff --git a/mercurial/repair.py b/mercurial/repair.py
--- a/mercurial/repair.py
+++ b/mercurial/repair.py
@@ -202,13 +202,15 @@ 
             if not repo.ui.verbose:
                 # silence internal shuffling chatter
                 repo.ui.pushbuffer()
+            tmpbundleurl = 'bundle:' + vfs.join(tmpbundlefile)
             if isinstance(gen, bundle2.unbundle20):
                 with repo.transaction('strip') as tr:
                     bundle2.applybundle(repo, gen, tr, source='strip',
-                                        url='bundle:' + vfs.join(tmpbundlefile))
+                                        url=tmpbundleurl)
             else:
-                gen.apply(repo, 'strip', 'bundle:' + vfs.join(tmpbundlefile),
-                          True)
+                txnname = "strip\n%s" % util.hidepassword(tmpbundleurl)
+                with repo.lock(), repo.transaction(txnname):
+                    gen.apply(repo, 'strip', tmpbundleurl, True)
             if not repo.ui.verbose:
                 repo.ui.popbuffer()
             f.close()