This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8948

rewriteutil: extract evolve code used to replace obsolete hashes in commits
ClosedPublic

Authored by mharbison72 on Aug 24 2020, 7:08 PM.

Details

Reviewers
marmoute
pulkit
Group Reviewers
hg-reviewers
Commits
rHG0a57ef4b3bdb: rewriteutil: extract evolve code used to replace obsolete hashes in commits
rHG2b89313a2263: rewriteutil: extract evolve code used to replace obsolete hashes in commits
Summary

The evolve command uses it, but there are core things like phabsend and
rebase that would also benefit.

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

mharbison72 created this revision.Aug 24 2020, 7:08 PM
marmoute requested changes to this revision.Aug 26 2020, 3:05 PM
marmoute added a subscriber: marmoute.

There are already a mercurial/rewriteutils.py files I think it should go there

This revision now requires changes to proceed.Aug 26 2020, 3:05 PM
mharbison72 planned changes to this revision.Aug 26 2020, 3:30 PM
In D8948#134093, @marmoute wrote:

There are already a mercurial/rewriteutils.py files I think it should go there

Indeed. I saw obsutil.py, but not this. I'll move it there, but what's the difference between these handful of utils (scmutil.py, obsutil.py, etc) and the stuff in the mercurial.utils package?

marmoute added a comment.Aug 26 2020, 3:34 PM

A mix of timing regarding policy changes and semantic. Module in utils are mostly a split of the old utils module. These modules are a generic utility module with function that might be useful in about any kind of other Mercurial code.

They are some other "utility" module that focus on quite specific part of Mercurial and should/will not be used from outside that part of Mercurial. For example the one contains inside the mercurial/revlogutils/ directory.

mharbison72 added a comment.Aug 26 2020, 7:34 PM
In D8948#134093, @marmoute wrote:

There are already a mercurial/rewriteutils.py files I think it should go there

So that causes a cycle:

--- c:/Users/Matt/hg/tests/test-check-module-imports.t
+++ c:/Users/Matt/hg/tests/test-check-module-imports.t.err
@@ -38,3 +38,5 @@
   > -X tests/test-imports-checker.t \
   > -X tests/test-verify-repo-operations.py \
   > | sed 's-\\-/-g' | "$PYTHON" "$import_checker" -
+  Import cycle: mercurial.cmdutil -> mercurial.rewriteutil -> mercurial.localrepo -> mercurial.extensions -> mercurial.cmdutil\r (esc)
+  [1]

That is really only used for type checking, so is that safe to move under if pycompat.TYPE_CHECKING: with the typing related imports, or is there another way to do this? Or should I just drop the type checking?

marmoute added a comment.Aug 27 2020, 2:48 AM
In D8948#134126, @mharbison72 wrote:
In D8948#134093, @marmoute wrote:

There are already a mercurial/rewriteutils.py files I think it should go there

So that causes a cycle:

--- c:/Users/Matt/hg/tests/test-check-module-imports.t
+++ c:/Users/Matt/hg/tests/test-check-module-imports.t.err
@@ -38,3 +38,5 @@
   > -X tests/test-imports-checker.t \
   > -X tests/test-verify-repo-operations.py \
   > | sed 's-\\-/-g' | "$PYTHON" "$import_checker" -
+  Import cycle: mercurial.cmdutil -> mercurial.rewriteutil -> mercurial.localrepo -> mercurial.extensions -> mercurial.cmdutil\r (esc)
+  [1]

That is really only used for type checking, so is that safe to move under if pycompat.TYPE_CHECKING: with the typing related imports, or is there another way to do this? Or should I just drop the type checking?

I am a bit confused about your question. What is "That" ?

mharbison72 added a comment.Aug 27 2020, 8:24 AM
In D8948#134127, @marmoute wrote:
In D8948#134126, @mharbison72 wrote:
In D8948#134093, @marmoute wrote:

There are already a mercurial/rewriteutils.py files I think it should go there

So that causes a cycle:

--- c:/Users/Matt/hg/tests/test-check-module-imports.t
+++ c:/Users/Matt/hg/tests/test-check-module-imports.t.err
@@ -38,3 +38,5 @@
   > -X tests/test-imports-checker.t \
   > -X tests/test-verify-repo-operations.py \
   > | sed 's-\\-/-g' | "$PYTHON" "$import_checker" -
+  Import cycle: mercurial.cmdutil -> mercurial.rewriteutil -> mercurial.localrepo -> mercurial.extensions -> mercurial.cmdutil\r (esc)
+  [1]

