Patchwork [STABLE] largefiles: remove directories left empty after a move (issue3515)

login
register
mail settings
Submitter Matt Harbison
Date April 26, 2014, 3:18 a.m.
Message ID <8f81353efd714d12568d.1398482283@Envy>
Download mbox | patch
Permalink /patch/4441/
State Accepted
Headers show

Comments

Matt Harbison - April 26, 2014, 3:18 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1398479649 14400
#      Fri Apr 25 22:34:09 2014 -0400
# Branch stable
# Node ID 8f81353efd714d12568dfabc77074f627a0f0ae6
# Parent  d36440d843284ba546858b241da9cc4817811fe5
largefiles: remove directories left empty after a move (issue3515)

Naming the standin for inexact matches is unfortunate, but an existing issue.
Maybe something similar to 7e2b9f6a2cd0 is a solution.
Mads Kiilerich - April 28, 2014, 10:04 p.m.
Thanks for the patch. I wouldn't complain if it was applied as it is ... 
but I think it would be worth the effort to improve it a bit:

On 04/26/2014 05:18 AM, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1398479649 14400
> #      Fri Apr 25 22:34:09 2014 -0400
> # Branch stable
> # Node ID 8f81353efd714d12568dfabc77074f627a0f0ae6
> # Parent  d36440d843284ba546858b241da9cc4817811fe5
> largefiles: remove directories left empty after a move (issue3515)

More clear, something like: remove directories left empty after their 
files has been moved

> Naming the standin for inexact matches is unfortunate, but an existing issue.

I don't get that comment. Please clarify how that is related to this 
patch and make it more clear ... or leave it out.

> Maybe something similar to 7e2b9f6a2cd0 is a solution.

That changeset introduced more issues than it fixed ... and I haven't 
found a way to fix it properly. So whatever the point is, don't make it 
too similar ;-)

> diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
> --- a/hgext/largefiles/overrides.py
> +++ b/hgext/largefiles/overrides.py
> @@ -579,6 +579,7 @@
>                           os.makedirs(destlfiledir)
>                       if rename:
>                           os.rename(repo.wjoin(srclfile), repo.wjoin(destlfile))
> +                        util.unlinkpath(repo.wjoin(srclfile), True)

Removing a file immediately after moving it seems a bit weird. It 
deserves a comment that we do it because of the side effect of 
unlinkpath that it removes empty directories.

Even better: introduce a platform specific util.unlinkpath and use that 
instead.

>                           lfdirstate.remove(srclfile)
>                       else:
>                           util.copyfile(repo.wjoin(srclfile),
> diff --git a/tests/test-largefiles.t b/tests/test-largefiles.t
> --- a/tests/test-largefiles.t
> +++ b/tests/test-largefiles.t
> @@ -214,8 +214,19 @@
>     ./baz/largefile
>     ./dirb
>     ./dirb/largefile
> -  ./foo

This missing foo directory shows that your patch works ... but fine that 
the added test coverage shows that it not only works when the last file 
explicitly is moved, but also when moving a whole directory.

> -  $ cd ../../a
> +  $ cd ..
> +  $ hg mv dira dirc
> +  moving .hglf/dira/baz/largefile to .hglf/dirc/baz/largefile (glob)
> +  moving .hglf/dira/dirb/largefile to .hglf/dirc/dirb/largefile (glob)
> +  $ find . -not -path './.hg*' | sort

Why not just keep it simple: find * | sort

> +  .
> +  ./dirc
> +  ./dirc/baz
> +  ./dirc/baz/largefile
> +  ./dirc/dirb
> +  ./dirc/dirb/largefile
> +  $ hg up -qC
> +  $ cd ../a
>

/Mads
Matt Harbison - April 29, 2014, 2:45 a.m.
Mads Kiilerich wrote:
> Thanks for the patch. I wouldn't complain if it was applied as it is ...
> but I think it would be worth the effort to improve it a bit:
>
> On 04/26/2014 05:18 AM, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1398479649 14400
>> # Fri Apr 25 22:34:09 2014 -0400
>> # Branch stable
>> # Node ID 8f81353efd714d12568dfabc77074f627a0f0ae6
>> # Parent d36440d843284ba546858b241da9cc4817811fe5
>> largefiles: remove directories left empty after a move (issue3515)
>
> More clear, something like: remove directories left empty after their
> files has been moved

Unfortunately, that doesn't fit with the issue marker.  My second choice 
doesn't fit either.  I'll put a new one in for V2, but I don't mind if 
whoever queues it tweaks it for clarity if needed.

>> Naming the standin for inexact matches is unfortunate, but an existing
>> issue.
>
> I don't get that comment. Please clarify how that is related to this
> patch and make it more clear ... or leave it out.
>
>> Maybe something similar to 7e2b9f6a2cd0 is a solution.
>
> That changeset introduced more issues than it fixed ... and I haven't
> found a way to fix it properly. So whatever the point is, don't make it
> too similar ;-)

