This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7969

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

Authored by mharbison72 on Jan 22 2020, 12:14 PM.

Details

Reviewers
marmoute
pulkit
Group Reviewers
hg-reviewers
Commits
rHG877805928f85: exchange: check the `ui.clonebundleprefers` form while processing (issue6257)
rHG5fb0592f90e7: exchange: check the `ui.clonebundleprefers` form while processing (issue6257)
Summary

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

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

mharbison72 created this revision.Jan 22 2020, 12:14 PM
marmoute requested changes to this revision.Feb 5 2020, 6:25 PM
marmoute added a subscriber: marmoute.

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

tests/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 ?

This revision now requires changes to proceed.Feb 5 2020, 6:25 PM
mharbison72 requested review of this revision.Feb 5 2020, 9:21 PM
mharbison72 added inline comments.
tests/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.

marmoute added inline comments.Feb 6 2020, 4:05 AM
tests/test-clonebundles.t
458–464

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]
mharbison72 updated this revision to Diff 19984.Feb 7 2020, 9:59 AM
marmoute accepted this revision.Feb 7 2020, 10:01 AM

Thanks

pulkit accepted this revision.Feb 7 2020, 3:25 PM
This revision is now accepted and ready to land.Feb 7 2020, 3:25 PM

Revision Contents
Changeset List

PathPackages
Mmercurial/exchange.py (10 lines)
Mtests/test-clonebundles.t (13 lines)

Diff 20001

mercurial/exchange.py

return self._cmp(other) != 0 return self._cmp(other) != 0
def sortclonebundleentries(ui, entries): def sortclonebundleentries(ui, entries):
prefers = ui.configlist(b'ui', b'clonebundleprefers') prefers = ui.configlist(b'ui', b'clonebundleprefers')
if not prefers: if not prefers:
return list(entries) 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) items = sorted(clonebundleentry(v, prefers) for v in entries)
return [i.value for i in items] return [i.value for i in items]
def trypullbundlefromurl(ui, repo, url): def trypullbundlefromurl(ui, repo, url):
"""Attempt to apply a bundle from a URL.""" """Attempt to apply a bundle from a URL."""
with repo.lock(), repo.transaction(b'bundleurl') as tr: with repo.lock(), repo.transaction(b'bundleurl') as tr:

tests/test-clonebundles.t

adding manifests adding manifests
adding file changes adding file changes
added 2 changesets with 2 changes to 2 files added 2 changesets with 2 changes to 2 files
finished applying clone bundle finished applying clone bundle
searching for changes searching for changes
no changes found no changes found
2 local changesets published 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]
$ hg --config ui.clonebundleprefers=key=val,bad,key2=val2 clone \
marmouteUnsubmitted
Not Done

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 ?

marmoute: Your patch is about testing error when only one of the items is bad. Then, your tests only test…
mharbison72AuthorUnsubmitted
Done

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.

mharbison72: Not sure I understand. There are a handful of tests in test-clonebundles.t that have multiple…
marmouteUnsubmitted
Not Done

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]
marmoute: a test mixing good and bad would check that we report error on the appropriate blocks.
> -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 Test interaction between clone bundles and --stream
A manifest with just a gzip bundle A manifest with just a gzip bundle
$ cat > server/.hg/clonebundles.manifest << EOF $ cat > server/.hg/clonebundles.manifest << EOF
> http://localhost:$HGPORT1/gz-a.hg BUNDLESPEC=gzip-v2 > http://localhost:$HGPORT1/gz-a.hg BUNDLESPEC=gzip-v2
> EOF > EOF