# Patchwork D8281: narrow: escape includepats/excludepats when sending over the wire (BC)

Submitter phabricator March 13, 2020, 4:12 a.m. mbox | patch /patch/45761/ New show

phabricator - March 13, 2020, 4:12 a.m.
spectral created this revision.
Herald added a reviewer: durin42.
Herald added a reviewer: martinvonz.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
When transmitting data over the wire, first run util.pconvert on each item (this is
just good practice in general to have the wire protocol have posix style paths).
Then, escape any existing backslash with \SlAsH, and any existing comma with
\CoMmA.  Finally, join everything together with commas like before.

This has a minor breaking change in some scenarios:

Old server, old client:

- paths with commas in them will cause errors if added to the narrowspec

Old server, new client:

- commas in paths will be interpreted by the server as \CoMmA, and not match
- backslashes in paths will be interpreted by the server as \SlAsH, and not match.  Due to the pconvert, windows clients shouldn't have any \ characters in their paths, and it's still probably very rare for non-windows clients to have paths with a \ in the name.

New server, old client:

- paths with commas in them will cause errors if added to the narrowspec
- any path that (pre-escaping) contains the substring \SlAsH or \CoMmA will have these converted to \ and , respectively, and likely not match. This is a case-sensitive comparison, so it's extremely unlikely to be a problem in practice.

New server, new client:

- everything should work

REPOSITORY
rHG Mercurial

BRANCH
default

REVISION DETAIL
https://phab.mercurial-scm.org/D8281

AFFECTED FILES
hgext/narrow/narrowbundle2.py
hgext/narrow/narrowwirepeer.py
mercurial/wireprototypes.py
mercurial/wireprotov1peer.py
mercurial/wireprotov1server.py
relnotes/next
tests/test-doctest.py
tests/test-narrow-exchange.t

CHANGE DETAILS

To: spectral, durin42, martinvonz, #hg-reviewers
Cc: mercurial-devel

phabricator - March 13, 2020, 5:13 a.m.
mharbison72 added a comment.

The Windows path changes seem like a good idea.

Would quoting paths with commas eliminate the need for custom escaping?  I don't feel strongly about it, but custom escaping always feels weird to me.  (I fact, a coworker did some homebrew escaping for CSV files a few days ago, but I forget how it ultimately ended up.)

REPOSITORY
rHG Mercurial

CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D8281/new/

REVISION DETAIL
https://phab.mercurial-scm.org/D8281

To: spectral, durin42, martinvonz, #hg-reviewers
Cc: mharbison72, mercurial-devel

phabricator - March 13, 2020, 5:29 a.m.
spectral added a comment.

In D8281#123621 <https://phab.mercurial-scm.org/D8281#123621>, @mharbison72 wrote:

> The Windows path changes seem like a good idea.
> Would quoting paths with commas eliminate the need for custom escaping?  I don't feel strongly about it, but custom escaping always feels weird to me.  (I fact, a coworker did some homebrew escaping for CSV files a few days ago, but I forget how it ultimately ended up.)

Let me play with that a bit, I think it'll work and be detectable on the server since the first character can't currently be a double-quote, so there wouldn't really be any BC issues aside from the pconvert (which wouldn't be as important anymore, but still probably a good idea?)

REPOSITORY
rHG Mercurial

CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D8281/new/

REVISION DETAIL
https://phab.mercurial-scm.org/D8281

To: spectral, durin42, martinvonz, #hg-reviewers
Cc: mharbison72, mercurial-devel

phabricator - March 13, 2020, 5:47 a.m.
martinvonz added a comment.

In D8281#123625 <https://phab.mercurial-scm.org/D8281#123625>, @spectral wrote:

> In D8281#123621 <https://phab.mercurial-scm.org/D8281#123621>, @mharbison72 wrote:
>
>> The Windows path changes seem like a good idea.
>> Would quoting paths with commas eliminate the need for custom escaping?  I don't feel strongly about it, but custom escaping always feels weird to me.  (I fact, a coworker did some homebrew escaping for CSV files a few days ago, but I forget how it ultimately ended up.)
>
> Let me play with that a bit, I think it'll work and be detectable on the server since the first character can't currently be a double-quote, so there wouldn't really be any BC issues aside from the pconvert (which wouldn't be as important anymore, but still probably a good idea?)

