( )⚙ D7705 phases: make the working directory consistently a draft

This is an archive of the discontinued Mercurial Phabricator instance.

phases: make the working directory consistently a draft
ClosedPublic

Authored by rdamazio on Dec 19 2019, 3:34 AM.

Details

Summary

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

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

rdamazio created this revision.Dec 19 2019, 3:34 AM

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.

marmoute requested changes to this revision.Dec 19 2019, 8:25 AM
This revision now requires changes to proceed.Dec 19 2019, 8:25 AM
rdamazio updated this revision to Diff 18921.Dec 23 2019, 9:04 PM

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.

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.

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?).

marmoute added a comment.EditedDec 27 2019, 3:45 PM

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

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?

marmoute added a comment.EditedDec 28 2019, 8:55 AM

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
rdamazio updated this revision to Diff 19050.Jan 6 2020, 7:56 PM

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.

marmoute accepted this revision.Jan 7 2020, 9:53 AM

Looks good to me

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.Jan 9 2020, 8:54 AM

@@ -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.

In 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.