This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7898

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

Authored by martinvonz on Jan 15 2020, 8:21 PM.

Details

Reviewers
marmoute
pulkit
Group Reviewers
hg-reviewers
Commits
rHGff22c76825b9: merge: don't call update hook when using in-memory context
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.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

martinvonz created this revision.Jan 15 2020, 8:21 PM
pulkit added a subscriber: pulkit.Jan 22 2020, 10:31 AM

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

martinvonz added a comment.Jan 22 2020, 11:15 AM
In 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.

marmoute accepted this revision.Jan 23 2020, 12:16 PM

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 ?

martinvonz added a comment.Jan 23 2020, 12:24 PM
In 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.

pulkit accepted this revision.Jan 24 2020, 7:54 AM
This revision is now accepted and ready to land.Jan 24 2020, 7:54 AM

Revision Contents
Changeset List

PathPackages
Mmercurial/merge.py (2 lines)

Diff 19549

mercurial/merge.py

if not branchmerge: if not branchmerge:
repo.dirstate.setbranch(p2.branch()) repo.dirstate.setbranch(p2.branch())
# If we're updating to a location, clean up any stale temporary includes # If we're updating to a location, clean up any stale temporary includes
# (ex: this happens during hg rebase --abort). # (ex: this happens during hg rebase --abort).
if not branchmerge: if not branchmerge:
sparse.prunetemporaryincludes(repo) sparse.prunetemporaryincludes(repo)
if not partial: if updatedirstate:
repo.hook( repo.hook(
b'update', parent1=xp1, parent2=xp2, error=stats.unresolvedcount b'update', parent1=xp1, parent2=xp2, error=stats.unresolvedcount
) )
return stats return stats
def graft( def graft(
repo, ctx, base, labels=None, keepparent=False, keepconflictparent=False repo, ctx, base, labels=None, keepparent=False, keepconflictparent=False