It's a little weird to me that there's no step corresponding to pconvert() in the decoding function. Maybe we should do the pconvert() step outside encodecsvpaths(), maybe in narrowspec.normalizepattern()? However, that would make it impossible to have backslashes in paths in the narrowspec on Windows even after this patch (because they would always be converted to slashes), including narrowspecs returned from the server. However^2, maybe it wouldn't work to check out such paths on Windows anyway, so it doesn't really matter (so it's fine to call pconvert())?

REPOSITORY
rHG Mercurial

CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D8281/new/

REVISION DETAIL
https://phab.mercurial-scm.org/D8281

To: spectral, durin42, martinvonz, #hg-reviewers
Cc: mharbison72, mercurial-devel

phabricator - March 13, 2020, 8:58 a.m.
This revision now requires changes to proceed.
marmoute added a comment.
marmoute requested changes to this revision.

The escaping scheme is a bit puzzling to me. Coudl we use something more standard for this ? (like urlencode).

(requesting change of the function name. Now that we can, lets make them readable)

> wireprototypes.py:183
>
> +def encodecsvpaths(paths):
> +    r'''escape and join a value of type 'csvpaths', producing a bytes object

Let's name it encode_csv_paths now that _ are permitted. It woul dbe easier to read.

> wireprototypes.py:220
> +
> +def decodecsvpaths(s):
> +    r'''decode an value of type 'csvpaths', producing a list

Let's name it decode_csv_paths now that _ are permitted. It woul dbe easier to read.

REPOSITORY
rHG Mercurial

CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D8281/new/

REVISION DETAIL
https://phab.mercurial-scm.org/D8281

To: spectral, durin42, martinvonz, #hg-reviewers, marmoute
Cc: marmoute, mharbison72, mercurial-devel

phabricator - March 13, 2020, 1:57 p.m.
mharbison72 added a comment.

In D8281#123625 <https://phab.mercurial-scm.org/D8281#123625>, @spectral wrote:

> In D8281#123621 <https://phab.mercurial-scm.org/D8281#123621>, @mharbison72 wrote:
>
>> The Windows path changes seem like a good idea.
>> Would quoting paths with commas eliminate the need for custom escaping?  I don't feel strongly about it, but custom escaping always feels weird to me.  (I fact, a coworker did some homebrew escaping for CSV files a few days ago, but I forget how it ultimately ended up.)
>
> Let me play with that a bit, I think it'll work and be detectable on the server since the first character can't currently be a double-quote, so there wouldn't really be any BC issues aside from the pconvert (which wouldn't be as important anymore, but still probably a good idea?)

I haven't played with narrow yet, so I'm not sure of the context.  Are these user input paths that would end up being ignored/rejected if a Windows user used path\to\file when talking to a Unix server?  Or are these stored in a tracked file? (Which I think could still cause problems.)  I can't think of a good reason to stay inconsistent, and it is still experimental, so it still seems like a good idea while we still can fix it.

REPOSITORY
rHG Mercurial

CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D8281/new/

REVISION DETAIL
https://phab.mercurial-scm.org/D8281

To: spectral, durin42, martinvonz, #hg-reviewers, marmoute
Cc: marmoute, mharbison72, mercurial-devel

phabricator - March 13, 2020, 3:17 p.m.
martinvonz added a comment.

In D8281#123659 <https://phab.mercurial-scm.org/D8281#123659>, @mharbison72 wrote:

> In D8281#123625 <https://phab.mercurial-scm.org/D8281#123625>, @spectral wrote:
>
>> In D8281#123621 <https://phab.mercurial-scm.org/D8281#123621>, @mharbison72 wrote:
>>
>>> The Windows path changes seem like a good idea.
>>> Would quoting paths with commas eliminate the need for custom escaping?  I don't feel strongly about it, but custom escaping always feels weird to me.  (I fact, a coworker did some homebrew escaping for CSV files a few days ago, but I forget how it ultimately ended up.)
>>
>> Let me play with that a bit, I think it'll work and be detectable on the server since the first character can't currently be a double-quote, so there wouldn't really be any BC issues aside from the pconvert (which wouldn't be as important anymore, but still probably a good idea?)
>
> I haven't played with narrow yet, so I'm not sure of the context.  Are these user input paths that would end up being ignored/rejected if a Windows user used path\to\file when talking to a Unix server?  Or are these stored in a tracked file? (Which I think could still cause problems.)  I can't think of a good reason to stay inconsistent, and it is still experimental, so it still seems like a good idea while we still can fix it.

