Patchwork [1,of,4] bundlerepo: update unlink in getremotechanges to use vfs

login
register
mail settings
Submitter Katsunori FUJIWARA
Date May 28, 2014, 12:25 a.m.
Message ID <ubnuivjex.wl%foozy@lares.dti.ne.jp>
Download mbox | patch
Permalink /patch/4872/
State Superseded
Headers show

Comments

Katsunori FUJIWARA - May 28, 2014, 12:25 a.m.
At Wed, 28 May 2014 08:22:40 +0900,
FUJIWARA Katsunori wrote:
> 
> At Wed, 28 May 2014 00:28:00 +0530,
> Chinmay Joshi wrote:
> > 
> > # HG changeset patch
> > # User Chinmay Joshi <c@chinmayjoshi.com>
> > # Date 1401207963 -19800
> > #      Tue May 27 21:56:03 2014 +0530
> > # Node ID 5dc8d26e1da18dfb8252115ac306d7f8fdd25582
> > # Parent  bee0e1cffdd36aab975090f1b51b81b6b14790fa
> > bundlerepo: update unlink in getremotechanges to use vfs
> > 
> > As per WindowsUTF8 plan, unlink from getremotechanges is updated to use vfs in this change.
> > 
> > diff --git a/mercurial/bundlerepo.py b/mercurial/bundlerepo.py
> > --- a/mercurial/bundlerepo.py
> > +++ b/mercurial/bundlerepo.py
> > @@ -352,7 +352,7 @@
> >      if not incoming:
> >          try:
> >              if bundlename:
> > -                os.unlink(bundlename)
> > +                repo.vfs.unlink(bundlename)
> >          except OSError:
> >              pass
> >          return repo, [], other.close
> > @@ -394,7 +394,7 @@
> >          if bundlerepo:
> >              bundlerepo.close()
> >          if bundle:
> > -            os.unlink(bundle)
> > +            repo.vfs.unlink(bundle)
> >          other.close()
> >  
> >      return (localrepo, csets, cleanup)
> 
> This patch should be backed out, because "bundlename" and "bundle" in
> this case are not relative paths to the root of repositories.
> 
> The former is specified via "hg incoming --bundle BUNDLENAME"
> (relative path to cwd, or absolute one), and the latter is generated
> in "changegroup.writebundle" by "tempfile.mkstemp" for internal
> temporary usage (absolute path).
> 
> To be exact, the latter hunk in this patch can be applied, because
> "os.join" for two absolute paths can generate correct result. But the
> former hunk can't, because it may unexpected result, if specified path
> is relative to cwd and cwd != root.
                         ^^^^^^^^^^^ "cwd != root/.hg"


Below patch for "test-incoming-outgoing.t" can detect this problem:

====================

Recent implementation can't unlink target bundle file, if there is no
incoming changeset: getremotechanges tries to unlink
"$TESTTMP/r1/.hg/x.hg" and avoids its failure.


