Patchwork [2,of,2] convert: execute merges in-memory (issue5076)

login
register
mail settings
Submitter Tony Tung
Date Feb. 23, 2016, 8:14 a.m.
Message ID <b7f61d7438ba1aed06af.1456215240@dev8692.prn1.facebook.com>
Download mbox | patch
Permalink /patch/13305/
State Changes Requested
Delegated to: Martin von Zweigbergk
Headers show

Comments

Tony Tung - Feb. 23, 2016, 8:14 a.m.
# HG changeset patch
# User Tony Tung <tonytung@merly.org>
# Date 1456215137 28800
#      Tue Feb 23 00:12:17 2016 -0800
# Node ID b7f61d7438ba1aed06afa1a53a2b5fda8bdc225f
# Parent  cbc605f28b61cc469e21186982d0569bf94f7d2a
convert: execute merges in-memory (issue5076)

Typically during merging two manifests, we detect if a subrepo is dirty
(i.e., the working directory differs from the hgsubstate).  If it is, we
mark it in the manifest.

When we are doing repo conversions, we do not change the working
directory.  All the changes are happening in-memory, and as such, the
computed substate will always mismatch.  This patch adds a flag to the
manifest merge logic to treat wctx as not a directory.

This is an alternate fix.  The original proposed fix was sent as
https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-February/079733.html
Augie Fackler - Feb. 23, 2016, 7:30 p.m.
On Tue, Feb 23, 2016 at 12:14:00AM -0800, Tony Tung wrote:
> # HG changeset patch
> # User Tony Tung <tonytung@merly.org>
> # Date 1456215137 28800
> #      Tue Feb 23 00:12:17 2016 -0800
> # Node ID b7f61d7438ba1aed06afa1a53a2b5fda8bdc225f
> # Parent  cbc605f28b61cc469e21186982d0569bf94f7d2a
> convert: execute merges in-memory (issue5076)

I think this makes sense. Sean, you've done in-memory merge stuff,
does this look sensible at least for convert to you?

Martin, does this make sense to you? You reviewed the last round.

>
> Typically during merging two manifests, we detect if a subrepo is dirty
> (i.e., the working directory differs from the hgsubstate).  If it is, we
> mark it in the manifest.
>
> When we are doing repo conversions, we do not change the working
> directory.  All the changes are happening in-memory, and as such, the
> computed substate will always mismatch.  This patch adds a flag to the
> manifest merge logic to treat wctx as not a directory.
>
> This is an alternate fix.  The original proposed fix was sent as
> https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-February/079733.html
>
> diff --git a/hgext/convert/hg.py b/hgext/convert/hg.py
> --- a/hgext/convert/hg.py
> +++ b/hgext/convert/hg.py
> @@ -194,6 +194,7 @@
>              True,  # force
>              False, # acceptremote
>              False, # followcopies
> +            inmemoryonly=True,
>          )
>
>          for file, (action, info, msg) in actions.iteritems():
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -769,13 +769,16 @@
>      return True
>
>  def manifestmerge(repo, wctx, p2, pa, branchmerge, force, matcher,
> -                  acceptremote, followcopies):
> +                  acceptremote, followcopies, inmemoryonly):
>      """
>      Merge p1 and p2 with ancestor pa and generate merge action list
>
>      branchmerge and force are as passed in to update
>      matcher = matcher to filter file lists
>      acceptremote = accept the incoming changes without prompting
> +    inmemoryonly = merge is happening in memory only; wctx does not
> +                   represent an actual working directory.  an example
> +                   of this would be repository conversion (convert extension).
>      """
>      if matcher is not None and matcher.always():
>          matcher = None
> @@ -799,7 +802,7 @@
>      copied = set(copy.values())
>      copied.update(movewithdir.values())
>
> -    if '.hgsubstate' in m1:
> +    if not inmemoryonly and '.hgsubstate' in m1:
>          # check whether sub state is modified
>          for s in sorted(wctx.substate):
>              if wctx.sub(s).dirty():
> @@ -931,12 +934,12 @@
>
>  def calculateupdates(repo, wctx, mctx, ancestors, branchmerge, force,
>                       acceptremote, followcopies, matcher=None,
> -                     mergeforce=False):
> +                     mergeforce=False, inmemoryonly=False):
>      "Calculate the actions needed to merge mctx into wctx using ancestors"
>      if len(ancestors) == 1: # default
>          actions, diverge, renamedelete = manifestmerge(
>              repo, wctx, mctx, ancestors[0], branchmerge, force, matcher,
> -            acceptremote, followcopies)
> +            acceptremote, followcopies, inmemoryonly)
>          _checkunknownfiles(repo, wctx, mctx, force, actions, mergeforce)
>
>      else: # only when merge.preferancestor=* - the default
> @@ -951,7 +954,7 @@
>              repo.ui.note(_('\ncalculating bids for ancestor %s\n') % ancestor)
>              actions, diverge1, renamedelete1 = manifestmerge(
>                  repo, wctx, mctx, ancestor, branchmerge, force, matcher,
> -                acceptremote, followcopies)
> +                acceptremote, followcopies, inmemoryonly)
>              _checkunknownfiles(repo, wctx, mctx, force, actions, mergeforce)
>
>              # Track the shortest set of warning on the theory that bid
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Martin von Zweigbergk - Feb. 23, 2016, 7:51 p.m.
On Tue, Feb 23, 2016 at 11:30 AM, Augie Fackler <raf@durin42.com> wrote:
> On Tue, Feb 23, 2016 at 12:14:00AM -0800, Tony Tung wrote:
>> # HG changeset patch
>> # User Tony Tung <tonytung@merly.org>
>> # Date 1456215137 28800
>> #      Tue Feb 23 00:12:17 2016 -0800
>> # Node ID b7f61d7438ba1aed06afa1a53a2b5fda8bdc225f
>> # Parent  cbc605f28b61cc469e21186982d0569bf94f7d2a
>> convert: execute merges in-memory (issue5076)
>
> I think this makes sense. Sean, you've done in-memory merge stuff,
> does this look sensible at least for convert to you?
>
> Martin, does this make sense to you? You reviewed the last round.

