Page MenuHomePhabricator

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

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

Details

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.Wed, Jan 22, 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.

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.Thu, Jan 23, 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 ?

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.Fri, Jan 24, 7:54 AM
This revision is now accepted and ready to land.Fri, Jan 24, 7:54 AM