This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8220

phabricator: don't infer the old `fctx` in `notutf8()`
ClosedPublic

Authored by mharbison72 on Mar 4 2020, 11:56 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG66a05dbb8b4c: phabricator: don't infer the old `fctx` in `notutf8()`
Summary

This is used along with fctx.isbinary() to gate addoldbinary(), so it seems
like a good idea to provide the caller similar control over the current and
parent filecontext. Unlike addoldbinary(), it doesn't need both previous and
current contexts at the same time, so make the caller responsible for testing
both cases, as appropriate. I haven't worked out all of the problems around
marking files as binary for move/remove/copy, but this will definitely help with
--no-stack too.

It also turns out to have been doing too much- in the remove case, it tested not
just the removed file in the parent context (which is what gets passed in that
case), but also in the parent of the parent context (which should be
irrelevant). The previous code also required the fctx.parents() check to work
in the add (but without rename) case. Now the add and remove cases test only
what they need to. But now that it is written this way, the fact that only the
current fctx is checked to be binary in the case of modification or being
renamed seems wrong.

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

mharbison72 created this revision.Mar 4 2020, 11:56 AM
mharbison72 added a commit: rHG66a05dbb8b4c: phabricator: don't infer the old `fctx` in `notutf8()`.Mar 4 2020, 2:07 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG66a05dbb8b4c: phabricator: don't infer the old `fctx` in `notutf8()` (authored by mharbison72). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext/phabricator.py (13 lines)

Diff 20482

hgext/phabricator.py

def notutf8(fctx): def notutf8(fctx):
"""detect non-UTF-8 text files since Phabricator requires them to be marked """detect non-UTF-8 text files since Phabricator requires them to be marked
as binary as binary
""" """
try: try:
fctx.data().decode('utf-8') fctx.data().decode('utf-8')
if fctx.parents():
fctx.p1().data().decode('utf-8')
return False return False
except UnicodeDecodeError: except UnicodeDecodeError:
fctx.repo().ui.write( fctx.repo().ui.write(
_(b'file %s detected as non-UTF-8, marked as binary\n') _(b'file %s detected as non-UTF-8, marked as binary\n')
% fctx.path() % fctx.path()
) )
return True return True
pdiff.addchange(pchange) pdiff.addchange(pchange)
def addmodified(pdiff, ctx, modified): def addmodified(pdiff, ctx, modified):
"""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 = fctx.p1()
pchange = phabchange(currentPath=fname, oldPath=fname) pchange = phabchange(currentPath=fname, oldPath=fname)
filemode = gitmode[ctx[fname].flags()] filemode = gitmode[ctx[fname].flags()]
originalmode = gitmode[ctx.p1()[fname].flags()] originalmode = gitmode[ctx.p1()[fname].flags()]
if filemode != originalmode: if filemode != originalmode:
pchange.addoldmode(originalmode) pchange.addoldmode(originalmode)
pchange.addnewmode(filemode) pchange.addnewmode(filemode)
if fctx.isbinary() or notutf8(fctx): if fctx.isbinary() or notutf8(fctx) or notutf8(oldfctx):
makebinary(pchange, fctx) makebinary(pchange, fctx)
addoldbinary(pchange, fctx.p1(), fctx) addoldbinary(pchange, fctx.p1(), fctx)
else: else:
maketext(pchange, ctx, fname) maketext(pchange, ctx, fname)
pdiff.addchange(pchange) pdiff.addchange(pchange)
def addadded(pdiff, ctx, added, removed): def addadded(pdiff, ctx, added, removed):
"""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 = {}
for fname in added: for fname in added:
fctx = ctx[fname] fctx = ctx[fname]
oldfctx = None
pchange = phabchange(currentPath=fname) pchange = phabchange(currentPath=fname)
filemode = gitmode[ctx[fname].flags()] filemode = gitmode[ctx[fname].flags()]
renamed = fctx.renamed() renamed = fctx.renamed()
if renamed: if renamed:
originalfname = renamed[0] originalfname = renamed[0]
originalmode = gitmode[ctx.p1()[originalfname].flags()] oldfctx = ctx.p1()[originalfname]
originalmode = gitmode[oldfctx.flags()]
pchange.oldPath = originalfname pchange.oldPath = originalfname
if originalfname in removed: if originalfname in removed:
origpchange = phabchange( origpchange = phabchange(
currentPath=originalfname, currentPath=originalfname,
oldPath=originalfname, oldPath=originalfname,
type=DiffChangeType.MOVE_AWAY, type=DiffChangeType.MOVE_AWAY,
awayPaths=[fname], awayPaths=[fname],
if filemode != originalmode: if filemode != originalmode:
pchange.addoldmode(originalmode) pchange.addoldmode(originalmode)
pchange.addnewmode(filemode) pchange.addnewmode(filemode)
else: # Brand-new file else: # Brand-new file
pchange.addnewmode(gitmode[fctx.flags()]) pchange.addnewmode(gitmode[fctx.flags()])
pchange.type = DiffChangeType.ADD pchange.type = DiffChangeType.ADD
if fctx.isbinary() or notutf8(fctx): if fctx.isbinary() or notutf8(fctx) or (oldfctx and notutf8(oldfctx)):
makebinary(pchange, fctx) makebinary(pchange, fctx)
if renamed: if renamed:
addoldbinary(pchange, fctx.p1(), fctx) addoldbinary(pchange, oldfctx, fctx)
else: else:
maketext(pchange, ctx, fname) maketext(pchange, ctx, fname)
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():