> This is also because that I haven't applied "vfs" migration for these
> code paths, and "mercurial/bundlerepo.py" is marked as "finished but
> some are still used for ABS paths" in WindowsUTF8Plan.
> 
>     http://mercurial.selenic.com/wiki/WindowsUTF8Plan#Status_of_each_files
> 
> (I'll revise "for ABS paths" into "cwd relative paths" or so :-))
> 
> 
> Should this detail be described in "bundlerepo.py" as comment, too ?
> 
> ----------------------------------------------------------------------
> [FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
> 

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Chinmay Joshi - June 5, 2014, 1:46 p.m.
On Wed, May 28, 2014 at 5:55 AM, FUJIWARA Katsunori
<foozy@lares.dti.ne.jp> wrote:
>
> At Wed, 28 May 2014 08:22:40 +0900,
> FUJIWARA Katsunori wrote:
>>
>> At Wed, 28 May 2014 00:28:00 +0530,
>> Chinmay Joshi wrote:
>> >
>> > # HG changeset patch
>> > # User Chinmay Joshi <c@chinmayjoshi.com>
>> > # Date 1401207963 -19800
>> > #      Tue May 27 21:56:03 2014 +0530
>> > # Node ID 5dc8d26e1da18dfb8252115ac306d7f8fdd25582
>> > # Parent  bee0e1cffdd36aab975090f1b51b81b6b14790fa
>> > bundlerepo: update unlink in getremotechanges to use vfs
>> >
>> > As per WindowsUTF8 plan, unlink from getremotechanges is updated to use vfs in this change.
>> >
>> > diff --git a/mercurial/bundlerepo.py b/mercurial/bundlerepo.py
>> > --- a/mercurial/bundlerepo.py
>> > +++ b/mercurial/bundlerepo.py
>> > @@ -352,7 +352,7 @@
>> >      if not incoming:
>> >          try:
>> >              if bundlename:
>> > -                os.unlink(bundlename)
>> > +                repo.vfs.unlink(bundlename)
>> >          except OSError:
>> >              pass
>> >          return repo, [], other.close
>> > @@ -394,7 +394,7 @@
>> >          if bundlerepo:
>> >              bundlerepo.close()
>> >          if bundle:
>> > -            os.unlink(bundle)
>> > +            repo.vfs.unlink(bundle)
>> >          other.close()
>> >
>> >      return (localrepo, csets, cleanup)
>>
>> This patch should be backed out, because "bundlename" and "bundle" in
>> this case are not relative paths to the root of repositories.
>>
>> The former is specified via "hg incoming --bundle BUNDLENAME"
>> (relative path to cwd, or absolute one), and the latter is generated
>> in "changegroup.writebundle" by "tempfile.mkstemp" for internal
>> temporary usage (absolute path).
>>
>> To be exact, the latter hunk in this patch can be applied, because
>> "os.join" for two absolute paths can generate correct result. But the
>> former hunk can't, because it may unexpected result, if specified path
>> is relative to cwd and cwd != root.
>                          ^^^^^^^^^^^ "cwd != root/.hg"
>

Thanks for illuminating me. I was confused in cwd and repository
related paths. I agree that this needed to be backed out. This is already
backed out for good.

>
> Below patch for "test-incoming-outgoing.t" can detect this problem:
>
> ====================
> diff -r 764b691b8bda tests/test-incoming-outgoing.t
> --- a/tests/test-incoming-outgoing.t    Tue May 27 23:02:05 2014 +0530
> +++ b/tests/test-incoming-outgoing.t    Wed May 28 09:14:11 2014 +0900
> @@ -484,8 +484,14 @@ incoming from empty remote repository
>    $ echo a > r1/foo
>    $ hg -R r1 ci -Ama
>    adding foo
> +  $ touch x.hg
>    $ hg -R r1 incoming r2 --bundle x.hg
>    comparing with r2
>    searching for changes
>    no changes found
>    [1]
> +
> +"./x.hg" should be unlinked, if there is no incoming changeset.
> +
> +  $ test -f x.hg
> +  [1]
> ====================
>
> Recent implementation can't unlink target bundle file, if there is no
> incoming changeset: getremotechanges tries to unlink
> "$TESTTMP/r1/.hg/x.hg" and avoids its failure.
>
>
>> This is also because that I haven't applied "vfs" migration for these
>> code paths, and "mercurial/bundlerepo.py" is marked as "finished but
>> some are still used for ABS paths" in WindowsUTF8Plan.
>>
>>     http://mercurial.selenic.com/wiki/WindowsUTF8Plan#Status_of_each_files
>>
>> (I'll revise "for ABS paths" into "cwd relative paths" or so :-))
>>

Thanks for creating this wiki page for providing up to date
information for project. I will keep it updated for future use of
functions.

>>
>> Should this detail be described in "bundlerepo.py" as comment, too ?
>>
>> ----------------------------------------------------------------------
>> [FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@selenic.com
>> http://selenic.com/mailman/listinfo/mercurial-devel
>>
>
> ----------------------------------------------------------------------
> [FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp

Patch

====================
diff -r 764b691b8bda tests/test-incoming-outgoing.t
--- a/tests/test-incoming-outgoing.t    Tue May 27 23:02:05 2014 +0530
+++ b/tests/test-incoming-outgoing.t    Wed May 28 09:14:11 2014 +0900
@@ -484,8 +484,14 @@  incoming from empty remote repository
   $ echo a > r1/foo
   $ hg -R r1 ci -Ama
   adding foo
+  $ touch x.hg
   $ hg -R r1 incoming r2 --bundle x.hg
   comparing with r2
   searching for changes
   no changes found
   [1]
+
+"./x.hg" should be unlinked, if there is no incoming changeset.
+
+  $ test -f x.hg
+  [1]