Yeah, I debated whether or not to put it in.  But I figured it would be 
useful to draw attention to it if anyone has a better idea now, or if 
someone in the future has too much time on their hands and tries to fix 
it.  I'll remove it though since the referenced cset is problematic.

The easiest fix is to factor the 'if ui.verbose or not exact' block in 
cmdutil.copyfile() into a function that largefiles can replace, to not 
print the standins.  But since I can't see any other use for that 
refactoring, I didn't think it would be well received, and certainly not 
during a freeze.  Are there any guidelines for when refactoring is OK? 
Largefiles tend to wrap a lot of stuff with a wide scope instead of 
being more surgical, which makes me think the bar is rather high.

As an aside, what is the problem with 7e2b9f6a2cd0?  I thought that was 
a clever (but unorthodox) fix.

>> diff --git a/hgext/largefiles/overrides.py
>> b/hgext/largefiles/overrides.py
>> --- a/hgext/largefiles/overrides.py
>> +++ b/hgext/largefiles/overrides.py
>> @@ -579,6 +579,7 @@
>> os.makedirs(destlfiledir)
>> if rename:
>> os.rename(repo.wjoin(srclfile), repo.wjoin(destlfile))
>> + util.unlinkpath(repo.wjoin(srclfile), True)
>
> Removing a file immediately after moving it seems a bit weird. It
> deserves a comment that we do it because of the side effect of
> unlinkpath that it removes empty directories.
>
> Even better: introduce a platform specific util.unlinkpath and use that
> instead.

I don't understand the platform specific part.  It looks like the 
existing util.unlinkpath is an alias (or whatever the python term is) 
for platform.unlinkpath.  I'll put a comment in to clarify for now.

>> lfdirstate.remove(srclfile)
>> else:
>> util.copyfile(repo.wjoin(srclfile),
>> diff --git a/tests/test-largefiles.t b/tests/test-largefiles.t
>> --- a/tests/test-largefiles.t
>> +++ b/tests/test-largefiles.t
>> @@ -214,8 +214,19 @@
>> ./baz/largefile
>> ./dirb
>> ./dirb/largefile
>> - ./foo
>
> This missing foo directory shows that your patch works ... but fine that
> the added test coverage shows that it not only works when the last file
> explicitly is moved, but also when moving a whole directory.

Agreed.  I know we want to avoid duplicating tests to keep the running 
time reasonable, but are there any guidelines on how to balance thorough 
vs minimal tests?

>> - $ cd ../../a
>> + $ cd ..
>> + $ hg mv dira dirc
>> + moving .hglf/dira/baz/largefile to .hglf/dirc/baz/largefile (glob)
>> + moving .hglf/dira/dirb/largefile to .hglf/dirc/dirb/largefile (glob)
>> + $ find . -not -path './.hg*' | sort
>
> Why not just keep it simple: find * | sort

Good call.  I had a one track mind to make find do it, and this is what 
I found when asking google how to make find ignore a directory.

Thanks for the feedback, I will submit v2 shortly.

--Matt

>> + .
>> + ./dirc
>> + ./dirc/baz
>> + ./dirc/baz/largefile
>> + ./dirc/dirb
>> + ./dirc/dirb/largefile
>> + $ hg up -qC
>> + $ cd ../a
>>
>
> /Mads
>
Mads Kiilerich - April 29, 2014, 12:33 p.m.
On 04/29/2014 04:45 AM, Matt Harbison wrote:
>>> Naming the standin for inexact matches is unfortunate, but an existing
>>> issue.
>>
>> I don't get that comment. Please clarify how that is related to this
>> patch and make it more clear ... or leave it out.
>>
>>> Maybe something similar to 7e2b9f6a2cd0 is a solution.
>>
>> That changeset introduced more issues than it fixed ... and I haven't
>> found a way to fix it properly. So whatever the point is, don't make it
>> too similar ;-)
>
> Yeah, I debated whether or not to put it in.  But I figured it would 
> be useful to draw attention to it if anyone has a better idea now, or 
> if someone in the future has too much time on their hands and tries to 
> fix it.  I'll remove it though since the referenced cset is problematic.

