This is an archive of the discontinued Mercurial Phabricator instance.

narrow: import experimental extension from narrowhg revision cb51d673e9c5
ClosedPublic

Authored by durin42 on Feb 1 2018, 6:00 PM.

Details

Summary

Adjustments:

  • renamed src to hgext/narrow
  • marked extension experimental
  • added correct copyright header where it was missing
  • updated hgrc extension enable line in library.sh
  • renamed library.sh to narrow-library.sh
  • dropped all files from repo root as they're not interesting
  • dropped test-pyflakes.t, test-check-code.t and test-check-py3-compat.t
  • renamed remaining tests to all be test-narrow-* when they didn't already
  • fixed test-narrow-expanddirstate.t to refer to narrow and not narrowhg
  • fixed tests that wanted update -C . instead of merge --abort
  • corrected a two-space indent in narrowspec.py
  • added a missing _() in narrowcommands.py
  • fixed imports to pass the import checker
  • narrow only adds its --include and --exclude to clone if sparse isn't enabled to avoid breaking test-duplicateoptions.py. This is a kludge, and we'll need to come up with a better solution in the future.

These were more or less the minimum to import something that would
pass tests and not create a bunch of files we'll never use.

Changes I intend to make as followups:

  • rework the test-narrow-*-tree.t tests to use the new testcases functionality in run-tests.py
  • remove lots of monkeypatches of core things

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

durin42 created this revision.Feb 1 2018, 6:00 PM

I glanced at the code and found the first set of needed follow-ups.

hgext/narrow/__init__.py
12–15

This seems inappropriate.

hgext/narrow/narrowbundle2.py
38–47

I think we'll want to UPPERCASE these since they are constants.

224

If this is referring to Mercurial versions, then this code is likely not needed, as a client with narrow support is newer than 3.1 or 3.2.

hgext/narrow/narrowrepo.py
27

I think we'll want to put exp somewhere in the requirement name until we're ready to mark the feature as non-experimental in core.

hgext/narrow/narrowrevlog.py
17–21

This needs to move to core because revision flags need to be reserved there.

hgext/narrow/narrowtemplates.py
46–51

We should be using the registrar API for declaring revsets and templates.

Should I add follow-up commits for the feedback? I think I'd slightly prefer doing that so that we import something that's as minimally modified as possible, and then fix it up all at once, but if you'd rather fix things here that's fine too.

(I agree with the things you caught, I just don't have time for them tonight - maybe tomorrow.)

hgext/narrow/__init__.py
12–15

Yeah, I noticed this after doing the first big extraction. It's done in D1980, but we can probably trivially fold that here. Do you have a preference?

Should I add follow-up commits for the feedback? I think I'd slightly prefer doing that so that we import something that's as minimally modified as possible, and then fix it up all at once, but if you'd rather fix things here that's fine too.

I think we should keep the import as similar to upstream as possible. That creates a cleaner transition of the code. So please make fixes in follow-up commits. We may want to check in a todo.rst file or similar in this initial commit to facilitate tracking known problems from the inception of the code.

durin42 marked 6 inline comments as done.Feb 2 2018, 10:56 AM
durin42 added a subscriber: martinvonz.

Alrighty, I've added some follow-ups. There's one question for Martin here, which I think will result in one more follow-up to improve a comment. Take another look?

hgext/narrow/narrowbundle2.py
224

Hmm, I think this might be talking about client versions in the server code. @martinvonz do you remember what's going on here? If not I can dig more.

martinvonz added inline comments.Feb 2 2018, 11:29 AM
hgext/narrow/narrowbundle2.py
224

Copied from here: https://www.mercurial-scm.org/repo/hg/file/5cfdf6137af8/mercurial/exchange.py#l1811. I'd say leave it since it's harmless and it makes the import simpler. We can drop it after, but that's probably also unnecessary, since the idea is to move this into core and then it will be naturally lost anyway (because it's already in the destination function).

I'm still reviewing. Just thought I'd flush another batch of feedback :)

hgext/narrow/narrowrepo.py
29–34

Is this really the best way to influence the content of the shared file? If it doesn't exist, a good follow-up would be a hook point in core to allow extensions to modify the content of this file so it is only written once.

36–44

Again, my reaction is "eww." But this one is a bit special since we're dealing with a non-standard file. So unless we create a "also write these files with this content" mechanism (which doesn't feel necessary), this is probably the best we can do.

This code should live in core eventually though. So it should work itself out.

indygreg added inline comments.Feb 2 2018, 2:15 PM
hgext/narrow/narrowrepo.py
76