They are stored in .hg/store/narrowspec. They usually get into that file via hg clone --narrow --include and similar, but the server may also send them. We should ideally do the conversion early when the user provides them. I think the pconvert in this patch is to handle existing repos as well as possible.

REPOSITORY
rHG Mercurial

CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D8281/new/

REVISION DETAIL
https://phab.mercurial-scm.org/D8281

To: spectral, durin42, martinvonz, #hg-reviewers, marmoute
Cc: marmoute, mharbison72, mercurial-devel

phabricator - March 19, 2020, 11:35 p.m.
marmoute added a comment.

Since narrow is still experimental, I don't think we should try too hard for backward compatibility. We could introduce a new end-point for a new encoding and drop the old one in a couple of version.

I am really not enthousiatic with having our own version of an encoding. Because this means extra overhead for people working on this in the future. Especially if it needs to be reimplemented (eg: in rust). If we drop the hard BC constraint on this, like starting from scratch. What would be your (@spectral ) encoding of choice ? could we got for something simple but widely available like url-encode ?

REPOSITORY
rHG Mercurial

CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D8281/new/

REVISION DETAIL
https://phab.mercurial-scm.org/D8281

To: spectral, durin42, martinvonz, #hg-reviewers, marmoute
Cc: marmoute, mharbison72, mercurial-devel

phabricator - March 20, 2020, 3:57 p.m.
durin42 added a comment.

In D8281#124058 <https://phab.mercurial-scm.org/D8281#124058>, @marmoute wrote:

> Since narrow is still experimental, I don't think we should try too hard for backward compatibility. We could introduce a new end-point for a new encoding and drop the old one in a couple of version.

+0, honestly. I won't require it, but I'd really rather we shaved this yak _now_ rather than when narrow has even more users.

REPOSITORY
rHG Mercurial

CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D8281/new/

REVISION DETAIL
https://phab.mercurial-scm.org/D8281

To: spectral, durin42, martinvonz, #hg-reviewers, marmoute
Cc: marmoute, mharbison72, mercurial-devel

phabricator - March 20, 2020, 11:09 p.m.
spectral added a comment.

In D8281#124129 <https://phab.mercurial-scm.org/D8281#124129>, @durin42 wrote:

> In D8281#124058 <https://phab.mercurial-scm.org/D8281#124058>, @marmoute wrote:
>
>> Since narrow is still experimental, I don't think we should try too hard for backward compatibility. We could introduce a new end-point for a new encoding and drop the old one in a couple of version.
>
> +0, honestly. I won't require it, but I'd really rather we shaved this yak _now_ rather than when narrow has even more users.

I'm getting a bit frustrated with how much time I've spent on this, made worse by the fact that I agree with everything everyone's saying and so it's not like I'm frustrated at the review process, just how slow I've been at accomplishing this.

So, before I go down another rabbit hole, here's what I'm thinking:

- Server emits a new capability narrow-exp-1-escaped (in addition to the current narrow-exp-1, this is not replacing the existing capability)
- wireprototypes's map will change these items from csv to csv.escaped
- Compatible clients will detect this capability from the server and send items of type csv.escaped during getbundle with keys like <previousname>.escaped (ex: include.escaped). If the server doesn't support csv.escaped, the client sends with the old names (unescaped).
- The escaping will be urllibcompat.quote
- The server will strip the .escaped suffix on the keys, split on comma, and urllibcompat.unquote the individual items
- I'm *not* expecting to do anything about \ -> / conversion.

Since these are part of getbundle, I haven't found a way of doing this that's not one of:

- a custom escaping mechanism that's backwards compatible
- adding a capability and renaming the keys that are sent (so the server can tell when it needs to unescape)
- having the client always send duplicate items (i.e. send include and include.escaped). I'm not even sure that older servers would tolerate receiving keys they aren't expecting.
- having the client only escape when necessary (i.e. it includes a comma), and then always send the path as include.escaped (which runs into the problem of old servers rejecting).
- having the server always unescape and the client always escape. This breaks the server's ability to interact with older clients that aren't escaping (which we'll need to support for at least a week or two).

For the non-getbundle parts (I think the wireproto command is 'widen'), we can easily make a widen2 or something, but it's probably easier to just keep the same command name and do the same thing as in getbundle: detect the capability, send as foo.escaped if supported.

REPOSITORY
rHG Mercurial

CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D8281/new/

REVISION DETAIL
https://phab.mercurial-scm.org/D8281

To: spectral, durin42, martinvonz, #hg-reviewers, marmoute
Cc: marmoute, mharbison72, mercurial-devel

phabricator - March 20, 2020, 11:32 p.m.
martinvonz added a comment.

In D8281#124246 <https://phab.mercurial-scm.org/D8281#124246>, @spectral wrote:

> In D8281#124129 <https://phab.mercurial-scm.org/D8281#124129>, @durin42 wrote:
>
>> In D8281#124058 <https://phab.mercurial-scm.org/D8281#124058>, @marmoute wrote:
>>
>>> Since narrow is still experimental, I don't think we should try too hard for backward compatibility. We could introduce a new end-point for a new encoding and drop the old one in a couple of version.
>>
>> +0, honestly. I won't require it, but I'd really rather we shaved this yak _now_ rather than when narrow has even more users.
>
> I'm getting a bit frustrated with how much time I've spent on this, made worse by the fact that I agree with everything everyone's saying and so it's not like I'm frustrated at the review process, just how slow I've been at accomplishing this.
> So, before I go down another rabbit hole, here's what I'm thinking:
>
> - Server emits a new capability narrow-exp-1-escaped (in addition to the current narrow-exp-1, this is not replacing the existing capability)

nit: I *think* the "1" in the name was supposed to be a version number, so the new capability's name would be narrow-exp-2.

> - wireprototypes's map will change these items from csv to csv.escaped
> - Compatible clients will detect this capability from the server and send items of type csv.escaped during getbundle with keys like <previousname>.escaped (ex: include.escaped). If the server doesn't support csv.escaped, the client sends with the old names (unescaped).
> - The escaping will be urllibcompat.quote
> - The server will strip the .escaped suffix on the keys, split on comma, and urllibcompat.unquote the individual items
> - I'm *not* expecting to do anything about \ -> / conversion.

This all sounds good to me.

> Since these are part of getbundle, I haven't found a way of doing this that's not one of:
>
> - a custom escaping mechanism that's backwards compatible
> - adding a capability and renaming the keys that are sent (so the server can tell when it needs to unescape)
> - having the client always send duplicate items (i.e. send include and include.escaped). I'm not even sure that older servers would tolerate receiving keys they aren't expecting.
> - having the client only escape when necessary (i.e. it includes a comma), and then always send the path as include.escaped (which runs into the problem of old servers rejecting).
> - having the server always unescape and the client always escape. This breaks the server's ability to interact with older clients that aren't escaping (which we'll need to support for at least a week or two).
>
> For the non-getbundle parts (I think the wireproto command is 'widen'), we can easily make a widen2 or something, but it's probably easier to just keep the same command name and do the same thing as in getbundle: detect the capability, send as foo.escaped if supported.

Maybe no one uses the "widen" command so we don't even need to worry about compatibility there?

REPOSITORY
rHG Mercurial

CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D8281/new/

REVISION DETAIL
https://phab.mercurial-scm.org/D8281

To: spectral, durin42, martinvonz, #hg-reviewers, marmoute
Cc: marmoute, mharbison72, mercurial-devel

phabricator - March 21, 2020, 2:26 a.m.
spectral added a comment.

In D8281#124247 <https://phab.mercurial-scm.org/D8281#124247>, @martinvonz wrote:

> In D8281#124246 <https://phab.mercurial-scm.org/D8281#124246>, @spectral wrote:
>
>> - Server emits a new capability narrow-exp-1-escaped (in addition to the current narrow-exp-1, this is not replacing the existing capability)
>
> nit: I *think* the "1" in the name was supposed to be a version number, so the new capability's name would be narrow-exp-2.

Yes, I had assumed that as well. This isn't really a new version of the protocol, though, just a minor tweak, and it's primarily to the 'csv' type used in getbundle (see the current version of this patch that adds 'qcsv' for the actual locations it's used). Honestly I went back and forth between announcing it as getbundle-csv-escaped and something related to narrow (and ended up on narrow, as you see, since while it's generically useful besides narrow, nothing else needs it today, and future things wouldn't need this to be announced forever - they'll always have used foo.escaped and been transmitted as escaped).

> Maybe no one uses the "widen" command so we don't even need to worry about compatibility there?

I don't know how often it's used :) I just know that there's something *not* getbundle-related in narrowwirepeer.py (looks like it's called narrow_widen that needed to be modified or else the tests wouldn't pass. I honestly don't even know if we're using it internally at Google right now. If not, that's fewer things to change, which I'm OK with :)

REPOSITORY
rHG Mercurial

CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D8281/new/

REVISION DETAIL
https://phab.mercurial-scm.org/D8281

To: spectral, durin42, martinvonz, #hg-reviewers, marmoute
Cc: marmoute, mharbison72, mercurial-devel

phabricator - March 21, 2020, 2:44 a.m.
mharbison72 added a comment.

In D8281#124246 <https://phab.mercurial-scm.org/D8281#124246>, @spectral wrote:

> - I'm *not* expecting to do anything about \ -> / conversion.

So would there be some interoperability issue between Windows and not-Windows if paths aren't pconverted, if paths can also come from the server as Martin mentioned?  Is there anything here that makes it more difficult to pconvert in the future?  (I assume it only came up in the first place to allow the custom escaping.  I understand your frustration, so I'm not looking to sign you up for more work.  But I only know about narrow from a very high conceptual level, so I figure I might as well ask now and save this info for later.)

