( )⚙ D6851 narrow: don't hexify paths and double-hexify known nodes on wire (BC)

This is an archive of the discontinued Mercurial Phabricator instance.

narrow: don't hexify paths and double-hexify known nodes on wire (BC)
ClosedPublic

Authored by martinvonz on Sep 14 2019, 1:11 PM.

Details

Summary

It isn't obvious, but wireprototypes.encodelist() is meant only for
binary nodeids. So when we used it for encoding hex nodeids and paths,
the encoded result was surprising and hard to read.

This patch changes the encoding to make the list of paths a
comma-separated list and the list of common nodes to be a
encodelist()-encoded list of binary nodeids (so the result is just
singly-hexified nodeids).

This is clearly a breaking change, but the feature is experimental and
we're not aware of anyone running a server using this command yet.

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

martinvonz created this revision.Sep 14 2019, 1:11 PM
martinvonz updated this revision to Diff 16544.Sep 14 2019, 1:14 PM

Hmm, I just noticed that we separate these paths by commas (not nulls) in other places, so I'll make this consistent with that. I guess that means we don't support paths containing commas for includes or excludes, but fixing that is a separate problem.

martinvonz edited the summary of this revision. (Show Details)Sep 16 2019, 12:49 AM
martinvonz updated this revision to Diff 16553.

This is clearly a breaking change, but the feature is experimental and
we're not aware of anyone running a server using this command yet.

@idlsoft and their company does use narrow extension. @idlsoft can you upgrade server and client at the same time?

@idlsoft and their company does use narrow extension. @idlsoft can you upgrade server and client at the same time?

I did that a little while ago to move to 5.0. It was not fun. It's server, teamcity, clients, docker images.
What operations does this affect? Regular push/pull/clone or only tracked --add-include?

@idlsoft and their company does use narrow extension. @idlsoft can you upgrade server and client at the same time?

I did that a little while ago to move to 5.0. It was not fun. It's server, teamcity, clients, docker images.
What operations does this affect? Regular push/pull/clone or only tracked --add-include?

Just tracked --add-include. A workaround to simplify the upgrade would be to change wireprototypes.SUPPORTED_ELLIPSESCAP to be (ELLIPSESCAP1, ) on the server from now until all clients have upgraded. But that may still be annoying and error-prone for you to deal with. @pulkit, I suppose we should just add a exp-narrow-2 capability to deal with this? It doesn't seem fair to make @idlsoft deal with it.

@idlsoft and their company does use narrow extension. @idlsoft can you upgrade server and client at the same time?

I did that a little while ago to move to 5.0. It was not fun. It's server, teamcity, clients, docker images.
What operations does this affect? Regular push/pull/clone or only tracked --add-include?

Just tracked --add-include. A workaround to simplify the upgrade would be to change wireprototypes.SUPPORTED_ELLIPSESCAP to be (ELLIPSESCAP1, ) on the server from now until all clients have upgraded. But that may still be annoying and error-prone for you to deal with. @pulkit, I suppose we should just add a exp-narrow-2 capability to deal with this? It doesn't seem fair to make @idlsoft deal with it.

Sounds like a good idea!

Just tracked --add-include. A workaround to simplify the upgrade would be to change wireprototypes.SUPPORTED_ELLIPSESCAP to be (ELLIPSESCAP1, ) on the server from now until all clients have upgraded. But that may still be annoying and error-prone for you to deal with. @pulkit, I suppose we should just add a exp-narrow-2 capability to deal with this? It doesn't seem fair to make @idlsoft deal with it.

Sounds like a good idea!

If it's just tracked --add-include then it's not a big deal, it won't disrupt regular flow.
If backward compatibility doesn't complicate the code - great, if not - don't worry about it.

Just tracked --add-include. A workaround to simplify the upgrade would be to change wireprototypes.SUPPORTED_ELLIPSESCAP to be (ELLIPSESCAP1, ) on the server from now until all clients have upgraded. But that may still be annoying and error-prone for you to deal with. @pulkit, I suppose we should just add a exp-narrow-2 capability to deal with this? It doesn't seem fair to make @idlsoft deal with it.

Sounds like a good idea!

If it's just tracked --add-include then it's not a big deal, it won't disrupt regular flow.
If backward compatibility doesn't complicate the code - great, if not - don't worry about it.

It does seem to be more work than I hoped at first, so it would be very much appreciated if you can live with hg tracked --add-include (and --remove-exclude) not working while you transition server and clients.

pulkit accepted this revision.Sep 17 2019, 1:45 PM

Just tracked --add-include. A workaround to simplify the upgrade would be to change wireprototypes.SUPPORTED_ELLIPSESCAP to be (ELLIPSESCAP1, ) on the server from now until all clients have upgraded. But that may still be annoying and error-prone for you to deal with. @pulkit, I suppose we should just add a exp-narrow-2 capability to deal with this? It doesn't seem fair to make @idlsoft deal with it.

Sounds like a good idea!

If it's just tracked --add-include then it's not a big deal, it won't disrupt regular flow.
If backward compatibility doesn't complicate the code - great, if not - don't worry about it.

Great, thanks!

This revision is now accepted and ready to land.Sep 17 2019, 1:45 PM