Patchwork [2,of,2] mq: update subrepos when applying / unapplying patches that change .hgsubstate

login
register
mail settings
Submitter Angel Ezquerra
Date Aug. 17, 2013, 12:21 a.m.
Message ID <8921ebf2a815106702b0.1376698871@Angel-PC.localdomain>
Download mbox | patch
Permalink /patch/2198/
State Superseded
Headers show

Comments

Angel Ezquerra - Aug. 17, 2013, 12:21 a.m.
# HG changeset patch
# User Angel Ezquerra <angel.ezquerra@gmail.com>
# Date 1376350710 -7200
#      Tue Aug 13 01:38:30 2013 +0200
# Node ID 8921ebf2a815106702b0cb4da07177bfe61133ed
# Parent  8d70ca4d1804ca496ecf5f19fc7987b54f2de1ba
mq: update subrepos when applying / unapplying patches that change .hgsubstate

Up until now applying or unapplying a patch that modified .hgsubstate would not
work as expected because it would not update the subrepos according to the
.hgsubstate change. This made it very easy to lose subrepo changes when using
mq.

This revision also changes the test-mq-subrepo test so that on the qpop / qpush
tests. We no longer use the debugsub command to check the state of the subrepos
after the qpop and qpush operations. Instead we directly run the id command on
the subrepos that we want to check. The reason is that using the debugsub
command is misleading because it does not really check the state of the subrepos
on the working directory (it just returns what the change that is specified on a
given revision). Because of this the tests did not detect the problem that this
revision fixes (i.e. that applying a patch did not update the subrepos to the
corresponding revisions).
Martin Geisler - Aug. 17, 2013, 10:11 a.m.
Angel Ezquerra <angel.ezquerra@gmail.com> writes:

> # HG changeset patch
> # User Angel Ezquerra <angel.ezquerra@gmail.com>
> # Date 1376350710 -7200
> #      Tue Aug 13 01:38:30 2013 +0200
> # Node ID 8921ebf2a815106702b0cb4da07177bfe61133ed
> # Parent  8d70ca4d1804ca496ecf5f19fc7987b54f2de1ba
> mq: update subrepos when applying / unapplying patches that change .hgsubstate
>
> Up until now applying or unapplying a patch that modified .hgsubstate
> would not work as expected because it would not update the subrepos
> according to the .hgsubstate change. This made it very easy to lose
> subrepo changes when using mq.
>
> This revision also changes the test-mq-subrepo test so that on the
> qpop / qpush tests. We no longer use the debugsub command to check the
> state of the subrepos after the qpop and qpush operations. Instead we
> directly run the id command on the subrepos that we want to check. The
> reason is that using the debugsub command is misleading because it
> does not really check the state of the subrepos on the working
> directory (it just returns what the change that is specified on a
> given revision). Because of this the tests did not detect the problem
> that this revision fixes (i.e. that applying a patch did not update
> the subrepos to the corresponding revisions).
>
> diff --git a/hgext/mq.py b/hgext/mq.py
> --- a/hgext/mq.py
> +++ b/hgext/mq.py
> @@ -800,6 +800,13 @@
>                  p1, p2 = repo.dirstate.parents()
>                  repo.setparents(p1, merge)
>  
> +            if all_files and '.hgsubstate' in all_files:
> +                for line in open(repo.wjoin('.hgsubstate'), 'r').readlines():
> +                    rev, s = line.split()
> +                    files.append(s)
> +                    state = (s, rev, 'any')
> +                    repo[None].sub(s).get(state, overwrite=True)

This functionality must be present somewhere else -- since the normal
update code also parses and updates subrepos as needed. I found it in
the subrepo.state function.