I was hoping we could instead do "m1 = m1.copy()" to avoid modifying
the manifest instance in the cache (I just sent out a little patch
that makes that simpler to do). Unfortunately, a lot of tests break if
we do that. Tony, do you think that makes sense? Do you have time to
see if you can get that to work?

>
>>
>> Typically during merging two manifests, we detect if a subrepo is dirty
>> (i.e., the working directory differs from the hgsubstate).  If it is, we
>> mark it in the manifest.
>>
>> When we are doing repo conversions, we do not change the working
>> directory.  All the changes are happening in-memory, and as such, the
>> computed substate will always mismatch.  This patch adds a flag to the
>> manifest merge logic to treat wctx as not a directory.
>>
>> This is an alternate fix.  The original proposed fix was sent as
>> https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-February/079733.html
>>
>> diff --git a/hgext/convert/hg.py b/hgext/convert/hg.py
>> --- a/hgext/convert/hg.py
>> +++ b/hgext/convert/hg.py
>> @@ -194,6 +194,7 @@
>>              True,  # force
>>              False, # acceptremote
>>              False, # followcopies
>> +            inmemoryonly=True,
>>          )
>>
>>          for file, (action, info, msg) in actions.iteritems():
>> diff --git a/mercurial/merge.py b/mercurial/merge.py
>> --- a/mercurial/merge.py
>> +++ b/mercurial/merge.py
>> @@ -769,13 +769,16 @@
>>      return True
>>
>>  def manifestmerge(repo, wctx, p2, pa, branchmerge, force, matcher,
>> -                  acceptremote, followcopies):
>> +                  acceptremote, followcopies, inmemoryonly):
>>      """
>>      Merge p1 and p2 with ancestor pa and generate merge action list
>>
>>      branchmerge and force are as passed in to update
>>      matcher = matcher to filter file lists
>>      acceptremote = accept the incoming changes without prompting
>> +    inmemoryonly = merge is happening in memory only; wctx does not
>> +                   represent an actual working directory.  an example
>> +                   of this would be repository conversion (convert extension).
>>      """
>>      if matcher is not None and matcher.always():
>>          matcher = None
>> @@ -799,7 +802,7 @@
>>      copied = set(copy.values())
>>      copied.update(movewithdir.values())
>>
>> -    if '.hgsubstate' in m1:
>> +    if not inmemoryonly and '.hgsubstate' in m1:
>>          # check whether sub state is modified
>>          for s in sorted(wctx.substate):
>>              if wctx.sub(s).dirty():
>> @@ -931,12 +934,12 @@
>>
>>  def calculateupdates(repo, wctx, mctx, ancestors, branchmerge, force,
>>                       acceptremote, followcopies, matcher=None,
>> -                     mergeforce=False):
>> +                     mergeforce=False, inmemoryonly=False):
>>      "Calculate the actions needed to merge mctx into wctx using ancestors"
>>      if len(ancestors) == 1: # default
>>          actions, diverge, renamedelete = manifestmerge(
>>              repo, wctx, mctx, ancestors[0], branchmerge, force, matcher,
>> -            acceptremote, followcopies)
>> +            acceptremote, followcopies, inmemoryonly)
>>          _checkunknownfiles(repo, wctx, mctx, force, actions, mergeforce)
>>
>>      else: # only when merge.preferancestor=* - the default
>> @@ -951,7 +954,7 @@
>>              repo.ui.note(_('\ncalculating bids for ancestor %s\n') % ancestor)
>>              actions, diverge1, renamedelete1 = manifestmerge(
>>                  repo, wctx, mctx, ancestor, branchmerge, force, matcher,
>> -                acceptremote, followcopies)
>> +                acceptremote, followcopies, inmemoryonly)
>>              _checkunknownfiles(repo, wctx, mctx, force, actions, mergeforce)
>>
>>              # Track the shortest set of warning on the theory that bid
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Sean Farley - Feb. 23, 2016, 9:13 p.m.
Augie Fackler <raf@durin42.com> writes:

