Page MenuHomePhabricator

wireprotov1peer: update all rpcs to use the new batchable scheme
ClosedPublic

Authored by valentin.gatienbaron on Jul 25 2021, 4:32 PM.

Details

Summary

If desired, we could keep the future class and the function that
upgrades an old style rpc instead of a new style, for extensions.

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

Alphare accepted this revision.Aug 24 2021, 10:09 AM
Alphare added a subscriber: Alphare.

Agreed that this is simpler, looks good to me.
I don't think the backwards compatibility concerns are too bad, but I might be wrong.

This revision is now accepted and ready to land.Aug 24 2021, 10:09 AM
baymax updated this revision to Diff 29997.Aug 24 2021, 11:14 AM

โœ… refresh by Heptapod after a successful CI run (๐Ÿ™ ๐Ÿ’š)
โš  This patch is intended for stable โš 

danchr added a subscriber: danchr.Aug 27 2021, 10:52 AM

I don't think the backwards compatibility concerns are too bad, but I might be wrong.

Please note that this breaks hg-git, as it currently relies on the future for wrapping a repository. We'll probably have to come up with a wrapper regardless.

See e.g. https://foss.heptapod.net/mercurial/hg-git/-/blob/branch/default/hggit/gitrepo.py#L113

I don't think the backwards compatibility concerns are too bad, but I might be wrong.

Please note that this breaks hg-git, as it currently relies on the future for wrapping a repository. We'll probably have to come up with a wrapper regardless.
See e.g. https://foss.heptapod.net/mercurial/hg-git/-/blob/branch/default/hggit/gitrepo.py#L113

Thanks for bringing this up. Does that represent a large amount of work for you?

I don't think the backwards compatibility concerns are too bad, but I might be wrong.

Please note that this breaks hg-git, as it currently relies on the future for wrapping a repository. We'll probably have to come up with a wrapper regardless.
See e.g. https://foss.heptapod.net/mercurial/hg-git/-/blob/branch/default/hggit/gitrepo.py#L113

Thanks for bringing this up. Does that represent a large amount of work for you?

Well, my main challenge here is that I have absolutely no idea what's going on โ€” I don't have much experience with the very low levels of networking in Mercurial, or the (relatively) new peer API. It seems to me that the point of that code is, essentially, to provide a minimal repository or peer that does nothing. Could something like that be moved to Mercurial core?

Thanks for bringing this up. Does that represent a large amount of work for you?

Well, my main challenge here is that I have absolutely no idea what's going on โ€” I don't have much experience with the very low levels of networking in Mercurial, or the (relatively) new peer API. It seems to me that the point of that code is, essentially, to provide a minimal repository or peer that does nothing. Could something like that be moved to Mercurial core?

Okay, I managed to fix it:

https://foss.heptapod.net/mercurial/hg-git/-/merge_requests/130/diffs
https://foss.heptapod.net/mercurial/hg-git/-/merge_requests/131/diffs

Would one of you perhaps take a look at those changes and see if they make sense? I did it in the form of a decorator that just wraps a regular function returning a value. Something like that might be useful for core as well, and simplify a simple method like branchmap().

Thanks for bringing this up. Does that represent a large amount of work for you?

Well, my main challenge here is that I have absolutely no idea what's going on โ€” I don't have much experience with the very low levels of networking in Mercurial, or the (relatively) new peer API. It seems to me that the point of that code is, essentially, to provide a minimal repository or peer that does nothing. Could something like that be moved to Mercurial core?

Okay, I managed to fix it:
https://foss.heptapod.net/mercurial/hg-git/-/merge_requests/130/diffs
https://foss.heptapod.net/mercurial/hg-git/-/merge_requests/131/diffs
Would one of you perhaps take a look at those changes and see if they make sense? I did it in the form of a decorator that just wraps a regular function returning a value. Something like that might be useful for core as well, and simplify a simple method like branchmap().

The two wrappers make sense, although it seems random to encode the query as {} in one case and None in other case (I also don't really understand why the old wrapper yields twice instead of yield+return, but I guess it doesn't really make a difference).
I can't really speak about the merge of the two classes.

The two wrappers make sense, although it seems random to encode the query as {} in one case and None in other case

Good point! Since both work, I simplified it to just always use None.

(I also don't really understand why the old wrapper yields twice instead of yield+return, but I guess it doesn't really make a difference).

Well, yield+return is a syntax error in Python 2.7, and just yielding once raises some odd exceptions I'd rather not dig into for legacy code.

I can't really speak about the merge of the two classes.

Thanks for taking a look anyway! (AFAICT the two classes were a somewhat convoluted way of handling compatibility with 4.8 and earlier; with the decorator, it's no longer needed.)

I'm a little late to the party, but the changes look good to me as well, thanks for fixing it in hg-git!