Patchwork [3,of,3] archive: support 'wdir()'

login
register
mail settings
Submitter Yuya Nishihara
Date June 16, 2015, 3:05 p.m.
Message ID <20150617000552.97026a6b27f624d1f7f4de7d@tcha.org>
Download mbox | patch
Permalink /patch/9662/
State Superseded
Commit 3ec8351fa6ed66ccfea9c9754abb4fe59fca959f
Headers show

Comments

Yuya Nishihara - June 16, 2015, 3:05 p.m.
On Mon, 15 Jun 2015 21:59:22 -0400, Matt Harbison wrote:
> On Mon, 15 Jun 2015 20:56:59 -0400, Matt Harbison <mharbison72@gmail.com>  
> wrote:
> 
> > On Mon, 15 Jun 2015 11:46:09 -0400, Yuya Nishihara <yuya@tcha.org> wrote:
> >
> >> On Mon, 15 Jun 2015 00:28:59 -0400, Matt Harbison wrote:
> >>> # HG changeset patch
> >>> # User Matt Harbison <matt_harbison@yahoo.com>
> >>> # Date 1434328768 14400
> >>> #      Sun Jun 14 20:39:28 2015 -0400
> >>> # Node ID 18d376a661ffabe8121ab5d57b245f4f347f8cd1
> >>> # Parent  f5f5e4ae488d9cae8111e9a212f647ed1430a019
> >>> archive: support 'wdir()'
> >>
> >>> diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
> >>> --- a/mercurial/subrepo.py
> >>> +++ b/mercurial/subrepo.py
> >>> @@ -751,8 +751,8 @@
> >>>      def archive(self, archiver, prefix, match=None):
> >>>          self._get(self._state + ('hg',))
> >>>          total = abstractsubrepo.archive(self, archiver, prefix, match)
> >>> -        rev = self._state[1]
> >>> -        ctx = self._repo[rev]
> >>> +        ctx = self._getctx()
> >>> +
> >>>          for subpath in ctx.substate:
> >>>              s = subrepo(ctx, subpath)
> >>>              submatch = matchmod.narrowmatcher(subpath, match)
> >>> @@ -918,18 +918,13 @@
> >>>
> >>>      @annotatesubrepoerror
> >>>      def files(self):
> >>> -        rev = self._state[1]
> >>> -        ctx = self._repo[rev]
> >>> -        return ctx.manifest().keys()
> >>> +        return self._getctx().manifest().keys()
> >>>
> >>>      def filedata(self, name):
> >>> -        rev = self._state[1]
> >>> -        return self._repo[rev][name].data()
> >>> +        return self._getctx()[name].data()
> >>>
> >>>      def fileflags(self, name):
> >>> -        rev = self._state[1]
> >>> -        ctx = self._repo[rev]
> >>> -        return ctx.flags(name)
> >>> +        return self._getctx().flags(name)
> >>
> >> Can't we switch to the hgsubrepo object representing wdir?
> >
> > We don't have such a thing- subrepo is always created based on the  
> > .hgsubstate file.  Do you mean do a hack like this, to patch state[1] to  
> > be None?
> >
> >    https://www.selenic.com/pipermail/mercurial-devel/2015-June/070763.html
> >
> > I know previously when I tried getting subrepo.subrepo() to return a  
> > subrepo with state[1] == None if the calling context was wdir(), status  
> > and diff broke.  I can't seem to get it to fail the same way now, so  
> > I'll fiddle with that some more.
> 
> A bit of followup:
> 
> 1) sub._substate[1] must reflect what is written to .hgsubstate in p1(),  
> otherwise sub.dirty() is wrong.  Archive doesn't care about dirty, but  
> this detail precludes a general solution I think.

True. subrepo shouldn't reflect the working directory changes in general.
Perhaps "archive" is one of exceptional cases.

> 2) If we were to create a local subrepo object in archive that is hacked  
> up to represent wdir(), I think we have to do it each time a subrepo is  
> entered into.  It isn't a matter of redirecting once at the top and  
> everything just works.  And since creating a subrepo involves pathauditor  
> (or if we call the constructor directly, creating a new repo object and  
> another path.exists()), it seems that the 'if rev is None' duplication  
> below is actually cheaper.

Hmm, I thought something like the following. But I agree, it wasn't nice
because sub.archive() had to know it want a wdir subrepo anyway.
Matt Harbison - June 17, 2015, 3:31 a.m.
On Tue, 16 Jun 2015 11:05:52 -0400, Yuya Nishihara <yuya@tcha.org> wrote:

> On Mon, 15 Jun 2015 21:59:22 -0400, Matt Harbison wrote:
>> On Mon, 15 Jun 2015 20:56:59 -0400, Matt Harbison  
>> <mharbison72@gmail.com>
>> wrote:
>>
>> > On Mon, 15 Jun 2015 11:46:09 -0400, Yuya Nishihara <yuya@tcha.org>  
>> wrote:
>> >
>> >> On Mon, 15 Jun 2015 00:28:59 -0400, Matt Harbison wrote:
>> >>> # HG changeset patch
>> >>> # User Matt Harbison <matt_harbison@yahoo.com>
>> >>> # Date 1434328768 14400
>> >>> #      Sun Jun 14 20:39:28 2015 -0400
>> >>> # Node ID 18d376a661ffabe8121ab5d57b245f4f347f8cd1
>> >>> # Parent  f5f5e4ae488d9cae8111e9a212f647ed1430a019
>> >>> archive: support 'wdir()'
>> >>
>> >>> diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
>> >>> --- a/mercurial/subrepo.py
>> >>> +++ b/mercurial/subrepo.py
>> >>> @@ -751,8 +751,8 @@
>> >>>      def archive(self, archiver, prefix, match=None):
>> >>>          self._get(self._state + ('hg',))
>> >>>          total = abstractsubrepo.archive(self, archiver, prefix,  
>> match)
>> >>> -        rev = self._state[1]
>> >>> -        ctx = self._repo[rev]
>> >>> +        ctx = self._getctx()
>> >>> +
>> >>>          for subpath in ctx.substate:
>> >>>              s = subrepo(ctx, subpath)
>> >>>              submatch = matchmod.narrowmatcher(subpath, match)
>> >>> @@ -918,18 +918,13 @@
>> >>>
>> >>>      @annotatesubrepoerror
>> >>>      def files(self):
>> >>> -        rev = self._state[1]
>> >>> -        ctx = self._repo[rev]
>> >>> -        return ctx.manifest().keys()
>> >>> +        return self._getctx().manifest().keys()
>> >>>
>> >>>      def filedata(self, name):
>> >>> -        rev = self._state[1]
>> >>> -        return self._repo[rev][name].data()
>> >>> +        return self._getctx()[name].data()
>> >>>
>> >>>      def fileflags(self, name):
>> >>> -        rev = self._state[1]
>> >>> -        ctx = self._repo[rev]
>> >>> -        return ctx.flags(name)
>> >>> +        return self._getctx().flags(name)
>> >>
>> >> Can't we switch to the hgsubrepo object representing wdir?
>> >
>> > We don't have such a thing- subrepo is always created based on the
>> > .hgsubstate file.  Do you mean do a hack like this, to patch state[1]  
>> to
>> > be None?
>> >
>> >     
>> https://www.selenic.com/pipermail/mercurial-devel/2015-June/070763.html
>> >
>> > I know previously when I tried getting subrepo.subrepo() to return a
>> > subrepo with state[1] == None if the calling context was wdir(),  
>> status
>> > and diff broke.  I can't seem to get it to fail the same way now, so
>> > I'll fiddle with that some more.
>>
>> A bit of followup:
>>
>> 1) sub._substate[1] must reflect what is written to .hgsubstate in p1(),
>> otherwise sub.dirty() is wrong.  Archive doesn't care about dirty, but
>> this detail precludes a general solution I think.
>
> True. subrepo shouldn't reflect the working directory changes in general.
> Perhaps "archive" is one of exceptional cases.
>
>> 2) If we were to create a local subrepo object in archive that is hacked
>> up to represent wdir(), I think we have to do it each time a subrepo is
>> entered into.  It isn't a matter of redirecting once at the top and
>> everything just works.  And since creating a subrepo involves  
>> pathauditor
>> (or if we call the constructor directly, creating a new repo object and
>> another path.exists()), it seems that the 'if rev is None' duplication
>> below is actually cheaper.
>
> Hmm, I thought something like the following. But I agree, it wasn't nice
> because sub.archive() had to know it want a wdir subrepo anyway.
>
> diff --git a/mercurial/archival.py b/mercurial/archival.py
> --- a/mercurial/archival.py
> +++ b/mercurial/archival.py
> @@ -315,7 +315,7 @@ def archive(repo, dest, node, kind, deco
>     if subrepos:
>          for subpath in sorted(ctx.substate):
> -            sub = ctx.sub(subpath)
> +            sub = ctx.workingsub(subpath)
>              submatch = matchmod.narrowmatcher(subpath, matchfn)
>              total += sub.archive(archiver, prefix, submatch)
> diff --git a/mercurial/context.py b/mercurial/context.py
> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -254,6 +254,11 @@ class basectx(object):
>      def nullsub(self, path, pctx):
>          return subrepo.nullsubrepo(self, path, pctx)
> +    def workingsub(self, path):
> +        state = self.substate[path]
> +        state = (state[0], self.subrev(path), state[2])  # XXX hack
> +        return subrepo.subrepo(self, path, state)
> +
>      def match(self, pats=[], include=None, exclude=None, default='glob',
>                listsubrepos=False, badfn=None):
>          r = self._repo
> diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
> --- a/mercurial/subrepo.py
> +++ b/mercurial/subrepo.py
> @@ -324,7 +324,7 @@ def _sanitize(ui, vfs, ignore):
>                            "in '%s'\n") % vfs.join(dirname))
>                  vfs.unlink(vfs.reljoin(dirname, f))
> -def subrepo(ctx, path):
> +def subrepo(ctx, path, state=None):
>      """return instance of the right subrepo class for subrepo in path"""
>      # subrepo inherently violates our import layering rules
>      # because it wants to make repo objects from deep inside the stack
> @@ -335,7 +335,8 @@ def subrepo(ctx, path):
>      hg = h
>     pathutil.pathauditor(ctx.repo().root)(path)
> -    state = ctx.substate[path]
> +    if not state:  # XXX hack
> +        state = ctx.substate[path]
>      if state[2] not in types:
>          raise util.Abort(_('unknown subrepo type %s') % state[2])
>      return types[state[2]](ctx, path, state[:2])
> @@ -754,7 +755,7 @@ class hgsubrepo(abstractsubrepo):
>          rev = self._state[1]
>          ctx = self._repo[rev]
>          for subpath in ctx.substate:
> -            s = subrepo(ctx, subpath)
> +            s = ctx.workingsub(subpath)  # XXX smells bad
>              submatch = matchmod.narrowmatcher(subpath, match)
>              total += s.archive(archiver, prefix + self._path + '/',  
> submatch)
>          return total

