Patchwork [4,of,6] resolve: abort when not applicable

login
register
mail settings
Submitter Gregory Szorc
Date May 9, 2014, 1:46 a.m.
Message ID <a5e623de6557ea8854ce.1399599982@vm-ubuntu-main.gateway.sonic.net>
Download mbox | patch
Permalink /patch/4692/
State Accepted
Headers show

Comments

Gregory Szorc - May 9, 2014, 1:46 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1397873312 25200
#      Fri Apr 18 19:08:32 2014 -0700
# Node ID a5e623de6557ea8854cefc108d5b069adb0f737a
# Parent  6d5b402aae2d697b93b19ccf8f4d0245c4fc29d3
resolve: abort when not applicable

The resolve command is only relevant when mergestate is present.
This patch will make resolve abort when no mergestate is present.

This change will let people know when they are using resolve when they
shouldn't be. This change will let people know when their use of resolve
doesn't do anything.

Previously, |hg resolve -m| would allow mergestate to be created. This
patch now forbids that. Strictly speaking, this is backwards
incompatible. The author of this patch believes creating mergestate via
resolve doesn't make much sense and this side-effect was unintended.
Pierre-Yves David - May 9, 2014, 2:15 a.m.
On 05/08/2014 06:46 PM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1397873312 25200
> #      Fri Apr 18 19:08:32 2014 -0700
> # Node ID a5e623de6557ea8854cefc108d5b069adb0f737a
> # Parent  6d5b402aae2d697b93b19ccf8f4d0245c4fc29d3
> resolve: abort when not applicable
>
> The resolve command is only relevant when mergestate is present.
> This patch will make resolve abort when no mergestate is present.
>
> This change will let people know when they are using resolve when they
> shouldn't be. This change will let people know when their use of resolve
> doesn't do anything.
>
> Previously, |hg resolve -m| would allow mergestate to be created. This
> patch now forbids that. Strictly speaking, this is backwards
> incompatible. The author of this patch believes creating mergestate via
> resolve doesn't make much sense and this side-effect was unintended.

This change make sense. I added a (BC) flag in the title to flag the 
backward compatibility breackage. I'm not super fan of the message. but 
I'll let native speaker bikeshed it.

I've queued this series to the clowncopter repo. I expect message fixup 
in follow up patches.
Mads Kiilerich - May 9, 2014, 9:54 a.m.
On 05/09/2014 03:46 AM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1397873312 25200
> #      Fri Apr 18 19:08:32 2014 -0700
> # Node ID a5e623de6557ea8854cefc108d5b069adb0f737a
> # Parent  6d5b402aae2d697b93b19ccf8f4d0245c4fc29d3
> resolve: abort when not applicable
>
> The resolve command is only relevant when mergestate is present.
> This patch will make resolve abort when no mergestate is present.
>
> This change will let people know when they are using resolve when they
> shouldn't be. This change will let people know when their use of resolve
> doesn't do anything.
>
> Previously, |hg resolve -m| would allow mergestate to be created. This
> patch now forbids that. Strictly speaking, this is backwards
> incompatible. The author of this patch believes creating mergestate via
> resolve doesn't make much sense and this side-effect was unintended.
>
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -4926,8 +4926,12 @@ def resolve(ui, repo, *pats, **opts):
>           raise util.Abort(_('no files or directories specified; '
>                              'use --all to remerge all files'))
>   
>       ms = mergemod.mergestate(repo)
> +
> +    if not ms.active():
> +        raise util.Abort(_('resolve command not applicable when not merging'))
> +
>       m = scmutil.match(repo[None], pats, opts)
>       ret = 0
>   
>       for f in ms:
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- 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.

