This is an archive of the discontinued Mercurial Phabricator instance.

revset: pass pre-optimized tree in revset.matchany()
AbandonedPublic

Authored by pulkit on Dec 12 2017, 8:55 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Summary

This is a part of series where we move logic around so that we can get the tree
at a higher level function such as repo.anyrevs(). This will help us in reading
the tree and doing certain operations like updating visibility exceptions on
base of that.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

pulkit created this revision.Dec 12 2017, 8:55 PM
pulkit added a subscriber: yuja.Dec 12 2017, 8:57 PM

@yuja am I headed in the right direction? (I am not sure about whether the API's I changed are used by extensions or not)

yuja added a comment.Dec 13 2017, 8:23 AM
In D1678#28686, @pulkit wrote:

@yuja am I headed in the right direction? (I am not sure about whether the API's I changed are used by extensions or not)

Sort of, but I was thinking of a simpler one. Just parse specs twice, which isn't
costly operation.

def unhidehashlikerevs(repo, specs, allowhidden|warnhidden):
    if not repo.filtername:
        return repo
    syms = set()
    for spec in specs:
        try:
            tree = revsetlang.parse(spec)
        except ParseError:
            continue  # will be reported later by scmutil.revrange()
        syms.update(revsetlang.hashlikesymbols(tree))
    if not syms:
        return repo
    pinned = perhaps_need_to_convert_to_nodes_or_revs?(syms)
    # filtered repo with the given extra pinned revisions; no idea if this is an ideal API
    return repo.filtered(repo.filtername, pinned)

This allows us to get rid of an intermediate state where a repo is flagged as
"visible-<x>", but "<x>" isn't added yet.

We can apply this function at dispatch:

def _dispatch(req):
    ...
            if repo:
                ui = repo.ui
                if options['hidden']:
                    repo = repo.unfiltered()
                else:
                    # perhaps we'll need a registrar flag to get arguments taking revspecs
                    repo = scmutil.unhidehashlikerevs(repo, cmdoptions.get('rev', []))

or simply at each command:

def diff(...):
    ...
    repo = scmutil.unhidehashlikerevs(repo, changesets, 'allowhidden')
    revs = scmutil.revrange(repo, changesets)
In D1678#28729, @yuja wrote:
In D1678#28686, @pulkit wrote:

@yuja am I headed in the right direction? (I am not sure about whether the API's I changed are used by extensions or not)

Sort of, but I was thinking of a simpler one. Just parse specs twice, which isn't
costly operation.

def unhidehashlikerevs(repo, specs, allowhidden|warnhidden):
    if not repo.filtername:
        return repo
    syms = set()
    for spec in specs:
        try:
            tree = revsetlang.parse(spec)
        except ParseError:
            continue  # will be reported later by scmutil.revrange()
        syms.update(revsetlang.hashlikesymbols(tree))
    if not syms:
        return repo
    pinned = perhaps_need_to_convert_to_nodes_or_revs?(syms)
    # filtered repo with the given extra pinned revisions; no idea if this is an ideal API
    return repo.filtered(repo.filtername, pinned)

This allows us to get rid of an intermediate state where a repo is flagged as
"visible-<x>", but "<x>" isn't added yet.
We can apply this function at dispatch:

def _dispatch(req):
    ...
            if repo:
                ui = repo.ui
                if options['hidden']:
                    repo = repo.unfiltered()
                else:
                    # perhaps we'll need a registrar flag to get arguments taking revspecs
                    repo = scmutil.unhidehashlikerevs(repo, cmdoptions.get('rev', []))

or simply at each command:

def diff(...):
    ...
    repo = scmutil.unhidehashlikerevs(repo, changesets, 'allowhidden')
    revs = scmutil.revrange(repo, changesets)

I like your idea. Thanks for it. After some hacking, I think we should call scmutil.unhidehashlikerevs() from each command level because at dispatch level we are not sure what revs are, some commands don't need --rev to take a rev like hg export. Since we are calling them at each command level, it will be better to call unhidehashlikerevs() in scmutil.revrange|revsingle. We will still need to store the filtername in dispatch and then later on decide on basis of that whether we want to uhide things.

yuja added a comment.Dec 15 2017, 8:00 AM

Since we are calling them at each command level, it will be better to call unhidehashlikerevs() in scmutil.revrange|revsingle.

I thought about that, but scmutil.rev*() would have to return new filtered repo
object, which didn't seem nice. And mutating a given repo argument has the
original problem, when to discard unhidden revisions. So my conclusion was a
separate function.

We will still need to store the filtername in dispatch and then later on
decide on basis of that whether we want to uhide things.

Doesn't each command know whether it should unhide or not?

pulkit abandoned this revision.Dec 19 2017, 7:15 AM
In D1678#29063, @yuja wrote:

Since we are calling them at each command level, it will be better to call unhidehashlikerevs() in scmutil.revrange|revsingle.

I thought about that, but scmutil.rev*() would have to return new filtered repo
object, which didn't seem nice. And mutating a given repo argument has the
original problem, when to discard unhidden revisions. So my conclusion was a
separate function.

We will still need to store the filtername in dispatch and then later on
decide on basis of that whether we want to uhide things.

Doesn't each command know whether it should unhide or not?

Thanks for all the ideas, I have send the new series as D1730 - D1735 as they are entirely different changesets.