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

[Mads Kiilerich]

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