REPOSITORY
rHG Mercurial

CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D8281/new/

REVISION DETAIL
https://phab.mercurial-scm.org/D8281

To: spectral, durin42, martinvonz, #hg-reviewers, marmoute
Cc: marmoute, mharbison72, mercurial-devel

phabricator - March 21, 2020, 4:16 a.m.
marmoute added a comment.

In D8281#124246 <https://phab.mercurial-scm.org/D8281#124246>, @spectral wrote:

> In D8281#124129 <https://phab.mercurial-scm.org/D8281#124129>, @durin42 wrote:
>
>> In D8281#124058 <https://phab.mercurial-scm.org/D8281#124058>, @marmoute wrote:
>>
>>> Since narrow is still experimental, I don't think we should try too hard for backward compatibility. We could introduce a new end-point for a new encoding and drop the old one in a couple of version.
>>
>> +0, honestly. I won't require it, but I'd really rather we shaved this yak _now_ rather than when narrow has even more users.
>
> I'm getting a bit frustrated with how much time I've spent on this, made worse by the fact that I agree with everything everyone's saying and so it's not like I'm frustrated at the review process, just how slow I've been at accomplishing this.

I know the feeling, thanks a lot for revisiting you original plan.

> So, before I go down another rabbit hole, here's what I'm thinking:
>
> - Server emits a new capability narrow-exp-1-escaped (in addition to the current narrow-exp-1, this is not replacing the existing capability)
> - wireprototypes's map will change these items from csv to csv.escaped
> - Compatible clients will detect this capability from the server and send items of type csv.escaped during getbundle with keys like <previousname>.escaped (ex: include.escaped). If the server doesn't support csv.escaped, the client sends with the old names (unescaped).
> - The escaping will be urllibcompat.quote
> - The server will strip the .escaped suffix on the keys, split on comma, and urllibcompat.unquote the individual items

This looks overall good to me.

> - I'm *not* expecting to do anything about \ -> / conversion.

Does this means the client side is expected to enforce using / as the directory separator ?

> Since these are part of getbundle, I haven't found a way of doing this that's not one of:
>
> - a custom escaping mechanism that's backwards compatible
> - adding a capability and renaming the keys that are sent (so the server can tell when it needs to unescape)
> - having the client always send duplicate items (i.e. send include and include.escaped). I'm not even sure that older servers would tolerate receiving keys they aren't expecting.

It would not work. Server would reject unknown arguments to getbundle.

