This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4819

identify: only query remote bookmarks if needed
ClosedPublic

Authored by valentin.gatienbaron on Oct 1 2018, 10:30 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHGc9026e9297e3: identify: only query remote bookmarks if needed
Summary

Instead of all the time when operating on a remote repo. This
perf regression was introduced in 15a79ac823e8, in 4.3.

This datahint method returns nothing for -Tjson, -Tpickle, -Tdebug
--config ui.formatdebug=true and --config ui.formatjson, so the
bookmarks won't show up. I don't know what these formatters are for.
plainformatter and templateformatter work properly, and the few other
uses of datahint should have the same kind of problem.

There is further weirdness where "--template '{node}'" is not enough
to avoid querying the bookmarks, you also need to pass --id or -q.

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

yuja added a subscriber: yuja.Oct 2 2018, 7:54 AM

+ bmscache = [None]

def getbms():

+ if bmscache[0] is not None:
+ return bmscache[0]

Looks good, but I think getbms() can be a @util.cachefunc.

yuja added a comment.Oct 3 2018, 8:20 AM

Queued, thanks.

This datahint method returns nothing for -Tjson, -Tpickle, -Tdebug
--config ui.formatdebug=true and --config ui.formatjson, so the
bookmarks won't show up. I don't know what these formatters are for.
plainformatter and templateformatter work properly, and the few other
uses of datahint should have the same kind of problem.

The datahint exists so that template keywords are populated automatically
in addition to the default data set specified by user options.

if bookmarks:
  • output.extend(bms)

+ output.extend(getbms())

elif default and not ui.quiet:
    # multiple bookmarks for a single parent separated by '/'
  • bm = '/'.join(bms)

+ bm = '/'.join(getbms())

    if bm:
        output.append(bm)
fm.data(node=hex(remoterev))
  • fm.data(bookmarks=fm.formatlist(bms, name='bookmark'))

+ if 'bookmarks' in fm.datahint():
+ fm.data(bookmarks=fm.formatlist(getbms(), name='bookmark'))

In this case, we'll have to load 'bookmarks' if --bookmarks is set. I have
no idea if default and not ui.quiet should be handled in the same way.

Can you send a followup?

Closed by commit rHGc9026e9297e3: identify: only query remote bookmarks if needed (authored by valentin.gatienbaron). · Explain WhyOct 3 2018, 8:29 AM
This revision was automatically updated to reflect the committed changes.
valentin.gatienbaron added a comment.Oct 3 2018, 8:56 AM

In this case, we'll have to load 'bookmarks' if --bookmarks is set. I have
no idea if default and not ui.quiet should be handled in the same way.
Can you send a followup?

Do you mean hg id remote-url -Tjson -B should show the bookmark field?

yuja added a comment.Oct 3 2018, 9:09 AM
Do you mean `hg id remote-url -Tjson -B` should show the bookmark field?

Yes, because the user requested to do so.

Revision Contents
Changeset List

PathPackages
Mmercurial/commands.py (9 lines)

Diff 11595

mercurial/commands.py

rev = "tip" rev = "tip"
remoterev = peer.lookup(rev) remoterev = peer.lookup(rev)
hexrev = fm.hexfunc(remoterev) hexrev = fm.hexfunc(remoterev)
if default or id: if default or id:
output = [hexrev] output = [hexrev]
fm.data(id=hexrev) fm.data(id=hexrev)
@util.cachefunc
def getbms(): def getbms():
bms = [] bms = []
if 'bookmarks' in peer.listkeys('namespaces'): if 'bookmarks' in peer.listkeys('namespaces'):
hexremoterev = hex(remoterev) hexremoterev = hex(remoterev)
bms = [bm for bm, bmr in peer.listkeys('bookmarks').iteritems() bms = [bm for bm, bmr in peer.listkeys('bookmarks').iteritems()
if bmr == hexremoterev] if bmr == hexremoterev]
return sorted(bms) return sorted(bms)
bms = getbms()
if bookmarks: if bookmarks:
output.extend(bms) output.extend(getbms())
elif default and not ui.quiet: elif default and not ui.quiet:
# multiple bookmarks for a single parent separated by '/' # multiple bookmarks for a single parent separated by '/'
bm = '/'.join(bms) bm = '/'.join(getbms())
if bm: if bm:
output.append(bm) output.append(bm)
fm.data(node=hex(remoterev)) fm.data(node=hex(remoterev))
fm.data(bookmarks=fm.formatlist(bms, name='bookmark')) if 'bookmarks' in fm.datahint():
fm.data(bookmarks=fm.formatlist(getbms(), name='bookmark'))
else: else:
if rev: if rev:
repo = scmutil.unhidehashlikerevs(repo, [rev], 'nowarn') repo = scmutil.unhidehashlikerevs(repo, [rev], 'nowarn')
ctx = scmutil.revsingle(repo, rev, None) ctx = scmutil.revsingle(repo, rev, None)
if ctx.rev() is None: if ctx.rev() is None:
ctx = repo[None] ctx = repo[None]
parents = ctx.parents() parents = ctx.parents()