This is an archive of the discontinued Mercurial Phabricator instance.

context: add overlayworkingcontext and overlayworkingfilectx
ClosedPublic

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

Details

Summary

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

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

phillco created this revision.Sep 3 2017, 3:41 PM
martinvonz added inline comments.
mercurial/context.py
1987–1989

It sounds like a shorter version of

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

is

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

maybe these IOErrors should be ProgrammingError?

2029–2034

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)
2035

a little unnecessary :-)

2064

can reduce some repetition by extracting something like

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

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

2085

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

2093

you're not setting date here, which seems inconsistent

2094

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)

2103

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

2126

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)

2145–2149

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
mercurial/context.py
2027

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

2030–2035

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

2038

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

2053

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

2099

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

2099–2111
if path not in self._cache:
    self._writeorder.append(path)
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
mercurial/context.py
2006

test-check-pyflakes.t fails with

+  mercurial/context.py:2009: undefined name 'ProgrammingError'
+  mercurial/context.py:2025: undefined name 'ProgrammingError'
+  mercurial/context.py:2068: undefined name 'ProgrammingError'

I'll qualify it in flight

phillco added inline comments.Sep 11 2017, 6:14 PM
mercurial/context.py
2006

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

2145–2149

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 context.py 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/__init__.py (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).