[PATCH 4 of 6] resolve: abort when not applicable

[Mads Kiilerich]

On 05/16/2014 11:07 PM, Gregory Szorc wrote:
> On 5/15/14, 12:31 AM, Pierre-Yves David wrote:
>>
>>
>> On 05/09/2014 10:23 AM, Pierre-Yves David wrote:
>>> On 05/09/2014 02:54 AM, Mads Kiilerich wrote:
>>>> On 05/09/2014 03:46 AM, Gregory Szorc wrote:
>> […]
>>>>> --- a/mercurial/merge.py
>>>>> +++ b/mercurial/merge.py
>>>>> @@ -175,8 +175,20 @@ class mergestate(object):
>>>>>               if err.errno != errno.ENOENT:
>>>>>                   raise
>>>>>           return records
>>>>> +    def active(self):
>>>>> +        """Whether mergestate is active.
>>>>> +
>>>>> +        Returns True if there appears to be mergestate. This is a
>>>>> rough proxy
>>>>> +        for "is a merge in progress."
>>>>> +        """
>>>>> +        # Check local variables before looking at filesystem for
>>>>> performance
>>>>> +        # reasons.
>>>>> +        return bool(self._local) or bool(self._state) or \
>>>>> +               self._repo.opener.exists(self.statepathv1) or \
>>>>> +               self._repo.opener.exists(self.statepathv2)
>>>>
>>>> Did you have a response to my question on
>>>> http://selenic.com/pipermail/mercurial-devel/2014-May/058585.html :
>>>>
>>>> Will there ever be a (relevant) case where _local doesn't give the 
>>>> full
>>>> answer? A mergestate without  a _local (and _other) is no mergestate.
>>>>
>>>> It seems to me like this code is more complex than necessary.
>>>
>>> Woops, I missed the fact your started reviewing this series.
>>>
>>> This change is already crewed. so please choose one of
>>>
>>> 1. request unqueuing
>>> 2. request followup commit
>>
>> So this changeset is now public. A followup commit would be nice.
>>
>
> I don't think we'll ever have _local without _other. The patch as 
> committed reflects this.

That do not answer the question, "Will there ever be a (relevant) case 
where _local doesn't give the full answer?"

Checking "everything" makes it look like we have no idea what is going 
on in other code in the same class and seems like something similar to a 
layering violation.

I do not buy the performance argument in this place. If file existence 
is what you want, checking file existence is very cheap. It is not worth 
the complexity of looking at variables that doesn't give exactly the 
same information anyway.

But I will argue that you have to define "ongoing merge that can be 
resolved" as either something that has a local parent or as something 
that has things to resolve and thus has a non-empty state. Checking more 
than that will be unmaintainable code.

/Mads