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

login
register
mail settings
Submitter phabricator
Date March 4, 2020, 4:56 p.m.
Message ID <differential-rev-PHID-DREV-rdraib6ohtez46vyj6h5-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/45473/
State Superseded
Headers show

Comments

phabricator - March 4, 2020, 4:56 p.m.
mharbison72 created this revision.
Herald added subscribers: mercurial-devel, Kwan.
Herald added a reviewer: hg-reviewers.

REVISION 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.

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D8220

AFFECTED FILES
  hgext/phabricator.py

CHANGE DETAILS




To: mharbison72, #hg-reviewers
Cc: Kwan, mercurial-devel

Patch

diff --git a/hgext/phabricator.py b/hgext/phabricator.py
--- a/hgext/phabricator.py
+++ b/hgext/phabricator.py
@@ -796,8 +796,6 @@ 
     """
     try:
         fctx.data().decode('utf-8')
-        if fctx.parents():
-            fctx.p1().data().decode('utf-8')
         return False
     except UnicodeDecodeError:
         fctx.repo().ui.write(
@@ -825,6 +823,7 @@ 
     """add modified files to the phabdiff"""
     for fname in modified:
         fctx = ctx[fname]
+        oldfctx = fctx.p1()
         pchange = phabchange(currentPath=fname, oldPath=fname)
         filemode = gitmode[ctx[fname].flags()]
         originalmode = gitmode[ctx.p1()[fname].flags()]
@@ -832,7 +831,7 @@ 
             pchange.addoldmode(originalmode)
             pchange.addnewmode(filemode)
 
-        if fctx.isbinary() or notutf8(fctx):
+        if fctx.isbinary() or notutf8(fctx) or notutf8(oldfctx):
             makebinary(pchange, fctx)
             addoldbinary(pchange, fctx.p1(), fctx)
         else:
@@ -849,6 +848,7 @@ 
     movedchanges = {}
     for fname in added:
         fctx = ctx[fname]
+        oldfctx = None
         pchange = phabchange(currentPath=fname)
 
         filemode = gitmode[ctx[fname].flags()]
@@ -856,7 +856,8 @@ 
 
         if renamed:
             originalfname = renamed[0]
-            originalmode = gitmode[ctx.p1()[originalfname].flags()]
+            oldfctx = ctx.p1()[originalfname]
+            originalmode = gitmode[oldfctx.flags()]
             pchange.oldPath = originalfname
 
             if originalfname in removed:
@@ -891,10 +892,10 @@ 
             pchange.addnewmode(gitmode[fctx.flags()])
             pchange.type = DiffChangeType.ADD
 
-        if fctx.isbinary() or notutf8(fctx):
+        if fctx.isbinary() or notutf8(fctx) or (oldfctx and notutf8(oldfctx)):
             makebinary(pchange, fctx)
             if renamed:
-                addoldbinary(pchange, fctx.p1(), fctx)
+                addoldbinary(pchange, oldfctx, fctx)
         else:
             maketext(pchange, ctx, fname)