Oh - you are referring to the "moving .hglf/x to .hglf/y" messages. I 
didn't get that. It could perhaps be stated more clearly.

> The easiest fix is to factor the 'if ui.verbose or not exact' block in 
> cmdutil.copyfile() into a function that largefiles can replace, to not 
> print the standins.  But since I can't see any other use for that 
> refactoring, I didn't think it would be well received, and certainly 
> not during a freeze.  Are there any guidelines for when refactoring is 
> OK? Largefiles tend to wrap a lot of stuff with a wide scope instead 
> of being more surgical, which makes me think the bar is rather high.
>

Largefiles started out of tree supporting multiple Mercurial revisions 
and haven't yet reached the same quality and integration as other 
extensions. Refactoring seems to be more work than starting from scratch 
... but that is hardly an option.

> As an aside, what is the problem with 7e2b9f6a2cd0?  I thought that 
> was a clever (but unorthodox) fix.

The problem is that 'hg commit foo' should commit both foo/* and 
.hglf/foo/* and it is ok that one of them fails to find a match if the 
other one do. That change did not handle all cases correctly.

>>> diff --git a/hgext/largefiles/overrides.py
>>> b/hgext/largefiles/overrides.py
>>> --- a/hgext/largefiles/overrides.py
>>> +++ b/hgext/largefiles/overrides.py
>>> @@ -579,6 +579,7 @@
>>> os.makedirs(destlfiledir)
>>> if rename:
>>> os.rename(repo.wjoin(srclfile), repo.wjoin(destlfile))
>>> + util.unlinkpath(repo.wjoin(srclfile), True)
>>
>> Removing a file immediately after moving it seems a bit weird. It
>> deserves a comment that we do it because of the side effect of
>> unlinkpath that it removes empty directories.
>>
>> Even better: introduce a platform specific util.unlinkpath and use that
>> instead.
>
> I don't understand the platform specific part.  It looks like the 
> existing util.unlinkpath is an alias (or whatever the python term is) 
> for platform.unlinkpath.

Sorry, I meant removedirs (where windows.unlinkpath currently calls a 
windows._removedirs) which is the side effect you really want.

/Mads

Patch

diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
--- a/hgext/largefiles/overrides.py
+++ b/hgext/largefiles/overrides.py
@@ -579,6 +579,7 @@ 
                         os.makedirs(destlfiledir)
                     if rename:
                         os.rename(repo.wjoin(srclfile), repo.wjoin(destlfile))
+                        util.unlinkpath(repo.wjoin(srclfile), True)
                         lfdirstate.remove(srclfile)
                     else:
                         util.copyfile(repo.wjoin(srclfile),
diff --git a/tests/test-largefiles.t b/tests/test-largefiles.t
--- a/tests/test-largefiles.t
+++ b/tests/test-largefiles.t
@@ -214,8 +214,19 @@ 
   ./baz/largefile
   ./dirb
   ./dirb/largefile
-  ./foo
-  $ cd ../../a
+  $ cd ..
+  $ hg mv dira dirc
+  moving .hglf/dira/baz/largefile to .hglf/dirc/baz/largefile (glob)
+  moving .hglf/dira/dirb/largefile to .hglf/dirc/dirb/largefile (glob)
+  $ find . -not -path './.hg*' | sort
+  .
+  ./dirc
+  ./dirc/baz
+  ./dirc/baz/largefile
+  ./dirc/dirb
+  ./dirc/dirb/largefile
+  $ hg up -qC
+  $ cd ../a
 
 #if serve
 Test display of largefiles in hgweb