> On Tue, Feb 23, 2016 at 12:14:00AM -0800, Tony Tung wrote:
>> # HG changeset patch
>> # User Tony Tung <tonytung@merly.org>
>> # Date 1456215137 28800
>> #      Tue Feb 23 00:12:17 2016 -0800
>> # Node ID b7f61d7438ba1aed06afa1a53a2b5fda8bdc225f
>> # Parent  cbc605f28b61cc469e21186982d0569bf94f7d2a
>> convert: execute merges in-memory (issue5076)
>
> I think this makes sense. Sean, you've done in-memory merge stuff,
> does this look sensible at least for convert to you?

My spider senses did go off with the subject line but I don't really see
what is changing here. There's a change in the method signature and a
check for it later but everything else with the merge machinery
(workingctx and its interaction with wwrite) remains untouched. Perhaps
the 'hg convert' code path takes a different route? I'm guessing Tony is
referring to merging the manifest (and not the other machinery I
mentioned)?

If so, then it seems this 'inmemoryonly' argument might be used (perhaps
by me) later for a first attempt of a pure in-memory merge.
Martin von Zweigbergk - Feb. 23, 2016, 9:32 p.m.
Tony, if you apply the patch I sent that adds dirty checking to flat
manifests, what failures do you see? Is it only covert that fails? I forgot
and I'm on mobile now.

On Tue, Feb 23, 2016, 13:13 Sean Farley <sean@farley.io> wrote:

>
> Augie Fackler <raf@durin42.com> writes:
>
> > On Tue, Feb 23, 2016 at 12:14:00AM -0800, Tony Tung wrote:
> >> # HG changeset patch
> >> # User Tony Tung <tonytung@merly.org>
> >> # Date 1456215137 28800
> >> #      Tue Feb 23 00:12:17 2016 -0800
> >> # Node ID b7f61d7438ba1aed06afa1a53a2b5fda8bdc225f
> >> # Parent  cbc605f28b61cc469e21186982d0569bf94f7d2a
> >> convert: execute merges in-memory (issue5076)
> >
> > I think this makes sense. Sean, you've done in-memory merge stuff,
> > does this look sensible at least for convert to you?
>
> My spider senses did go off with the subject line but I don't really see
> what is changing here. There's a change in the method signature and a
> check for it later but everything else with the merge machinery
> (workingctx and its interaction with wwrite) remains untouched. Perhaps
> the 'hg convert' code path takes a different route? I'm guessing Tony is
> referring to merging the manifest (and not the other machinery I
> mentioned)?
>
> If so, then it seems this 'inmemoryonly' argument might be used (perhaps
> by me) later for a first attempt of a pure in-memory merge.
>
Tony Tung - Feb. 23, 2016, 9:40 p.m.
> On Feb 23, 2016, at 1:13 PM, Sean Farley <sean@farley.io> wrote:
> 
> 
> Augie Fackler <raf@durin42.com> writes:
> 
>>> On Tue, Feb 23, 2016 at 12:14:00AM -0800, Tony Tung wrote:
>>> # HG changeset patch
>>> # User Tony Tung <tonytung@merly.org>
>>> # Date 1456215137 28800
>>> #      Tue Feb 23 00:12:17 2016 -0800
>>> # Node ID b7f61d7438ba1aed06afa1a53a2b5fda8bdc225f
>>> # Parent  cbc605f28b61cc469e21186982d0569bf94f7d2a
>>> convert: execute merges in-memory (issue5076)
>> 
>> I think this makes sense. Sean, you've done in-memory merge stuff,
>> does this look sensible at least for convert to you?
> 
> My spider senses did go off with the subject line but I don't really see
> what is changing here. There's a change in the method signature and a
> check for it later but everything else with the merge machinery
> (workingctx and its interaction with wwrite) remains untouched. Perhaps
> the 'hg convert' code path takes a different route? I'm guessing Tony is
> referring to merging the manifest (and not the other machinery I
> mentioned)?
> 
> If so, then it seems this 'inmemoryonly' argument might be used (perhaps
> by me) later for a first attempt of a pure in-memory merge.

The hg convert path will not check for dirty subrepos because the working copy remains untouched throughout the entire conversion process.

Thanks,
Tony
Tony Tung - Feb. 23, 2016, 9:41 p.m.
I don't think I saw your patch. Can you resend it?

Thanks,
Tony

On Feb 23, 2016, at 1:32 PM, Martin von Zweigbergk <martinvonz@google.com<mailto:martinvonz@google.com>> wrote:


Tony, if you apply the patch I sent that adds dirty checking to flat manifests, what failures do you see? Is it only covert that fails? I forgot and I'm on mobile now.

On Tue, Feb 23, 2016, 13:13 Sean Farley <sean@farley.io<mailto:sean@farley.io>> wrote:

Augie Fackler <raf@durin42.com<mailto:raf@durin42.com>> writes:

> On Tue, Feb 23, 2016 at 12:14:00AM -0800, Tony Tung wrote:
>> # HG changeset patch
>> # User Tony Tung <tonytung@merly.org<mailto:tonytung@merly.org>>
>> # Date 1456215137 28800
>> #      Tue Feb 23 00:12:17 2016 -0800
>> # Node ID b7f61d7438ba1aed06afa1a53a2b5fda8bdc225f
>> # Parent  cbc605f28b61cc469e21186982d0569bf94f7d2a
>> convert: execute merges in-memory (issue5076)
>
> I think this makes sense. Sean, you've done in-memory merge stuff,
> does this look sensible at least for convert to you?

My spider senses did go off with the subject line but I don't really see
what is changing here. There's a change in the method signature and a
check for it later but everything else with the merge machinery
(workingctx and its interaction with wwrite) remains untouched. Perhaps
the 'hg convert' code path takes a different route? I'm guessing Tony is
referring to merging the manifest (and not the other machinery I
mentioned)?

If so, then it seems this 'inmemoryonly' argument might be used (perhaps
by me) later for a first attempt of a pure in-memory merge.
Sean Farley - Feb. 23, 2016, 9:45 p.m.
Tony Tung <tonytung@instagram.com> writes:

>> On Feb 23, 2016, at 1:13 PM, Sean Farley <sean@farley.io> wrote:
>> 
>> 
>> Augie Fackler <raf@durin42.com> writes:
>> 
>>>> On Tue, Feb 23, 2016 at 12:14:00AM -0800, Tony Tung wrote:
>>>> # HG changeset patch
>>>> # User Tony Tung <tonytung@merly.org>
>>>> # Date 1456215137 28800
>>>> #      Tue Feb 23 00:12:17 2016 -0800
>>>> # Node ID b7f61d7438ba1aed06afa1a53a2b5fda8bdc225f
>>>> # Parent  cbc605f28b61cc469e21186982d0569bf94f7d2a
>>>> convert: execute merges in-memory (issue5076)
>>> 
>>> I think this makes sense. Sean, you've done in-memory merge stuff,
>>> does this look sensible at least for convert to you?
>> 
>> My spider senses did go off with the subject line but I don't really see
>> what is changing here. There's a change in the method signature and a
>> check for it later but everything else with the merge machinery
>> (workingctx and its interaction with wwrite) remains untouched. Perhaps
>> the 'hg convert' code path takes a different route? I'm guessing Tony is
>> referring to merging the manifest (and not the other machinery I
>> mentioned)?
>> 
>> If so, then it seems this 'inmemoryonly' argument might be used (perhaps
>> by me) later for a first attempt of a pure in-memory merge.
>
> The hg convert path will not check for dirty subrepos because the working copy remains untouched throughout the entire conversion process.

Ok, so this has nothing to do with an actual in-memory merge. Just
converting without touching the working directory.
Tony Tung - Feb. 23, 2016, 9:50 p.m.
On Feb 23, 2016, at 1:45 PM, Sean Farley <sean@farley.io> wrote:

>>> On Feb 23, 2016, at 1:13 PM, Sean Farley <sean@farley.io> wrote:
>>> 
>>> 
>>> Augie Fackler <raf@durin42.com> writes:
>>> 
>>>>> On Tue, Feb 23, 2016 at 12:14:00AM -0800, Tony Tung wrote:
>>>>> # HG changeset patch
>>>>> # User Tony Tung <tonytung@merly.org>
>>>>> # Date 1456215137 28800
>>>>> #      Tue Feb 23 00:12:17 2016 -0800
>>>>> # Node ID b7f61d7438ba1aed06afa1a53a2b5fda8bdc225f
>>>>> # Parent  cbc605f28b61cc469e21186982d0569bf94f7d2a
>>>>> convert: execute merges in-memory (issue5076)
>>>> 
>>>> I think this makes sense. Sean, you've done in-memory merge stuff,
>>>> does this look sensible at least for convert to you?
>>> 
>>> My spider senses did go off with the subject line but I don't really see
>>> what is changing here. There's a change in the method signature and a
>>> check for it later but everything else with the merge machinery
>>> (workingctx and its interaction with wwrite) remains untouched. Perhaps
>>> the 'hg convert' code path takes a different route? I'm guessing Tony is
>>> referring to merging the manifest (and not the other machinery I
>>> mentioned)?
>>> 
>>> If so, then it seems this 'inmemoryonly' argument might be used (perhaps
>>> by me) later for a first attempt of a pure in-memory merge.
>> 
>> The hg convert path will not check for dirty subrepos because the working copy remains untouched throughout the entire conversion process.
> 
> Ok, so this has nothing to do with an actual in-memory merge. Just
> converting without touching the working directory.

I assume this is in reference to a mercurial-specific concept, because otherwise I am very confused. :)

