Patchwork D2394: histedit: make histedit's commands accept revsets (issue5746)

login
register
mail settings
Submitter phabricator
Date Feb. 23, 2018, 6:20 a.m.
Message ID <differential-rev-PHID-DREV-objdgumot62j55fxy5qj-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/28265/
State Superseded
Headers show

Comments

phabricator - Feb. 23, 2018, 6:20 a.m.
sangeet259 created this revision.
Herald added a reviewer: durin42.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2394

AFFECTED FILES
  hgext/histedit.py

CHANGE DETAILS




To: sangeet259, durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - Feb. 23, 2018, 7:01 a.m.
sangeet259 added a comment.


  **What I did**: @durin42 suggested in the issue page <https://bz.mercurial-scm.org/show_bug.cgi?id=5746> that the error was in the `verify` function of the `histedit.py` file. But when I ran through debugger using a revision number, the code was throwing the error before it reaches the `verify` function. Specifically in the `fromrules` function of the `histedit.py` file.
  It was earlier only accepting rulehashes, I changed that to accept any revset using the `scmutil.revsingle` function suggested by @timeless on the IRC channel.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2394

To: sangeet259, durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - Feb. 23, 2018, 7:04 a.m.
sangeet259 added a comment.


  Oh, I forgot adding tests as @durin42 told. Doing that right away!

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2394

To: sangeet259, durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - Feb. 23, 2018, 7:06 a.m.
rishabhmadan96 added a comment.


  It'd be better if you could update the commit message with the necessary details that you mentioned in your previous comment.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2394

To: sangeet259, durin42, #hg-reviewers
Cc: rishabhmadan96, mercurial-devel
phabricator - Feb. 23, 2018, 5:44 p.m.
krbullock added a comment.


  Can we get some test coverage on this?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2394

To: sangeet259, durin42, #hg-reviewers
Cc: krbullock, rishabhmadan96, mercurial-devel
phabricator - Feb. 23, 2018, 5:44 p.m.
krbullock added a comment.


  Oh sorry, I see you're already on the case. Carry on!

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2394

To: sangeet259, durin42, #hg-reviewers
Cc: krbullock, rishabhmadan96, mercurial-devel
phabricator - March 3, 2018, 4:19 p.m.
sangeet259 added a comment.


  added the tests @krbullock

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2394

To: sangeet259, durin42, #hg-reviewers
Cc: tom.prince, krbullock, rishabhmadan96, mercurial-devel
phabricator - March 4, 2018, 3:50 p.m.
durin42 added a comment.


  Nice! I've got one suggested edit to the code that should still pass tests.

INLINE COMMENTS

> histedit.py:428-432
>          try:
> -            rev = node.bin(rulehash)
> +            rev = node.bin(ruleid)
>          except TypeError:
> -            raise error.ParseError("invalid changeset %s" % rulehash)
> +            try:
> +                _ctx = scmutil.revsingle(state.repo, ruleid)

I think you can replace this outer try with a revsingle call, something like this:

try:

  ruleid = rule.strip().split(' ', 1)[0]
  _ctx = scmutil.revsingle(state.repo, ruleid)
  rulehash = _ctx.hex()
  rev = node.bin(rulehash)

except error.RepoLookupError:

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2394

To: sangeet259, durin42, #hg-reviewers
Cc: tom.prince, krbullock, rishabhmadan96, mercurial-devel
phabricator - March 7, 2018, 3:33 p.m.
sangeet259 added a comment.


  @durin42  Even I thought of this construct before as it appears more elegant. But it is failing some tests!
  Specifically, the error messages are changed in this one.
  
    Failed test-histedit-arguments.t: output changed
    Failed test-histedit-edit.t: output changed
    # Ran 14 tests, 0 skipped, 2 failed.
    python hash seed: 3573457086

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2394

