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

login
register
mail settings
Submitter phabricator
Date March 13, 2020, 4:12 a.m.
Message ID <differential-rev-PHID-DREV-zvpkdjtjeqan6hhi3f3u-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/45761/
State New
Headers show

Comments

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)

INLINE COMMENTS

> 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

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 = {
     b'heads': b'nodes',
@@ -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.