> - having the client only escape when necessary (i.e. it includes a comma), and then always send the path as include.escaped (which runs into the problem of old servers rejecting).

In my opinion, I don't think we get much benefit of conditional escaping. So keeping things simple seems better.

> - having the server always unescape and the client always escape. This breaks the server's ability to interact with older clients that aren't escaping (which we'll need to support for at least a week or two).

As much as I think we don't need strong BC on narrow because it is experimental, have a couple of version that can still speak to each other is preferable.

> For the non-getbundle parts (I think the wireproto command is 'widen'), we can easily make a widen2 or something, but it's probably easier to just keep the same command name and do the same thing as in getbundle: detect the capability, send as foo.escaped if supported.

REPOSITORY
rHG Mercurial

CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D8281/new/

REVISION DETAIL
https://phab.mercurial-scm.org/D8281

To: spectral, durin42, martinvonz, #hg-reviewers, marmoute
Cc: marmoute, mharbison72, mercurial-devel

phabricator - April 22, 2020, 4:11 p.m.
Herald added a subscriber: mercurial-patches.
This revision now requires changes to proceed.
marmoute added a comment.
marmoute requested changes to this revision.

If I understood the situation correctly, a rewrite in planned.

REPOSITORY
rHG Mercurial

CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D8281/new/

REVISION DETAIL
https://phab.mercurial-scm.org/D8281

To: spectral, durin42, martinvonz, #hg-reviewers, marmoute
Cc: mercurial-patches, marmoute, mharbison72, mercurial-devel


## Patch

diff --git a/tests/test-narrow-exchange.t b/tests/test-narrow-exchange.t
--- a/tests/test-narrow-exchange.t
+++ b/tests/test-narrow-exchange.t
@@ -223,3 +223,62 @@
remote: rollback completed (lfs-on !)
remote: abort: data/inside2/f.i@f59b4e021835: no match found! (lfs-on !)
abort: stream ended unexpectedly (got 0 bytes, expected 4) (lfs-on !)
+
+Test paths with commas in them
+  $cd$TESTTMP
+  $hg init commas-master +$ cd commas-master
+  $mkdir a,b +$ mkdir a,b/c,d
+  $mkdir a,b/e,f +$ mkdir g
+  $echo abcd > a,b/c,d/abcd +$ echo abef > a,b/e,f/abef
+  $echo ghi > g/h,i +$ hg ci -qAm r0
+  $echo abcd2 >> a,b/c,d/abcd +$ echo abef2 >> a,b/e,f/abef
+  $echo ghi2 >> g/h,i +$ hg ci -qm r1
+  $cd .. + +Test that we can pull and push with a file that has a comma in the name, even +though the commas don't appear in the narrowspec file (since they're just +filenames) +$ hg clone --narrow ssh://user@dummy/commas-master commas-in-file \
+  >   --include g -qr 0
+  $cd commas-in-file +$ hg pull -q
+  $echo ghi3 >> g/h,i +$ hg ci -qm 'modify g/h,i'
+  $hg push -qf +$ cd ..
+
+Test commas in the --include, plus pull+push
+  $hg clone --narrow ssh://user@dummy/commas-master commas-in-dir \ + > --include a,b --exclude a,b/c,d -qr 0 +$ cd commas-in-dir
+  $hg pull -q +$ echo abef3 >> a,b/e,f/abef
+  $hg ci -qm 'modify a,b/e,f' +$ hg push -qf
+
+Test that --{add,remove}{include,exclude} work with commas in the directory
+names.
+  $hg tracked + I path:a,b + X path:a,b/c,d +$ hg tracked --removeexclude a,b/c,d --addinclude a,b/e,f -q
+  $hg tracked + I path:a,b + I path:a,b/e,f +$ hg files
+  a,b/c,d/abcd
+  a,b/e,f/abef
+  $hg tracked --removeinclude a,b/e,f --addexclude a,b/c,d -q +$ hg tracked
+  I path:a,b
+  X path:a,b/c,d
+  $hg files + a,b/e,f/abef +$ cd ..
diff --git a/tests/test-doctest.py b/tests/test-doctest.py
--- a/tests/test-doctest.py
+++ b/tests/test-doctest.py
@@ -83,6 +83,7 @@
testmod('mercurial.utils.stringutil')
if os.name == 'nt':
testmod('mercurial.windows')
+testmod('mercurial.wireprototypes')
testmod('hgext.convert.convcmd')
testmod('hgext.convert.cvsps')
testmod('hgext.convert.filemap')
diff --git a/relnotes/next b/relnotes/next
--- a/relnotes/next
+++ b/relnotes/next
@@ -23,6 +23,10 @@
Will use zstd compression for new repositories is available, and will
simply fall back to zlib if not.