Please add a docstring, since this is a new public method.

86–88

I agree with the inline todo :)

90–92

See comment in narrowspec.py about needing to hold the wlock somewhere.

94–97

I agree with the comment. I'm not an expert on dirstate/status, but this does seem like the wrong place. I'm pretty sure we have code paths that call lower-level status() functions. I would expect this to live in a similar place to where sparse lives.

I think we can overlook this for the initial landing. This feels like a bit of work to resolve and can be deferred to the stabilization period.

hgext/narrow/narrowrevlog.py
2

I'd like to see this file's content moved into core sooner rather than later. There are a lot of implications for storage that need to be in people's minds when they are touching code in core.

32

This condition should always be true.

41

Please add docstring.

58

docstring

67–68

I think we prefer error.ProgrammingError these days.

70

docstring

75–88

error.ProgrammingError

91–95

Consider doing that and changing this to raise if called.

106

Nit: dir is a builtin. If this matches core, fine. But I'd prefer avoiding the name collision.

115–116

In-place mutation of low-level types. Yummy.

Please add a todo for the post-landing list to construct the proper type from the beginning. This likely requires some API changes in core. I'm thinking some function should return the type to use for new revlogs. Or we should spawn this type and call super.init from its init.

129–132

I think this wants a comment explaining why we lie about rename metadata when the destination is outside of the narrow spec. Also, we'll want to flag this for further review, since there could be some interesting implications to lying here.

135–140

This is making assumptions about code that hasn't landed yet. Not sure if we should replace this with a TODO or what.

hgext/narrow/narrowspec.py
147–151

This is called from narrowrepo.setnarrowparts() and neither of them cares about locking. Somewhere we should ensure we hold the wlock.

Another batch before I head off to a meeting...

hgext/narrow/narrowcopies.py
2

Copy code is complex and has performance concerns. I'd like to see this moved into core sooner rather than later. Perhaps a good first step would be to add the narrowmatch method to localrepository? Or something on localrepository in core so we can detect presence of narrow in a one-liner.

hgext/narrow/narrowdirstate.py
2

I'm not an expert on dirstate. But I know enough to know that "here be dragons." I'd like to see everything dirstate moved into core ASAP so it can be better integrated with sparse, etc. People hacking on this code in core need to be aware of the implications for narrow.

hgext/narrow/narrowmerge.py
2

More code that should be on the fast track to core.

26

The more I see this pattern, the more I think there needs to be an attribute on localrepository that can be used for quickly testing if narrow is in play. Actually, should we assume all repos are narrow (with the *all matcher*) and fast path the special case of the default?

hgext/narrow/narrowspec.py
27

The format needs documentation somewhere inline.

32–43

Oh, hey, this looks just like sparse profiles! I sense some code conversion in our future...

50–55

Playing devil's advocate, do we really need two formats doing the same thing?

84

Can we use str.count() to avoid creating objects for each line?

94–96

What about \ as a path separator? Should we also ban that? I assume we're interpreting the value here and paths as bytes, so \ will never be used as an escape character?

111–116

It is better to build a list of lines and '\n'.join() them.

134–139

This reinvents repo.vfs.tryread().

155–163

The purpose of this function was not clear on initial reading. Could you please elaborate a bit more in the docstring?

180

We use set(repo_includes) below to copy a set. Let's make things consistent.

186–193

I feel like there's a more efficient mechanism lurking here. But I'm not sure if we care about the performance implications of this function.

198

Mutating an argument feels a bit wonky. Should this be returned instead?

hgext/narrow/narrowwirepeer.py
28

There is no server-side implementation of this capability/command yet. We may want to remove the reference from core.

Also, the wire protocol capability should have exp or experimental in it until we formalize the API.

I'm still only partly through the changegroup and bundle code. Haven't looked at commands or tests yet. I think I'm going to take a break for a bit because this is a lot of code to absorb!

hgext/narrow/narrowbundle2.py
38

This wants an exp or experimental in its name. Once we start advertising it, there's no going back. So we shouldn't squat on final names until we're sure.

127

map() returns an iterator on Python 3.

141–143

No, we can't. There were bugs in e.g. rebase that caused changed files to be dropped from the files list in the changeset. Walking manifests is the only way to be sure you are getting all changes.

261

Would it make sense to send the narrow spec before the changegroup part?

263–267

Part parameters are effectively header data for bundle2 parts. As such, they are read and written as atomic units. And they are URL quoted for transport safety.

Since narrow specs are unbound in size, I think they should be transferred as part of the part payload, not in parameters. This allows for streaming and doesn't add overhead for encoding/decoding their content.

