This is an archive of the discontinued Mercurial Phabricator instance.

Differential D158

hiddenoverride: optionally log details
ClosedPublic

Authored by quark on Jul 20 2017, 5:55 PM.
Tags
None
Subscribers
stash

Details

Reviewers
durham
stash
Group Reviewers
Restricted Project
Commits
rFBHGX77e840e8c8c7: hiddenoverride: optionally log details
Summary

This allows us to log what nodes are pinned by what commands. So we can
manually investigate what was happening when we get user complaints.

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Jul 20 2017, 5:55 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 20 2017, 5:55 PM
stash requested changes to this revision.Jul 21 2017, 4:11 AM
stash added a subscriber: stash.
stash added inline comments.
hgext3rd/fbamend/hiddenoverride.py
101–102

loadpinnednodes returns generator. So in this case it outputs empty list. There are two ways to fix it - use list(loadpinnednodes(repo)) here or return list instead of generator in loadpinnednodes. I vote for latter

tests/test-fbamend-hiddenoverride.t
63–93

before is always []. It's incorrect, isn't it?

And I think I know what's the problem, see above

This revision now requires changes to proceed.Jul 21 2017, 4:11 AM
quark edited edge metadata.Jul 21 2017, 10:52 AM
quark updated this revision to Diff 366.
quark added inline comments.Jul 21 2017, 10:52 AM
hgext3rd/fbamend/hiddenoverride.py
101–102

Good catch!

quark added a comment.Jul 21 2017, 1:48 PM

I'm pushing this in the hope that it helps identify related issues if we got related complaints again. We have rolled back too many times this week and I'd like to get some clues at least next time.

Closed by commit rFBHGX77e840e8c8c7: hiddenoverride: optionally log details (authored by quark). · Explain WhyJul 21 2017, 1:58 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 366

hgext3rd/fbamend/hiddenoverride.py