Works for me.  The funny thing is I did something almost identical a few  
months ago and rejected it as too hacky.

Patch

diff --git a/mercurial/archival.py b/mercurial/archival.py
--- a/mercurial/archival.py
+++ b/mercurial/archival.py
@@ -315,7 +315,7 @@  def archive(repo, dest, node, kind, deco
 
     if subrepos:
         for subpath in sorted(ctx.substate):
-            sub = ctx.sub(subpath)
+            sub = ctx.workingsub(subpath)
             submatch = matchmod.narrowmatcher(subpath, matchfn)
             total += sub.archive(archiver, prefix, submatch)
 
diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -254,6 +254,11 @@  class basectx(object):
     def nullsub(self, path, pctx):
         return subrepo.nullsubrepo(self, path, pctx)
 
+    def workingsub(self, path):
+        state = self.substate[path]
+        state = (state[0], self.subrev(path), state[2])  # XXX hack
+        return subrepo.subrepo(self, path, state)
+
     def match(self, pats=[], include=None, exclude=None, default='glob',
               listsubrepos=False, badfn=None):
         r = self._repo
diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
--- a/mercurial/subrepo.py
+++ b/mercurial/subrepo.py
@@ -324,7 +324,7 @@  def _sanitize(ui, vfs, ignore):
                           "in '%s'\n") % vfs.join(dirname))
                 vfs.unlink(vfs.reljoin(dirname, f))
 
-def subrepo(ctx, path):
+def subrepo(ctx, path, state=None):
     """return instance of the right subrepo class for subrepo in path"""
     # subrepo inherently violates our import layering rules
     # because it wants to make repo objects from deep inside the stack
@@ -335,7 +335,8 @@  def subrepo(ctx, path):
     hg = h
 
     pathutil.pathauditor(ctx.repo().root)(path)
-    state = ctx.substate[path]
+    if not state:  # XXX hack
+        state = ctx.substate[path]
     if state[2] not in types:
         raise util.Abort(_('unknown subrepo type %s') % state[2])
     return types[state[2]](ctx, path, state[:2])
@@ -754,7 +755,7 @@  class hgsubrepo(abstractsubrepo):
         rev = self._state[1]
         ctx = self._repo[rev]
         for subpath in ctx.substate:
-            s = subrepo(ctx, subpath)
+            s = ctx.workingsub(subpath)  # XXX smells bad
             submatch = matchmod.narrowmatcher(subpath, match)
             total += s.archive(archiver, prefix + self._path + '/', submatch)
         return total