Patchwork [1,of,4] subrepo: do not try to get hidden revisions

login
register
mail settings
Submitter Angel Ezquerra
Date Nov. 25, 2013, 8:46 p.m.
Message ID <adb3d2a8dfd8bb1c6531.1385412407@Angel-PC.localdomain>
Download mbox | patch
Permalink /patch/3134/
State Superseded
Commit d6939f29b3b358f4e06282c0289365b85bd096aa
Headers show

Comments

Angel Ezquerra - Nov. 25, 2013, 8:46 p.m.
# HG changeset patch
# User Angel Ezquerra <angel.ezquerra@gmail.com>
# Date 1385255414 -3600
#      Sun Nov 24 02:10:14 2013 +0100
# Node ID adb3d2a8dfd8bb1c6531225ef84be55f082bac0f
# Parent  1c46b18b0e1c47fa4cecf21b78c083a54ae9903f
subrepo: do not try to get hidden revisions

If a subrepo revision is hidden (because it was amended, for example) it does
not make sense to try to "get" it from the remote subrepository.

Note that in order to avoid making the change look bigger than it is, this adds
an unnecessary else clause. This will be removed on a follow up patch.
Pierre-Yves David - Jan. 16, 2014, midnight
On 11/25/2013 12:46 PM, Angel Ezquerra wrote:
> # HG changeset patch
> # User Angel Ezquerra <angel.ezquerra@gmail.com>
> # Date 1385255414 -3600
> #      Sun Nov 24 02:10:14 2013 +0100
> # Node ID adb3d2a8dfd8bb1c6531225ef84be55f082bac0f
> # Parent  1c46b18b0e1c47fa4cecf21b78c083a54ae9903f
> subrepo: do not try to get hidden revisions
>
> If a subrepo revision is hidden (because it was amended, for example) it does
> not make sense to try to "get" it from the remote subrepository.
>
> Note that in order to avoid making the change look bigger than it is, this adds
> an unnecessary else clause. This will be removed on a follow up patch.
>
> diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
> --- a/mercurial/subrepo.py
> +++ b/mercurial/subrepo.py
> @@ -648,7 +648,10 @@
>   
>       def _get(self, state):
>           source, revision, kind = state
> -        if revision not in self._repo:
> +        urepo = self._repo.unfiltered()
> +        if revision in urepo:
> +            return

The right way to do it is:

1) having a new exception to "FilteredLookupError" inheriting from 
"LookupError" (may need to add a Repo here somewhere),

2) Catch that specific exception when you need to distinct between 
"revision is missing" and "revision is filtered",

I see only one reason to not request for this approach here:

Why are you not using `if revision in self._repo.unfiltered(): return` 
In the first place ?
Angel Ezquerra - Jan. 16, 2014, 7:04 a.m.
On Wed, Jan 15, 2014 at 4:00 PM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
> On 11/25/2013 12:46 PM, Angel Ezquerra wrote:
>>
>> # HG changeset patch
>> # User Angel Ezquerra <angel.ezquerra@gmail.com>
>> # Date 1385255414 -3600
>> #      Sun Nov 24 02:10:14 2013 +0100
>> # Node ID adb3d2a8dfd8bb1c6531225ef84be55f082bac0f
>> # Parent  1c46b18b0e1c47fa4cecf21b78c083a54ae9903f
>> subrepo: do not try to get hidden revisions
>>
>> If a subrepo revision is hidden (because it was amended, for example) it
>> does
>> not make sense to try to "get" it from the remote subrepository.
>>
>> Note that in order to avoid making the change look bigger than it is, this
>> adds
>> an unnecessary else clause. This will be removed on a follow up patch.
>>
>> diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
>> --- a/mercurial/subrepo.py
>> +++ b/mercurial/subrepo.py
>> @@ -648,7 +648,10 @@
>>         def _get(self, state):
>>           source, revision, kind = state
>> -        if revision not in self._repo:
>> +        urepo = self._repo.unfiltered()
>> +        if revision in urepo:
>> +            return
>
>
> The right way to do it is:
>
> 1) having a new exception to "FilteredLookupError" inheriting from
> "LookupError" (may need to add a Repo here somewhere),
>
> 2) Catch that specific exception when you need to distinct between "revision
> is missing" and "revision is filtered",
>
> I see only one reason to not request for this approach here:
>
> Why are you not using `if revision in self._repo.unfiltered(): return` In
> the first place ?

