Patchwork D7969: exchange: check the `ui.clonebundleprefers` form while processing (issue6257)

login
register
mail settings
Submitter phabricator
Date Jan. 22, 2020, 5:15 p.m.
Message ID <differential-rev-PHID-DREV-oa3ayh44qf5ngb67gsvt-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/44574/
State Superseded
Headers show

Comments

phabricator - Jan. 22, 2020, 5:15 p.m.
mharbison72 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Otherwise the clone command will emit a long stacktrace if there is no `=`
  character.

REPOSITORY
  rHG Mercurial

BRANCH
  stable

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

AFFECTED FILES
  mercurial/exchange.py
  tests/test-clonebundles.t

CHANGE DETAILS




To: mharbison72, #hg-reviewers
Cc: mercurial-devel
phabricator - Feb. 5, 2020, 11:26 p.m.
This revision now requires changes to proceed.
marmoute added a comment.
marmoute requested changes to this revision.


  (feel free to put it back in review-needed if my question was silly).

INLINE COMMENTS

> test-clonebundles.t:458-464
> +Test a bad attribute list
> +
> +  $ hg --config ui.clonebundleprefers=bad clone -U http://localhost:$HGPORT bad-input
> +  abort: invalid ui.clonebundleprefers item: bad
> +  (each comma separated item should be key=value pairs)
> +  [255]
> +

Your patch is about testing error when only one of the items is bad. Then, your tests only test it with a single item. Am I missing something or should we have a tests with multiple items in the list ?

REPOSITORY
  rHG Mercurial

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

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

To: mharbison72, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - Feb. 6, 2020, 2:21 a.m.
mharbison72 added inline comments.
mharbison72 requested review of this revision.

INLINE COMMENTS

> marmoute wrote in test-clonebundles.t:458-464
> Your patch is about testing error when only one of the items is bad. Then, your tests only test it with a single item. Am I missing something or should we have a tests with multiple items in the list ?

Not sure I understand.  There are a handful of tests in test-clonebundles.t that have multiple key/value pairs already, so the good case is covered.  (I missed this before and thought that I commented there was no coverage, but I was wrong and don't see that comment now.)

The setting needs to be in `ui.clonebundles=key=value[,key=value]` form.  So if it catches a single malformed key/value pair, I'm not sure that it buys us anything to have a `good,bad` test too, unless I'm missing something.

REPOSITORY
  rHG Mercurial

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

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

To: mharbison72, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - Feb. 6, 2020, 9:05 a.m.
marmoute added inline comments.

INLINE COMMENTS

> mharbison72 wrote in test-clonebundles.t:458-464
> Not sure I understand.  There are a handful of tests in test-clonebundles.t that have multiple key/value pairs already, so the good case is covered.  (I missed this before and thought that I commented there was no coverage, but I was wrong and don't see that comment now.)
> 
> The setting needs to be in `ui.clonebundles=key=value[,key=value]` form.  So if it catches a single malformed key/value pair, I'm not sure that it buys us anything to have a `good,bad` test too, unless I'm missing something.

a test mixing good and bad would check that we report error on the appropriate blocks.

something like:

$ hg --config ui.clonebundleprefers=extra=b,bad,COMPRESSION=unknown clone -U http://localhost:$HGPORT bad-input

  abort: invalid ui.clonebundleprefers item: bad
  (each comma separated item should be key=value pairs)
  [255]

REPOSITORY
  rHG Mercurial

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

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

To: mharbison72, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - Feb. 7, 2020, 3:01 p.m.
marmoute added a comment.
marmoute accepted this revision.


  Thanks

REPOSITORY
  rHG Mercurial

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

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

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

Patch

diff --git a/tests/test-clonebundles.t b/tests/test-clonebundles.t
--- a/tests/test-clonebundles.t
+++ b/tests/test-clonebundles.t
@@ -455,6 +455,13 @@ 
   no changes found
   2 local changesets published
 
+Test a bad attribute list
+
+  $ hg --config ui.clonebundleprefers=bad clone -U http://localhost:$HGPORT bad-input
+  abort: invalid ui.clonebundleprefers item: bad
+  (each comma separated item should be key=value pairs)
+  [255]
+
 Test interaction between clone bundles and --stream
 
 A manifest with just a gzip bundle
diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -3072,7 +3072,15 @@ 
     if not prefers:
         return list(entries)
 
-    prefers = [p.split(b'=', 1) for p in prefers]
+    def _split(p):
+        if b'=' not in p:
+            hint = _(b"each comma separated item should be key=value pairs")
+            raise error.Abort(
+                _(b"invalid ui.clonebundleprefers item: %s") % p, hint=hint
+            )
+        return p.split(b'=', 1)
+
+    prefers = [_split(p) for p in prefers]
 
     items = sorted(clonebundleentry(v, prefers) for v in entries)
     return [i.value for i in items]