This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8968

phabricator: Pass through diffoptions
Needs RevisionPublic

Authored by sfink on Aug 28 2020, 5:00 PM.

Details

Reviewers
Alphare
indygreg
Group Reviewers
hg-reviewers

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

sfink created this revision.Aug 28 2020, 5:00 PM

Nothing uses this directly, but it makes it much easier to another extension to call into this code and be able to request different diff options

Alphare accepted this revision.Sep 10 2020, 4:17 AM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
hgext/phabricator.py
725

I know it's not technically part of this change, but making 32767 a "constant" with an explanation or at least a good name would be nice.

indygreg requested changes to this revision.Sep 17 2020, 10:09 PM
indygreg added a subscriber: indygreg.
indygreg added inline comments.
hgext/phabricator.py
721

[] and {} or any dynamic value should not be used as an argument default because the initially passed value will be used on subsequent invocations if it isn't passed. See https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments.

For this pattern, do something like:

def func(optional=None):
    optional = optional or {}

or

def func(optional=None):
    if optional is None:
        optional = {}
726

It's weird to pass an options dict/argument only to use a single key.

Perhaps we should do something like this instead:

options = dict(options or {})
options['git'] = True

diffopts = mdiff.diffopts(**options)
1082

Mutable default argument.

This revision now requires changes to proceed.Sep 17 2020, 10:09 PM

Revision Contents
Changeset List

PathPackages
Mhgext/phabricator.py (24 lines)

Diff 22486

hgext/phabricator.py

def addchange(self, change): def addchange(self, change):
if not isinstance(change, phabchange): if not isinstance(change, phabchange):
raise error.Abort(b'phabdiff.addchange only takes phabchanges') raise error.Abort(b'phabdiff.addchange only takes phabchanges')
self.changes[change.currentPath] = pycompat.byteskwargs( self.changes[change.currentPath] = pycompat.byteskwargs(
attr.asdict(change) attr.asdict(change)
) )
def maketext(pchange, basectx, ctx, fname): def maketext(pchange, basectx, ctx, fname, options={}):
indygregUnsubmitted
Not Done

[] and {} or any dynamic value should not be used as an argument default because the initially passed value will be used on subsequent invocations if it isn't passed. See https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments.

For this pattern, do something like:

def func(optional=None):
    optional = optional or {}

or

def func(optional=None):
    if optional is None:
        optional = {}
indygreg: `[]` and `{}` or any dynamic value should not be used as an argument default because the…
"""populate the phabchange for a text file""" """populate the phabchange for a text file"""
repo = ctx.repo() repo = ctx.repo()
fmatcher = match.exact([fname]) fmatcher = match.exact([fname])
diffopts = mdiff.diffopts(git=True, context=32767) context = options.get(b'context', 32767)
AlphareUnsubmitted
Not Done

I know it's not technically part of this change, but making 32767 a "constant" with an explanation or at least a good name would be nice.

Alphare: I know it's not technically part of this change, but making `32767` a "constant" with an…
diffopts = mdiff.diffopts(git=True, context=context)
indygregUnsubmitted
Not Done

It's weird to pass an options dict/argument only to use a single key.

Perhaps we should do something like this instead:

options = dict(options or {})
options['git'] = True

