Patchwork [1,of,3] diff: use fctx.isbinary() to test binary

login
register
mail settings
Submitter Jun Wu
Date May 4, 2017, 7 a.m.
Message ID <f28cc5c61b7f45e96159.1493881213@x1c>
Download mbox | patch
Permalink /patch/20427/
State Changes Requested
Headers show

Comments

Jun Wu - May 4, 2017, 7 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1493875014 25200
#      Wed May 03 22:16:54 2017 -0700
# Node ID f28cc5c61b7f45e961590ba45f6cbac576d83ef6
# Parent  2cfdf5241096f6c0c2d45d32b2f1a41575835025
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r f28cc5c61b7f
diff: use fctx.isbinary() to test binary

The end goal is to avoid calling fctx.data() when unnecessary. For example,
if diff.nobinary=1 and files are binary, the expected behavior is to print
"Binary file has changed". That could avoid reading fctx.data() sometimes.

This is mainly to enable an external LFS extension to skip expensive binary
file loading sometimes (read: most of the time with diff.nobinary=1 and
diff.text=0), without any behavior changes to mercurial (i.e. whether a file
is LFS or not does not change any behavior, LFS could be 100% transparent to
users).
Yuya Nishihara - May 5, 2017, 2:21 a.m.
On Thu, 4 May 2017 00:00:13 -0700, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1493875014 25200
> #      Wed May 03 22:16:54 2017 -0700
> # Node ID f28cc5c61b7f45e961590ba45f6cbac576d83ef6
> # Parent  2cfdf5241096f6c0c2d45d32b2f1a41575835025
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r f28cc5c61b7f
> diff: use fctx.isbinary() to test binary

This series looks good except for the complex 'if' condition in the last patch.

> --- a/mercurial/patch.py
> +++ b/mercurial/patch.py
> @@ -2524,17 +2524,21 @@ def trydiff(repo, revs, ctx1, ctx2, modi
>          content1 = None
>          content2 = None
> +        fctx1 = None
> +        fctx2 = None
>          flag1 = None
>          flag2 = None
>          if f1:
> -            content1 = getfilectx(f1, ctx1).data()
> +            fctx1 = getfilectx(f1, ctx1)
> +            content1 = fctx1.data()
>              if opts.git or losedatafn:
>                  flag1 = ctx1.flags(f1)
>          if f2:
> -            content2 = getfilectx(f2, ctx2).data()
> +            fctx2 = getfilectx(f2, ctx2)
> +            content2 = fctx2.data()
>              if opts.git or losedatafn:
>                  flag2 = ctx2.flags(f2)
>          binary = False
>          if opts.git or losedatafn:
> -            binary = util.binary(content1) or util.binary(content2)
> +            binary = any(f.isbinary() for f in [fctx1, fctx2] if f)

bool(f) may be False if f is a null filectx, but that shouldn't matter here.

Patch

diff --git a/mercurial/patch.py b/mercurial/patch.py
--- a/mercurial/patch.py
+++ b/mercurial/patch.py
@@ -2524,17 +2524,21 @@  def trydiff(repo, revs, ctx1, ctx2, modi
         content1 = None
         content2 = None
+        fctx1 = None
+        fctx2 = None
         flag1 = None
         flag2 = None
         if f1:
-            content1 = getfilectx(f1, ctx1).data()
+            fctx1 = getfilectx(f1, ctx1)
+            content1 = fctx1.data()
             if opts.git or losedatafn:
                 flag1 = ctx1.flags(f1)
         if f2:
-            content2 = getfilectx(f2, ctx2).data()
+            fctx2 = getfilectx(f2, ctx2)
+            content2 = fctx2.data()
             if opts.git or losedatafn:
                 flag2 = ctx2.flags(f2)
         binary = False
         if opts.git or losedatafn:
-            binary = util.binary(content1) or util.binary(content2)
+            binary = any(f.isbinary() for f in [fctx1, fctx2] if f)
 
         if losedatafn and not opts.git: