Patchwork D7705: phases: make the working directory consistently a draft

login
register
mail settings
Submitter phabricator
Date Dec. 19, 2019, 8:34 a.m.
Message ID <differential-rev-PHID-DREV-cbjlamcprgoxtrwioo2a-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/43991/
State Superseded
Headers show

Comments

phabricator - Dec. 19, 2019, 8:34 a.m.
rdamazio created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Before this change, `hg log -r 'wdir() and public()'` would return it.

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D7705

AFFECTED FILES
  mercurial/phases.py
  tests/test-phases.t

CHANGE DETAILS




To: rdamazio, #hg-reviewers
Cc: mercurial-devel
phabricator - Dec. 19, 2019, 1:25 p.m.
marmoute added a comment.


  The working copy is not necessarly draft phase. For example, it will be secret is the working copy parent is secret. One can also control it using the `phases.new-commit` config.
  
  The `mercurial.phases.newcommitphase(ui)` function can help you there.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7705/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7705

To: rdamazio, #hg-reviewers
Cc: marmoute, mercurial-devel
phabricator - Dec. 24, 2019, 2:05 a.m.
rdamazio added a comment.


  In D7705#113383 <https://phab.mercurial-scm.org/D7705#113383>, @marmoute wrote:
  
  > The working copy is not necessarly draft phase. For example, it will be secret is the working copy parent is secret. One can also control it using the `phases.new-commit` config.
  > The `mercurial.phases.newcommitphase(ui)` function can help you there.
  
  Thanks.
  About `newcommitphase` - that only accounts for the config option, and it seems that the working directory doesn't change phase based on that in other parts of the code (e.g. in `commitablectx`), so I kept that consistent here.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7705/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7705

To: rdamazio, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - Dec. 27, 2019, 4:01 p.m.
marmoute added a comment.


  In D7705#113667 <https://phab.mercurial-scm.org/D7705#113667>, @rdamazio wrote:
  
  > In D7705#113383 <https://phab.mercurial-scm.org/D7705#113383>, @marmoute wrote:
  >
  >> The working copy is not necessarly draft phase. For example, it will be secret is the working copy parent is secret. One can also control it using the `phases.new-commit` config.
  >> The `mercurial.phases.newcommitphase(ui)` function can help you there.
  >
  > Thanks.
  > About `newcommitphase` - that only accounts for the config option, and it seems that the working directory doesn't change phase based on that in other parts of the code (e.g. in `commitablectx`), so I kept that consistent here.
  
  `commitablectx` code seems wrong here. I am sending a patch.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7705/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7705

To: rdamazio, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - Dec. 27, 2019, 8:42 p.m.
rdamazio added a comment.


  > commitablectx code seems wrong here. I am sending a patch.
  
  Sure. Let me know if your patch is going to somehow cover the use case I'm trying to address here, or what else you'd like me to do with this patch (is it just a matter of adding more tests here to account for the config option mix as well, once you add that?).

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7705/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7705

To: rdamazio, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - Dec. 27, 2019, 8:45 p.m.
marmoute added a comment.


  I simply sent this. If am I following your changeset right, we should have `wdir()` match `draft()` or `secret()` according to the projected phase (config + parent)

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7705/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7705

To: rdamazio, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - Dec. 27, 2019, 11:21 p.m.
rdamazio added a comment.


  In D7705#113845 <https://phab.mercurial-scm.org/D7705#113845>, @marmoute wrote:
  
  > I simply sent this. If am I following your changeset right, we should have `wdir()` match `draft()` or `secret()` according to the projected phase (config + parent)
  
  I don't understand what you're asking me to do here, can you clarify?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7705/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7705

To: rdamazio, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - Dec. 28, 2019, 1:55 p.m.
marmoute added a comment.


  In D7705#114045 <https://phab.mercurial-scm.org/D7705#114045>, @rdamazio wrote:
  
  > In D7705#113845 <https://phab.mercurial-scm.org/D7705#113845>, @marmoute wrote:
  >
  >> I simply sent this. If am I following your changeset right, we should have `wdir()` match `draft()` or `secret()` according to the projected phase (config + parent)
  >
  > I don't understand what you're asking me to do here, can you clarify?
  
  In short we should have a test matching
  
    $ hg log -r 'wdir() and secret()' -T '{phase}\n' --config phases.new-commit=secret
    secret

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7705/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7705

