durin42 added a comment.
Okay, I've pushed more followups. The bulk of the TODOs are recorded in a
file introduced in https://phab.mercurial-scm.org/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.
> indygreg wrote in narrowbundle2.py:138-151
> I'm not an expert on the manifest APIs. There //may// be a more optimal way
> to implement this...
I wrote lazymanifest, and this was the best adgar and I could come up with...
> indygreg wrote in narrowrepo.py:85-87
> I agree with the inline todo :)
Okay if we come back to that, since it's already logged as a TODO?
> indygreg wrote in narrowrevlog.py:1
> 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.
Yes, noted this in a TODO.rst.
> indygreg wrote in narrowrevlog.py:90-94
> Consider doing that and changing this to raise if called.
Added to a TODO file.
> indygreg wrote in narrowrevlog.py:105
> Nit: `dir` is a builtin. If this matches core, fine. But I'd prefer avoiding
> the name collision.
Ouch, good catch. This does match core, so I fixed both in my followup.
> indygreg wrote in narrowrevlog.py:114-115
> 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__.
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.
> indygreg wrote in narrowrevlog.py:128-131
> 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.
Yeah, all I remember about this
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...
> indygreg wrote in narrowrevlog.py:134-139
> This is making assumptions about code that hasn't landed yet. Not sure if we
> should replace this with a TODO or what.
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
> indygreg wrote in narrowspec.py:31-42
> Oh, hey, this looks just like sparse profiles! I sense some code conversion
> in our future...
Yes, the goal is that the two extensions will share a fair amount of logic. :)
> indygreg wrote in narrowspec.py:49-54
> Playing devil's advocate, do we really need two formats doing the same thing?
That's a great question. I don't really know the answer offhand. Added a TODO...
> indygreg wrote in narrowspec.py:83
> Can we use `str.count()` to avoid creating objects for each line?
Gave it a shot, and things broke all over the place. :(
> indygreg wrote in narrowspec.py:93-95
> 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?
@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.
> indygreg wrote in narrowspec.py:110-115
> It is better to build a list of lines and `'\n'.join()` them.
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.
> indygreg wrote in narrowspec.py:133-138
> This reinvents `repo.vfs.tryread()`.
Yep, but it also does some extra cache invalidation, so I don't think I can
> indygreg wrote in narrowspec.py:146-150
> This is called from `narrowrepo.setnarrowparts()` and neither of them cares
> about locking. Somewhere we should ensure we hold the wlock.
Added to my TODO doc because I don't want reasoning about locking to block
getting out of this pile-of-comments hell.
> indygreg wrote in narrowwirepeer.py:45-48
> 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.
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.)
> indygreg wrote in test-narrow-archive.t:30
> 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.
Noted. AFAIK this should be portable at least to bsdtar, but probably solaris
or someone will eventually complain?
To: durin42, #hg-reviewers
Cc: idlsoft, martinvonz, indygreg, mercurial-devel
Mercurial-devel mailing list