Patchwork [3,of,5,resolve-ux] resolve: abort when not applicable

login
register
mail settings
Submitter Gregory Szorc
Date May 3, 2014, 9:27 p.m.
Message ID <df175a8704f7ab51eed3.1399152459@vm-ubuntu-main.gateway.sonic.net>
Download mbox | patch
Permalink /patch/4588/
State Accepted
Headers show

Comments

Gregory Szorc - May 3, 2014, 9:27 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1397873312 25200
#      Fri Apr 18 19:08:32 2014 -0700
# Branch stable
# Node ID df175a8704f7ab51eed349d5e166b40a1b414227
# Parent  812bb7bf59e8ff0e2e60c1d73857206c6efd8b32
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.
Mads Kiilerich - May 4, 2014, 8:15 p.m.
On 05/03/2014 11:27 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
> # Branch stable
> # Node ID df175a8704f7ab51eed349d5e166b40a1b414227
> # Parent  812bb7bf59e8ff0e2e60c1d73857206c6efd8b32
> 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.

It seems like the change could be perfectly justified with a phrasing like:

Previously, resolve called without an ongoing merge would create an 
invalid, empty and useless mergestate. Now, it will fail cleanly with: 
abort: ..."

> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -4922,16 +4922,21 @@ def resolve(ui, repo, *pats, **opts):
>           raise util.Abort(_("too many options specified"))
>       if pats and all:
>           raise util.Abort(_("can't specify --all and patterns"))
>       if not (all or pats or show or mark or unmark):
>           raise util.Abort(_('no files or directories specified; '
>                              'use --all to remerge all files'))
>   
>       ms = mergemod.mergestate(repo)

(Hmm. More context than usual. Feels odd ... but do no serious harm ;-) )

> +
> +    if not ms.active():
> +        raise util.Abort(_('no merge in progress; '
> +                           'resolve command not applicable'))

I think we usually tell the important things first and let the 
explanation come later (even though it not really is a hint we have 
here). Alternatives could perhaps be "resolve command not applicable, no 
merge in progress" or "resolve command not applicable when not merging".

> +
>       m = scmutil.match(repo[None], pats, opts)
>       ret = 0
>   
>       for f in ms:
>           if not m(f):
>               continue
>   
>           if show:
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -46,26 +46,39 @@ class mergestate(object):
>       F: a file to be merged entry
>       '''
>       statepathv1 = "merge/state"
>       statepathv2 = "merge/state2"
>   
>       def __init__(self, repo):
>           self._repo = repo
>           self._dirty = False
> +        self._local = None
> +        self._other = None

This seems to be an unrelated change. And while init did initialize 
_dirty, it relied and still relies on _read for initializing _state. 
That seems inconsistent. The inconsistency makes it hard to handle 
_local and _other "correctly" which fixing that first ;-)

But shouldn't _local and _other be initialized in _read in all cases? 
Whether they also should be "declared" is primarily a matter of taste 
... but it should be consistent.

>           self._read()
>   
>       def reset(self, node=None, other=None):
>           self._state = {}
>           if node:
>               self._local = node
>               self._other = other
>           shutil.rmtree(self._repo.join("merge"), True)
>           self._dirty = False

(Not directly related, but apparent when reviewing this code: The 
conditional initialization of _local and _other seems odd. I guess this 
function should be split up in a create() and a discard() function...)

>   
> +    def active(self):
> +        """Whether mergestate is active.
> +
> +        Returns True if there appears to be mergestate.
> +        """
> +        # Check local variables before looking at filesystem for performance
> +        # reasons.
> +        return bool(self._local) or bool(self._other) or bool(self._state) or \
> +               self._repo.opener.exists(self.statepathv1) or \
> +               self._repo.opener.exists(self.statepathv2)

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.

(Minor thought: I would place this function a bit later in the code, not 
between init and the _read it is calling. The function is not _that_ 
important.)

> +
>       def _read(self):
>           """Analyse each record content to restore a serialized state from disk
>   
>           This function process "record" entry produced by the de-serialization
>           of on disk file.
>           """
>           self._state = {}
>           records = self._readrecords()

/Mads

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -4922,16 +4922,21 @@  def resolve(ui, repo, *pats, **opts):
         raise util.Abort(_("too many options specified"))
     if pats and all:
         raise util.Abort(_("can't specify --all and patterns"))
     if not (all or pats or show or mark or unmark):
         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(_('no merge in progress; '
+                           'resolve command not applicable'))
+
     m = scmutil.match(repo[None], pats, opts)
     ret = 0
 
     for f in ms:
         if not m(f):
             continue
 
         if show:
diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -46,26 +46,39 @@  class mergestate(object):
     F: a file to be merged entry
     '''
     statepathv1 = "merge/state"
     statepathv2 = "merge/state2"
 
     def __init__(self, repo):
         self._repo = repo
         self._dirty = False
+        self._local = None
+        self._other = None
         self._read()
 
     def reset(self, node=None, other=None):
         self._state = {}
         if node:
             self._local = node
             self._other = other
         shutil.rmtree(self._repo.join("merge"), True)
         self._dirty = False
 
+    def active(self):
+        """Whether mergestate is active.
+
+        Returns True if there appears to be mergestate.
+        """
+        # Check local variables before looking at filesystem for performance
+        # reasons.
+        return bool(self._local) or bool(self._other) or bool(self._state) or \
+               self._repo.opener.exists(self.statepathv1) or \
+               self._repo.opener.exists(self.statepathv2)
+
     def _read(self):
         """Analyse each record content to restore a serialized state from disk
 
         This function process "record" entry produced by the de-serialization
         of on disk file.
         """
         self._state = {}
         records = self._readrecords()
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
@@ -80,16 +80,18 @@  edit the history
 
 
 abort the edit
   $ hg histedit --abort 2>&1 | fixbundle
   2 files updated, 0 files merged, 0 files removed, 0 files unresolved
 
 log after abort
   $ hg resolve -l
+  abort: no merge in progress; resolve command not applicable
+  [255]
   $ hg log --graph
   @  changeset:   6:bfa474341cc9
   |  tag:         tip
   |  user:        test
   |  date:        Thu Jan 01 00:00:00 1970 +0000
   |  summary:     does not commute with e
   |
   o  changeset:   5:652413bf663e
diff --git a/tests/test-resolve.t b/tests/test-resolve.t
--- a/tests/test-resolve.t
+++ b/tests/test-resolve.t
@@ -32,19 +32,21 @@  resolve -l should contain an unresolved 
   U file
 
 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: no merge in progress; resolve command not applicable
+  [255]
 
 test crashed merge with empty mergestate
 
   $ mkdir .hg/merge
   $ touch .hg/merge/state
 
 resolve -l, should be empty
 
diff --git a/tests/test-shelve.t b/tests/test-shelve.t
--- a/tests/test-shelve.t
+++ b/tests/test-shelve.t
@@ -257,16 +257,18 @@  abort the unshelve and be happy
   $ hg parents
   changeset:   3:2e69b451d1ea
   tag:         tip
   user:        test
   date:        Thu Jan 01 00:00:00 1970 +0000
   summary:     second
   
   $ hg resolve -l
+  abort: no merge in progress; resolve command not applicable
+  [255]
   $ hg status
   A foo/foo
   ? a/a.orig
 
 try to continue with no unshelve underway
 
   $ hg unshelve -c
   abort: no unshelve operation underway