That is really only used for type checking, so is that safe to move under if pycompat.TYPE_CHECKING: with the typing related imports, or is there another way to do this? Or should I just drop the type checking?

I am a bit confused about your question. What is "That" ?

The mercurial.localrepo import in a subsequent patch. That import is only used for the # type: ... comment on the function.

pulkit added a subscriber: pulkit.Sep 2 2020, 12:11 PM
In D8948#134200, @mharbison72 wrote:
In D8948#134127, @marmoute wrote:
In D8948#134126, @mharbison72 wrote:
In D8948#134093, @marmoute wrote:

There are already a mercurial/rewriteutils.py files I think it should go there

So that causes a cycle:

--- c:/Users/Matt/hg/tests/test-check-module-imports.t
+++ c:/Users/Matt/hg/tests/test-check-module-imports.t.err
@@ -38,3 +38,5 @@
   > -X tests/test-imports-checker.t \
   > -X tests/test-verify-repo-operations.py \
   > | sed 's-\\-/-g' | "$PYTHON" "$import_checker" -
+  Import cycle: mercurial.cmdutil -> mercurial.rewriteutil -> mercurial.localrepo -> mercurial.extensions -> mercurial.cmdutil\r (esc)
+  [1]

That is really only used for type checking, so is that safe to move under if pycompat.TYPE_CHECKING: with the typing related imports, or is there another way to do this? Or should I just drop the type checking?

I am a bit confused about your question. What is "That" ?

The mercurial.localrepo import in a subsequent patch. That import is only used for the # type: ... comment on the function.

I believe in future we might want to call some utilities function of this new file from cmdutil.py which will create a cycle. I think we should drop the #type .. part for now as adding imports for this is going to be quite a problem since repository object is used at lot of places.

mharbison72 updated this revision to Diff 22540.Sep 4 2020, 2:19 PM
pulkit accepted this revision.Sep 8 2020, 3:28 AM
mharbison72 added a commit: rHG2b89313a2263: rewriteutil: extract evolve code used to replace obsolete hashes in commits.Sep 8 2020, 8:07 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG2b89313a2263: rewriteutil: extract evolve code used to replace obsolete hashes in commits (authored by mharbison72). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 22575

mercurial/rewriteutil.py

# rewriteutil.py - utility functions for rewriting changesets # rewriteutil.py - utility functions for rewriting changesets
# #
# Copyright 2017 Octobus <contact@octobus.net> # Copyright 2017 Octobus <contact@octobus.net>
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
from __future__ import absolute_import from __future__ import absolute_import
import re
from .i18n import _ from .i18n import _
from . import ( from . import (
error, error,
node, node,
obsolete, obsolete,
obsutil,
revset, revset,
scmutil,
) )
sha1re = re.compile(br'\b[0-9a-f]{6,40}\b')
def precheck(repo, revs, action=b'rewrite'): def precheck(repo, revs, action=b'rewrite'):
"""check if revs can be rewritten """check if revs can be rewritten
action is used to control the error message. action is used to control the error message.
Make sure this function is called after taking the lock. Make sure this function is called after taking the lock.
""" """
if node.nullrev in revs: if node.nullrev in revs:
msg = _(b"cannot %s null changeset") % action msg = _(b"cannot %s null changeset") % action
raise error.ConfigError( raise error.ConfigError(
_( _(
b"%s doesn't know how to handle config " b"%s doesn't know how to handle config "
b"rewrite.empty-successor=%s (only 'skip' and 'keep' are " b"rewrite.empty-successor=%s (only 'skip' and 'keep' are "
b"supported)" b"supported)"
) )
% (command, empty_successor) % (command, empty_successor)
) )
def update_hash_refs(repo, commitmsg):
"""Replace all obsolete commit hashes in the message with the current hash.
If the obsolete commit was split or is divergent, the hash is not replaced
as there's no way to know which successor to choose.
"""
cache = {}
sha1s = re.findall(sha1re, commitmsg)
unfi = repo.unfiltered()
for sha1 in sha1s:
fullnode = scmutil.resolvehexnodeidprefix(unfi, sha1)
if fullnode is None:
continue
ctx = unfi[fullnode]
if not ctx.obsolete():
continue
successors = obsutil.successorssets(repo, ctx.node(), cache=cache)
# We can't make any assumptions about how to update the hash if the
# cset in question was split or diverged.
if len(successors) == 1 and len(successors[0]) == 1:
newsha1 = node.hex(successors[0][0])
commitmsg = commitmsg.replace(sha1, newsha1[: len(sha1)])
else:
repo.ui.note(
_(
b'The stale commit message reference to %s could '
b'not be updated\n'
)
% sha1
)
return commitmsg