Maybe you could even use the entire submerge command? That should
automatically handle things like changed subrepo sources too.
Angel Ezquerra - Aug. 21, 2013, 6:44 p.m.
On Sat, Aug 17, 2013 at 12:11 PM, Martin Geisler <martin@geisler.net> wrote:
> Angel Ezquerra <angel.ezquerra@gmail.com> writes:
>
>> # HG changeset patch
>> # User Angel Ezquerra <angel.ezquerra@gmail.com>
>> # Date 1376350710 -7200
>> #      Tue Aug 13 01:38:30 2013 +0200
>> # Node ID 8921ebf2a815106702b0cb4da07177bfe61133ed
>> # Parent  8d70ca4d1804ca496ecf5f19fc7987b54f2de1ba
>> mq: update subrepos when applying / unapplying patches that change .hgsubstate
>>
>> Up until now applying or unapplying a patch that modified .hgsubstate
>> would not work as expected because it would not update the subrepos
>> according to the .hgsubstate change. This made it very easy to lose
>> subrepo changes when using mq.
>>
>> This revision also changes the test-mq-subrepo test so that on the
>> qpop / qpush tests. We no longer use the debugsub command to check the
>> state of the subrepos after the qpop and qpush operations. Instead we
>> directly run the id command on the subrepos that we want to check. The
>> reason is that using the debugsub command is misleading because it
>> does not really check the state of the subrepos on the working
>> directory (it just returns what the change that is specified on a
>> given revision). Because of this the tests did not detect the problem
>> that this revision fixes (i.e. that applying a patch did not update
>> the subrepos to the corresponding revisions).
>>
>> diff --git a/hgext/mq.py b/hgext/mq.py
>> --- a/hgext/mq.py
>> +++ b/hgext/mq.py
>> @@ -800,6 +800,13 @@
>>                  p1, p2 = repo.dirstate.parents()
>>                  repo.setparents(p1, merge)
>>
>> +            if all_files and '.hgsubstate' in all_files:
>> +                for line in open(repo.wjoin('.hgsubstate'), 'r').readlines():
>> +                    rev, s = line.split()
>> +                    files.append(s)
>> +                    state = (s, rev, 'any')
>> +                    repo[None].sub(s).get(state, overwrite=True)

Martin, thanks for the review. Sorry it took me a while to get back to you...

> This functionality must be present somewhere else -- since the normal
> update code also parses and updates subrepos as needed. I found it in
> the subrepo.state function.
>
> Maybe you could even use the entire submerge command? That should
> automatically handle things like changed subrepo sources too.

I'll look into it. My first impression is that you may be right.

, although in this case there would be no ancestor, or perhaps I could
check if the original parent of the patch is still on the repository,
and use that as ancestor? I have not really looked into the mq code
before this patch so I don't really know if that is something that mq
usually does... Or perhaps I could just use use the nullstate as the
ancestor?

My other concern is that usually subrepos are merged by doing an
actual merge on the subrepo which is something that I definitely think
should _not_ be done when you apply a patch! I don't know if submerge
is the one who does that, so I'll have to look into that as well.

In fact, this subrepo behavior (doing actual merges on subrepos when
merging the parent) is one of the most annoying things when using
subrepos (IMHO) and one I have been asked how to avoid tons of times
since I started supporting mercurial on my company... maybe I should
look into that as well :-)

Anyway, I'll see if I can use subrepo.state and subrepo.merge as you
suggest. There is already quite a lot of code that mq seems to almost
duplicate from regular mercurial commands. Let's see if I can avoid
adding even more :-)

Cheers,