This is obviously a BC change to the wire protocol and bundle storage.

hgext/narrow/narrowchangegroup.py
48

Might want a comment here that the wrapped method will get inherited by later versions of the changegroup packer. It feels weird to see this definition on cg1packer since code above removes support for changegroup versions 01 and 02.

61–82

I don't fully understand linknodes. This should be scrutinized by someone who does, especially since there are likely performance concerns here.

103–106

I'm not a fan of getattr() and del here. Is there no way to always set these attributes on instance creation?

(One solution is to move this code into changegroup.py.)

hgext/narrow/narrowwirepeer.py
46–49

This will blindly add includepats and excludepats as wire protocol arguments to the unbundle command. In the Python server, wire protocol arguments have to be defined as part of the command handler or the server will refuse to serve the request.

So, this code is missing the server-side declaration of these arguments.

And, the client/peer code here is buggy for not conditionally adding these arguments based on the presence of a server capability advertising support for receiving these arguments.

OK. I've now looked at all the Python code. I haven't yet spent much time on tests. But the amount of tests is encouraging.

I did glance over some of the complicated parts of the Python. Namely some of the algorithms around changegroup and bundle semantics. I will want to understand that mechanism someday. But I think I can delay that to when functionality is moved into core.

As it stands, I'm overall pretty happy with the state of things. Perfect is the enemy of good and I'm totally on board with landing mostly intact and refactoring and applying higher scrutiny once it lands.

The biggest concerns are around the parts that touch storage and wire protocol compatibility. If we write files or advertise things over the wire protocol, we need to be extra careful about the strings used for requirements, capabilities, bundle part names, etc as any future Mercurial client could encounter a requirement, capability, bundle, etc from an old Mercurial and the peers may disagree about the semantics of a given name. As long as these issues are addressed before 4.6 is released, we should be fine. But once something is used in the wild (which I equate to shipping a release), we have backwards and forwards compatibility risks to consider. I'm confident we'll address these before 4.6 is released. I'd like to see more judicial use of experimental in these names as soon as possible though, as that will keep the window for considering compatibility concerns more... narrow.

I'm really tempted to sign off on this review right now. However, I left a pile of comments. And the more of those that can be addressed in the initial landing, the better I'll feel about things. I'm find with this series growing to a few dozen in-flight changesets before anything lands if you are. Although I do want to get the code in as quickly as possible, as that is the best way to bake a feature.

hgext/narrow/narrowbundle2.py
139–152

I'm not an expert on the manifest APIs. There may be a more optimal way to implement this...

271–275

Should this code be above the if narrowservebrokenellipses block above? It seems like it is doing generic argument validation and that validation should be near the top of the function.

337

We may want to remove this feature for the time being, as it is very Google specific. I'm happy to provide a mechanism for extensions to hook in here to enforce ACLs. But there's nothing in core that deals with path-based access control and I think things should remain that way in order to keep the code simple.

For the time being at least. I do acknowledge value in the feature and it is something I'd like to see in core some day. I just think we should stabilize the partial clone functionality first before we start forcing people to think about path-based access control.

379–381

Silently adding a repo requirement during a pull or unbundle operation doesn't feel right to me. At least not at this juncture. For at least as long as the feature is experimental, I think we should refuse to apply (or fetch) a narrow bundle if the repo isn't already narrow. We can provide an hg debug* command to add the requirement to make people's lives easier.

416–417

This compatibility with older Mercurial versions can be removed.

419–420

Doing strip-based operations as part of bundle application breaks read-only consistency of the repository during those operations.