diffopts = mdiff.diffopts(**options)
indygreg: It's weird to pass an `options` dict/argument only to use a single key. Perhaps we should do…
_pfctx, _fctx, header, fhunks = next( _pfctx, _fctx, header, fhunks = next(
patch.diffhunks(repo, basectx.p1(), ctx, fmatcher, opts=diffopts) patch.diffhunks(repo, basectx.p1(), ctx, fmatcher, opts=diffopts)
) )
for fhunk in fhunks: for fhunk in fhunks:
(oldOffset, oldLength, newOffset, newLength), lines = fhunk (oldOffset, oldLength, newOffset, newLength), lines = fhunk
corpus = b''.join(lines[1:]) corpus = b''.join(lines[1:])
shunk = list(header) shunk = list(header)
oldfctx = basectx.p1()[fname] oldfctx = basectx.p1()[fname]
pchange.addoldmode(gitmode[oldfctx.flags()]) pchange.addoldmode(gitmode[oldfctx.flags()])
if not (oldfctx.isbinary() or notutf8(oldfctx)): if not (oldfctx.isbinary() or notutf8(oldfctx)):
maketext(pchange, basectx, ctx, fname) maketext(pchange, basectx, ctx, fname)
pdiff.addchange(pchange) pdiff.addchange(pchange)
def addmodified(pdiff, basectx, ctx, modified): def addmodified(pdiff, basectx, ctx, modified, diffoptions):
"""add modified files to the phabdiff""" """add modified files to the phabdiff"""
for fname in modified: for fname in modified:
fctx = ctx[fname] fctx = ctx[fname]
oldfctx = basectx.p1()[fname] oldfctx = basectx.p1()[fname]
pchange = phabchange(currentPath=fname, oldPath=fname) pchange = phabchange(currentPath=fname, oldPath=fname)
filemode = gitmode[fctx.flags()] filemode = gitmode[fctx.flags()]
originalmode = gitmode[oldfctx.flags()] originalmode = gitmode[oldfctx.flags()]
if filemode != originalmode: if filemode != originalmode:
pchange.addoldmode(originalmode) pchange.addoldmode(originalmode)
pchange.addnewmode(filemode) pchange.addnewmode(filemode)
if ( if (
fctx.isbinary() fctx.isbinary()
or notutf8(fctx) or notutf8(fctx)
or oldfctx.isbinary() or oldfctx.isbinary()
or notutf8(oldfctx) or notutf8(oldfctx)
): ):
makebinary(pchange, fctx) makebinary(pchange, fctx)
addoldbinary(pchange, oldfctx, fctx) addoldbinary(pchange, oldfctx, fctx)
else: else:
maketext(pchange, basectx, ctx, fname) maketext(pchange, basectx, ctx, fname, diffoptions)
pdiff.addchange(pchange) pdiff.addchange(pchange)
def addadded(pdiff, basectx, ctx, added, removed): def addadded(pdiff, basectx, ctx, added, removed, diffoptions):
"""add file adds to the phabdiff, both new files and copies/moves""" """add file adds to the phabdiff, both new files and copies/moves"""
# Keep track of files that've been recorded as moved/copied, so if there are # Keep track of files that've been recorded as moved/copied, so if there are
# additional copies we can mark them (moves get removed from removed) # additional copies we can mark them (moves get removed from removed)
copiedchanges = {} copiedchanges = {}
movedchanges = {} movedchanges = {}
copy = {} copy = {}
if basectx != ctx: if basectx != ctx:
fctx.isbinary() fctx.isbinary()
or notutf8(fctx) or notutf8(fctx)
or (oldfctx and (oldfctx.isbinary() or notutf8(oldfctx))) or (oldfctx and (oldfctx.isbinary() or notutf8(oldfctx)))
): ):
makebinary(pchange, fctx) makebinary(pchange, fctx)
if renamed: if renamed:
addoldbinary(pchange, oldfctx, fctx) addoldbinary(pchange, oldfctx, fctx)
else: else:
maketext(pchange, basectx, ctx, fname) maketext(pchange, basectx, ctx, fname, diffoptions)
pdiff.addchange(pchange) pdiff.addchange(pchange)
for _path, copiedchange in copiedchanges.items(): for _path, copiedchange in copiedchanges.items():
pdiff.addchange(copiedchange) pdiff.addchange(copiedchange)
for _path, movedchange in movedchanges.items(): for _path, movedchange in movedchanges.items():
pdiff.addchange(movedchange) pdiff.addchange(movedchange)
def creatediff(basectx, ctx): def creatediff(basectx, ctx, diffoptions={}):
"""create a Differential Diff""" """create a Differential Diff"""
repo = ctx.repo() repo = ctx.repo()
repophid = getrepophid(repo) repophid = getrepophid(repo)
# Create a "Differential Diff" via "differential.creatediff" API # Create a "Differential Diff" via "differential.creatediff" API
pdiff = phabdiff( pdiff = phabdiff(
sourceControlBaseRevision=b'%s' % basectx.p1().hex(), sourceControlBaseRevision=b'%s' % basectx.p1().hex(),
branch=b'%s' % ctx.branch(), branch=b'%s' % ctx.branch(),
) )
modified, added, removed, _d, _u, _i, _c = basectx.p1().status(ctx) modified, added, removed, _d, _u, _i, _c = basectx.p1().status(ctx)
# addadded will remove moved files from removed, so addremoved won't get # addadded will remove moved files from removed, so addremoved won't get
# them # them
addadded(pdiff, basectx, ctx, added, removed) addadded(pdiff, basectx, ctx, added, removed, diffoptions)
addmodified(pdiff, basectx, ctx, modified) addmodified(pdiff, basectx, ctx, modified, diffoptions)
addremoved(pdiff, basectx, ctx, removed) addremoved(pdiff, basectx, ctx, removed)
if repophid: if repophid:
pdiff.repositoryPHID = repophid pdiff.repositoryPHID = repophid
diff = callconduit( diff = callconduit(
repo.ui, repo.ui,
b'differential.creatediff', b'differential.creatediff',
pycompat.byteskwargs(attr.asdict(pdiff)), pycompat.byteskwargs(attr.asdict(pdiff)),
) )
ctxs, ctxs,
revid=None, revid=None,
parentrevphid=None, parentrevphid=None,
oldbasenode=None, oldbasenode=None,
oldnode=None, oldnode=None,
olddiff=None, olddiff=None,
actions=None, actions=None,
comment=None, comment=None,
diffoptions={},
indygregUnsubmitted
Not Done

