Patchwork D8196: commit: clear resolved mergestate even if working copy is clean

login
register
mail settings
Submitter phabricator
Date Feb. 28, 2020, 7:58 p.m.
Message ID <differential-rev-PHID-DREV-qxm7fw3cichah722geza-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/45409/
State Superseded
Headers show

Comments

phabricator - Feb. 28, 2020, 7:58 p.m.
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  If the mergestate has resolved conflicts and a commit is successfully
  created (either because there are changes in the working copy or
  because ui.allowemptycommit=yes), we will also clear the merge
  state. However, if the working copy is clean (and
  ui.allowemptycommit=no), we leave the mergestate there. The user may
  notice it in `hg resolve -l` output (but not in `hg status -v`
  output). It's not clear how the user should clear it, but probably via
  `hg co -C .`. It's also quite likely that they won't even notice it
  and it will get cleared by a later `hg commit` (of unrelated
  changes).
  
  This patch makes it so that `hg commit` also clears resolved merge
  conflicts even if the command doesn't end up writing a commit because
  the working copy was empty. That's probably a little weird (commands
  that abort should generally avoid changing the repo), but it still
  seems mostly harmless, and it reduces the risk of more bugs like
  https://bz.mercurial-scm.org/show_bug.cgi?id=5494. I just ran into a
  version of that bug in the Evolve extension and that's what triggered
  this series.

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D8196

AFFECTED FILES
  mercurial/localrepo.py
  relnotes/next
  tests/test-update-branches.t

CHANGE DETAILS




To: martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - Feb. 29, 2020, 3:34 p.m.
pulkit added a comment.


  To me,  having a `hg commit` which aborts,  doing something seems a bit weird. For the use case of rebase, shelve etc., a function argument or internal config can be better.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8196/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8196

To: martinvonz, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - Feb. 29, 2020, 4:15 p.m.
martinvonz added a comment.


  In D8196#121923 <https://phab.mercurial-scm.org/D8196#121923>, @pulkit wrote:
  
  > To me,  having a `hg commit` which aborts,  doing something seems a bit weird.
  
  Yes, I agree, as I said in the commit message.
  
  > For the use case of rebase, shelve etc., a function argument or internal config can be better.
  
  I considered that but I also felt that it's weird that we just leave the state there otherwise until the user creates an unrelated commit or `hg co -C`. If we care about the state, we should be clear that we do (e.g. by registering it as an unfinished state that the user has to clear before they do anything else).
  
  I'm fine with still adding the flag to make `hg commit` behave differently from internal calls. Still want me to do that or want me to register merge state as an unfinished state?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8196/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8196

To: martinvonz, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - March 2, 2020, 12:43 p.m.
pulkit added a comment.


  In D8196#121924 <https://phab.mercurial-scm.org/D8196#121924>, @martinvonz wrote:
  
  > I'm fine with still adding the flag to make `hg commit` behave differently from internal calls. Still want me to do that or want me to register merge state as an unfinished state?
  
  I thought we already register merge state as an unfinished state but now I see why we don't. Adding a flag sounds good to me.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8196/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8196

To: martinvonz, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - March 4, 2020, 7:56 p.m.
marmoute added a comment.
marmoute accepted this revision.


  The case is a bit strange, but the proposed change seems a good way out.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8196/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8196

To: martinvonz, #hg-reviewers, marmoute
Cc: marmoute, pulkit, mercurial-devel
phabricator - March 5, 2020, 8:29 a.m.
pulkit added inline comments.

INLINE COMMENTS

> localrepo.py:2959
>              if not allowemptycommit:
> +                ms.reset()
>                  return None

Lets add a `ui.debug()` here about clearing of mergestate.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8196/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8196

To: martinvonz, #hg-reviewers, marmoute
Cc: marmoute, pulkit, mercurial-devel

Patch

diff --git a/tests/test-update-branches.t b/tests/test-update-branches.t
--- a/tests/test-update-branches.t
+++ b/tests/test-update-branches.t
@@ -357,7 +357,6 @@ 
   nothing changed
   [1]
   $ hg resolve -l
-  R a
 
 Change/delete conflict is not allowed
   $ hg up -qC 3
diff --git a/relnotes/next b/relnotes/next
--- a/relnotes/next
+++ b/relnotes/next
@@ -56,6 +56,9 @@ 
  * `hg debugmergestate` output format changed. Let us know if that is
    causing you problems and we'll roll it back.
 
+ * Resolved merge conflicts are now cleared by `hg commit` even if the
+   working copy has no changes.
+
 
 == Internal API Changes ==
 
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -2956,6 +2956,7 @@ 
                 or self.ui.configbool(b'ui', b'allowemptycommit')
             )
             if not allowemptycommit:
+                ms.reset()
                 return None
 
             if merge and cctx.deleted():