Patchwork [3,of,3] lfs: teach '{lfs_files}' to handle removed files

login
register
mail settings
Submitter Matt Harbison
Date Feb. 10, 2018, 1:28 a.m.
Message ID <op.zd641cr89lwrgf@envy>
Download mbox | patch
Permalink /patch/27515/
State New
Headers show

Comments

Matt Harbison - Feb. 10, 2018, 1:28 a.m.
On Fri, 09 Feb 2018 08:04:04 -0500, Yuya Nishihara <yuya@tcha.org> wrote:

> On Fri, 09 Feb 2018 00:30:23 -0500, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1517098935 18000
>> #      Sat Jan 27 19:22:15 2018 -0500
>> # Node ID e545487fa7fa9ff2fa4f85e2482bea8ba4040ef6
>> # Parent  2b6579d78fe2dde40f8e0e99abeb35df93a4ade5
>> lfs: teach '{lfs_files}' to handle removed files
>>
>> diff --git a/hgext/lfs/__init__.py b/hgext/lfs/__init__.py
>> --- a/hgext/lfs/__init__.py
>> +++ b/hgext/lfs/__init__.py
>> @@ -360,10 +360,11 @@
>>
>>  @templatekeyword('lfs_files')
>>  def lfsfiles(repo, ctx, **args):
>> -    """List of strings. LFS files added or modified by the  
>> changeset."""
>> +    """List of strings. All files modified, added, or removed by this
>> +    changeset."""
>>      args = pycompat.byteskwargs(args)
>>
>> -    pointers = wrapper.pointersfromctx(ctx) # {path: pointer}
>> +    pointers = wrapper.pointersfromctx(ctx, removed=True) # {path:  
>> pointer}
>>      files = sorted(pointers.keys())
>
>> +{lfs_files} will list deleted files too
>> +
>> +  $ hg log -T "{lfs_files % '{rev} {file}: {lfspointer.oid}\n'}"
>> +  6 lfs.test:  
>> sha256:43f8f41171b6f62a6b61ba4ce98a8a6c1649240a47ebafd43120aa215ac9e7f6
>> +  5 lfs.test:  
>> sha256:43f8f41171b6f62a6b61ba4ce98a8a6c1649240a47ebafd43120aa215ac9e7f6
>> +  3 lfs.catchall:  
>> sha256:31f43b9c62b540126b0ad5884dc013d21a61c9329b77de1fceeae2fc58511573
>> +  3 lfs.test:  
>> sha256:8acd23467967bc7b8cc5a280056589b0ba0b17ff21dbd88a7b6474d6290378a6
>> +  2 lfs.catchall:  
>> sha256:d4ec46c2869ba22eceb42a729377432052d9dd75d82fc40390ebaadecee87ee9
>> +  2 lfs.test:  
>> sha256:5489e6ced8c36a7b267292bde9fd5242a5f80a7482e8f23fa0477393dfaa4d6c
>
> Including removed files makes sense, but I think the pointer should be  
> "null"
> in which case because the file doesn't exist. I have no idea how the  
> "null"
> lfs pointer should look like, though.

Interesting thought, I hadn't considered it.  I guess was going for being  
able to identify which file was removed.  But that doesn't quite work,  
because lfs files can transition to non-lfs first.

My only concern would be not complicating the template writing.  Having to  
wrap lfspointer inside an {if()} before accessing lfspointer.foo or  
lfspointer % oid seems unfortunate.  IDK how to represent a null pointer  
either (internally or externally), so I guess I'll let it sit a few days  
to see if anyone has an idea.  I'm waiting on stable to merge before I can  
submit a similar patch for set:lfs().

In the meantime, here's a total hatchet job, trying to see what avoiding  
{if()} looks like in the tests.  I can't say I like it either.

+  3 lfs.catchall: version=https://git-lfs.github.com/spec/v1  
oid=sha256:31f43b9c62b540126b0ad5884dc013d21a61c9329b77de1fceeae2fc58511573  
size=19 x-is-binary=0
+  3 lfs.test: version=https://git-lfs.github.com/spec/v1  
oid=sha256:8acd23467967bc7b8cc5a280056589b0ba0b17ff21dbd88a7b6474d6290378a6  
size=11 x-is-binary=0
+  2 lfs.catchall: version=https://git-lfs.github.com/spec/v1  
oid=sha256:d4ec46c2869ba22eceb42a729377432052d9dd75d82fc40390ebaadecee87ee9  
size=18 x-is-binary=0
+  2 lfs.test: version=https://git-lfs.github.com/spec/v1  
oid=sha256:5489e6ced8c36a7b267292bde9fd5242a5f80a7482e8f23fa0477393dfaa4d6c  
size=10 x-is-binary=0
+
  TODO: This should notice the deleted lfs files in rev 6
    $ hg log -r 'file("set:lfs()")' -T '{rev} {join(lfs_files, ", ")}\n'
    2 lfs.catchall, lfs.test
Yuya Nishihara - Feb. 10, 2018, 12:16 p.m.
On Fri, 09 Feb 2018 20:28:14 -0500, Matt Harbison wrote:
> On Fri, 09 Feb 2018 08:04:04 -0500, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Fri, 09 Feb 2018 00:30:23 -0500, Matt Harbison wrote:
> >> +  $ hg log -T "{lfs_files % '{rev} {file}: {lfspointer.oid}\n'}"
> >> +  6 lfs.test:  
> >> sha256:43f8f41171b6f62a6b61ba4ce98a8a6c1649240a47ebafd43120aa215ac9e7f6
> >> +  5 lfs.test:  
> >> sha256:43f8f41171b6f62a6b61ba4ce98a8a6c1649240a47ebafd43120aa215ac9e7f6
> >> +  3 lfs.catchall:  
> >> sha256:31f43b9c62b540126b0ad5884dc013d21a61c9329b77de1fceeae2fc58511573
> >> +  3 lfs.test:  
> >> sha256:8acd23467967bc7b8cc5a280056589b0ba0b17ff21dbd88a7b6474d6290378a6
> >> +  2 lfs.catchall:  
> >> sha256:d4ec46c2869ba22eceb42a729377432052d9dd75d82fc40390ebaadecee87ee9
> >> +  2 lfs.test:  
> >> sha256:5489e6ced8c36a7b267292bde9fd5242a5f80a7482e8f23fa0477393dfaa4d6c
> >
> > Including removed files makes sense, but I think the pointer should be  
> > "null"
> > in which case because the file doesn't exist. I have no idea how the  
> > "null"
> > lfs pointer should look like, though.
> 
> Interesting thought, I hadn't considered it.  I guess was going for being  
> able to identify which file was removed.  But that doesn't quite work,  
> because lfs files can transition to non-lfs first.
> 
> My only concern would be not complicating the template writing.  Having to  
> wrap lfspointer inside an {if()} before accessing lfspointer.foo or  
> lfspointer % oid seems unfortunate. IDK how to represent a null pointer

If lfspointer is an empty dict, lfspointer.foo and % oid should just work.

> either (internally or externally), so I guess I'll let it sit a few days  
> to see if anyone has an idea.  I'm waiting on stable to merge before I can  
> submit a similar patch for set:lfs().

Okay, merged and pushed to hg-committed.

Patch

diff --git a/hgext/lfs/__init__.py b/hgext/lfs/__init__.py
--- a/hgext/lfs/__init__.py
+++ b/hgext/lfs/__init__.py
@@ -375,8 +375,8 @@ 

      makemap = lambda v: {
          'file': v,
-        'lfsoid': pointers[v].oid(),
-        'lfspointer': templatekw.hybriddict(pointer(v)),
+        'lfsoid': pointers[v].oid() if pointers[v].get('oid') else None,
+        'lfspointer': templatekw.hybriddict(pointer(v))
      }

      # TODO: make the separator ', '?
diff --git a/hgext/lfs/wrapper.py b/hgext/lfs/wrapper.py
--- a/hgext/lfs/wrapper.py
+++ b/hgext/lfs/wrapper.py
@@ -375,8 +375,12 @@ 
      result = {}
      for f in ctx.files():
          p = pointerfromctx(ctx, f, removed=removed)
+        p2 = pointerfromctx(ctx, f)
          if p:
-            result[f] = p
+            if p2:
+                result[f] = p
+            else:
+                result[f] = {k: None for k, v in p.iteritems()}
      return result

  def uploadblobs(repo, pointers):
diff --git a/tests/test-lfs.t b/tests/test-lfs.t
--- a/tests/test-lfs.t
+++ b/tests/test-lfs.t
@@ -1035,13 +1035,21 @@ 
  {lfs_files} will list deleted files too

    $ hg log -T "{lfs_files % '{rev} {file}: {lfspointer.oid}\n'}"
-  6 lfs.test:  
sha256:43f8f41171b6f62a6b61ba4ce98a8a6c1649240a47ebafd43120aa215ac9e7f6
+  6 lfs.test:
    5 lfs.test:  
sha256:43f8f41171b6f62a6b61ba4ce98a8a6c1649240a47ebafd43120aa215ac9e7f6
    3 lfs.catchall:  
sha256:31f43b9c62b540126b0ad5884dc013d21a61c9329b77de1fceeae2fc58511573
    3 lfs.test:  
sha256:8acd23467967bc7b8cc5a280056589b0ba0b17ff21dbd88a7b6474d6290378a6
    2 lfs.catchall:  
sha256:d4ec46c2869ba22eceb42a729377432052d9dd75d82fc40390ebaadecee87ee9
    2 lfs.test:  
sha256:5489e6ced8c36a7b267292bde9fd5242a5f80a7482e8f23fa0477393dfaa4d6c

+  $ hg log -T "{lfs_files % '{rev} {file}: {lfspointer}\n'}"
+  6 lfs.test: version=None oid=None size=None x-is-binary=None
+  5 lfs.test: version=https://git-lfs.github.com/spec/v1  
oid=sha256:43f8f41171b6f62a6b61ba4ce98a8a6c1649240a47ebafd43120aa215ac9e7f6  
size=25 x-is-binary=0