[MERGE] Implement hard-link support for branch and checkout

[Aaron Bentley]

Ian Clatworthy wrote:
> Aaron Bentley wrote:
>> +    def create_hardlink(self, path, trans_id):
>> +        """Schedule creation of a hard link"""
>> +        name = self._limbo_name(trans_id)
>> +        os.link(path, name)
>> +        try:
>> +            unique_add(self._new_contents, trans_id, 'file')
>> +        except:
>> +            # Clean up the file, it never got registered so
>> +            # TreeTransform.finalize() won't clean it up.
>> +            os.unlink(name)
>> +            raise
>> +
> 
> Bare except clauses are considered bad. Please be explicit about the
> class of exceptions being caught.

As Andrew notes, catching and then re-raising is generally the exception
to "bare excepts bad".  There is no particular class of exception I am
handling.  Whatever the exception, I want to unlink here, or else the
TreeTransform will be in a bogus state, and the limbo directory won't be
cleaned out properly.

>> +            offset = num +1 - len(deferred_contents)
> 
> Need a space between the + and 1.

Sure.

>>          iter = accelerator_tree._iter_changes(tree, include_unchanged=True)
>>          unchanged = dict((f, p[0]) for (f, p, c, v, d, n, k, e)
>> -                         in iter if not c)
>> +                         in iter if not (c or e[0] != e[1]))
> 
> This edit caught my attention. Why isn't a changed execute flag detected
> by _iter_changes() as a change?

It is detected as a change by _iter_changes.  If the include_unchanged
flag weren't there, any file with its execute bit changed would still be
listed.

But the c flag represents a content change, and e is considered a
metadata change, not a content change.  Similarly, we don't set 'c' if
the file is moved or renamed, because those are name changes, not
content changes

The behavior is described in InterTree._iter_changes:
        Changed_content is True if the file's content has changed.  This
        includes changes to its kind, and to a symlink's target.

If there are ways you think the docstring could be improved, please
suggest them.  But there is no generic "file was modified in some way"
flag, because as originally conceived, any file listed by _iter_changes
has been modified in some way.

>> +    count = 0
>> +    for count, (trans_id, contents) in enumerate(tree.iter_files_bytes(
>> +                                                 new_desired_files)):
>> +        tt.create_file(contents, trans_id)
>> +        pb.update('Adding file contents', count+offset, total)
> 
> The 'count = 0' line is redundant.

Okay, will fix.

> As part of this patch
> though, can you please add a note, over and above the option text, to
> the online help for branch and checkout saying something like:
> 
> "If you use the hardlink option, be sure to configure your editor to
> break hardlinks when saving." or something like that.

Okay.

> I wonder what makes the most sense UI wise if the hardlink option is
> specified but not supported on a given platform?

Hard links are supported on Mac, Windows and Unix, but I think Python
doesn't provide the API on Windows.  It might make sense to omit the
option on platforms that don't support it.

> I think this ought to
> fail cleanly with an explicit error message. This looks easy to trap in
> cmd_branch and cmd_checkout but the code and tests can come in a
> follow-up patch.

Sure, that would be another way of handling it.

> I'm also thinking that the users wanting this feature will
...
> make it an alias, i.e.
> 
>   branch=branch --hardlink
> 
> Given this, if the platform supports it but the filesystem doesn't or
> they cross filesystem boundaries, we might want to think about whether
> failing outright (as I think the code will do now) is really the right
> thing to do. We could output a warning that the request for hardlinking
> is being ignored and continue, perhaps?

I think frequently they'll want to fix it by changing their target so
that it's on the correct filesystem.  So I think failing and letting
them decide how to correct it is fine.  They can easily override the
alias with --no-hardlink.

Aaron