This is an archive of the discontinued Mercurial Phabricator instance.

context: add overlayworkingcontext and overlayworkingfilectx

Authored by phillco on Sep 3 2017, 3:41 PM.



These two classes will be used extensively in the first in-memory merge

Diff Detail

rHG Mercurial
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

phillco created this revision.Sep 3 2017, 3:41 PM
martinvonz added inline comments.

It sounds like a shorter version of

If `exists` is True, either `data` or `flags` must be non-None (either both, or just `flags`)


If `exists` is True, `flags` must be non-None

maybe these IOErrors should be ProgrammingError?


looks like this can be both simpler and more efficient if written with _markdirty() like this:

def _markdirty(self, path, flags=None, exists=True, data=None, date=None)

a little unnecessary :-)


can reduce some repetition by extracting something like

entry = self._cache[path]
if entry['exists'] is True:
    data = entry['data']
    flags = entry['flags']

doesn't seem like this can happen. i'd drop this and the "is True/False" in the conditions above


should this be "private"? seems kind of risky to call it outside of flushall()


you're not setting date here, which seems inconsistent


i think no flags are usually expressed as empty string, not None, so i think we should at least make sure the return value of flags() follows that (and the simplest way of doing that is probably to change this line)


This can also be written "self._wrappedctx[path]", which seems similarly short and clear, so I'm not sure this method buys us much


Why is it safe to call lexists() here? I don't know how exists() is used, so it's not clear to me. Maybe add a comment (if it actually is safe)


Unrelated to this patch, but this interface seems pretty uglily asymmetric (flags() returns a string of character flags, setflags() take two boolean flags). And that's why you're forced to make the flag handling in your setflags() above ugly.

quark added a subscriber: quark.Sep 8 2017, 2:18 PM

This is where I guess it might be implemented differently. Storing fctx in overlayworkingctx._cache may be possible. It allows us to have some fast path for LFS use-case (ex. skip reading .data() but reuse .rawdata()). It could also simplify read methods by proxying them to getattr(_underlyingctx(path), methodname).

That said, I'm fine with this being committed as is. I'll follow up later once we really need the LFS fast path.

phillco marked 10 inline comments as done.Sep 10 2017, 11:54 PM
phillco updated this revision to Diff 1699.
phillco updated this revision to Diff 1705.Sep 11 2017, 1:22 AM
phillco updated this revision to Diff 1706.Sep 11 2017, 1:25 AM
phillco updated this revision to Diff 1707.
phillco updated this revision to Diff 1708.Sep 11 2017, 1:39 AM
martinvonz added inline comments.Sep 11 2017, 1:55 PM

use flags='' here too and you can drop the check below


forgot to update this section to use _markdirty(path, data=data, flags=flags, date=util.makedate())


drop "exists=True" since it's the default (or remove the default value from the method if you prefer)


I've heard of symlinks that have a trailing newline. I wonder if we need to strip that here.


nit: move "exists" before "flags" since it always makes sense and it determines whether flags+data+date make sense. would be nice to match the order in the dict below

if path not in self._cache:
self._cache[path] = {
        'exists': exists,
        'data': data,
        'date': date,
        'flags': flags,
phillco marked 6 inline comments as done.Sep 11 2017, 4:02 PM
phillco updated this revision to Diff 1723.
martinvonz added inline comments.Sep 11 2017, 5:39 PM

test-check-pyflakes.t fails with

+  mercurial/ undefined name 'ProgrammingError'
+  mercurial/ undefined name 'ProgrammingError'
+  mercurial/ undefined name 'ProgrammingError'

I'll qualify it in flight

phillco added inline comments.Sep 11 2017, 6:14 PM

Ah, thanks. Didn't have pyflakes installed on my test box, as it turned out...


Yes, it is. Possibly worth a future refactor.

This revision was automatically updated to reflect the committed changes.

This is already pushed, but please consider breaking up into a package in the near future - it's far too large to comprehend, and I think it'd help organization if we had each of the various file{ctx} types (mem, working, overlay) in their own file and then re-exported in mercurial/context/ (so no other code would have to change, and external-to-hg clients of memctx (of which there are many) could be ignorant of the move).