+ * The experimental narrow extension will now be able to have include or
+   exclude patterns that have a comma in the name when both client and server
+   are updated.
+
== New Experimental Features ==

* hg copy now supports a --at-rev argument to mark files as
@@ -49,6 +53,15 @@
* hg recover does not verify the validity of the whole repository
anymore. You can pass --verify or call hg verify if necessary.

+ * The experimental narrow extension wireproto serialization format is
+   changing slightly. Windows clients will transmit paths with / as the path
+   separator instead of \. Any , or \ in the path for include or exclude
+   patterns will be escaped. Newer servers speaking with older clients may
+   accidentally unescape paths that weren't actually escaped by the client, but
+   this is extremely unlikely to happen in practice. Newer clients speaking with
+   older servers will escape any \ character and this will not be interpreted
+   properly by the server, causing the path to not match.
+
== Internal API Changes ==

* The deprecated ui.progress() has now been deleted. Please use
diff --git a/mercurial/wireprotov1server.py b/mercurial/wireprotov1server.py
--- a/mercurial/wireprotov1server.py
+++ b/mercurial/wireprotov1server.py
@@ -448,6 +448,8 @@
opts[k] = list(v.split(b','))
elif keytype == b'scsv':
opts[k] = set(v.split(b','))
+        elif keytype == b'csvpaths':
+            opts[k] = wireprototypes.decodecsvpaths(v)
elif keytype == b'boolean':
# Client should serialize False as '0', which is a non-empty string
# so it evaluates as a True bool.
diff --git a/mercurial/wireprotov1peer.py b/mercurial/wireprotov1peer.py
--- a/mercurial/wireprotov1peer.py
+++ b/mercurial/wireprotov1peer.py
@@ -462,6 +462,8 @@
value = b','.join(value)
elif keytype == b'scsv':
value = b','.join(sorted(value))
+            elif keytype == b'csvpaths':
+                value = wireprototypes.encodecsvpaths(value)
elif keytype == b'boolean':
value = b'%i' % bool(value)
elif keytype != b'plain':
diff --git a/mercurial/wireprototypes.py b/mercurial/wireprototypes.py
--- a/mercurial/wireprototypes.py
+++ b/mercurial/wireprototypes.py
@@ -161,6 +161,8 @@
# :nodes: list of binary nodes, transmitted as space-separated hex nodes
# :csv:   list of values, transmitted as comma-separated values
# :scsv:  set of values, transmitted as comma-separated values
+# :csvpaths: list of values, transmitted as comma-separated values individually
+#            escaped with encodecsvpaths
# :plain: string with no transformation needed.
GETBUNDLE_ARGUMENTS = {
@@ -173,11 +175,85 @@
b'cg': b'boolean',
b'cbattempted': b'boolean',
b'stream': b'boolean',
-    b'includepats': b'csv',
-    b'excludepats': b'csv',
+    b'includepats': b'csvpaths',
+    b'excludepats': b'csvpaths',
}

