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
Automatic diff as part of commit; lint not applicable.
Unit
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.
mercurial/context.py
1990–1992

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
2010

maybe these IOErrors should be ProgrammingError?

2032–2037

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

a little unnecessary :-)

2067

can reduce some repetition by extracting something like

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

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

2088

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

2096

you're not setting date here, which seems inconsistent

2097

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)

2106

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

2129

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)

2148–2152

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
2030

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

2033–2038

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

2041

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

2056

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

2102

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

2102–2114
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
2009

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
2009

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

2148–2152

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