Angel
Angel Ezquerra - Aug. 21, 2013, 8:40 p.m.
On Wed, Aug 21, 2013 at 8:44 PM, Angel Ezquerra
<angel.ezquerra@gmail.com> wrote:
> On Sat, Aug 17, 2013 at 12:11 PM, Martin Geisler <martin@geisler.net> wrote:
>> Angel Ezquerra <angel.ezquerra@gmail.com> writes:
>>
>>> # HG changeset patch
>>> # User Angel Ezquerra <angel.ezquerra@gmail.com>
>>> # Date 1376350710 -7200
>>> #      Tue Aug 13 01:38:30 2013 +0200
>>> # Node ID 8921ebf2a815106702b0cb4da07177bfe61133ed
>>> # Parent  8d70ca4d1804ca496ecf5f19fc7987b54f2de1ba
>>> mq: update subrepos when applying / unapplying patches that change .hgsubstate
>>>
>>> Up until now applying or unapplying a patch that modified .hgsubstate
>>> would not work as expected because it would not update the subrepos
>>> according to the .hgsubstate change. This made it very easy to lose
>>> subrepo changes when using mq.
>>>
>>> This revision also changes the test-mq-subrepo test so that on the
>>> qpop / qpush tests. We no longer use the debugsub command to check the
>>> state of the subrepos after the qpop and qpush operations. Instead we
>>> directly run the id command on the subrepos that we want to check. The
>>> reason is that using the debugsub command is misleading because it
>>> does not really check the state of the subrepos on the working
>>> directory (it just returns what the change that is specified on a
>>> given revision). Because of this the tests did not detect the problem
>>> that this revision fixes (i.e. that applying a patch did not update
>>> the subrepos to the corresponding revisions).
>>>
>>> diff --git a/hgext/mq.py b/hgext/mq.py
>>> --- a/hgext/mq.py
>>> +++ b/hgext/mq.py
>>> @@ -800,6 +800,13 @@
>>>                  p1, p2 = repo.dirstate.parents()
>>>                  repo.setparents(p1, merge)
>>>
>>> +            if all_files and '.hgsubstate' in all_files:
>>> +                for line in open(repo.wjoin('.hgsubstate'), 'r').readlines():
>>> +                    rev, s = line.split()
>>> +                    files.append(s)
>>> +                    state = (s, rev, 'any')
>>> +                    repo[None].sub(s).get(state, overwrite=True)
>
> Martin, thanks for the review. Sorry it took me a while to get back to you...
>
>> This functionality must be present somewhere else -- since the normal
>> update code also parses and updates subrepos as needed. I found it in
>> the subrepo.state function.
>>
>> Maybe you could even use the entire submerge command? That should
>> automatically handle things like changed subrepo sources too.
>
> I'll look into it. My first impression is that you may be right.
>
> , although in this case there would be no ancestor, or perhaps I could
> check if the original parent of the patch is still on the repository,
> and use that as ancestor? I have not really looked into the mq code
> before this patch so I don't really know if that is something that mq
> usually does... Or perhaps I could just use use the nullstate as the
> ancestor?
>
> My other concern is that usually subrepos are merged by doing an
> actual merge on the subrepo which is something that I definitely think
> should _not_ be done when you apply a patch! I don't know if submerge
> is the one who does that, so I'll have to look into that as well.
>
> In fact, this subrepo behavior (doing actual merges on subrepos when
> merging the parent) is one of the most annoying things when using
> subrepos (IMHO) and one I have been asked how to avoid tons of times
> since I started supporting mercurial on my company... maybe I should
> look into that as well :-)
>
> Anyway, I'll see if I can use subrepo.state and subrepo.merge as you
> suggest. There is already quite a lot of code that mq seems to almost
> duplicate from regular mercurial commands. Let's see if I can avoid
> adding even more :-)
>
> Cheers,
>
> Angel

It turns out that it was actually quite easy to reuse
subrepo.submerge. Thanks for the great advice, Martin!

Cheers,

Angel
Martin Geisler - Aug. 22, 2013, 7:24 a.m.
Angel Ezquerra <angel.ezquerra@gmail.com> writes:

> On Wed, Aug 21, 2013 at 8:44 PM, Angel Ezquerra
> <angel.ezquerra@gmail.com> wrote:
>
>> Anyway, I'll see if I can use subrepo.state and subrepo.merge as you
>> suggest. There is already quite a lot of code that mq seems to almost
>> duplicate from regular mercurial commands. Let's see if I can avoid
>> adding even more :-)
>
> It turns out that it was actually quite easy to reuse
> subrepo.submerge. Thanks for the great advice, Martin!

That's great to hear!