Mutable default argument.

indygreg: Mutable default argument.
): ):
"""create or update a Differential Revision """create or update a Differential Revision
If revid is None, create a new Differential Revision, otherwise update If revid is None, create a new Differential Revision, otherwise update
revid. If parentrevphid is not None, set it as a dependency. revid. If parentrevphid is not None, set it as a dependency.
If there is a single commit for the new Differential Revision, ``ctxs`` will If there is a single commit for the new Differential Revision, ``ctxs`` will
be a list of that single context. Otherwise, it is a list that covers the be a list of that single context. Otherwise, it is a list that covers the
neednewdiff = getdiff(basectx, ctx, diffopts) != getdiff( neednewdiff = getdiff(basectx, ctx, diffopts) != getdiff(
oldbasectx, oldctx, diffopts oldbasectx, oldctx, diffopts
) )
else: else:
neednewdiff = True neednewdiff = True
transactions = [] transactions = []
if neednewdiff: if neednewdiff:
diff = creatediff(basectx, ctx) diff = creatediff(basectx, ctx, diffoptions)
transactions.append({b'type': b'update', b'value': diff[b'phid']}) transactions.append({b'type': b'update', b'value': diff[b'phid']})
if comment: if comment:
transactions.append({b'type': b'comment', b'value': comment}) transactions.append({b'type': b'comment', b'value': comment})
else: else:
# Even if we don't need to upload a new diff because the patch content # Even if we don't need to upload a new diff because the patch content
# does not change. We might still need to update its metadata so # does not change. We might still need to update its metadata so
# pushers could know the correct node metadata. # pushers could know the correct node metadata.
assert olddiff assert olddiff
map( map(
lambda phid: b'blocking(%s)' % phid, lambda phid: b'blocking(%s)' % phid,
userphids(repo.ui, blockers), userphids(repo.ui, blockers),
) )
) )
if phids: if phids:
actions.append({b'type': b'reviewers.add', b'value': phids}) actions.append({b'type': b'reviewers.add', b'value': phids})
diffoptions = {}
drevids = [] # [int] drevids = [] # [int]
diffmap = {} # {newnode: diff} diffmap = {} # {newnode: diff}
# Send patches one by one so we know their Differential Revision PHIDs and # Send patches one by one so we know their Differential Revision PHIDs and
# can provide dependency relationship # can provide dependency relationship
lastrevphid = None lastrevphid = None
for ctx in ctxs: for ctx in ctxs:
if fold: if fold:
ctxs if fold else [ctx], ctxs if fold else [ctx],
revid, revid,
lastrevphid, lastrevphid,
oldbasenode, oldbasenode,
oldnode, oldnode,
olddiff, olddiff,
actions, actions,
opts.get(b'comment'), opts.get(b'comment'),
diffoptions,
) )
if fold: if fold:
for ctx in ctxs: for ctx in ctxs:
diffmap[ctx.node()] = diff diffmap[ctx.node()] = diff
else: else:
diffmap[ctx.node()] = diff diffmap[ctx.node()] = diff