Submitter | Pierre-Yves David |
---|---|
Date | April 13, 2019, 11:40 p.m. |
Message ID | <0adcfded9b03fff84190.1555198834@nodosa.octopoid.net> |
Download | mbox | patch |
Permalink | /patch/39598/ |
State | Accepted |
Headers | show |
Comments
On Sat, Apr 13, 2019 at 5:56 PM Pierre-Yves David < pierre-yves.david@ens-lyon.org> wrote: > # HG changeset patch > # User Pierre-Yves David <pierre-yves.david@octobus.net> > # Date 1554472565 -7200 > # Fri Apr 05 15:56:05 2019 +0200 > # Node ID 0adcfded9b03fff84190594ef29e37110967419f > # Parent d5f42ea7b06825ee86620cdc18aaa3a53504bff5 > # EXP-Topic hgweb-obsolete > # Available At https://bitbucket.org/octobus/mercurial-devel/ > # hg pull https://bitbucket.org/octobus/mercurial-devel/ -r > 0adcfded9b03 > pull: deal with locally filtered changeset passed into --rev > > Nowadays, it is possible to explicitly pull a remote revision that end up > being > hidden locally (eg: obsoleted locally). However before this patch, some > internal processing where crashing trying to resolve a filtered revision. > > Without this patches, the pull output result a confusing output: > > $ hg pull ../repo-Bob --rev 956063ac4557 > pulling from ../repo-Bob > searching for changes > adding changesets > adding manifests > adding file changes > added 2 changesets with 0 changes to 2 files (+1 heads) > (2 other changesets obsolete on arrival) > abort: 00changelog.i@956063ac4557828781733b2d5677a351ce856f59: filtered > node! > The existing abort message is bad and should be improved because typical users won't have a clue what it means. But I have reservations about this patch because it isn't clear what will happen with `pull -u -r <hidden>`. If the working directory will be updated to a hidden revision without --hidden specified, this feels wrong to me. Could we get test coverage showing what happens in this case? Please also check for behavior with `hg clone -r <hidden>` and `hg clone -u <rev>` as well. Also, I'm a little confused about "checkout" and "brev" both doing similar things. It seems that "checkout" is used internally and "brev" is used for user-facing output. I wish this code were better documented. But that is scope bloat... > > diff --git a/mercurial/commands.py b/mercurial/commands.py > --- a/mercurial/commands.py > +++ b/mercurial/commands.py > @@ -4488,7 +4488,7 @@ def pull(ui, repo, source="default", **o > brev = None > > if checkout: > - checkout = repo.changelog.rev(checkout) > + checkout = repo.unfiltered().changelog.rev(checkout) > > # order below depends on implementation of > # hg.addbranchrevs(). opts['bookmark'] is ignored, > diff --git a/tests/test-obsolete-distributed.t > b/tests/test-obsolete-distributed.t > --- a/tests/test-obsolete-distributed.t > +++ b/tests/test-obsolete-distributed.t > @@ -488,6 +488,22 @@ decision is made in that case, so receiv > d33b0a3a64647d79583526be8107802b1f9fedfa > 5b5708a437f27665db42c5a261a539a1bcb2a8c2 0 (Thu Jan 01 00:00:00 1970 +0000) > {'ef1': '1', 'operation': 'amend', 'user': 'bob'} > ef908e42ce65ef57f970d799acaddde26f58a4cc > 5ffb9e311b35f6ab6f76f667ca5d6e595645481b 0 (Thu Jan 01 00:00:00 1970 +0000) > {'ef1': '4', 'operation': 'rebase', 'user': 'bob'} > > + > +Same tests, but with --rev, this prevent regressing case where `hg pull > --rev > +X` has to process a X that is filtered locally. > + > + $ hg rollback > + repository tip rolled back to revision 4 (undo unbundle) > + $ hg pull ../repo-Bob --rev 956063ac4557 > + pulling from ../repo-Bob > + searching for changes > + adding changesets > + adding manifests > + adding file changes > + added 2 changesets with 0 changes to 2 files (+1 heads) > + (2 other changesets obsolete on arrival) > + (run 'hg heads' to see heads) > + > $ cd .. > > Test pull report consistency > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel >
On Tue, Apr 16, 2019 at 4:54 AM Gregory Szorc <gregory.szorc@gmail.com> wrote: > On Sat, Apr 13, 2019 at 5:56 PM Pierre-Yves David < > pierre-yves.david@ens-lyon.org> wrote: > >> # HG changeset patch >> # User Pierre-Yves David <pierre-yves.david@octobus.net> >> # Date 1554472565 -7200 >> # Fri Apr 05 15:56:05 2019 +0200 >> # Node ID 0adcfded9b03fff84190594ef29e37110967419f >> # Parent d5f42ea7b06825ee86620cdc18aaa3a53504bff5 >> # EXP-Topic hgweb-obsolete >> # Available At https://bitbucket.org/octobus/mercurial-devel/ >> # hg pull https://bitbucket.org/octobus/mercurial-devel/ -r >> 0adcfded9b03 >> pull: deal with locally filtered changeset passed into --rev >> >> Nowadays, it is possible to explicitly pull a remote revision that end up >> being >> hidden locally (eg: obsoleted locally). However before this patch, some >> internal processing where crashing trying to resolve a filtered revision. >> >> Without this patches, the pull output result a confusing output: >> >> $ hg pull ../repo-Bob --rev 956063ac4557 >> pulling from ../repo-Bob >> searching for changes >> adding changesets >> adding manifests >> adding file changes >> added 2 changesets with 0 changes to 2 files (+1 heads) >> (2 other changesets obsolete on arrival) >> abort: 00changelog.i@956063ac4557828781733b2d5677a351ce856f59: >> filtered node! >> > > The existing abort message is bad and should be improved because typical > users won't have a clue what it means. > > But I have reservations about this patch because it isn't clear what will > happen with `pull -u -r <hidden>`. If the working directory will be updated > to a hidden revision without --hidden specified, this feels wrong to me. > Oh - maybe part 5 (and later?) address my concerns? > > Could we get test coverage showing what happens in this case? Please also > check for behavior with `hg clone -r <hidden>` and `hg clone -u <rev>` as > well. > > Also, I'm a little confused about "checkout" and "brev" both doing similar > things. It seems that "checkout" is used internally and "brev" is used for > user-facing output. I wish this code were better documented. But that is > scope bloat... > > >> >> diff --git a/mercurial/commands.py b/mercurial/commands.py >> --- a/mercurial/commands.py >> +++ b/mercurial/commands.py >> @@ -4488,7 +4488,7 @@ def pull(ui, repo, source="default", **o >> brev = None >> >> if checkout: >> - checkout = repo.changelog.rev(checkout) >> + checkout = repo.unfiltered().changelog.rev(checkout) >> >> # order below depends on implementation of >> # hg.addbranchrevs(). opts['bookmark'] is ignored, >> diff --git a/tests/test-obsolete-distributed.t >> b/tests/test-obsolete-distributed.t >> --- a/tests/test-obsolete-distributed.t >> +++ b/tests/test-obsolete-distributed.t >> @@ -488,6 +488,22 @@ decision is made in that case, so receiv >> d33b0a3a64647d79583526be8107802b1f9fedfa >> 5b5708a437f27665db42c5a261a539a1bcb2a8c2 0 (Thu Jan 01 00:00:00 1970 +0000) >> {'ef1': '1', 'operation': 'amend', 'user': 'bob'} >> ef908e42ce65ef57f970d799acaddde26f58a4cc >> 5ffb9e311b35f6ab6f76f667ca5d6e595645481b 0 (Thu Jan 01 00:00:00 1970 +0000) >> {'ef1': '4', 'operation': 'rebase', 'user': 'bob'} >> >> + >> +Same tests, but with --rev, this prevent regressing case where `hg pull >> --rev >> +X` has to process a X that is filtered locally. >> + >> + $ hg rollback >> + repository tip rolled back to revision 4 (undo unbundle) >> + $ hg pull ../repo-Bob --rev 956063ac4557 >> + pulling from ../repo-Bob >> + searching for changes >> + adding changesets >> + adding manifests >> + adding file changes >> + added 2 changesets with 0 changes to 2 files (+1 heads) >> + (2 other changesets obsolete on arrival) >> + (run 'hg heads' to see heads) >> + >> $ cd .. >> >> Test pull report consistency >> _______________________________________________ >> Mercurial-devel mailing list >> Mercurial-devel@mercurial-scm.org >> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel >> >
On 4/16/19 12:56 PM, Gregory Szorc wrote: > On Tue, Apr 16, 2019 at 4:54 AM Gregory Szorc <gregory.szorc@gmail.com > <mailto:gregory.szorc@gmail.com>> wrote: > > On Sat, Apr 13, 2019 at 5:56 PM Pierre-Yves David > <pierre-yves.david@ens-lyon.org > <mailto:pierre-yves.david@ens-lyon.org>> wrote: > > # HG changeset patch > # User Pierre-Yves David <pierre-yves.david@octobus.net > <mailto:pierre-yves.david@octobus.net>> > # Date 1554472565 -7200 > # Fri Apr 05 15:56:05 2019 +0200 > # Node ID 0adcfded9b03fff84190594ef29e37110967419f > # Parent d5f42ea7b06825ee86620cdc18aaa3a53504bff5 > # EXP-Topic hgweb-obsolete > # Available At https://bitbucket.org/octobus/mercurial-devel/ > # hg pull > https://bitbucket.org/octobus/mercurial-devel/ -r 0adcfded9b03 > pull: deal with locally filtered changeset passed into --rev > > Nowadays, it is possible to explicitly pull a remote revision > that end up being > hidden locally (eg: obsoleted locally). However before this > patch, some > internal processing where crashing trying to resolve a filtered > revision. > > Without this patches, the pull output result a confusing output: > > $ hg pull ../repo-Bob --rev 956063ac4557 > pulling from ../repo-Bob > searching for changes > adding changesets > adding manifests > adding file changes > added 2 changesets with 0 changes to 2 files (+1 heads) > (2 other changesets obsolete on arrival) > abort: > 00changelog.i@956063ac4557828781733b2d5677a351ce856f59: filtered > node! > > > The existing abort message is bad and should be improved because > typical users won't have a clue what it means. > > But I have reservations about this patch because it isn't clear what > will happen with `pull -u -r <hidden>`. If the working directory > will be updated to a hidden revision without --hidden specified, > this feels wrong to me. > > > Oh - maybe part 5 (and later?) address my concerns? It does. Regarding the abort message. I know we have a better message available for this kind of error. I am not sure why that better message is not used here and I intend to dig into that next cycle (there are a couple of usual suspect: wrong exception types, bits still in evolve extension, etc…) > Could we get test coverage showing what happens in this case? Please > also check for behavior with `hg clone -r <hidden>` and `hg clone -u > <rev>` as well. (as you already noticed, this is addressed in the next patch). > Also, I'm a little confused about "checkout" and "brev" both doing > similar things. It seems that "checkout" is used internally and > "brev" is used for user-facing output. I wish this code were better > documented. But that is scope bloat... +1 Cheers,
On Tue, Apr 16, 2019 at 3:37 PM Pierre-Yves David < pierre-yves.david@ens-lyon.org> wrote: > > > On 4/16/19 12:56 PM, Gregory Szorc wrote: > > On Tue, Apr 16, 2019 at 4:54 AM Gregory Szorc <gregory.szorc@gmail.com > > <mailto:gregory.szorc@gmail.com>> wrote: > > > > On Sat, Apr 13, 2019 at 5:56 PM Pierre-Yves David > > <pierre-yves.david@ens-lyon.org > > <mailto:pierre-yves.david@ens-lyon.org>> wrote: > > > > # HG changeset patch > > # User Pierre-Yves David <pierre-yves.david@octobus.net > > <mailto:pierre-yves.david@octobus.net>> > > # Date 1554472565 -7200 > > # Fri Apr 05 15:56:05 2019 +0200 > > # Node ID 0adcfded9b03fff84190594ef29e37110967419f > > # Parent d5f42ea7b06825ee86620cdc18aaa3a53504bff5 > > # EXP-Topic hgweb-obsolete > > # Available At https://bitbucket.org/octobus/mercurial-devel/ > > # hg pull > > https://bitbucket.org/octobus/mercurial-devel/ -r 0adcfded9b03 > > pull: deal with locally filtered changeset passed into --rev > > > > Nowadays, it is possible to explicitly pull a remote revision > > that end up being > > hidden locally (eg: obsoleted locally). However before this > > patch, some > > internal processing where crashing trying to resolve a filtered > > revision. > > > > Without this patches, the pull output result a confusing output: > > > > $ hg pull ../repo-Bob --rev 956063ac4557 > > pulling from ../repo-Bob > > searching for changes > > adding changesets > > adding manifests > > adding file changes > > added 2 changesets with 0 changes to 2 files (+1 heads) > > (2 other changesets obsolete on arrival) > > abort: > > 00changelog.i@956063ac4557828781733b2d5677a351ce856f59: filtered > > node! > > > > > > The existing abort message is bad and should be improved because > > typical users won't have a clue what it means. > > > > But I have reservations about this patch because it isn't clear what > > will happen with `pull -u -r <hidden>`. If the working directory > > will be updated to a hidden revision without --hidden specified, > > this feels wrong to me. > > > > > > Oh - maybe part 5 (and later?) address my concerns? > > It does. Regarding the abort message. I know we have a better message > available for this kind of error. I am not sure why that better message > is not used here and I intend to dig into that next cycle (there are a > couple of usual suspect: wrong exception types, bits still in evolve > extension, etc…) > > > Could we get test coverage showing what happens in this case? Please > > also check for behavior with `hg clone -r <hidden>` and `hg clone -u > > <rev>` as well. > > (as you already noticed, this is addressed in the next patch). > Queued 4 and 5. Many thanks! > > > Also, I'm a little confused about "checkout" and "brev" both doing > > similar things. It seems that "checkout" is used internally and > > "brev" is used for user-facing output. I wish this code were better > > documented. But that is scope bloat... > > +1 > I also tried to understand and was not successful. Maybe hg.addbranchrevs() needs docs.
Patch
diff --git a/mercurial/commands.py b/mercurial/commands.py --- a/mercurial/commands.py +++ b/mercurial/commands.py @@ -4488,7 +4488,7 @@ def pull(ui, repo, source="default", **o brev = None if checkout: - checkout = repo.changelog.rev(checkout) + checkout = repo.unfiltered().changelog.rev(checkout) # order below depends on implementation of # hg.addbranchrevs(). opts['bookmark'] is ignored, diff --git a/tests/test-obsolete-distributed.t b/tests/test-obsolete-distributed.t --- a/tests/test-obsolete-distributed.t +++ b/tests/test-obsolete-distributed.t @@ -488,6 +488,22 @@ decision is made in that case, so receiv d33b0a3a64647d79583526be8107802b1f9fedfa 5b5708a437f27665db42c5a261a539a1bcb2a8c2 0 (Thu Jan 01 00:00:00 1970 +0000) {'ef1': '1', 'operation': 'amend', 'user': 'bob'} ef908e42ce65ef57f970d799acaddde26f58a4cc 5ffb9e311b35f6ab6f76f667ca5d6e595645481b 0 (Thu Jan 01 00:00:00 1970 +0000) {'ef1': '4', 'operation': 'rebase', 'user': 'bob'} + +Same tests, but with --rev, this prevent regressing case where `hg pull --rev +X` has to process a X that is filtered locally. + + $ hg rollback + repository tip rolled back to revision 4 (undo unbundle) + $ hg pull ../repo-Bob --rev 956063ac4557 + pulling from ../repo-Bob + searching for changes + adding changesets + adding manifests + adding file changes + added 2 changesets with 0 changes to 2 files (+1 heads) + (2 other changesets obsolete on arrival) + (run 'hg heads' to see heads) + $ cd .. Test pull report consistency