Thanks,
Tony
Martin von Zweigbergk - Feb. 23, 2016, 9:53 p.m.
I'm afraid not (at least not from mobile). I stupidly sent it as a link to
paste.debian.net or similar. I should have just sent it inline. Now that
paste has expired :-(. It simply set a flag in manifestdict.__setitem__()
and others and checked that flag in manifest. read(). You can repeat, or I
can resend when I get home in maybe an hour.

On Tue, Feb 23, 2016, 13:41 Tony Tung <tonytung@instagram.com> wrote:

> I don't think I saw your patch. Can you resend it?
>
> Thanks,
> Tony
>
> On Feb 23, 2016, at 1:32 PM, Martin von Zweigbergk <martinvonz@google.com>
> wrote:
>
> Tony, if you apply the patch I sent that adds dirty checking to flat
> manifests, what failures do you see? Is it only covert that fails? I forgot
> and I'm on mobile now.
>
> On Tue, Feb 23, 2016, 13:13 Sean Farley <sean@farley.io> wrote:
>
>>
>> Augie Fackler <raf@durin42.com> writes:
>>
>> > On Tue, Feb 23, 2016 at 12:14:00AM -0800, Tony Tung wrote:
>> >> # HG changeset patch
>> >> # User Tony Tung <tonytung@merly.org>
>> >> # Date 1456215137 28800
>> >> #      Tue Feb 23 00:12:17 2016 -0800
>> >> # Node ID b7f61d7438ba1aed06afa1a53a2b5fda8bdc225f
>> >> # Parent  cbc605f28b61cc469e21186982d0569bf94f7d2a
>> >> convert: execute merges in-memory (issue5076)
>> >
>> > I think this makes sense. Sean, you've done in-memory merge stuff,
>> > does this look sensible at least for convert to you?
>>
>> My spider senses did go off with the subject line but I don't really see
>> what is changing here. There's a change in the method signature and a
>> check for it later but everything else with the merge machinery
>> (workingctx and its interaction with wwrite) remains untouched. Perhaps
>> the 'hg convert' code path takes a different route? I'm guessing Tony is
>> referring to merging the manifest (and not the other machinery I
>> mentioned)?
>>
>> If so, then it seems this 'inmemoryonly' argument might be used (perhaps
>> by me) later for a first attempt of a pure in-memory merge.
>>
>
Sean Farley - Feb. 23, 2016, 10:03 p.m.
Tony Tung <tonytung@instagram.com> writes:

> On Feb 23, 2016, at 1:45 PM, Sean Farley <sean@farley.io> wrote:
>
>>>> On Feb 23, 2016, at 1:13 PM, Sean Farley <sean@farley.io> wrote:
>>>> 
>>>> 
>>>> Augie Fackler <raf@durin42.com> writes:
>>>> 
>>>>>> On Tue, Feb 23, 2016 at 12:14:00AM -0800, Tony Tung wrote:
>>>>>> # HG changeset patch
>>>>>> # User Tony Tung <tonytung@merly.org>
>>>>>> # Date 1456215137 28800
>>>>>> #      Tue Feb 23 00:12:17 2016 -0800
>>>>>> # Node ID b7f61d7438ba1aed06afa1a53a2b5fda8bdc225f
>>>>>> # Parent  cbc605f28b61cc469e21186982d0569bf94f7d2a
>>>>>> convert: execute merges in-memory (issue5076)
>>>>> 
>>>>> I think this makes sense. Sean, you've done in-memory merge stuff,
>>>>> does this look sensible at least for convert to you?
>>>> 
>>>> My spider senses did go off with the subject line but I don't really see
>>>> what is changing here. There's a change in the method signature and a
>>>> check for it later but everything else with the merge machinery
>>>> (workingctx and its interaction with wwrite) remains untouched. Perhaps
>>>> the 'hg convert' code path takes a different route? I'm guessing Tony is
>>>> referring to merging the manifest (and not the other machinery I
>>>> mentioned)?
>>>> 
>>>> If so, then it seems this 'inmemoryonly' argument might be used (perhaps
>>>> by me) later for a first attempt of a pure in-memory merge.
>>> 
>>> The hg convert path will not check for dirty subrepos because the working copy remains untouched throughout the entire conversion process.
>> 
>> Ok, so this has nothing to do with an actual in-memory merge. Just
>> converting without touching the working directory.
>
> I assume this is in reference to a mercurial-specific concept, because otherwise I am very confused. :)

Your description is:

+    inmemoryonly = merge is happening in memory only; wctx does not
+                   represent an actual working directory.  an example
+                   of this would be repository conversion (convert extension).