/Mads
Gregory Szorc - May 9, 2014, 3:28 p.m.
On 5/9/2014 2:54 AM, Mads Kiilerich wrote:
> On 05/09/2014 03:46 AM, Gregory Szorc wrote:
>> # HG changeset patch
>> # User Gregory Szorc <gregory.szorc@gmail.com>
>> # Date 1397873312 25200
>> #      Fri Apr 18 19:08:32 2014 -0700
>> # Node ID a5e623de6557ea8854cefc108d5b069adb0f737a
>> # Parent  6d5b402aae2d697b93b19ccf8f4d0245c4fc29d3
>> resolve: abort when not applicable
>>
>> The resolve command is only relevant when mergestate is present.
>> This patch will make resolve abort when no mergestate is present.
>>
>> This change will let people know when they are using resolve when they
>> shouldn't be. This change will let people know when their use of resolve
>> doesn't do anything.
>>
>> Previously, |hg resolve -m| would allow mergestate to be created. This
>> patch now forbids that. Strictly speaking, this is backwards
>> incompatible. The author of this patch believes creating mergestate via
>> resolve doesn't make much sense and this side-effect was unintended.
>>
>> diff --git a/mercurial/commands.py b/mercurial/commands.py
>> --- a/mercurial/commands.py
>> +++ b/mercurial/commands.py
>> @@ -4926,8 +4926,12 @@ def resolve(ui, repo, *pats, **opts):
>>           raise util.Abort(_('no files or directories specified; '
>>                              'use --all to remerge all files'))
>>         ms = mergemod.mergestate(repo)
>> +
>> +    if not ms.active():
>> +        raise util.Abort(_('resolve command not applicable when not
>> merging'))
>> +
>>       m = scmutil.match(repo[None], pats, opts)
>>       ret = 0
>>         for f in ms:
>> diff --git a/mercurial/merge.py b/mercurial/merge.py
>> --- 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.

I don't know! I concede my knowledge of everything merge code is
somewhat shallow.

There is a single test that touches a mergestate file and puts no
content in it (simulating a process crash or some such). Not changing
behavior of that test is why I put the opener.exists() bits in there.
I'm not sure if that was the proper course.
Pierre-Yves David - May 9, 2014, 5:23 p.m.
On 05/09/2014 02:54 AM, Mads Kiilerich wrote:
> On 05/09/2014 03:46 AM, Gregory Szorc wrote:
>> # HG changeset patch
>> # User Gregory Szorc <gregory.szorc@gmail.com>
>> # Date 1397873312 25200
>> #      Fri Apr 18 19:08:32 2014 -0700
>> # Node ID a5e623de6557ea8854cefc108d5b069adb0f737a
>> # Parent  6d5b402aae2d697b93b19ccf8f4d0245c4fc29d3
>> resolve: abort when not applicable
>>
>> The resolve command is only relevant when mergestate is present.
>> This patch will make resolve abort when no mergestate is present.
>>
>> This change will let people know when they are using resolve when they
>> shouldn't be. This change will let people know when their use of resolve
>> doesn't do anything.
>>
>> Previously, |hg resolve -m| would allow mergestate to be created. This
>> patch now forbids that. Strictly speaking, this is backwards
>> incompatible. The author of this patch believes creating mergestate via
>> resolve doesn't make much sense and this side-effect was unintended.
>>
>> diff --git a/mercurial/commands.py b/mercurial/commands.py
>> --- a/mercurial/commands.py
>> +++ b/mercurial/commands.py
>> @@ -4926,8 +4926,12 @@ def resolve(ui, repo, *pats, **opts):
>>           raise util.Abort(_('no files or directories specified; '
>>                              'use --all to remerge all files'))
>>       ms = mergemod.mergestate(repo)
>> +
>> +    if not ms.active():
>> +        raise util.Abort(_('resolve command not applicable when not
>> merging'))
>> +
>>       m = scmutil.match(repo[None], pats, opts)
>>       ret = 0
>>       for f in ms:
>> diff --git a/mercurial/merge.py b/mercurial/merge.py
>> --- 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 comit
Pierre-Yves David - May 15, 2014, 7:31 a.m.
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.
Gregory Szorc - May 16, 2014, 9:07 p.m.
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.
Mads Kiilerich - May 16, 2014, 9:47 p.m.
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

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -4926,8 +4926,12 @@  def resolve(ui, repo, *pats, **opts):
         raise util.Abort(_('no files or directories specified; '
                            'use --all to remerge all files'))
 
     ms = mergemod.mergestate(repo)
+
+    if not ms.active():
+        raise util.Abort(_('resolve command not applicable when not merging'))
+
     m = scmutil.match(repo[None], pats, opts)
     ret = 0
 
     for f in ms:
diff --git a/mercurial/merge.py b/mercurial/merge.py
--- 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)
+
     def commit(self):
         """Write current state on disk (if necessary)"""
         if self._dirty:
             records = []
diff --git a/tests/test-histedit-non-commute-abort.t b/tests/test-histedit-non-commute-abort.t
--- a/tests/test-histedit-non-commute-abort.t
+++ b/tests/test-histedit-non-commute-abort.t
@@ -84,8 +84,10 @@  abort the edit
   2 files updated, 0 files merged, 0 files removed, 0 files unresolved
 
 log after abort
   $ hg resolve -l
+  abort: resolve command not applicable when not merging
+  [255]
   $ hg log --graph
   @  changeset:   6:bfa474341cc9
   |  tag:         tip
   |  user:        test
diff --git a/tests/test-resolve.t b/tests/test-resolve.t
--- a/tests/test-resolve.t
+++ b/tests/test-resolve.t
@@ -36,11 +36,13 @@  resolve the failure
   $ echo resolved > file
   $ hg resolve -m file
   $ hg commit -m 'resolved'
 
-resolve -l, should be empty
+resolve -l should error since no merge in progress
 
   $ hg resolve -l
+  abort: resolve command not applicable when not merging
+  [255]
 
 test crashed merge with empty mergestate
 
   $ mkdir .hg/merge
diff --git a/tests/test-shelve.t b/tests/test-shelve.t
--- a/tests/test-shelve.t
+++ b/tests/test-shelve.t
@@ -261,8 +261,10 @@  abort the unshelve and be happy
   date:        Thu Jan 01 00:00:00 1970 +0000
   summary:     second
   
   $ hg resolve -l
+  abort: resolve command not applicable when not merging
+  [255]
   $ hg status
   A foo/foo
   ? a/a.orig