I think in this case it makes more sense to just do as you suggest and
do `if revision in self._repo.unfiltered(): return`, since the urepo
variable that I introduced is not necessary.

Would you be OK with a new patch that did just that (rather than
introducing the new exception type that you suggest)?

Angel
Pierre-Yves David - Jan. 16, 2014, 9:06 p.m.
On 01/15/2014 11:04 PM, Angel Ezquerra wrote:
> On Wed, Jan 15, 2014 at 4:00 PM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
>> On 11/25/2013 12:46 PM, Angel Ezquerra wrote:
>>> # HG changeset patch
>>> # User Angel Ezquerra <angel.ezquerra@gmail.com>
>>> # Date 1385255414 -3600
>>> #      Sun Nov 24 02:10:14 2013 +0100
>>> # Node ID adb3d2a8dfd8bb1c6531225ef84be55f082bac0f
>>> # Parent  1c46b18b0e1c47fa4cecf21b78c083a54ae9903f
>>> subrepo: do not try to get hidden revisions
>>>
>>> If a subrepo revision is hidden (because it was amended, for example) it
>>> does
>>> not make sense to try to "get" it from the remote subrepository.
>>>
>>> Note that in order to avoid making the change look bigger than it is, this
>>> adds
>>> an unnecessary else clause. This will be removed on a follow up patch.
>>>
>>> diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
>>> --- a/mercurial/subrepo.py
>>> +++ b/mercurial/subrepo.py
>>> @@ -648,7 +648,10 @@
>>>          def _get(self, state):
>>>            source, revision, kind = state
>>> -        if revision not in self._repo:
>>> +        urepo = self._repo.unfiltered()
>>> +        if revision in urepo:
>>> +            return
>>
>> The right way to do it is:
>>
>> 1) having a new exception to "FilteredLookupError" inheriting from
>> "LookupError" (may need to add a Repo here somewhere),
>>
>> 2) Catch that specific exception when you need to distinct between "revision
>> is missing" and "revision is filtered",
>>
>> I see only one reason to not request for this approach here:
>>
>> Why are you not using `if revision in self._repo.unfiltered(): return` In
>> the first place ?
> I think in this case it makes more sense to just do as you suggest and
> do `if revision in self._repo.unfiltered(): return`, since the urepo
> variable that I introduced is not necessary.
>
> Would you be OK with a new patch that did just that (rather than
> introducing the new exception type that you suggest)?
>
Yes

Patch

# HG changeset patch
# User Angel Ezquerra <angel.ezquerra@gmail.com>
# Date 1385255414 -3600
#      Sun Nov 24 02:10:14 2013 +0100
# Node ID adb3d2a8dfd8bb1c6531225ef84be55f082bac0f
# Parent  1c46b18b0e1c47fa4cecf21b78c083a54ae9903f
subrepo: do not try to get hidden revisions

If a subrepo revision is hidden (because it was amended, for example) it does
not make sense to try to "get" it from the remote subrepository.

Note that in order to avoid making the change look bigger than it is, this adds
an unnecessary else clause. This will be removed on a follow up patch.

diff --git a/mercurial/subrepo.py b/mercurial/subrepo.py
--- a/mercurial/subrepo.py
+++ b/mercurial/subrepo.py
@@ -648,7 +648,10 @@ 
 
     def _get(self, state):
         source, revision, kind = state
-        if revision not in self._repo:
+        urepo = self._repo.unfiltered()
+        if revision in urepo:
+            return
+        else:
             self._repo._subsource = source
             srcurl = _abssource(self._repo)
             other = hg.peer(self._repo, {}, srcurl)