Hm, is this unrelated change?

Partially, I discovered in the next change that hgext.git doesn't expose read() and it was easier to do it earlier. I can pull it out into a separate commit as it is also a micro-optimisation.

Yes please do.

Done in D9626.

Hm, shouldn't this be b'\0' * (max(...) - rbccur)?

Better to mention upto which mercurial version the part was used and what recent versions do.

1. We can get the node from changsetrevision, hence no need to pass it separately.

2. minor name nit: we can call it regitser_new_changeset, the extra new will make it more obvious. Else we will require some documentation for this function.

3. How about not making this a method on localrepo object? Having a normal function will help extensions wrap it easily.

The goal is to have room to grow for the next change to avoid quadratic resizing. If we need to resize, double the total size. The max size is to cheaply avoid a few iterations for the first few revisions. I've added a comment to make that clear.

1. It's passed in to avoid the extra lookup and parsing for the important case of bundle operations. This means a minor penalty for commit, but it makes a measurable difference for the other cases.

2. IMO "register" already implies that it is something new. I can add a few words in the interface definition though.

3. Both work for me, no opinion yet on what is less effort for extensions.

That signature change is weird.

1. we end up doing an extra lookup for nodeid
2. we end up exposing implementation details about extra to code that did not needed it before.

I don't understand why we need to shift responsability here and have the branch cache care about this.

In the comment, I seems mention of resizing double the size. But I don't see this happening in final code. Am I missing something ?

Why not simply pass the revision number to the API and let lower lever create the changelogrevision if they need it ?

new naming allow this to be register_changeset, we should probably use that.

WE should probably introduce this API in a different changeset to simplify the discussion around it.

What about the cache the latest changelogrevision accessed to make this free and help in other cases ?

I think redundant keys are an interface bug. The node was available for all, so at least onchangelog in changegroup.py would have to do a lookup anyway. For the commit caller it would mean shuffling things around a bit.

I'll check the extra logic here again, it's been inconsistent across the code base. This is a bit of a left over from earlier iterations, but it can likely be made tighter and simpler now.

Extending by requiredsize will normally double the size, except when it was sparse before. In that case the gap will count toward to extension, which seems a reasonable compromise.

readfiles does the expensive part of creating the changelogrevision already. Not having to process the text twice makes a noticable difference. Note that we don't have the revision number here.

I'm not attached to the name. The other concern was if it should be function on the module scope to allow easier monkey patching. Not sure if that question warrants splitting the change.

I tried adding a cache for the last changelogrevision, but it turned out to be more expensive than shuffling things around a bit here. See the remark about readfiles earlier, which is the primarily time sink right now.

The extra logic is essentially what changelog.branchinfo does. D9603 refactors that to put it into revbranchcache as the only consumer in core. I've kept it for the moment since topics interacts with this code and would break.

My main concerns is responsibility and implementation details leaking into a simple cache. Could we have the extra-extraction done in the register_changeset method itself ? or probably better, putting them on changelogrevision since it is the one responsible for the details of the data access.

Both changegroup and commit application should have a revision number handy (that just the size of the revlog before creating the changeset). Revision number are much more efficient that nodeid overall. I would much prefer this kind of low level API to standardize on using them.

I get it now. I don't know if it me being tired of the code being a bit too sneaky. but it might be worth having some explicit arithmetic or a clarification comment to highlight that the operation result in a array that is "current_size + currently needed size" leading to (about) a ×2 size increase.

Wrapping of repository level method is a common operation in extension. That is probably a good place to put it. (And also a place because they are too many method on localrepo… but these one is not a too bad fit in my opinion.)

The motivation to split the change is also about making the first half easier to land sooner, and to avoid mixing discussion on two different topic. That is not a requirement, but it might help.

could we cache the result of readfiles ? to reuse it at that time ?

I've moved the extra logic into changelogrevision, so it is again in one place only. Shouldn't make a performance difference and is cleaner. I would really prefer to not add assumptions about the revisions of new nodes, that seems to be error prone and also not what we do right now. Otherwise phases.registernew wouldn't lookup the revision either. I do expect this to be optimized in the revlog one way or the other.

Right, the adjusted comment should explain that.