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
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
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
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
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]