But that only works for the convert extension. Issuing a 'hg merge' will
call merge.update which will apply all the repo.wwrite (writing to the
working directory).
Tony Tung - Feb. 23, 2016, 10:17 p.m.
> On Feb 23, 2016, at 2:03 PM, Sean Farley <sean@farley.io> wrote:
> 
> 
> Tony Tung <tonytung@instagram.com> writes:
> 
>> On Feb 23, 2016, at 1:45 PM, Sean Farley <sean@farley.io> wrote:
>> 
>>>>> On Feb 23, 2016, at 1:13 PM, Sean Farley <sean@farley.io> wrote:
>>>>> 
>>>>> 
>>>>> Augie Fackler <raf@durin42.com> writes:
>>>>> 
>>>>>>> On Tue, Feb 23, 2016 at 12:14:00AM -0800, Tony Tung wrote:
>>>>>>> # HG changeset patch
>>>>>>> # User Tony Tung <tonytung@merly.org>
>>>>>>> # Date 1456215137 28800
>>>>>>> #      Tue Feb 23 00:12:17 2016 -0800
>>>>>>> # Node ID b7f61d7438ba1aed06afa1a53a2b5fda8bdc225f
>>>>>>> # Parent  cbc605f28b61cc469e21186982d0569bf94f7d2a
>>>>>>> convert: execute merges in-memory (issue5076)
>>>>>> 
>>>>>> I think this makes sense. Sean, you've done in-memory merge stuff,
>>>>>> does this look sensible at least for convert to you?
>>>>> 
>>>>> My spider senses did go off with the subject line but I don't really see
>>>>> what is changing here. There's a change in the method signature and a
>>>>> check for it later but everything else with the merge machinery
>>>>> (workingctx and its interaction with wwrite) remains untouched. Perhaps
>>>>> the 'hg convert' code path takes a different route? I'm guessing Tony is
>>>>> referring to merging the manifest (and not the other machinery I
>>>>> mentioned)?
>>>>> 
>>>>> If so, then it seems this 'inmemoryonly' argument might be used (perhaps
>>>>> by me) later for a first attempt of a pure in-memory merge.
>>>> 
>>>> The hg convert path will not check for dirty subrepos because the working copy remains untouched throughout the entire conversion process.
>>> 
>>> Ok, so this has nothing to do with an actual in-memory merge. Just
>>> converting without touching the working directory.
>> 
>> I assume this is in reference to a mercurial-specific concept, because otherwise I am very confused. :)
> 
> Your description is:
> 
> +    inmemoryonly = merge is happening in memory only; wctx does not
> +                   represent an actual working directory.  an example
> +                   of this would be repository conversion (convert extension).
> 
> But that only works for the convert extension. Issuing a 'hg merge' will
> call merge.update which will apply all the repo.wwrite (writing to the
> working directory).

Right, so inmemoryonly shouldn't be set from 'hg merge'.

