Patchwork [04,of,14,"] pull: deal with locally filtered changeset passed into --rev

login
register
mail settings
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

Pierre-Yves David - April 13, 2019, 11:40 p.m.
# 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!
Gregory Szorc - April 16, 2019, 10:54 a.m.
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
>
Gregory Szorc - April 16, 2019, 10:56 a.m.
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
>>
>
Pierre-Yves David - April 16, 2019, 12:26 p.m.
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,
Pulkit Goyal - April 16, 2019, 12:42 p.m.
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