Patchwork [4,of,4] clfilter: fallback to unfiltered version when linkrev point to filtered history

login
register
mail settings
Submitter Pierre-Yves David
Date Jan. 2, 2013, 1:36 a.m.
Message ID <a397f42becefc44489d4.1357090560@yamac.lan>
Download mbox | patch
Permalink /patch/365/
State Accepted
Commit 518c1403838f1d869fec357488d9221fea765177
Headers show

Comments

Pierre-Yves David - Jan. 2, 2013, 1:36 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david at ens-lyon.org>
# Date 1356738018 -3600
# Node ID a397f42becefc44489d4d2cffb14ac1999aea375
# Parent  d2f15a6470d3566b722379d48da8c88b26f398fd
clfilter: fallback to unfiltered version when linkrev point to filtered history


On `filectx`, linkrev may point to any revision in the repository. When the
repository is filtered this may lead to `filectx` trying to build `changectx`
for filtered revision. In such case we fallback to creating `changectx` on the
unfiltered version of the reposition. This fallback should not be an issue
because `changectx` from `filectx` are not used in complex operation that
care about filtering. It is complicated to work around the issue in a
clearer way as code raising such `filectx` rarely have access to the
repository directly.

Linkrevs create a lot of issue with filtering. It is stored in revlog entry at
creation time and never changed. Nothing prevent the changeset revision pointed
to become filtered. Several bogus behavior emerge from such situation. Those
bugs are complex to solve and not part of the current effort to install
filtering. This changeset is simple hack that prevent plain crash in favor on
minor misbehavior without visible effect.

This "hack" is longly documented in to code itself to help people that would
look at it in the future.
Kevin Bullock - Jan. 3, 2013, 9:41 p.m.
On Jan 1, 2013, at 7:36 PM, Pierre-Yves David wrote:

> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david at ens-lyon.org>
> # Date 1356738018 -3600
> # Node ID a397f42becefc44489d4d2cffb14ac1999aea375
> # Parent  d2f15a6470d3566b722379d48da8c88b26f398fd
> clfilter: fallback to unfiltered version when linkrev point to filtered history
> [...]
> diff --git a/mercurial/context.py b/mercurial/context.py
> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -416,11 +416,30 @@ class filectx(object):
>         if fileid is not None:
>             self._fileid = fileid
> 
>     @propertycache
>     def _changectx(self):
> -        return changectx(self._repo, self._changeid)
> +        try:
> +            return changectx(self._repo, self._changeid)
> +        except error.RepoLookupError:
> +            # Linkrev may point to any revision in the repository.  When the
> +            # repository is filtered this may lead to `filectx` trying to build
> +            # `changectx` for filtered revision. In such case we fallback to
> +            # creating `changectx` on the unfiltered version of the reposition.
> +            # This fallback should not be an issue because`changectx` from
> +            # `filectx` are not used in complexe operation that care about
> +            # filtering.
> +            #
> +            # This fallback is a cheap and dirty fix that prevent several
> +            # crash. It does not ensure the behavior is correct. However the
> +            # behavior was not correct before filtering either and "incorrect
> +            # behavior" is seen as better as "crash"
> +            #
> +            # Linkrevs have several serious troubles with filtering that are
> +            # complicated to solve. Proper handling of the issue here should be
> +            # considered when solving linkrev issue are on the table.
> +            return changectx(self._repo.unfiltered(), self._changeid)

Won't this blow the stack if the linkrev isn't in the changelog? This would only happen on a corrupt repo, but it seems like an unnecessarily spectacular way to fail on a bad filelog.

pacem in terris / ??? / ?????? / ????????? / ??
Kevin R. Bullock
Pierre-Yves David - Jan. 3, 2013, 11:37 p.m.
On 3 janv. 2013, at 22:41, Kevin Bullock wrote:

> On Jan 1, 2013, at 7:36 PM, Pierre-Yves David wrote:
> 
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david at ens-lyon.org>
>> # Date 1356738018 -3600
>> # Node ID a397f42becefc44489d4d2cffb14ac1999aea375
>> # Parent  d2f15a6470d3566b722379d48da8c88b26f398fd
>> clfilter: fallback to unfiltered version when linkrev point to filtered history
>> [...]
>> diff --git a/mercurial/context.py b/mercurial/context.py
>> --- a/mercurial/context.py
>> +++ b/mercurial/context.py
>> @@ -416,11 +416,30 @@ class filectx(object):
>>        if fileid is not None:
>>            self._fileid = fileid
>> 
>>    @propertycache
>>    def _changectx(self):
>> -        return changectx(self._repo, self._changeid)
>> +        try:
>> +            return changectx(self._repo, self._changeid)
>> +        except error.RepoLookupError:
>> +            # Linkrev may point to any revision in the repository.  When the
>> +            # repository is filtered this may lead to `filectx` trying to build
>> +            # `changectx` for filtered revision. In such case we fallback to
>> +            # creating `changectx` on the unfiltered version of the reposition.
>> +            # This fallback should not be an issue because`changectx` from
>> +            # `filectx` are not used in complexe operation that care about
>> +            # filtering.
>> +            #
>> +            # This fallback is a cheap and dirty fix that prevent several
>> +            # crash. It does not ensure the behavior is correct. However the
>> +            # behavior was not correct before filtering either and "incorrect
>> +            # behavior" is seen as better as "crash"
>> +            #
>> +            # Linkrevs have several serious troubles with filtering that are
>> +            # complicated to solve. Proper handling of the issue here should be
>> +            # considered when solving linkrev issue are on the table.
>> +            return changectx(self._repo.unfiltered(), self._changeid)
> 
> Won't this blow the stack if the linkrev isn't in the changelog? This would only happen on a corrupt repo, but it seems like an unnecessarily spectacular way to fail on a bad filelog.

