Patchwork [STABLE] lfs: avoid a potential variable reference before assignment error in cmdserver

login
register
mail settings
Submitter Matt Harbison
Date Oct. 10, 2018, 5:19 p.m.
Message ID <51e0af18304005dc62d1.1539191993@mharbison-pc.attotech.com>
Download mbox | patch
Permalink /patch/35621/
State Accepted
Headers show

Comments

Matt Harbison - Oct. 10, 2018, 5:19 p.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1539188728 14400
#      Wed Oct 10 12:25:28 2018 -0400
# Branch stable
# Node ID 51e0af18304005dc62d1debe1bba6614140056ab
# Parent  3e61146d7b243078101ba1a554d90490fc915f71
lfs: avoid a potential variable reference before assignment error in cmdserver

A coworker hit this once yesterday when pulling in thg (a retry worked), and
then I hit it with strip after a pull.  I had a difficult time recreating a test
for this (at least one of the tricks was to not use '-R', which seems to cause
reposetup() to be called for each command), so I'm not sure how large of a
window there actually is for this.  Calling reposetup() *after* the requirement
is added will skip the hook entirely.

The other issue I had was adding a couple `ui.status()` lines around the check
that installs the hook.  On Windows, the cmdserver process ballooned to 1.6GB
and hung.  Changing that to `ui.warn()` avoided the hang.  It also hung on
macOS, but without the large memory usage.
Yuya Nishihara - Oct. 12, 2018, 5:25 a.m.
On Wed, 10 Oct 2018 13:19:53 -0400, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1539188728 14400
> #      Wed Oct 10 12:25:28 2018 -0400
> # Branch stable
> # Node ID 51e0af18304005dc62d1debe1bba6614140056ab
> # Parent  3e61146d7b243078101ba1a554d90490fc915f71
> lfs: avoid a potential variable reference before assignment error in cmdserver

Queued for stable, thanks.

Patch

diff --git a/hgext/lfs/__init__.py b/hgext/lfs/__init__.py
--- a/hgext/lfs/__init__.py
+++ b/hgext/lfs/__init__.py
@@ -229,13 +229,15 @@  def reposetup(ui, repo):
 
     if 'lfs' not in repo.requirements:
         def checkrequireslfs(ui, repo, **kwargs):
-            if 'lfs' not in repo.requirements:
-                last = kwargs.get(r'node_last')
-                _bin = node.bin
-                if last:
-                    s = repo.set('%n:%n', _bin(kwargs[r'node']), _bin(last))
-                else:
-                    s = repo.set('%n', _bin(kwargs[r'node']))
+            if 'lfs' in repo.requirements:
+                return 0
+
+            last = kwargs.get(r'node_last')
+            _bin = node.bin
+            if last:
+                s = repo.set('%n:%n', _bin(kwargs[r'node']), _bin(last))
+            else:
+                s = repo.set('%n', _bin(kwargs[r'node']))
             match = repo.narrowmatch()
             for ctx in s:
                 # TODO: is there a way to just walk the files in the commit?
diff --git a/tests/test-lfs-serve.t b/tests/test-lfs-serve.t
--- a/tests/test-lfs-serve.t
+++ b/tests/test-lfs-serve.t
@@ -220,6 +220,30 @@  should have an 'lfs' requirement after i
   $TESTTMP/client3_pull/.hg/requires:lfs (lfsremote-on !)
   $TESTTMP/server/.hg/requires:lfs (lfsremote-on !)
 
+Test that the commit/changegroup requirement check hook can be run multiple
+times.
+
+  $ hg clone -qr 0 http://localhost:$HGPORT $TESTTMP/cmdserve_client3
+
+  $ cd ../cmdserve_client3
+
+  >>> from __future__ import absolute_import
+  >>> from hgclient import check, readchannel, runcommand
+  >>> @check
+  ... def addrequirement(server):
+  ...     readchannel(server)
+  ...     # change the repo in a way that adds the lfs requirement
+  ...     runcommand(server, ['pull', '-qu'])
+  ...     # Now cause the requirement adding hook to fire again, without going
+  ...     # through reposetup() again.
+  ...     with open('file.txt', 'wb') as fp:
+  ...         fp.write('data')
+  ...     runcommand(server, ['ci', '-Aqm', 'non-lfs'])
+  *** runcommand pull -qu
+  *** runcommand ci -Aqm non-lfs
+
+  $ cd ../client
+
 The difference here is the push failed above when the extension isn't
 enabled on the server.
   $ hg identify http://localhost:$HGPORT