Patchwork D7898: merge: don't call update hook when using in-memory context

login
register
mail settings
Submitter phabricator
Date Jan. 16, 2020, 1:21 a.m.
Message ID <differential-rev-PHID-DREV-rflcb4hq6dnxcpbkvv2k-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/44405/
State Superseded
Headers show

Comments

phabricator - Jan. 16, 2020, 1:21 a.m.
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  I'm pretty sure many hook implementors will assume that they can
  inspect the working copy and/or dirstate parents when the hook is
  called, so I don't think we should call the hook when using an
  in-memory context. The new behavior matches that of the preupdate
  hook.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  mercurial/merge.py

CHANGE DETAILS




To: martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - Jan. 22, 2020, 3:31 p.m.
pulkit added a comment.


  Do we have user-facing documentation for these behavior? I tried to find documentation related to hooks but was unable to find any.

REPOSITORY
  rHG Mercurial

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

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

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


  In D7898#117079 <https://phab.mercurial-scm.org/D7898#117079>, @pulkit wrote:
  
  > Do we have user-facing documentation for these behavior? I tried to find documentation related to hooks but was unable to find any.
  
  Yes, `hg help config.hooks`. But even without reading that, it seems pretty obvious to me that we shouldn't run the "update" hook when not updating the working copy.

REPOSITORY
  rHG Mercurial

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

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

To: martinvonz, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - Jan. 23, 2020, 5:16 p.m.
marmoute added a comment.
marmoute accepted this revision.


  I don't see any mention of "in-memory" in the current hook documentation. Can you clarify to which section we are thinking about?
  
  Otherwise, yes… we should not call the `update` hook if we are not touching the working directory. Should this hook call be elsewhere ? (for example, at the place where we update the dirstate parents ?

REPOSITORY
  rHG Mercurial

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

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

To: martinvonz, #hg-reviewers, marmoute
Cc: marmoute, pulkit, mercurial-devel
phabricator - Jan. 23, 2020, 5:24 p.m.
martinvonz added a comment.


  In D7898#117294 <https://phab.mercurial-scm.org/D7898#117294>, @marmoute wrote:
  
  > I don't see any mention of "in-memory" in the current hook documentation. Can you clarify to which section we are thinking about?
  
  Specifically the section about the "update" hook:
  
    $ hg help config.hooks.update
        "hooks.update"
          Run after updating the working directory. The changeset ID of first new
          parent is in "$HG_PARENT1". If updating to a merge, the ID of second new
          parent is in "$HG_PARENT2". If the update succeeded, "$HG_ERROR=0". If
          the update failed (e.g. because conflicts were not resolved),
          "$HG_ERROR=1".
  
  As you can see, the first sentence is "Run after updating the working directory.". Since in-memory rebase doesn't update the working copy, it doesn't make sense to call the hook (just like we don't call the "pre-update" hook).
  
  > Otherwise, yes… we should not call the `update` hook if we are not touching the working directory. Should this hook call be elsewhere ? (for example, at the place where we update the dirstate parents ?
  
  I think it's right after `sparse.prunetemporaryincludes(repo)` on purpose.

REPOSITORY
  rHG Mercurial

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

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

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

Patch

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -2574,7 +2574,7 @@ 
     if not branchmerge:
         sparse.prunetemporaryincludes(repo)
 
-    if not partial:
+    if updatedirstate:
         repo.hook(
             b'update', parent1=xp1, parent2=xp2, error=stats.unresolvedcount
         )