| martinvonz |
| hg-reviewers |
With in-memory merge, backup files might be overlayworkingfilectxs stored
in memory. But they could also be real files if the user's backup directory is
outside the working dir.
Rather than have two code paths everywhere, let's use arbitraryfilectx so they
can be consistent.
| Lint Skipped |
| Unit Tests Skipped |
| mercurial/context.py | ||
|---|---|---|
| 700–704 | Feel like checking out the "abc" module and see if that's helpful here? | |
| 854 | Looks like either _customcmp should be part of abstractfilectx or checked for here (util.safehasattr()) | |
| mercurial/filemerge.py | ||
| 744 | The documentation for filecmp.cmp() says Unless shallow is given and is false, files with identical os.stat() signatures are taken to be equal. Are we losing out on that optimization with this patch (when not doing in-memory merge). Do you have any sense of how relevant that optimization is? | |
| tests/test-dirstate-race.t | ||
| 85 | Why did this change? | |
| tests/test-dirstate-race.t | ||
|---|---|---|
| 85 | It's caused by the addition of this block into abstractfilectx. if isinstance(fctx, abstractfilectx):
return self.data() != fctx.data()When fctx is a workingfilectx, and has been replaced by a directory, calling data() on it raises an IOError instead of returning True like this function used to do. Interestingly, the test behavior is caused by this error being caught by an existing block inside workingctx._checklookup(): except (IOError, OSError):
# A file become inaccessible in between? Mark it as deleted,
# matching dirstate behavior (issue5584).
# The dirstate has more complex behavior around whether a
# missing file matches a directory, etc, but we don't need to
# bother with that: if f has made it to this point, we're sure
# it's in the dirstate.
deleted.append(f)Which seems to somewhat predict the case of files being unavailable and raising raw IOErrors, although not this case specifically. Also, @sid0's comment around the test case says: XXX Note that this returns M for files that got replaced by directories. This is definitely a bug, but the fix for that is hard and the next status run is fine anyway. Which is in fact the case for e. So maybe continuing to throw is actually the better behavior here, given that it would also throw if fctx was missing so the idea isn't unprecedented. On the other hand, just catching the error and returning True is the most conservative path forward, so I might just do that here. | |
| mercurial/filemerge.py | ||
|---|---|---|
| 744 | Yes, and it's a bigger impact than past changes of this nature, since each merged file, and its backup file, will be read again. So I think we need some way to reintroduce this fast-path for two disk-backed files inside cmp. I'd propose adding something like this to filectx: def ondisk(): """Returns True iff this filectx is directly backed by a file in the filesystem and not some other abstraction. If so, callers can run system file functions on it for better performance. """ It'd be True only for workingfilectxs and abstractfilectxs. Then, inside abstractfilectx.cmp(), check if both the caller and other are on-disk and use filecmp in that case. It's a naive first take though, so improvements are appreciated. | |
| mercurial/filemerge.py | ||
|---|---|---|
| 744 | Can we not simply make arbitraryfilectx.cmp() something like def cmp(self, fctx):
if isinstance(fctx, arbitraryfilectx):
return filecmp.cmp(self.path(), fctx.path())
return self.data() != otherfilectx.data() | |
| mercurial/filemerge.py | ||
|---|---|---|
| 744 | fcd is a workingfilectx, so it'd need to need to be something like: return filecmp.cmp(self.path(), self._repo.wjoin(fctx.path())) and arbitraryfilectx doesn't have a _repo because we use it in contrib/simplemerge. Maybe we could make it an optional property and raise in this case if it's missing? contrib/simplemerge doesn't need it. | |
One extra comment: maybe include some "why" as well as "what" in your commit message. :)
| mercurial/context.py | ||
|---|---|---|
| 700–705 | abstractfilectx doesn't seem referenced anywhere else, so just revert this part? Maybe you meant to make arbitraryfilectx extend it? That would make sense. If we do, I feel like we should implement the smart cmp() in this abstract base class to avoid the ugly asymmetry in the implementation otherwise (a.cmp(b) might be faster or slower than b.cmp(a) and one might perhaps even crash?). Perhaps we should even make it a top-level function so it will be easier to override by extensions that want to add their own subclasses? Something like: def filectxcmp(fctx1, fctx2):
...
class abstractfilectx(object):
def cmp(self, otherfctx): # don't override in subclasses, wrap filefctxcmp() instead
return filectxcmp(fctx1, fctx2)Maybe there's precedence for this kind of thing elsewhere in Mercurial? Surely at least elsewhere in Python. Or maybe I'm just overthinking this and we're pretty sure all call sites will pass it as arbitraryfilectx.cmp(filectx) and not the other way around, so it won't be a problem in practice. I'm not even sure I got that right (and I don't know where overlayfilectx fits in), which seems like a sign that it's best to have a single cmp() method. | |
| mercurial/filemerge.py | ||
|---|---|---|
| 611 | Isn't repo.wjoin(fcd.path()) the same thing as _workingpath(repo, fcd) inlined? You call the same thing further down, so why not keep the "a" variable (possibly renamed to something better)? | |
| 616 | Why care whether it's in the working directory when doing in-memory merge? Why not instead always (when doing in-memory merge) either redirect the backup to memory or put it outside of the working directory? Is it so if a there's a conflict, it will get flushed to the place the user expects? | |
One extra comment: maybe include some "why" as well as "what" in your commit message. :)
Sorry I missed this, the comment is very valid. Will send a new version.
| mercurial/filemerge.py | ||
|---|---|---|
| 611 | Yeah, not sure why I reverted to wjoin. I'll switch it back. | |
| 616 |
Yes, basically. | |
| mercurial/filemerge.py | ||
|---|---|---|
| 617–624 | I mentioned on IRC the other day that this *could* be left for another patch and that I didn't expect it to be here after reading the commit message. I didn't insist that had to be done, but Phil liked the idea, so he said he'd do it. I just thought I'd point that out here too to get the status right on the dashboard. | |
Feel like checking out the "abc" module and see if that's helpful here?