In D4606#70323, @yuja wrote:

Looks good to me. One nit.

• • a/mercurial/manifest.py +++ b/mercurial/manifest.py @@ -1203,7 +1203,7 @@ s._dirty = False self._loadfunc = _load_for_read
• def writesubtrees(self, m1, m2, writesubtree): + def writesubtrees(self, m1, m2, writesubtree, match=None):

Can you remove the unsupported default value match=None from inner functions?

In D4370#69050, @yuja wrote:

Queued, thanks.

+ def _loadchildrensetlazy(self, visit):
+ if not visit:
+ return None
+ if visit == 'all' or visit == 'this':
+ self._loadalllazy()
+ return None
+
+ todel = []
+ for k in visit:
+ kslash = k + '/'
+ ld = self._lazydirs.get(kslash)
+ if ld:
+ path, node, readsubtree = ld
+ self._dirs[kslash] = readsubtree(path, node)
+ todel.append(kslash)
+ for kslash in todel:
+ del self._lazydirs[kslash]

Any reason not to use self._loadlazy(kslash)?

In D4366#67153, @spectral wrote:

In D4366#67144, @indygreg wrote:

I'm not sure how I feel about so many methods having the if dir in self._lazydirs: self._loadlazy(dir) pattern.

On one hand, action at a distance when it involves caching can be dangerous. And doing the lookup inline will avoid a Python function call.

On the other, it seems very redundant.

Overall I'm OK with the patch. I just feel like using a collections.defaultdict or implementing __missing__ to automagically resolve missing keys might work out better. Do you think this is a reasonable request?

I think it's a reasonable request, but I'm not sure it's feasible with how collections.defaultdict/__missing__ work. They are *only* called by getitem. We'd need a separate dict subclass for _dirs that handles contains as well, at the very least, for all the cases of if dir not in self._dirs: return <something>

I'm playing around with such a thing, it's not difficult but I think might be more awkward than helpful. Even if that doesn't bear fruit, while implementing it I noticed something: I'm not actually populating self._lazydirs in this patch, I appear to have lost it during split/merge/whatever. Oops. I have to hope that the numbers I have in the commit description were before I lost this, since otherwise it's completely incomprehensible that this patch would cause a performance benefit (well, maybe not, if dead code can cause a performance loss, why not the other way around?)

In D4351#67751, @yuja wrote:

Can't apply. Can you rebase?

(Not marking accepted, though I'm fine with it going in as is, so that we get a chance to answer the questions before it lands)

Would it make sense to combine this into one hg invocation? I think it wouldn't be that hard (hmm, not as trivial as I thought, but.. not the worst thing ever? :))

(Accept conditional on fixing/documenting the difference I noticed)

In D4366#67144, @indygreg wrote:

I'm not sure how I feel about so many methods having the if dir in self._lazydirs: self._loadlazy(dir) pattern.

On one hand, action at a distance when it involves caching can be dangerous. And doing the lookup inline will avoid a Python function call.

On the other, it seems very redundant.

Overall I'm OK with the patch. I just feel like using a collections.defaultdict or implementing __missing__ to automagically resolve missing keys might work out better. Do you think this is a reasonable request?

In D4363#67017, @martinvonz wrote:

In D4363#67016, @spectral wrote:

In D4363#67015, @martinvonz wrote:

In D4363#67012, @martinvonz wrote:

Can we have a test for this?

Oh, sorry, there are updated tests (my browser tab was stale when I reviewed this). That also explains why two minlines changed by 2: the ~ is also removed, which I didn't expect. I think I would like to have the ~ in there. Without them, it may look like two independent branches come in sequence, no?

Yes, it might. I kinda went back and forth on this, as you probably saw on the FR from our users; I came to the conclusion that those using graphshorten are already losing : vs | distinctions, so seems to already be opting in to potentially misleading graphs. When I realized that hg show work also used graphshorten and this wasn't an explicit choice by the user, I was less sure on this and requested input from indygreg. If we agree that we need the ~ by default, I can easily add it back. Having never used hg show work, doing most of my changes in a repo where we don't really have branches that would cause this to be confusing, and that we have the branch name in the compact log format that I showed in the commit description, I don't think I'm a good judge of the importance of the ~ in these cases :)

I feel pretty strongly that making a non-linear graph appear linear is much more misleading than eliding intermediate nodes, so I'd really like the ~ back.

In D4363#67015, @martinvonz wrote:

In D4363#67012, @martinvonz wrote:

Can we have a test for this?

Oh, sorry, there are updated tests (my browser tab was stale when I reviewed this). That also explains why two minlines changed by 2: the ~ is also removed, which I didn't expect. I think I would like to have the ~ in there. Without them, it may look like two independent branches come in sequence, no?

Since this touches hg show work test output, I've added indygreg as an explicit reviewer.

Any updates on this? I'm poking at updating this function in core, and not too excited to have to also modify the copy in evolve that seems to have spawned :/