(I have a feeling we're talking past each other; feel free to ping me off the mailing list if you feel the same.)

Thanks,
Tony
Sean Farley - Feb. 23, 2016, 10:26 p.m.
Tony Tung <tonytung@instagram.com> writes:

>> On Feb 23, 2016, at 2:03 PM, Sean Farley <sean@farley.io> wrote:
>> 
>> 
>> Tony Tung <tonytung@instagram.com> writes:
>> 
>>> On Feb 23, 2016, at 1:45 PM, Sean Farley <sean@farley.io> wrote:
>>> 
>>>>>> On Feb 23, 2016, at 1:13 PM, Sean Farley <sean@farley.io> wrote:
>>>>>> 
>>>>>> 
>>>>>> Augie Fackler <raf@durin42.com> writes:
>>>>>> 
>>>>>>>> On Tue, Feb 23, 2016 at 12:14:00AM -0800, Tony Tung wrote:
>>>>>>>> # HG changeset patch
>>>>>>>> # User Tony Tung <tonytung@merly.org>
>>>>>>>> # Date 1456215137 28800
>>>>>>>> #      Tue Feb 23 00:12:17 2016 -0800
>>>>>>>> # Node ID b7f61d7438ba1aed06afa1a53a2b5fda8bdc225f
>>>>>>>> # Parent  cbc605f28b61cc469e21186982d0569bf94f7d2a
>>>>>>>> convert: execute merges in-memory (issue5076)
>>>>>>> 
>>>>>>> I think this makes sense. Sean, you've done in-memory merge stuff,
>>>>>>> does this look sensible at least for convert to you?
>>>>>> 
>>>>>> My spider senses did go off with the subject line but I don't really see
>>>>>> what is changing here. There's a change in the method signature and a
>>>>>> check for it later but everything else with the merge machinery
>>>>>> (workingctx and its interaction with wwrite) remains untouched. Perhaps
>>>>>> the 'hg convert' code path takes a different route? I'm guessing Tony is
>>>>>> referring to merging the manifest (and not the other machinery I
>>>>>> mentioned)?
>>>>>> 
>>>>>> If so, then it seems this 'inmemoryonly' argument might be used (perhaps
>>>>>> by me) later for a first attempt of a pure in-memory merge.
>>>>> 
>>>>> The hg convert path will not check for dirty subrepos because the working copy remains untouched throughout the entire conversion process.
>>>> 
>>>> Ok, so this has nothing to do with an actual in-memory merge. Just
>>>> converting without touching the working directory.
>>> 
>>> I assume this is in reference to a mercurial-specific concept, because otherwise I am very confused. :)
>> 
>> Your description is:
>> 
>> +    inmemoryonly = merge is happening in memory only; wctx does not
>> +                   represent an actual working directory.  an example
>> +                   of this would be repository conversion (convert extension).
>> 
>> But that only works for the convert extension. Issuing a 'hg merge' will
>> call merge.update which will apply all the repo.wwrite (writing to the
>> working directory).
>
> Right, so inmemoryonly shouldn't be set from 'hg merge'.
>
> (I have a feeling we're talking past each other; feel free to ping me off the mailing list if you feel the same.)

Yeah, this was just me walking through the patch and seeing how it
related to my work on in-memory merging. Anyway, the patch looks to me.
Martin von Zweigbergk - Feb. 25, 2016, 6:33 p.m.
On Tue, Feb 23, 2016 at 11:51 AM, Martin von Zweigbergk
<martinvonz@google.com> wrote:
> On Tue, Feb 23, 2016 at 11:30 AM, Augie Fackler <raf@durin42.com> wrote:
>> On Tue, Feb 23, 2016 at 12:14:00AM -0800, Tony Tung wrote:
>>> # HG changeset patch
>>> # User Tony Tung <tonytung@merly.org>
>>> # Date 1456215137 28800
>>> #      Tue Feb 23 00:12:17 2016 -0800
>>> # Node ID b7f61d7438ba1aed06afa1a53a2b5fda8bdc225f
>>> # Parent  cbc605f28b61cc469e21186982d0569bf94f7d2a
>>> convert: execute merges in-memory (issue5076)
>>
>> I think this makes sense. Sean, you've done in-memory merge stuff,
>> does this look sensible at least for convert to you?
>>
>> Martin, does this make sense to you? You reviewed the last round.
>
> I was hoping we could instead do "m1 = m1.copy()" to avoid modifying
> the manifest instance in the cache (I just sent out a little patch
> that makes that simpler to do). Unfortunately, a lot of tests break if
> we do that. Tony, do you think that makes sense? Do you have time to
> see if you can get that to work?

Okay, I've wasted another day or so trying to fix subrepo merge without
success :-(

So for now, I'd say we do the simplest possible. I think you can even drop
the extra argument ("inmemoryonly") and just check "wctx.rev() is None"
instead. Please add back your test case too (but not in test-treemanifest.t,
as we talked about).

For the record, here's what I'd like to happen to subrepo merge:

 * One can not currently (I think) re-resolve a subrepo conflict, which
   suggests it should be moved into a tool (thanks to Sid for the idea)
 * Move more of the subrepo merge logic out of merge.py and into submerge.
   - Make submerge treat an empty .hgsubstate just like a missing one. When
     the resulting merge is an empty .hgsubstate, remove it. Return a
     "deleted" flag to the caller (this lines up well with the item about
     moving it into a tool). For example, merging deletions of two subrepos
     on different branches can result in an empty file, which we probably
     don't handle today.
   - Don't calculate actions for .hgsubstate in manifestmerge(). Perhaps
     limit it to whether there were any changes at all and then hand off to
     submerge(). This lets us remove the added '+', which this patch is
     about. There is also some duplication around these checks already in
     submerge().

Ideally someone else will take care of this :-). I don't care about subrepos
myself, but I do care about merge.
Tony Tung - Feb. 25, 2016, 9:59 p.m.
> On Feb 25, 2016, at 10:33 AM, Martin von Zweigbergk <martinvonz@google.com> wrote:
> 
> On Tue, Feb 23, 2016 at 11:51 AM, Martin von Zweigbergk
> <martinvonz@google.com> wrote:
>> On Tue, Feb 23, 2016 at 11:30 AM, Augie Fackler <raf@durin42.com> wrote:
>>> On Tue, Feb 23, 2016 at 12:14:00AM -0800, Tony Tung wrote:
>>>> # HG changeset patch
>>>> # User Tony Tung <tonytung@merly.org>
>>>> # Date 1456215137 28800
>>>> #      Tue Feb 23 00:12:17 2016 -0800
>>>> # Node ID b7f61d7438ba1aed06afa1a53a2b5fda8bdc225f
>>>> # Parent  cbc605f28b61cc469e21186982d0569bf94f7d2a
>>>> convert: execute merges in-memory (issue5076)
>>> 
>>> I think this makes sense. Sean, you've done in-memory merge stuff,
>>> does this look sensible at least for convert to you?
>>> 
>>> Martin, does this make sense to you? You reviewed the last round.
>> 
>> I was hoping we could instead do "m1 = m1.copy()" to avoid modifying
>> the manifest instance in the cache (I just sent out a little patch
>> that makes that simpler to do). Unfortunately, a lot of tests break if
>> we do that. Tony, do you think that makes sense? Do you have time to
>> see if you can get that to work?
> 
> Okay, I've wasted another day or so trying to fix subrepo merge without
> success :-(
> 
> So for now, I'd say we do the simplest possible. I think you can even drop
> the extra argument ("inmemoryonly") and just check "wctx.rev() is None"
> instead. Please add back your test case too (but not in test-treemanifest.t,
> as we talked about).

Is that really the case?  As I understand it, you can convert a repo, and subsequently convert more revs.  If that is the case, your destination may not always be at rev id null.  I think the inmemoryonly parameter is needed.

Thanks,
Tony
Martin von Zweigbergk - Feb. 26, 2016, 12:06 a.m.
On Thu, Feb 25, 2016 at 1:59 PM, Tony Tung <tonytung@instagram.com> wrote:
>
>> On Feb 25, 2016, at 10:33 AM, Martin von Zweigbergk <martinvonz@google.com> wrote:
>>
>> On Tue, Feb 23, 2016 at 11:51 AM, Martin von Zweigbergk
>> <martinvonz@google.com> wrote:
>>> On Tue, Feb 23, 2016 at 11:30 AM, Augie Fackler <raf@durin42.com> wrote:
>>>> On Tue, Feb 23, 2016 at 12:14:00AM -0800, Tony Tung wrote:
>>>>> # HG changeset patch
>>>>> # User Tony Tung <tonytung@merly.org>
>>>>> # Date 1456215137 28800
>>>>> #      Tue Feb 23 00:12:17 2016 -0800
>>>>> # Node ID b7f61d7438ba1aed06afa1a53a2b5fda8bdc225f
>>>>> # Parent  cbc605f28b61cc469e21186982d0569bf94f7d2a
>>>>> convert: execute merges in-memory (issue5076)
>>>>
>>>> I think this makes sense. Sean, you've done in-memory merge stuff,
>>>> does this look sensible at least for convert to you?
>>>>
>>>> Martin, does this make sense to you? You reviewed the last round.
>>>
>>> I was hoping we could instead do "m1 = m1.copy()" to avoid modifying
>>> the manifest instance in the cache (I just sent out a little patch
>>> that makes that simpler to do). Unfortunately, a lot of tests break if
>>> we do that. Tony, do you think that makes sense? Do you have time to
>>> see if you can get that to work?
>>
>> Okay, I've wasted another day or so trying to fix subrepo merge without
>> success :-(
>>
>> So for now, I'd say we do the simplest possible. I think you can even drop
>> the extra argument ("inmemoryonly") and just check "wctx.rev() is None"
>> instead. Please add back your test case too (but not in test-treemanifest.t,
>> as we talked about).
>
> Is that really the case?  As I understand it, you can convert a repo, and subsequently convert more revs.  If that is the case, your destination may not always be at rev id null.  I think the inmemoryonly parameter is needed.

I don't follow. I meant to replace "*not* inmemoryonly" by "wctx.rev()
is None". Does that clarify?
Pierre-Yves David - March 7, 2016, 3:13 p.m.
This is marked as "Under Review", assigned to Martin Von Zweigbergk for 
about two weeks. What's the status of it?
Tony Tung - March 7, 2016, 7:02 p.m.
This is my bad.  I need to follow up on Martin’s comments, but haven’t had a chance to.

Thanks,
Tony

> On Mar 7, 2016, at 7:13 AM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:

> 

> This is marked as "Under Review", assigned to Martin Von Zweigbergk for 

> about two weeks. What's the status of it?

> 

> -- 

> Pierre-Yves David

Patch

diff --git a/hgext/convert/hg.py b/hgext/convert/hg.py
--- a/hgext/convert/hg.py
+++ b/hgext/convert/hg.py
@@ -194,6 +194,7 @@ 
             True,  # force
             False, # acceptremote
             False, # followcopies
+            inmemoryonly=True,
         )
 
         for file, (action, info, msg) in actions.iteritems():
diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -769,13 +769,16 @@ 
     return True
 
 def manifestmerge(repo, wctx, p2, pa, branchmerge, force, matcher,
-                  acceptremote, followcopies):
+                  acceptremote, followcopies, inmemoryonly):
     """
     Merge p1 and p2 with ancestor pa and generate merge action list
 
     branchmerge and force are as passed in to update
     matcher = matcher to filter file lists
     acceptremote = accept the incoming changes without prompting
+    inmemoryonly = merge is happening in memory only; wctx does not
+                   represent an actual working directory.  an example
+                   of this would be repository conversion (convert extension).
     """
     if matcher is not None and matcher.always():
         matcher = None
@@ -799,7 +802,7 @@ 
     copied = set(copy.values())
     copied.update(movewithdir.values())
 
-    if '.hgsubstate' in m1:
+    if not inmemoryonly and '.hgsubstate' in m1:
         # check whether sub state is modified
         for s in sorted(wctx.substate):
             if wctx.sub(s).dirty():
@@ -931,12 +934,12 @@ 
 
 def calculateupdates(repo, wctx, mctx, ancestors, branchmerge, force,
                      acceptremote, followcopies, matcher=None,
-                     mergeforce=False):
+                     mergeforce=False, inmemoryonly=False):
     "Calculate the actions needed to merge mctx into wctx using ancestors"
     if len(ancestors) == 1: # default
         actions, diverge, renamedelete = manifestmerge(
             repo, wctx, mctx, ancestors[0], branchmerge, force, matcher,
-            acceptremote, followcopies)
+            acceptremote, followcopies, inmemoryonly)
         _checkunknownfiles(repo, wctx, mctx, force, actions, mergeforce)
 
     else: # only when merge.preferancestor=* - the default
@@ -951,7 +954,7 @@ 
             repo.ui.note(_('\ncalculating bids for ancestor %s\n') % ancestor)
             actions, diverge1, renamedelete1 = manifestmerge(
                 repo, wctx, mctx, ancestor, branchmerge, force, matcher,
-                acceptremote, followcopies)
+                acceptremote, followcopies, inmemoryonly)
             _checkunknownfiles(repo, wctx, mctx, force, actions, mergeforce)
 
             # Track the shortest set of warning on the theory that bid