To: sangeet259, durin42, #hg-reviewers
Cc: tom.prince, krbullock, rishabhmadan96, mercurial-devel
phabricator - March 7, 2018, 3:45 p.m.
durin42 added a comment.


  In https://phab.mercurial-scm.org/D2394#43612, @sangeet259 wrote:
  
  > @durin42  Even I thought of this construct before as it appears more elegant. But it is failing some tests!
  >  Specifically, the error messages are changed in this one.
  >
  >   Failed test-histedit-arguments.t: output changed
  >   Failed test-histedit-edit.t: output changed
  >   # Ran 14 tests, 0 skipped, 2 failed.
  >   python hash seed: 3573457086
  >  
  >
  
  
  Huh. Can you pastebin those failures so I can see what the changes were like?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2394

To: sangeet259, durin42, #hg-reviewers
Cc: tom.prince, krbullock, rishabhmadan96, mercurial-devel
phabricator - March 7, 2018, 3:51 p.m.
sangeet259 added a comment.


  @durin42  Here is the output! https://bpaste.net/show/7ccf4a9fea50

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2394

To: sangeet259, durin42, #hg-reviewers
Cc: tom.prince, krbullock, rishabhmadan96, mercurial-devel
phabricator - March 9, 2018, 5:43 p.m.
sangeet259 added a comment.


  @durin42  Should I make any changes?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2394

To: sangeet259, durin42, #hg-reviewers
Cc: tom.prince, krbullock, rishabhmadan96, mercurial-devel
phabricator - March 10, 2018, 9:02 a.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D2394#44294, @sangeet259 wrote:
  
  > @durin42  Should I make any changes?
  
  
  Yep, you should try fixing the failures you mentioned.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2394

To: sangeet259, durin42, #hg-reviewers
Cc: pulkit, tom.prince, krbullock, rishabhmadan96, mercurial-devel
phabricator - March 10, 2018, 11:40 a.m.
sangeet259 added a comment.


  @pulkit  It is not failing as of now. It's failing on the changes proposed by @durin42, which he assumed wont fail, but that's not the case!

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2394

To: sangeet259, durin42, #hg-reviewers
Cc: pulkit, tom.prince, krbullock, rishabhmadan96, mercurial-devel
phabricator - March 24, 2018, 7:02 p.m.
durin42 accepted this revision.
durin42 added a comment.
This revision is now accepted and ready to land.


  queued, many thanks

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2394

To: sangeet259, durin42, #hg-reviewers
Cc: pulkit, tom.prince, krbullock, rishabhmadan96, mercurial-devel
phabricator - March 25, 2018, 5:14 a.m.
yuja added inline comments.

INLINE COMMENTS

> histedit.py:437
> +                rulehash = _ctx.hex()
> +                rev = node.bin(rulehash)
> +            except error.RepoLookupError:

This could be `rev = scmutil.revsingle(...).node()`.

> histedit.py:438
> +                rev = node.bin(rulehash)
> +            except error.RepoLookupError:
> +                raise error.ParseError("invalid changeset %s" % ruleid)

Note that `error.LookupError` may be raised if `ruleid` is odd-length and if it is an ambiguous hash.

I think the error message would be still correct, though.

> histedit.py:439
> +            except error.RepoLookupError:
> +                raise error.ParseError("invalid changeset %s" % ruleid)
>          return cls(state, rev)

`_("invalid changeset %s")` for translation.

Can you send a followup?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2394

To: sangeet259, durin42, #hg-reviewers
Cc: yuja, pulkit, tom.prince, krbullock, rishabhmadan96, mercurial-devel

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -422,7 +422,14 @@ 
     def fromrule(cls, state, rule):
         """Parses the given rule, returning an instance of the histeditaction.
         """
-        rulehash = rule.strip().split(' ', 1)[0]
+        ruleid = rule.strip().split(' ', 1)[0]
+        # The ruleid can be anything like revison no,rulehashes, "tip","@" etc
+        # Check for validation of rule ids and get the rulehash 
+        try:
+            _ctx = scmutil.revsingle(state.repo,ruleid)
+            rulehash = _ctx.hex()[:12]
+        except RepoLookupError :
+            raise error.ParseError("invalid changeset %s" % ruleid)
         try:
             rev = node.bin(rulehash)
         except TypeError: