Patchwork largefiles: Commit directories that only contain largefiles (issue3548)

login
register
mail settings
Submitter Levi Bard
Date Dec. 10, 2012, 2:01 p.m.
Message ID <fcf5fda9138ea94bce52.1355148091@retina-monoteam>
Download mbox | patch
Permalink /patch/39/
State Superseded
Commit 7e2b9f6a2cd043966fca50a29a9867fb12387a22
Headers show

Comments

Levi Bard - Dec. 10, 2012, 2:01 p.m.
# HG changeset patch
# User Levi Bard <levi at unity3d.com>
# Date 1355147922 -3600
# Node ID fcf5fda9138ea94bce52a03c7af618904b50f16c
# Parent  40374059d227850ec2f5fb4f21a1b619136e2a6a
largefiles: Commit directories that only contain largefiles (issue3548)

If we pass a directory to commit whose only commitable files
are largefiles, the core commit code aborts before finding
the largefiles.
So we do the following:
For directories that only have largefiles as matches,
we explicitly add the largefiles to the matchlist and remove
the directory.
In other cases, we leave the match list unmodified.
Kevin Bullock - Dec. 10, 2012, 6:20 p.m.
On Dec 10, 2012, at 8:01 AM, Levi Bard wrote:

> # HG changeset patch
> # User Levi Bard <levi at unity3d.com>
> # Date 1355147922 -3600
> # Node ID fcf5fda9138ea94bce52a03c7af618904b50f16c
> # Parent  40374059d227850ec2f5fb4f21a1b619136e2a6a
> largefiles: Commit directories that only contain largefiles (issue3548)
> 
> If we pass a directory to commit whose only commitable files
> are largefiles, the core commit code aborts before finding
> the largefiles.
> So we do the following:
> For directories that only have largefiles as matches,
> we explicitly add the largefiles to the matchlist and remove
> the directory.
> In other cases, we leave the match list unmodified.

Oof, the wrapping here makes my eyes burn. Whitespace and bullets (asterisks or hyphens) are your friends. :)

Also, the first sentence of the long description isn't clear if it's describing the state before or after this patch.

Code looks superficially good, but I'm willfully unfamiliar with the largefiles code.

> diff -r 40374059d227 -r fcf5fda9138e tests/test-largefiles.t
> --- a/tests/test-largefiles.t	Tue Nov 27 22:24:02 2012 +0100
> +++ b/tests/test-largefiles.t	Mon Dec 10 14:58:42 2012 +0100
> @@ -345,6 +345,59 @@
>   A sub2/large6
>   A sub2/large7
> 
> +Committing directories containing only large files.

+1 for adding a test without adding an `hg init`.

pacem in terris / ??? / ?????? / ????????? / ??
Kevin R. Bullock
Matt Harbison - Dec. 11, 2012, 5:37 a.m.
On Mon, 10 Dec 2012 15:01:31 +0100, Levi Bard wrote:

> # HG changeset patch
> # User Levi Bard <levi at unity3d.com>
> # Date 1355147922 -3600
> # Node ID fcf5fda9138ea94bce52a03c7af618904b50f16c
> # Parent  40374059d227850ec2f5fb4f21a1b619136e2a6a
> largefiles: Commit directories that only contain largefiles (issue3548)
              ^

One small issue- this command used to get rejected:

  $ hg ci -m "standin" .hglf
  abort: .hglf: no match under directory!
  [255]

But now it succeeds, sort of:

  $ hg ci -m "standin" .hglf
  Invoking status precommit hook
  M large3
  A large5
  A sub2/large6
  A sub2/large7
  A z/y/x/m/normal
  $ hg st
  M large3
  A large5
  A sub2/large6
  A sub2/large7
  A z/y/x/m/normal

(No error/exit failure, but no apparent change either, in which case the
exit code should be 1.  I'm not sure which of the before and after is the
proper behavior.)

A question below, but otherwise looks good.
 
> diff -r 40374059d227 -r fcf5fda9138e hgext/largefiles/reposetup.py
> --- a/hgext/largefiles/reposetup.py	Tue Nov 27 22:24:02 2012 +0100
> +++ b/hgext/largefiles/reposetup.py	Mon Dec 10 14:58:42 2012 +0100
...
> @@ -467,6 +463,60 @@
>              return super(lfilesrepo, self).push(remote, force, revs,
>                  newbranch)
>  
> +        def _subdirlfs(self, files, lfiles):
> +            '''
> +            Adjust matched file list
> +            If we pass a directory to commit whose only commitable files
> +            are largefiles, the core commit code aborts before finding
> +            the largefiles.
> +            So we do the following:
> +            For directories that only have largefiles as matches,
> +            we explicitly add the largefiles to the matchlist and remove
> +            the directory.
> +            In other cases, we leave the match list unmodified.
> +            '''
> +            actualfiles = []
> +            dirs = []
> +            regulars = []
> +
> +            for f in files:
> +                if lfutil.isstandin(f):
> +                    raise util.Abort(
> +                        _('file "%s" is a largefile standin') % f,
> +                        hint=('commit the largefile itself instead'))
> +                # Scan directories
> +                if os.path.isdir(self.wjoin(f)):
> +                    dirs.append(f)
> +                else:
> +                    regulars.append(f)
> +
> +            for f in dirs:
> +                matcheddir = False
> +                d = self.dirstate.normalize(f) + '/'
> +                # Check for matched normal files
> +                for mf in regulars:
> +                    if self.dirstate.normalize(mf).startswith(d):
> +                        actualfiles.append(f)
> +                        matcheddir = True
> +                        break
> +                if not matcheddir:
> +                    # If no normal match, manually append
> +                    # any matching largefiles
> +                    for lf in lfiles:
> +                        if self.dirstate.normalize(lf).startswith(d):
> +                            actualfiles.append(lf)
> +                            if not matcheddir:
> +                                actualfiles.append(lfutil.standin(f))
> +                                matcheddir = True

I can't figure out why the previous 3 lines are needed.  I commented them
out and the test blew up, so they are needed.  It appears that all of the
largefiles are added to actualfiles, and at the same time, only the first
corresponding standin (per directory) is.  Why are the standins needed at
all here, since the largefiles are provided?  And since at least one
seems to be needed, why aren't all of the standins needed?

> +                # Nothing in dir, so readd it
> +                # and let commit reject it
> +                if not matcheddir:
> +                    actualfiles.append(f)
> +
> +            # Always add normal files
> +            actualfiles += regulars
> +            return actualfiles
> +
>      repo.__class__ = lfilesrepo
>  
>      def checkrequireslfiles(ui, repo, **kwargs):
Levi Bard - Dec. 11, 2012, 11:53 a.m.
> One small issue- this command used to get rejected:

I'll revise the patch to address this behavior change.

> > +                            if not matcheddir:
> > +                                actualfiles.append(lfutil.standin(f))
> > +                                matcheddir = True
>
> I can't figure out why the previous 3 lines are needed.  I commented them
> out and the test blew up, so they are needed.  It appears that all of the
> largefiles are added to actualfiles, and at the same time, only the first
> corresponding standin (per directory) is.  Why are the standins needed at
> all here, since the largefiles are provided?  And since at least one
> seems to be needed, why aren't all of the standins needed?

It's adding the standin for the directory, for consistency.

Patch

diff -r 40374059d227 -r fcf5fda9138e hgext/largefiles/reposetup.py
--- a/hgext/largefiles/reposetup.py	Tue Nov 27 22:24:02 2012 +0100
+++ b/hgext/largefiles/reposetup.py	Mon Dec 10 14:58:42 2012 +0100
@@ -359,11 +359,8 @@ 
                     lfdirstate.write()
                     return result
 
-                for f in match.files():
-                    if lfutil.isstandin(f):
-                        raise util.Abort(
-                            _('file "%s" is a largefile standin') % f,
-                            hint=('commit the largefile itself instead'))
+                lfiles = lfutil.listlfiles(self)
+                match._files = self._subdirlfs(match.files(), lfiles)
 
                 # Case 2: user calls commit with specified patterns: refresh
                 # any matching big files.
@@ -394,7 +391,6 @@ 
                 # standins corresponding to the big files requested by the
                 # user.  Have to modify _files to prevent commit() from
                 # complaining "not tracked" for big files.
-                lfiles = lfutil.listlfiles(self)
                 match = copy.copy(match)
                 origmatchfn = match.matchfn
 
@@ -467,6 +463,60 @@ 
             return super(lfilesrepo, self).push(remote, force, revs,
                 newbranch)
 
+        def _subdirlfs(self, files, lfiles):
+            '''
+            Adjust matched file list
+            If we pass a directory to commit whose only commitable files
+            are largefiles, the core commit code aborts before finding
+            the largefiles.
+            So we do the following:
+            For directories that only have largefiles as matches,
+            we explicitly add the largefiles to the matchlist and remove
+            the directory.
+            In other cases, we leave the match list unmodified.
+            '''
+            actualfiles = []
+            dirs = []
+            regulars = []
+
+            for f in files:
+                if lfutil.isstandin(f):
+                    raise util.Abort(
+                        _('file "%s" is a largefile standin') % f,
+                        hint=('commit the largefile itself instead'))
+                # Scan directories
+                if os.path.isdir(self.wjoin(f)):
+                    dirs.append(f)
+                else:
+                    regulars.append(f)
+
+            for f in dirs:
+                matcheddir = False
+                d = self.dirstate.normalize(f) + '/'
+                # Check for matched normal files
+                for mf in regulars:
+                    if self.dirstate.normalize(mf).startswith(d):
+                        actualfiles.append(f)
+                        matcheddir = True
+                        break
+                if not matcheddir:
+                    # If no normal match, manually append
+                    # any matching largefiles
+                    for lf in lfiles:
+                        if self.dirstate.normalize(lf).startswith(d):
+                            actualfiles.append(lf)
+                            if not matcheddir:
+                                actualfiles.append(lfutil.standin(f))
+                                matcheddir = True
+                # Nothing in dir, so readd it
+                # and let commit reject it
+                if not matcheddir:
+                    actualfiles.append(f)
+
+            # Always add normal files
+            actualfiles += regulars
+            return actualfiles
+
     repo.__class__ = lfilesrepo
 
     def checkrequireslfiles(ui, repo, **kwargs):
diff -r 40374059d227 -r fcf5fda9138e tests/test-largefiles.t
--- a/tests/test-largefiles.t	Tue Nov 27 22:24:02 2012 +0100
+++ b/tests/test-largefiles.t	Mon Dec 10 14:58:42 2012 +0100
@@ -345,6 +345,59 @@ 
   A sub2/large6
   A sub2/large7
 
+Committing directories containing only largefiles.
+
+  $ mkdir -p z/y/x/m
+  $ touch z/y/x/m/large1
+  $ touch z/y/x/large2
+  $ hg add --large z/y/x/m/large1 z/y/x/large2
+  $ hg commit -m "Subdir with directory only containing largefiles" z
+  Invoking status precommit hook
+  M large3
+  A large5
+  A sub2/large6
+  A sub2/large7
+  A z/y/x/large2
+  A z/y/x/m/large1
+  $ hg rollback --quiet
+  $ touch z/y/x/m/normal
+  $ hg add z/y/x/m/normal
+  $ hg commit -m "Subdir with mixed contents" z
+  Invoking status precommit hook
+  M large3
+  A large5
+  A sub2/large6
+  A sub2/large7
+  A z/y/x/large2
+  A z/y/x/m/large1
+  A z/y/x/m/normal
+  $ hg st
+  M large3
+  A large5
+  A sub2/large6
+  A sub2/large7
+  $ hg rollback --quiet
+  $ hg revert z/y/x/large2 z/y/x/m/large1
+  $ rm z/y/x/large2 z/y/x/m/large1
+  $ hg commit -m "Subdir with normal contents" z
+  Invoking status precommit hook
+  M large3
+  A large5
+  A sub2/large6
+  A sub2/large7
+  A z/y/x/m/normal
+  $ hg st
+  M large3
+  A large5
+  A sub2/large6
+  A sub2/large7
+  $ hg rollback --quiet
+  $ hg revert --quiet z
+  $ hg commit -m "Empty subdir" z
+  abort: z: no match under directory!
+  [255]
+  $ rm -rf z
+
 Test "hg status" with combination of 'file pattern' and 'directory
 pattern' for largefiles: