Patchwork subrepo: move prompts out of the if (issue5505)

login
register
mail settings
Submitter Simon Farnsworth
Date March 17, 2017, 8:10 p.m.
Message ID <0ccc212fc0224209864e.1489781457@devvm022.lla2.facebook.com>
Download mbox | patch
Permalink /patch/19426/
State Changes Requested
Headers show

Comments

Simon Farnsworth - March 17, 2017, 8:10 p.m.
# HG changeset patch
# User Simon Farnsworth <simonfar@fb.com>
# Date 1489781443 25200
#      Fri Mar 17 13:10:43 2017 -0700
# Node ID 0ccc212fc0224209864e8c902920a91acd1bc414
# Parent  a5bad127128d8f60060be53d161acfa7a32a17d5
subrepo: move prompts out of the if (issue5505)

Prompts weren't available in the else clause
Simon Farnsworth - March 17, 2017, 8:18 p.m.
On 17/03/2017 13:10, Simon Farnsworth wrote:
<snip>
> diff --git a/tests/test-subrepo.t b/tests/test-subrepo.t
> --- a/tests/test-subrepo.t
> +++ b/tests/test-subrepo.t
> @@ -349,7 +349,7 @@
>  local removed, remote changed, keep changed
>
>    $ hg merge 6
> -   remote [merge rev] changed subrepository s which local [working copy] removed
> +   remote [merge rev] changed subrepository t which local [working copy] removed
These change because for some reason, the subrepo prompted for was wrong 
before. I'm not clear on why the code motion affected this, and this 
should motivate extra review from someone who's interested in subrepo 
support.
Katsunori FUJIWARA - March 18, 2017, 12:28 a.m.
At Fri, 17 Mar 2017 13:10:57 -0700,
Simon Farnsworth wrote:
> 
> # HG changeset patch
> # User Simon Farnsworth <simonfar@fb.com>
> # Date 1489781443 25200
> #      Fri Mar 17 13:10:43 2017 -0700
> # Node ID 0ccc212fc0224209864e8c902920a91acd1bc414
> # Parent  a5bad127128d8f60060be53d161acfa7a32a17d5
> subrepo: move prompts out of the if (issue5505)
> 
> Prompts weren't available in the else clause
> 
> diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
> --- a/mercurial/subrepo.py
> +++ b/mercurial/subrepo.py
> @@ -195,6 +195,7 @@
>              r = "%s:%s:%s" % r
>          repo.ui.debug("  subrepo %s: %s %s\n" % (s, msg, r))
>  
> +    prompts = filemerge.partextras(labels)
>      for s, l in sorted(s1.iteritems()):
>          a = sa.get(s, nullstate)
>          ld = l # local state with possible dirty flag for compares
> @@ -203,9 +204,8 @@
>          if wctx == actx: # overwrite
>              a = ld
>  
> +        prompts['s'] = s
>          if s in s2:
> -            prompts = filemerge.partextras(labels)
> -            prompts['s'] = s
>              r = s2[s]
>              if ld == r or r == a: # no change or local is newer
>                  sm[s] = l
> @@ -275,6 +275,7 @@
>              mctx.sub(s).get(r)
>              sm[s] = r
>          elif r != sa[s]:
> +            prompts['s'] = s
>              if repo.ui.promptchoice(
>                  _(' remote%(o)s changed subrepository %(s)s'
>                    ' which local%(l)s removed\n'

How about copying before modification at each repetitions ?

    promptssrc = filemerge.partextras(labels)
    for s, l in sorted(s1.iteritems()):
        ....
        prompts = promptssrc.copy()
        prompts['s'] = s

    ....
    for s, r in sorted(s2.items()):
        ....
        elif r != sa[s]:
            prompts = promptssrc.copy()
            prompts['s'] = s


("del prompts" before s2 loop for safety is over-killing ? :-) )

Sharing result of filemerge.partextras() between each repetitions and
modifying it directly might cause wrong information, which is assigned
at previous repetition, like below.

In this case, shallow copy seems cheap enough.

> diff --git a/tests/test-subrepo.t b/tests/test-subrepo.t
> --- a/tests/test-subrepo.t
> +++ b/tests/test-subrepo.t
> @@ -349,7 +349,7 @@
>  local removed, remote changed, keep changed
>  
>    $ hg merge 6
> -   remote [merge rev] changed subrepository s which local [working copy] removed
> +   remote [merge rev] changed subrepository t which local [working copy] removed
>    use (c)hanged version or (d)elete? c
>    0 files updated, 0 files merged, 0 files removed, 0 files unresolved
>    (branch merge, don't forget to commit)
> @@ -380,7 +380,7 @@
>    $ hg merge --config ui.interactive=true 6 <<EOF
>    > d
>    > EOF
> -   remote [merge rev] changed subrepository s which local [working copy] removed
> +   remote [merge rev] changed subrepository t which local [working copy] removed
>    use (c)hanged version or (d)elete? d
>    0 files updated, 0 files merged, 0 files removed, 0 files unresolved
>    (branch merge, don't forget to commit)

According to "hg debugsub" output in test-subrepo.t, merging revision
6 into revision 11 decreases managed subrepos from "s, t" to "s".

  $ hg ci -m6 # change sub
  committing subrepository t
  $ hg debugsub
  path s
   source   s
   revision e4ece1bf43360ddc8f6a96432201a37b7cd27ae4
  path t
   source   t
   revision 6747d179aa9a688023c4b0cad32e4c92bb7f34ad

  ....

  $ hg ci -m11
  created new head
  $ hg debugsub
  path s
   source   s
   revision e4ece1bf43360ddc8f6a96432201a37b7cd27ae4

Therefore, subrepo removed on "local" side is not "s" but "t" at this
merging, and changes (or fixing wrong expectations) above seem
reasonable.


> @@ -404,7 +404,7 @@
>    $ hg co -C 6
>    2 files updated, 0 files merged, 0 files removed, 0 files unresolved
>    $ hg merge 11
> -   local [working copy] changed subrepository s which remote [merge rev] removed
> +   local [working copy] changed subrepository t which remote [merge rev] removed
>    use (c)hanged version or (d)elete? c
>    1 files updated, 0 files merged, 0 files removed, 0 files unresolved
>    (branch merge, don't forget to commit)
> @@ -436,7 +436,7 @@
>    $ hg merge --config ui.interactive=true 11 <<EOF
>    > d
>    > EOF
> -   local [working copy] changed subrepository s which remote [merge rev] removed
> +   local [working copy] changed subrepository t which remote [merge rev] removed
>    use (c)hanged version or (d)elete? d
>    1 files updated, 0 files merged, 0 files removed, 0 files unresolved
>    (branch merge, don't forget to commit)

On the other hand, merging revision 11 into revision 6 increases
managed subrepos from "s" to "s, t".

Therefore, subrepo removed on "remote" side is not "s" but "t" at this
merging, and changes above seem reasonable, too.


> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Simon Farnsworth - March 18, 2017, 12:31 a.m.
On 17/03/2017 17:28, FUJIWARA Katsunori wrote:
> At Fri, 17 Mar 2017 13:10:57 -0700,
> Simon Farnsworth wrote:
>>
>> # HG changeset patch
>> # User Simon Farnsworth <simonfar@fb.com>
>> # Date 1489781443 25200
>> #      Fri Mar 17 13:10:43 2017 -0700
>> # Node ID 0ccc212fc0224209864e8c902920a91acd1bc414
>> # Parent  a5bad127128d8f60060be53d161acfa7a32a17d5
>> subrepo: move prompts out of the if (issue5505)
>>
>> Prompts weren't available in the else clause
>>
>> diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
>> --- a/mercurial/subrepo.py
>> +++ b/mercurial/subrepo.py
>> @@ -195,6 +195,7 @@
>>              r = "%s:%s:%s" % r
>>          repo.ui.debug("  subrepo %s: %s %s\n" % (s, msg, r))
>>
>> +    prompts = filemerge.partextras(labels)
>>      for s, l in sorted(s1.iteritems()):
>>          a = sa.get(s, nullstate)
>>          ld = l # local state with possible dirty flag for compares
>> @@ -203,9 +204,8 @@
>>          if wctx == actx: # overwrite
>>              a = ld
>>
>> +        prompts['s'] = s
>>          if s in s2:
>> -            prompts = filemerge.partextras(labels)
>> -            prompts['s'] = s
>>              r = s2[s]
>>              if ld == r or r == a: # no change or local is newer
>>                  sm[s] = l
>> @@ -275,6 +275,7 @@
>>              mctx.sub(s).get(r)
>>              sm[s] = r
>>          elif r != sa[s]:
>> +            prompts['s'] = s
>>              if repo.ui.promptchoice(
>>                  _(' remote%(o)s changed subrepository %(s)s'
>>                    ' which local%(l)s removed\n'
>
> How about copying before modification at each repetitions ?
>
>     promptssrc = filemerge.partextras(labels)
>     for s, l in sorted(s1.iteritems()):
>         ....
>         prompts = promptssrc.copy()
>         prompts['s'] = s
>
>     ....
>     for s, r in sorted(s2.items()):
>         ....
>         elif r != sa[s]:
>             prompts = promptssrc.copy()
>             prompts['s'] = s
>
>
> ("del prompts" before s2 loop for safety is over-killing ? :-) )
>
> Sharing result of filemerge.partextras() between each repetitions and
> modifying it directly might cause wrong information, which is assigned
> at previous repetition, like below.
>
> In this case, shallow copy seems cheap enough.
>

That seems like a good idea - del prompts does seem like overkill, but 
ending each repetition with prompts = None would ensure that the bug I 
introduced into the test case is caught. I'll do it in a v2 when I'm 
back in the UK.

>> diff --git a/tests/test-subrepo.t b/tests/test-subrepo.t
>> --- a/tests/test-subrepo.t
>> +++ b/tests/test-subrepo.t
>> @@ -349,7 +349,7 @@
>>  local removed, remote changed, keep changed
>>
>>    $ hg merge 6
>> -   remote [merge rev] changed subrepository s which local [working copy] removed
>> +   remote [merge rev] changed subrepository t which local [working copy] removed
>>    use (c)hanged version or (d)elete? c
>>    0 files updated, 0 files merged, 0 files removed, 0 files unresolved
>>    (branch merge, don't forget to commit)
>> @@ -380,7 +380,7 @@
>>    $ hg merge --config ui.interactive=true 6 <<EOF
>>    > d
>>    > EOF
>> -   remote [merge rev] changed subrepository s which local [working copy] removed
>> +   remote [merge rev] changed subrepository t which local [working copy] removed
>>    use (c)hanged version or (d)elete? d
>>    0 files updated, 0 files merged, 0 files removed, 0 files unresolved
>>    (branch merge, don't forget to commit)
>
> According to "hg debugsub" output in test-subrepo.t, merging revision
> 6 into revision 11 decreases managed subrepos from "s, t" to "s".
>
>   $ hg ci -m6 # change sub
>   committing subrepository t
>   $ hg debugsub
>   path s
>    source   s
>    revision e4ece1bf43360ddc8f6a96432201a37b7cd27ae4
>   path t
>    source   t
>    revision 6747d179aa9a688023c4b0cad32e4c92bb7f34ad
>
>   ....
>
>   $ hg ci -m11
>   created new head
>   $ hg debugsub
>   path s
>    source   s
>    revision e4ece1bf43360ddc8f6a96432201a37b7cd27ae4
>
> Therefore, subrepo removed on "local" side is not "s" but "t" at this
> merging, and changes (or fixing wrong expectations) above seem
> reasonable.
>
>
>> @@ -404,7 +404,7 @@
>>    $ hg co -C 6
>>    2 files updated, 0 files merged, 0 files removed, 0 files unresolved
>>    $ hg merge 11
>> -   local [working copy] changed subrepository s which remote [merge rev] removed
>> +   local [working copy] changed subrepository t which remote [merge rev] removed
>>    use (c)hanged version or (d)elete? c
>>    1 files updated, 0 files merged, 0 files removed, 0 files unresolved
>>    (branch merge, don't forget to commit)
>> @@ -436,7 +436,7 @@
>>    $ hg merge --config ui.interactive=true 11 <<EOF
>>    > d
>>    > EOF
>> -   local [working copy] changed subrepository s which remote [merge rev] removed
>> +   local [working copy] changed subrepository t which remote [merge rev] removed
>>    use (c)hanged version or (d)elete? d
>>    1 files updated, 0 files merged, 0 files removed, 0 files unresolved
>>    (branch merge, don't forget to commit)
>
> On the other hand, merging revision 11 into revision 6 increases
> managed subrepos from "s" to "s, t".
>
> Therefore, subrepo removed on "remote" side is not "s" but "t" at this
> merging, and changes above seem reasonable, too.
>
>
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=DwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=mEgSWILcY4c4W3zjApBQLA&m=5FUTH1DZr0EUZ3iiFiObxZR7K0xxDGibYVF3AiXl7Ss&s=AZxsqg6MSSCqsT5_aQsG9EXABcdybWAgD6F12hta4iE&e=
>
Yuya Nishihara - March 18, 2017, 5:06 a.m.
On Fri, 17 Mar 2017 13:18:35 -0700, Simon Farnsworth wrote:
> On 17/03/2017 13:10, Simon Farnsworth wrote:
> <snip>
> > diff --git a/tests/test-subrepo.t b/tests/test-subrepo.t
> > --- a/tests/test-subrepo.t
> > +++ b/tests/test-subrepo.t
> > @@ -349,7 +349,7 @@
> >  local removed, remote changed, keep changed
> >
> >    $ hg merge 6
> > -   remote [merge rev] changed subrepository s which local [working copy] removed
> > +   remote [merge rev] changed subrepository t which local [working copy] removed
> These change because for some reason, the subrepo prompted for was wrong 
> before. I'm not clear on why the code motion affected this, and this 
> should motivate extra review from someone who's interested in subrepo 
> support.

Perhaps the remote repo name (s of s2.items()) wasn't set before and
old prompts['s'] would be used instead.

Can you rebase this to stable branch and make V2 per foozy's comment?

Patch

diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
--- a/mercurial/subrepo.py
+++ b/mercurial/subrepo.py
@@ -195,6 +195,7 @@ 
             r = "%s:%s:%s" % r
         repo.ui.debug("  subrepo %s: %s %s\n" % (s, msg, r))
 
+    prompts = filemerge.partextras(labels)
     for s, l in sorted(s1.iteritems()):
         a = sa.get(s, nullstate)
         ld = l # local state with possible dirty flag for compares
@@ -203,9 +204,8 @@ 
         if wctx == actx: # overwrite
             a = ld
 
+        prompts['s'] = s
         if s in s2:
-            prompts = filemerge.partextras(labels)
-            prompts['s'] = s
             r = s2[s]
             if ld == r or r == a: # no change or local is newer
                 sm[s] = l
@@ -275,6 +275,7 @@ 
             mctx.sub(s).get(r)
             sm[s] = r
         elif r != sa[s]:
+            prompts['s'] = s
             if repo.ui.promptchoice(
                 _(' remote%(o)s changed subrepository %(s)s'
                   ' which local%(l)s removed\n'
diff --git a/tests/test-subrepo.t b/tests/test-subrepo.t
--- a/tests/test-subrepo.t
+++ b/tests/test-subrepo.t
@@ -349,7 +349,7 @@ 
 local removed, remote changed, keep changed
 
   $ hg merge 6
-   remote [merge rev] changed subrepository s which local [working copy] removed
+   remote [merge rev] changed subrepository t which local [working copy] removed
   use (c)hanged version or (d)elete? c
   0 files updated, 0 files merged, 0 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
@@ -380,7 +380,7 @@ 
   $ hg merge --config ui.interactive=true 6 <<EOF
   > d
   > EOF
-   remote [merge rev] changed subrepository s which local [working copy] removed
+   remote [merge rev] changed subrepository t which local [working copy] removed
   use (c)hanged version or (d)elete? d
   0 files updated, 0 files merged, 0 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
@@ -404,7 +404,7 @@ 
   $ hg co -C 6
   2 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ hg merge 11
-   local [working copy] changed subrepository s which remote [merge rev] removed
+   local [working copy] changed subrepository t which remote [merge rev] removed
   use (c)hanged version or (d)elete? c
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
@@ -436,7 +436,7 @@ 
   $ hg merge --config ui.interactive=true 11 <<EOF
   > d
   > EOF
-   local [working copy] changed subrepository s which remote [merge rev] removed
+   local [working copy] changed subrepository t which remote [merge rev] removed
   use (c)hanged version or (d)elete? d
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   (branch merge, don't forget to commit)