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

[Matt Harbison]

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 at 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
>