No, it's an alternative call, not a recursive one.
When the second call fails re exception reach the caller as before.
Kevin Bullock - Jan. 3, 2013, 11:49 p.m.
On 3 Jan 2013, at 5:37 PM, Pierre-Yves David wrote:

> On 3 janv. 2013, at 22:41, Kevin Bullock wrote:
> 
>> On Jan 1, 2013, at 7:36 PM, Pierre-Yves David wrote:
>> 
>>> # HG changeset patch
>>> # User Pierre-Yves David <pierre-yves.david at ens-lyon.org>
>>> # Date 1356738018 -3600
>>> # Node ID a397f42becefc44489d4d2cffb14ac1999aea375
>>> # Parent  d2f15a6470d3566b722379d48da8c88b26f398fd
>>> clfilter: fallback to unfiltered version when linkrev point to filtered history
>>> [...]
>>> diff --git a/mercurial/context.py b/mercurial/context.py
>>> --- a/mercurial/context.py
>>> +++ b/mercurial/context.py
>>> @@ -416,11 +416,30 @@ class filectx(object):
>>>       if fileid is not None:
>>>           self._fileid = fileid
>>> 
>>>   @propertycache
>>>   def _changectx(self):
>>> -        return changectx(self._repo, self._changeid)
>>> +        try:
>>> +            return changectx(self._repo, self._changeid)
>>> +        except error.RepoLookupError:
>>> +            # Linkrev may point to any revision in the repository.  When the
>>> +            # repository is filtered this may lead to `filectx` trying to build
>>> +            # `changectx` for filtered revision. In such case we fallback to
>>> +            # creating `changectx` on the unfiltered version of the reposition.
>>> +            # This fallback should not be an issue because`changectx` from
>>> +            # `filectx` are not used in complexe operation that care about
>>> +            # filtering.
>>> +            #
>>> +            # This fallback is a cheap and dirty fix that prevent several
>>> +            # crash. It does not ensure the behavior is correct. However the
>>> +            # behavior was not correct before filtering either and "incorrect
>>> +            # behavior" is seen as better as "crash"
>>> +            #
>>> +            # Linkrevs have several serious troubles with filtering that are
>>> +            # complicated to solve. Proper handling of the issue here should be
>>> +            # considered when solving linkrev issue are on the table.
>>> +            return changectx(self._repo.unfiltered(), self._changeid)
>> 
>> Won't this blow the stack if the linkrev isn't in the changelog? This would only happen on a corrupt repo, but it seems like an unnecessarily spectacular way to fail on a bad filelog.
> 
> No, it's an alternative call, not a recursive one.
> When the second call fails re exception reach the caller as before.

Ahh, right, okay.

pacem in terris / ??? / ?????? / ????????? / ??
Kevin R. Bullock
Kevin Bullock - Jan. 4, 2013, 3:34 a.m.
On 1 Jan 2013, at 7:36 PM, Pierre-Yves David wrote:

> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david at ens-lyon.org>
> # Date 1356738018 -3600
> # Node ID a397f42becefc44489d4d2cffb14ac1999aea375
> # Parent  d2f15a6470d3566b722379d48da8c88b26f398fd
> clfilter: fallback to unfiltered version when linkrev point to filtered history

Pushed this series to crew as 518c1403838f.

pacem in terris / ??? / ?????? / ????????? / ??
Kevin R. Bullock

Patch

diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -416,11 +416,30 @@  class filectx(object):
         if fileid is not None:
             self._fileid = fileid
 
     @propertycache
     def _changectx(self):
-        return changectx(self._repo, self._changeid)
+        try:
+            return changectx(self._repo, self._changeid)
+        except error.RepoLookupError:
+            # Linkrev may point to any revision in the repository.  When the
+            # repository is filtered this may lead to `filectx` trying to build
+            # `changectx` for filtered revision. In such case we fallback to
+            # creating `changectx` on the unfiltered version of the reposition.
+            # This fallback should not be an issue because`changectx` from
+            # `filectx` are not used in complexe operation that care about
+            # filtering.
+            #
+            # This fallback is a cheap and dirty fix that prevent several
+            # crash. It does not ensure the behavior is correct. However the
+            # behavior was not correct before filtering either and "incorrect
+            # behavior" is seen as better as "crash"
+            #
+            # Linkrevs have several serious troubles with filtering that are
+            # complicated to solve. Proper handling of the issue here should be
+            # considered when solving linkrev issue are on the table.
+            return changectx(self._repo.unfiltered(), self._changeid)
 
     @propertycache
     def _filelog(self):
         return self._repo.file(self._path)