The confusing part (which I should have mentioned) is that *all* updates
are done as merges in Mercurial. That is, 'hg update' becomes a merge
where the ancestor is equal to the one parent and the other parent is
the update target.

The call chain for 'hg update' is like this:

  mercurial.commands.update calls
  -> mercurial.hg.update
     -> mercurial.hg.updaterepo
        -> mercurial.merge.update

The calls for 'hg merge' are:

  mercurial.commands.merge
  -> mercurial.hg.merge
     -> mercurial.hg.updaterepo
        -> mercurial.merge.update

The arguments differ, of course :)

Patch

# HG changeset patch
# User Angel Ezquerra <angel.ezquerra@gmail.com>
# Date 1376350710 -7200
#      Tue Aug 13 01:38:30 2013 +0200
# Node ID 8921ebf2a815106702b0cb4da07177bfe61133ed
# Parent  8d70ca4d1804ca496ecf5f19fc7987b54f2de1ba
mq: update subrepos when applying / unapplying patches that change .hgsubstate

Up until now applying or unapplying a patch that modified .hgsubstate would not
work as expected because it would not update the subrepos according to the
.hgsubstate change. This made it very easy to lose subrepo changes when using
mq.

This revision also changes the test-mq-subrepo test so that on the qpop / qpush
tests. We no longer use the debugsub command to check the state of the subrepos
after the qpop and qpush operations. Instead we directly run the id command on
the subrepos that we want to check. The reason is that using the debugsub
command is misleading because it does not really check the state of the subrepos
on the working directory (it just returns what the change that is specified on a
given revision). Because of this the tests did not detect the problem that this
revision fixes (i.e. that applying a patch did not update the subrepos to the
corresponding revisions).

diff --git a/hgext/mq.py b/hgext/mq.py
--- a/hgext/mq.py
+++ b/hgext/mq.py
@@ -800,6 +800,13 @@ 
                 p1, p2 = repo.dirstate.parents()
                 repo.setparents(p1, merge)
 
+            if all_files and '.hgsubstate' in all_files:
+                for line in open(repo.wjoin('.hgsubstate'), 'r').readlines():
+                    rev, s = line.split()
+                    files.append(s)
+                    state = (s, rev, 'any')
+                    repo[None].sub(s).get(state, overwrite=True)
+
             match = scmutil.matchfiles(repo, files or [])
             oldtip = repo['tip']
             n = newcommit(repo, None, message, ph.user, ph.date, match=match,
@@ -1459,6 +1466,8 @@ 
                 self.ui.status(_("popping %s\n") % patch.name)
             del self.applied[start:end]
             self.strip(repo, [rev], update=False, backup='strip')
+            for s, state in repo['.'].substate.items():
+                repo['.'].sub(s).get(state)
             if self.applied:
                 self.ui.write(_("now at: %s\n") % self.applied[-1].name)
             else:
diff --git a/tests/test-mq-subrepo.t b/tests/test-mq-subrepo.t
--- a/tests/test-mq-subrepo.t
+++ b/tests/test-mq-subrepo.t
@@ -244,13 +244,11 @@ 
   popping 1.diff
   now at: 0.diff
   $ hg status -AS
-  M sub/a
   C .hgsub
   C .hgsubstate
-  $ hg debugsub
-  path sub
-   source   sub
-   revision b2fdb12cd82b021c3b7053d67802e77b6eeaee31
+  C sub/a
+  $ hg -R sub id --id
+  b2fdb12cd82b
 
 qpush
   $ hg -R sub update 0000
@@ -265,13 +263,11 @@ 
   applying 1.diff
   now at: 1.diff
   $ hg status -AS
-  M .hgsubstate
   C .hgsub
+  C .hgsubstate
   C sub/a
-  $ hg debugsub
-  path sub
-   source   sub
-   revision aa037b301eba54f350c75951b5486727fb98cbb5
+  $ hg -R sub id --id
+  aa037b301eba
 
   $ cd ..