Patchwork D3158: histedit: look up partial nodeid as partial nodeid

login
register
mail settings
Submitter phabricator
Date April 6, 2018, 4:55 p.m.
Message ID <differential-rev-PHID-DREV-4nkc3bcwoxgqlrx4vwoz-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/30441/
State Superseded
Headers show

Comments

phabricator - April 6, 2018, 4:55 p.m.
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[<partial hex nodeid>]. 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




To: martinvonz, durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - April 6, 2018, 6:57 p.m.
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
phabricator - April 6, 2018, 7:01 p.m.
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
phabricator - April 6, 2018, 7:13 p.m.
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
phabricator - April 7, 2018, 7:36 a.m.
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
phabricator - April 7, 2018, 7:45 a.m.
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
phabricator - April 7, 2018, 7:37 p.m.
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
phabricator - April 7, 2018, 9:36 p.m.
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

Patch

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):