# hiddenoverride.py - lightweight hidden-ness override # hiddenoverride.py - lightweight hidden-ness override
# #
# Copyright 2017 Facebook, Inc. # Copyright 2017 Facebook, Inc.
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
from __future__ import absolute_import from __future__ import absolute_import
import contextlib import contextlib
import os import os
from mercurial.node import short
from mercurial import ( from mercurial import (
dispatch, dispatch,
error, error,
extensions, extensions,
lock as lockmod, lock as lockmod,
obsolete, obsolete,
repoview, repoview,
util, util,
"""yield pinned nodes that are obsoleted and should be visible""" """yield pinned nodes that are obsoleted and should be visible"""
if repo is None or not repo.local(): if repo is None or not repo.local():
return return
# the "pinned nodes" file name is "obsinhibit" for compatibility reason # the "pinned nodes" file name is "obsinhibit" for compatibility reason
content = repo.svfs.tryread('obsinhibit') or '' content = repo.svfs.tryread('obsinhibit') or ''
unfi = repo.unfiltered() unfi = repo.unfiltered()
nodemap = unfi.changelog.nodemap nodemap = unfi.changelog.nodemap
offset = 0 offset = 0
result = []
while True: while True:
node = content[offset:offset + 20] node = content[offset:offset + 20]
if not node: if not node:
break break
# remove unnecessary (non-obsoleted) nodes since pinnedrevs should only # remove unnecessary (non-obsoleted) nodes since pinnedrevs should only
# affect obsoleted revs. # affect obsoleted revs.
if node in nodemap and unfi[node].obsolete(): if node in nodemap and unfi[node].obsolete():
yield node result.append(node)
offset += 20 offset += 20
return result
def shouldpinnodes(repo): def shouldpinnodes(repo):
"""get nodes that should be pinned: working parent + bookmarks for now""" """get nodes that should be pinned: working parent + bookmarks for now"""
result = set() result = set()
if repo and repo.local(): if repo and repo.local():
# working copy parent # working copy parent
try: try:
wnode = repo.vfs('dirstate').read(20) wnode = repo.vfs('dirstate').read(20)
lockfd = os.open(lockpath, os.O_RDONLY | os.O_CREAT, 0o664) lockfd = os.open(lockpath, os.O_RDONLY | os.O_CREAT, 0o664)
fcntl.flock(lockfd, fcntl.LOCK_EX) fcntl.flock(lockfd, fcntl.LOCK_EX)
try: try:
yield yield
finally: finally:
fcntl.flock(lockfd, fcntl.LOCK_UN) fcntl.flock(lockfd, fcntl.LOCK_UN)
os.close(lockfd) os.close(lockfd)
def savepinnednodes(repo, newpin, newunpin): def savepinnednodes(repo, newpin, newunpin, fullargs):
# take a narrowed lock so it does not affect repo lock # take a narrowed lock so it does not affect repo lock
with flock(repo.svfs.join('obsinhibit.lock')): with flock(repo.svfs.join('obsinhibit.lock')):
nodes = set(loadpinnednodes(repo)) orignodes = loadpinnednodes(repo)
nodes = set(orignodes)
stashUnsubmitted
Not Done

loadpinnednodes returns generator. So in this case it outputs empty list. There are two ways to fix it - use list(loadpinnednodes(repo)) here or return list instead of generator in loadpinnednodes. I vote for latter

stash: `loadpinnednodes` returns generator. So in this case it outputs empty list. There are two ways…
quarkAuthorUnsubmitted
Not Done

Good catch!

quark: Good catch!
nodes |= set(newpin) nodes |= set(newpin)
nodes -= set(newunpin) nodes -= set(newunpin)
with util.atomictempfile(repo.svfs.join('obsinhibit')) as f: with util.atomictempfile(repo.svfs.join('obsinhibit')) as f:
f.write(''.join(nodes)) f.write(''.join(nodes))
desc = lambda s: [short(n) for n in s]
repo.ui.log('pinnednodes', 'pinnednodes: %r newpin=%r newunpin=%r '
'before=%r after=%r\n', fullargs, desc(newpin),
desc(newunpin), desc(orignodes), desc(nodes))
def runcommand(orig, lui, repo, cmd, fullargs, *args): def runcommand(orig, lui, repo, cmd, fullargs, *args):
# return directly for non-repo command # return directly for non-repo command
if not repo: if not repo:
return orig(lui, repo, cmd, fullargs, *args) return orig(lui, repo, cmd, fullargs, *args)
shouldpinbefore = shouldpinnodes(repo) | set(loadpinnednodes(repo)) shouldpinbefore = shouldpinnodes(repo) | set(loadpinnednodes(repo))
result = orig(lui, repo, cmd, fullargs, *args) result = orig(lui, repo, cmd, fullargs, *args)
# after a command completes, make sure working copy parent and all # after a command completes, make sure working copy parent and all
# bookmarks get "pinned". # bookmarks get "pinned".
newpin = shouldpinnodes(repo) - shouldpinbefore newpin = shouldpinnodes(repo) - shouldpinbefore
newunpin = getattr(repo.unfiltered(), '_tounpinnodes', set()) newunpin = getattr(repo.unfiltered(), '_tounpinnodes', set())
# filter newpin by obsolte - ex. if newpin is on a non-obsoleted commit, # filter newpin by obsolte - ex. if newpin is on a non-obsoleted commit,
# ignore it. # ignore it.
unfi = repo.unfiltered() unfi = repo.unfiltered()
newpin = set(n for n in newpin if unfi[n].obsolete()) newpin = set(n for n in newpin if unfi[n].obsolete())
# only do a write if something has changed # only do a write if something has changed
if newpin or newunpin: if newpin or newunpin:
savepinnednodes(repo, newpin, newunpin) savepinnednodes(repo, newpin, newunpin, fullargs)
return result return result
def createmarkers(orig, repo, rels, *args, **kwargs): def createmarkers(orig, repo, rels, *args, **kwargs):
# this is a way to unpin revs - precursors are unpinned # this is a way to unpin revs - precursors are unpinned
# note: hg debugobsolete does not call this function # note: hg debugobsolete does not call this function
unfi = repo.unfiltered() unfi = repo.unfiltered()
tounpin = getattr(unfi, '_tounpinnodes', set()) tounpin = getattr(unfi, '_tounpinnodes', set())
for r in rels: for r in rels:
try: try:
tounpin.add(r[0].node()) tounpin.add(r[0].node())
except error.RepoLookupError: except error.RepoLookupError:
pass pass
unfi._tounpinnodes = tounpin unfi._tounpinnodes = tounpin
return orig(repo, rels, *args, **kwargs) return orig(repo, rels, *args, **kwargs)

tests/test-fbamend-hiddenoverride.t

$ cat >> $HGRCPATH << EOF $ cat >> $HGRCPATH << EOF
> [extensions] > [extensions]
> blackbox=
> fbamend=$TESTDIR/../hgext3rd/fbamend > fbamend=$TESTDIR/../hgext3rd/fbamend
> drawdag=$RUNTESTDIR/drawdag.py > drawdag=$RUNTESTDIR/drawdag.py
> [experimental] > [experimental]
> evolution = all > evolution = all
> [blackbox]
> track = command, commandfinish, commandexception,
> pinnednodes
> EOF > EOF
$ hg init $ hg init
$ hg debugdrawdag <<'EOS' $ hg debugdrawdag <<'EOS'
> B C # amend: B -> C > B C # amend: B -> C
> |/ > |/
> A > A
> EOS > EOS
$ hg bookmark -d BOOK -q $ hg bookmark -d BOOK -q
$ hg log -G -T '{rev} {desc}\n' $ hg log -G -T '{rev} {desc}\n'
o 2 C o 2 C
| |
| x 1 B | x 1 B
|/ |/
@ 0 A @ 0 A
Check blackbox logs
$ hg blackbox -l 10000
*> debugdrawdag (glob)
*> pinnednodes: ['debugdrawdag'] newpin=[] newunpin=['112478962961'] before=[] after=[] (glob)
*> debugdrawdag exited 0 after * (glob)
*> log -G -T '{rev} {desc}\n' (glob)
*> log -G -T '{rev} {desc}\n' exited 0 after * (glob)
*> log -G -T '{rev} {desc}\n' --hidden (glob)
*> log -G -T '{rev} {desc}\n' --hidden exited 0 after * (glob)
*> update 1 --hidden -q (glob)
*> pinnednodes: ['update', '1', '--hidden', '-q'] newpin=['112478962961'] newunpin=[] before=[] after=['112478962961'] (glob)
*> update 1 --hidden -q exited 0 after * (glob)
*> update 0 -q (glob)
*> update 0 -q exited 0 after * (glob)
*> log -G -T '{rev} {desc}\n' (glob)
*> log -G -T '{rev} {desc}\n' exited 0 after * (glob)
*> prune 1 -q (glob)
*> pinnednodes: ['prune', '1', '-q'] newpin=[] newunpin=['112478962961'] before=['112478962961'] after=[] (glob)
*> prune 1 -q exited 0 after * (glob)
*> log -G -T '{rev} {desc}\n' (glob)
*> log -G -T '{rev} {desc}\n' exited 0 after * (glob)
*> bookmark -ir 1 BOOK --hidden -q (glob)
*> pinnednodes: ['bookmark', '-ir', '1', 'BOOK', '--hidden', '-q'] newpin=['112478962961'] newunpin=[] before=[] after=['112478962961'] (glob)
*> bookmark -ir 1 BOOK --hidden -q exited 0 after * (glob)
*> bookmark -d BOOK -q (glob)
*> bookmark -d BOOK -q exited 0 after * (glob)
*> log -G -T '{rev} {desc}\n' (glob)
*> log -G -T '{rev} {desc}\n' exited 0 after * (glob)
*> blackbox -l 10000 (glob)
stashUnsubmitted
Not Done

before is always []. It's incorrect, isn't it?

And I think I know what's the problem, see above

stash: before is always `[]`. It's incorrect, isn't it? And I think I know what's the problem, see…
The order matters - putting bookmarks or moving working copy on non-obsoleted The order matters - putting bookmarks or moving working copy on non-obsoleted
commits do not pin them. Test this using "debugobsolete" which will not call commits do not pin them. Test this using "debugobsolete" which will not call
"createmarkers". "createmarkers".
Obsolete working copy, and move working copy away should make things disappear Obsolete working copy, and move working copy away should make things disappear
$ rm -rf .hg && hg init && hg debugdrawdag <<'EOS' $ rm -rf .hg && hg init && hg debugdrawdag <<'EOS'
> C E > C E