D3158: histedit: look up partial nodeid as partial nodeid
This revision was automatically updated to reflect the committed changes.
Closed by commit rHGc4131138eadb: histedit: look up partial nodeid as partial
nodeid (authored by martinvonz, committed by ).
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D3158?vs=7879&id=7936
REVISION DETAIL
https://phab.mercurial-scm.org/D3158
AFFECTED FILES
hgext/histedit.py
CHANGE DETAILS
diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -443,11 +443,9 @@
""" Verifies semantic correctness of the rule"""
repo = self.repo
ha = node.hex(self.node)
-try:
-self.node = repo[ha].node()
-except error.RepoError:
-raise error.ParseError(_('unknown changeset %s listed')
- % ha[:12])
+self.node = scmutil.resolvepartialhexnodeid(repo, ha)
+if self.node is None:
+raise error.ParseError(_('unknown changeset %s listed') % ha[:12])
self._verifynodeconstraints(prev, expected, seen)
def _verifynodeconstraints(self, prev, expected, seen):
To: martinvonz, durin42, #hg-reviewers, indygreg
Cc: yuja, indygreg, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D3158: histedit: look up partial nodeid as partial nodeid
martinvonz updated this revision to Diff 7879.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST UPDATE
https://phab.mercurial-scm.org/D3158?vs=7783&id=7879
REVISION DETAIL
https://phab.mercurial-scm.org/D3158
AFFECTED FILES
hgext/histedit.py
CHANGE DETAILS
diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -443,11 +443,9 @@
""" Verifies semantic correctness of the rule"""
repo = self.repo
ha = node.hex(self.node)
-try:
-self.node = repo[ha].node()
-except error.RepoError:
-raise error.ParseError(_('unknown changeset %s listed')
- % ha[:12])
+self.node = scmutil.resolvepartialhexnodeid(repo, ha)
+if self.node is None:
+raise error.ParseError(_('unknown changeset %s listed') % ha[:12])
self._verifynodeconstraints(prev, expected, seen)
def _verifynodeconstraints(self, prev, expected, seen):
To: martinvonz, durin42, #hg-reviewers, indygreg
Cc: yuja, indygreg, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D3158: histedit: look up partial nodeid as partial nodeid
martinvonz added a comment. In https://phab.mercurial-scm.org/D3158#51161, @indygreg wrote: > +1 to having a new function to handle cases for things like partial match. I agree, that seems much better. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3158 To: martinvonz, durin42, #hg-reviewers, indygreg Cc: yuja, indygreg, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D3158: histedit: look up partial nodeid as partial nodeid
indygreg added a comment. +1 to having a new function to handle cases for things like partial match. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3158 To: martinvonz, durin42, #hg-reviewers, indygreg Cc: yuja, indygreg, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D3158: histedit: look up partial nodeid as partial nodeid
yuja added inline comments. INLINE COMMENTS > yuja wrote in histedit.py:446 > Nah. It's for performance reason on ambiguous case, radix tree lookup vs > linear search. I've sent a patch to fix the inconsistency, > but it was rejected because of that. > > Maybe we'll need a scmutil function to document that. We shouldn't > spill around the weird implementation detail. BTW, it's probably wrong to rely only on _partialmatch() of unfiltered repo. In changectx.__init__, we verify the result with `repo.changelog.rev()`. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3158 To: martinvonz, durin42, #hg-reviewers, indygreg Cc: yuja, indygreg, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D3158: histedit: look up partial nodeid as partial nodeid
yuja added inline comments.
INLINE COMMENTS
> martinvonz wrote in histedit.py:446
> > If the previous code worked on on the filtered repo, shouldn't this code?
>
> The previous code just *looked like* it worked on the filtered repo :) This
> is copied from changectx.__init__(), which is where this would end up getting
> resolved before.
>
> I don't remember what the reason is for *that* code to use the unfiltered
> repo (I think it had something to do with making {shortest(node)} length
> match what's actually unambiguous. Either way, this patch should not be
> changing any behavior, I think.
Nah. It's for performance reason on ambiguous case, radix tree lookup vs linear
search. I've sent a patch to fix the inconsistency,
but it was rejected because of that.
Maybe we'll need a scmutil function to document that. We shouldn't
spill around the weird implementation detail.
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D3158
To: martinvonz, durin42, #hg-reviewers, indygreg
Cc: yuja, indygreg, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D3158: histedit: look up partial nodeid as partial nodeid
martinvonz added inline comments.
INLINE COMMENTS
> indygreg wrote in histedit.py:446
> Err wait. Why is `repo.unfiltered()` being used here? If the previous code
> worked on on the filtered repo, shouldn't this code?
>
> The side effect of this change is that a histedit rule could reference a
> hidden changeset. That feels wrong.
> If the previous code worked on on the filtered repo, shouldn't this code?
The previous code just *looked like* it worked on the filtered repo :) This is
copied from changectx.__init__(), which is where this would end up getting
resolved before.
I don't remember what the reason is for *that* code to use the unfiltered repo
(I think it had something to do with making {shortest(node)} length match
what's actually unambiguous. Either way, this patch should not be changing any
behavior, I think.
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D3158
To: martinvonz, durin42, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D3158: histedit: look up partial nodeid as partial nodeid
indygreg requested changes to this revision.
indygreg added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> histedit.py:446
> ha = node.hex(self.node)
> -try:
> -self.node = repo[ha].node()
> -except error.RepoError:
> -raise error.ParseError(_('unknown changeset %s listed')
> - % ha[:12])
> +self.node = repo.unfiltered().changelog._partialmatch(ha)
> +if self.node is None:
Err wait. Why is `repo.unfiltered()` being used here? If the previous code
worked on on the filtered repo, shouldn't this code?
The side effect of this change is that a histedit rule could reference a hidden
changeset. That feels wrong.
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D3158
To: martinvonz, durin42, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D3158: histedit: look up partial nodeid as partial nodeid
indygreg accepted this revision. indygreg added a comment. This revision is now accepted and ready to land. This is a layering violation. But I don't care because we don't yet have an interface for changelog. Once we do, we'll likely promote `_partialmatch` to a public method. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3158 To: martinvonz, durin42, #hg-reviewers, indygreg Cc: indygreg, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D3158: histedit: look up partial nodeid as partial nodeid
martinvonz created this revision.
Herald added a reviewer: durin42.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.
REVISION SUMMARY
I'm about to remove support for repo[]. In the
verify() method, we know that self.node is always a partial or full
binary nodeid, so the most correct way to look up the revision is by
using changelog._partialmatch(), so let's do that. (It's closer to the
current code to do scmutil.revsymbol(), but that's less correct
because it will match a bookmark or tag that happens to have the same
prefix as the node.)
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D3158
AFFECTED FILES
hgext/histedit.py
CHANGE DETAILS
diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -443,11 +443,9 @@
""" Verifies semantic correctness of the rule"""
repo = self.repo
ha = node.hex(self.node)
-try:
-self.node = repo[ha].node()
-except error.RepoError:
-raise error.ParseError(_('unknown changeset %s listed')
- % ha[:12])
+self.node = repo.unfiltered().changelog._partialmatch(ha)
+if self.node is None:
+raise error.ParseError(_('unknown changeset %s listed') % ha[:12])
self._verifynodeconstraints(prev, expected, seen)
def _verifynodeconstraints(self, prev, expected, seen):
To: martinvonz, durin42, #hg-reviewers
Cc: mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