To: rdamazio, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - Jan. 7, 2020, 12:56 a.m.
rdamazio added a comment.


  In D7705#114157 <https://phab.mercurial-scm.org/D7705#114157>, @marmoute wrote:
  
  > In D7705#114045 <https://phab.mercurial-scm.org/D7705#114045>, @rdamazio wrote:
  >
  >> I don't understand what you're asking me to do here, can you clarify?
  >
  > In short we should have a test matching
  >
  >   $ hg log -r 'wdir() and secret()' -T '{phase}\n' --config phases.new-commit=secret
  >   secret
  
  Done.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7705/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7705

To: rdamazio, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - Jan. 7, 2020, 2:53 p.m.
marmoute added a comment.
marmoute accepted this revision.


  Looks good to me

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7705/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7705

To: rdamazio, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - Jan. 14, 2020, 2:55 a.m.
rdamazio added a comment.


  In D7705#114943 <https://phab.mercurial-scm.org/D7705#114943>, @yuja wrote:
  
  >> @@ -252,25 +254,44 @@
  >>
  >>       revs = set.union(*[self._phasesets[p] for p in phases])
  >>   if repo.changelog.filteredrevs:
  >>       revs = revs - repo.changelog.filteredrevs
  >>
  >> +
  >>
  >>   if subset is None:
  >>       return smartset.baseset(revs)
  >>   else:
  >>
  >> +                if wdirrev in subset and repo[None].phase() in phases:
  >> +                    # The working dir would never be in the cache, but it was
  >> +                    # in the subset being filtered for its phase, so add it to
  >> +                    # the output.
  >> +                    revs.add(wdirrev)
  >
  > Suppose `self._phasesets[]` is a read-only object in this method,
  > I think `revs` shouldn't be mutated if `revs is self._phasesets[p]`.
  >
  >> +            if wdirrev in subset and repo[None].phase() in phases:
  >> +                # The working dir is in the subset being filtered, and its
  >> +                # phase is in the phases *not* being returned, so add it to the
  >> +                # set of revisions to filter out.
  >> +                revs.add(wdirrev)
  >
  > Same here.
  
  Good point, I'll send another change.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7705/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7705

To: rdamazio, #hg-reviewers, marmoute
Cc: yuja, marmoute, mercurial-devel
phabricator - Jan. 14, 2020, 2:55 a.m.
rdamazio added a comment.


  x

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7705/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7705

To: rdamazio, #hg-reviewers, marmoute
Cc: yuja, marmoute, mercurial-devel

Patch

diff --git a/tests/test-phases.t b/tests/test-phases.t
--- a/tests/test-phases.t
+++ b/tests/test-phases.t
@@ -48,6 +48,15 @@ 
   1 1 B
   0 1 A
 
+Working directory is a draft.
+
+  $ hg log -r 'wdir()' -T '{phase}\n'
+  draft
+  $ hg log -r 'wdir() and public()' -T '{phase}\n'
+  $ hg log -r 'wdir() and draft()' -T '{phase}\n'
+  draft
+  $ hg log -r 'wdir() and secret()' -T '{phase}\n'
+
 Draft commit are properly created over public one:
 
   $ hg phase --public .
diff --git a/mercurial/phases.py b/mercurial/phases.py
--- a/mercurial/phases.py
+++ b/mercurial/phases.py
@@ -112,6 +112,7 @@ 
     nullid,
     nullrev,
     short,
+    wdirrev,
 )
 from .pycompat import (
     getattr,
@@ -252,25 +253,43 @@ 
                 revs = set.union(*[self._phasesets[p] for p in phases])
             if repo.changelog.filteredrevs:
                 revs = revs - repo.changelog.filteredrevs
+
             if subset is None:
                 return smartset.baseset(revs)
             else:
+                if wdirrev in subset and draft in phases:
+                    # The working dir would never be in the cache, but it should
+                    # be considered a draft - if it's in the subset being
+                    # filtered for drafts, add it to the output.
+                    revs.add(wdirrev)
+
                 return subset & smartset.baseset(revs)
         else:
+            # phases keeps all the *other* phases.
             phases = set(allphases).difference(phases)
             if not phases:
                 return smartset.fullreposet(repo)
+
+            # revs has the revisions in all *other* phases.
             if len(phases) == 1:
                 [p] = phases
                 revs = self._phasesets[p]
             else:
                 revs = set.union(*[self._phasesets[p] for p in phases])
+
             if subset is None:
                 subset = smartset.fullreposet(repo)
             if not revs:
                 return subset
+            if wdirrev in subset and draft in phases:
+                # The working dir is in the subset being filtered, and draft is
+                # in the phases *not* being returned, so add it to the set of
+                # revisions to filter out.
+                revs.add(wdirrev)
+
             return subset.filter(lambda r: r not in revs)
 
+
     def copy(self):
         # Shallow copy meant to ensure isolation in
         # advance/retractboundary(), nothing more.