IMO a blocking issue to eventually making this feature non-experimental will be either a refactored store (so we don't need to perform strips to backfill data) or reader locks on the store (to lock out readers when operations such as strip that make the store inconsistent are being performed). Either of those will require a new repo requirement. That requirement is centered around the store and not narrow/shallow/partial clone support. So it needs to be considered a separate feature from all this clone feature work.

444–445

This should be using a context manager instead of try..finally.

451–454

I think strip should be something else to denote this from an actual strip operation.

462–467

This reinvents vfs.tryunlink(). Although you'll still want to catch the OSError from tryunlink() to turn it into a warning. But at least you can not worry about ENOENT.

476–482

I'm not super keen on tacking all these arguments onto the existing getbundle wire protocol command. Especially without experimental labels. I'd rather we invent new wire protocol commands when we add major new features like this. That's something I hope to address in the new wire protocol.

I'm fine with this for now. Just as long as the client does some form of capabilities screening to know which set of arguments the server-side wire protocol command accepts. As long as we fix this during the 4.6 release period, we should be fine. Once version 2 of the wire protocol is done, I'll likely establish a new getbundle like command that is version 2 only and will remove narrow support from the existing getbundle command.

488–490

More ACL code that we may remove from core.

hgext/narrow/narrowchangegroup.py
144–152

Eek - this fully reimplements generate()?! This is probably the hackiest part of this code I've seen so far. I know I've said I want a lot of things moved into core. But considering the complexity of this duplicate code, I think this belongs as front as possible in the queue.

158

This should change to cl.changelogrevision(x) so we return an object with named attributes instead of a tuple. That will make e.g. c[0] change to cl.manifest. cl.read() is just a proxy around changelogrevision() anyway. If the changegroup code uses read(), it should be changed as well.

166

Nit: cl.rev() is called here and inside the conditional. My recollection is this loop is pretty tight, so aliasing clrev = cl.rev and saving the result to avoid another call might have an impact.

314–323

This code is rather difficult to reason about. I'm inclined to defer on a comprehensive review until this code is moved to core. It isn't helping that this file reinvents so much code already in core and it is difficult to reason about how everything plays together. Although the inline comments noting the narrow changes are very useful and appreciated.

hgext/narrow/narrowcommands.py
172–173

This function does a lot and wants a docstring.

172–182

I believe a few operations in this function do obtain a write lock. However, the repo needs to be consistent for the duration of this function. So I think a write lock needs to be taken explicitly as part of the function.

190

Use of repo.changelog in a loop creates perf problems. It's recommended to alias repo.changelog.node to a local and call that, as that also avoids the attribute lookup overhead, which does matter in tight loops.

210

.changelog.node in a tight loop.

219–236

This is extremely low-level code to have in this file. It belongs somewhere in the store API, since it assumes a specific filesystem layout in the store.

254

This function does a lot. It wants a docstring.

tests/test-narrow-archive.t
31

I'm not sure if this invocation of tar is portable. I'm sure others will fix things later if it breaks tests on other platforms.

tests/test-narrow-clone-non-narrow-server.t
22–24

The capabilities command isn't used by the SSH/stdio protocol. This should be using hello instead.

I suspect I'll soon write a patch that doesn't expose the capabilities command to the SSH protocol because it isn't supposed to be used...

I know it isn't a viable feature for Google, but I have an idea that may greatly reduce the burden to getting narrow non-experimental...

What do you think about adding a mode that transfers the full changelog, all manifest revisions for relevant trees, and all filelog revisions for subscribed files. There would be no shallow cloning, ellipsis nodes, linkrev rewriting, etc. This would be a purely narrow clone. I think this would eliminate all the technical problems around having to backfill a store (via strip operations).

If this is technically viable, I think it would allow us to put non-shallow narrow clone on a faster track for being non-experimental, since a purely narrow repo would operate much like a traditional repository: we'd just be interacting with a subset of filelogs on client and server.

This line of thought causes me to ask questions like whether we should split the repo requirement, wire protocol capabilities, etc to more differentiate narrow support from shallow.

None of this should block initial landing. But it's definitely something I'd like the maintainers of narrowhg to think about.

idlsoft added a subscriber: idlsoft.Feb 8 2018, 1:16 PM
idlsoft added inline comments.Feb 8 2018, 1:52 PM
hgext/narrow/narrowbundle2.py
337

I've contributed the ACL code originally.
It's a relatively trivial piece of code, at least compared to the rest of the codebase, and it is called conditionally.
I hope it can stay, my company is about to migrate from git to hg because of it.

379–381

This was added to support seamless clone from an ACLed repo. The user doesn't know ahead of time about server-side narrowing.

durin42 marked 12 inline comments as done.Feb 12 2018, 3:02 PM
durin42 updated this revision to Diff 5498.

Okay, I've pushed more followups. The bulk of the TODOs are recorded in a file introduced in D2196, but there are also some added inline in the code.

Let me know how I can help - I'll make time to video conference this week if it'd facilitate getting this landed with minimal additional lost sanity.

Thanks!

hgext/narrow/narrowbundle2.py
139–152

I wrote lazymanifest, and this was the best adgar and I could come up with...

hgext/narrow/narrowrepo.py
86–88

Okay if we come back to that, since it's already logged as a TODO?

hgext/narrow/narrowrevlog.py
2

Yes, noted this in a TODO.rst.

91–95

Added to a TODO file.

106

Ouch, good catch. This does match core, so I fixed both in my followup.

115–116

Yeah, this is one of several places where I want to just hoist internals changes to core, with the only customer being narrow (for now, at least). It's super gross the way it currently is.

129–132

Yeah, all I remember about this (https://bitbucket.org/Google/narrowhg/commits/3cd72b1a1b41c9e46f12eba78c253da169277374) is that git-diffs break in the case of a rename from outside->inside. There are almost certainly other problems lurking in the weeds here, but to my knowledge we've not seen them at Google...

135–140

Added an explicit TODO. We can make this a TON cleaner when narrowhg can be formally aware of remotefilelog, this was mostly a kludge to work around them being in disjoint repos and not wanting to assume we could find remotefilelog cleanly.

hgext/narrow/narrowspec.py
32–43

Yes, the goal is that the two extensions will share a fair amount of logic. :)

50–55

That's a great question. I don't really know the answer offhand. Added a TODO...

84

Gave it a shot, and things broke all over the place. :(

94–96

@martinvonz please correct me if this is wrong.

The paths are stored in the narrowspec with canonical (that is /) path separators, just like the rest of the internals. We're reading things as bytes, so yes, \ should never occur.

111–116

It shouldn't be: this particular "str with refcount of 1" behavior was optimized a long time ago in cpython to not be a horrendous stack of copies.

If it shows up in a profile, we can obviously fix it.

134–139

Yep, but it also does some extra cache invalidation, so I don't think I can fuse them.

147–151

Added to my TODO doc because I don't want reasoning about locking to block getting out of this pile-of-comments hell.

hgext/narrow/narrowwirepeer.py
46–49

Yeah, this is a mess. I've added a TODO, as I'm not entirely sure how to work around this for now (calling capabilities in here seems...fraught.)

tests/test-narrow-archive.t
31

Noted. AFAIK this should be portable at least to bsdtar, but probably solaris or someone will eventually complain?

indygreg accepted this revision.Feb 12 2018, 6:02 PM

I think I've seen enough follow-ups to feel comfortable taking this in core. There's still a ton of work that needs to get done. But it will be easier to iterate and for others to get involved when the code is committed than when it is sitting around in review.

This revision is now accepted and ready to land.Feb 12 2018, 6:02 PM
This revision was automatically updated to reflect the committed changes.
mharbison72 added inline comments.
tests/test-narrow-strip.t
81

The test failures here on Windows seem to be a case of incoming and unbundle getting confused. Here's some debug info (starting right after the rm on line 77), but I'm not sure what to do with it:

$ hg log -G
o  changeset:   2:f505d5e96aa8
|  tag:         tip
|  parent:      0:a99f4d53924d
|  user:        test
|  date:        Thu Jan 01 00:00:00 1970 +0000
|  summary:     modify inside
|
| @  changeset:   1:9e48d953700d
|/   user:        test
|    date:        Thu Jan 01 00:00:00 1970 +0000
|    summary:     modify outside again
|
o  changeset:   0:a99f4d53924d
   user:        test
   date:        Thu Jan 01 00:00:00 1970 +0000
   summary:     initial

$ hg strip .
0 files updated, 0 files merged, 0 files removed, 0 files unresolved
saved backup bundle to $TESTTMP/narrow/.hg/strip-backup/*-backup.hg (glob)
$ hg log -G
o  changeset:   1:f505d5e96aa8
|  tag:         tip
|  user:        test
|  date:        Thu Jan 01 00:00:00 1970 +0000
|  summary:     modify inside
|
@  changeset:   0:a99f4d53924d
   user:        test
   date:        Thu Jan 01 00:00:00 1970 +0000
   summary:     initial

$ hg in --debug --bundle .hg/strip-backup/*-backup.hg
bundle2-input-part: total payload size 521
bundle2-input-part: total payload size 24
comparing with .hg/strip-backup/f505d5e96aa8-2b55e253-backup.hg
query 1; heads
searching for changes
all remote heads known locally
no changes found
[1]
$ hg unbundle --debug .hg/strip-backup/*-backup.hg
bundle2-input-bundle: 1 params with-transaction
bundle2-input-part: "changegroup" (params: 1 mandatory 1 advisory) supported
adding changesets
add changeset f505d5e96aa8
adding manifests
adding file changes
adding inside/f1 revisions
added 0 changesets with 0 changes to 1 files
bundle2-input-part: total payload size 521
bundle2-input-part: "phase-heads" supported
bundle2-input-part: total payload size 24
bundle2-input-bundle: 1 parts total
(run 'hg update' to get a working copy)
$ hg log -G -T '{rev} {desc}\n'
o  1 modify inside
|
@  0 initial