+def encodecsvpaths(paths):
+    r'''escape and join a value of type 'csvpaths', producing a bytes object
+
+    pconvert (so for windows clients, paths like a\b -> a/b, while non-windows
+    clients allow a\b through without issue). Then replace \ with \s, then
+    replace , with \c.
+
+    >>> from mercurial.pycompat import sysstr
+    >>> def check(paths):
+    ...     return sysstr(encodecsvpaths(paths))
+    >>> encodecsvpaths([b'a', b'b', b'c'])
+    'a,b,c'
+    >>> encodecsvpaths([br'a\b', b'c'])
+    'a\\SlAsHb,c'
+    >>> encodecsvpaths([b'a,b', b'c'])
+    'a\\CoMmAb,c'
+    >>> encodecsvpaths([b'a\\cb,\\'])
+    'a\\SlAsHcb\\CoMmA\\SlAsH'
+    >>> encodecsvpaths([b','])
+    '\\CoMmA'
+    >>> encodecsvpaths([b'\\'])
+    '\\SlAsH'
+    >>> encodecsvpaths([])
+    ''
+    >>> encodecsvpaths([b''])
+    ''
+    '''
+    return b','.join(
+        [
+            util.pconvert(p)
+            .replace(b'\\', br'\SlAsH')
+            .replace(b',', br'\CoMmA')
+            for p in paths
+        ]
+    )
+
+
+def decodecsvpaths(s):
+    r'''decode an value of type 'csvpaths', producing a list
+
+    If s is an empty string, decodes to an empty list.
+
+    >>> from mercurial.pycompat import sysstr
+    >>> def check(s):
+    ...     return [sysstr(x) for x in decodecsvpaths(s)]
+    >>> check(br'a,b,c')
+    ['a', 'b', 'c']
+    >>> check(br'a\SlAsHb,c')
+    ['a\\b', 'c']
+    >>> check(br'a\CoMmAb,c')
+    ['a,b', 'c']
+    >>> check(br'a\SlAsHcb\CoMmA\SlAsH')
+    ['a\\cb,\\']
+    >>> check(br'\CoMmA')
+    [',']
+    >>> check(br'\SlAsH')
+    ['\\']
+    >>> check(br'')
+    []
+
+    >>> # We may receive sequences like this from old clients.
+    >>> check(br'\x')
+    ['\\x']
+    '''
+    return (
+        [
+            p.replace(br'\CoMmA', b',').replace(br'\SlAsH', b'\\')
+            for p in s.split(b',')
+        ]
+        if s
+        else []
+    )
+
+
class baseprotocolhandler(interfaceutil.Interface):
"""Abstract base class for wire protocol handlers.

diff --git a/hgext/narrow/narrowwirepeer.py b/hgext/narrow/narrowwirepeer.py
--- a/hgext/narrow/narrowwirepeer.py
+++ b/hgext/narrow/narrowwirepeer.py
@@ -79,14 +79,10 @@
preferuncompressed = False
try:

-        def splitpaths(data):
-            # work around ''.split(',') => ['']
-            return data.split(b',') if data else []
-
-        oldincludes = splitpaths(oldincludes)
-        newincludes = splitpaths(newincludes)
-        oldexcludes = splitpaths(oldexcludes)
-        newexcludes = splitpaths(newexcludes)
+        oldincludes = wireprototypes.decodecsvpaths(oldincludes)
+        newincludes = wireprototypes.decodecsvpaths(newincludes)
+        oldexcludes = wireprototypes.decodecsvpaths(oldexcludes)
+        newexcludes = wireprototypes.decodecsvpaths(newexcludes)
# validate the patterns
narrowspec.validatepatterns(set(oldincludes))
narrowspec.validatepatterns(set(newincludes))
@@ -143,7 +139,7 @@
kwargs[ch] = wireprototypes.encodelist(kwargs[ch])

for ch in ('oldincludes', 'newincludes', 'oldexcludes', 'newexcludes'):
-        kwargs[ch] = b','.join(kwargs[ch])
+        kwargs[ch] = wireprototypes.encodecsvpaths(kwargs[ch])

kwargs['ellipses'] = b'%i' % bool(kwargs['ellipses'])
f = remote._callcompressable(b'narrow_widen', **kwargs)
diff --git a/hgext/narrow/narrowbundle2.py b/hgext/narrow/narrowbundle2.py
--- a/hgext/narrow/narrowbundle2.py
+++ b/hgext/narrow/narrowbundle2.py
@@ -304,8 +304,8 @@

getbundleargs[b'narrow'] = b'boolean'
getbundleargs[b'depth'] = b'plain'
-    getbundleargs[b'oldincludepats'] = b'csv'
-    getbundleargs[b'oldexcludepats'] = b'csv'
+    getbundleargs[b'oldincludepats'] = b'csvpaths'
+    getbundleargs[b'oldexcludepats'] = b'csvpaths'
getbundleargs[b'known'] = b'csv'

# Extend changegroup serving to handle requests from narrow clients.