This is an archive of the discontinued Mercurial Phabricator instance.

histedit: make histedit's commands accept revsets (issue5746)
ClosedPublic

Authored by sangeet259 on Feb 23 2018, 1:19 AM.

Details

Summary

Earlier the code was only looking for rulehashes and neglecting
all other revision identifiers, this code intercepts the fromrule function
and calls scmutil.revsingle() on anything that is not a rulehash and then
obtains the rulehash from the changectx object returned, rest of the pipeline
follows as it was

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

sangeet259 created this revision.Feb 23 2018, 1:19 AM
sangeet259 updated this revision to Diff 6008.Feb 23 2018, 1:43 AM

What I did: @durin42 suggested in the issue page 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.

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

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

Can we get some test coverage on this?

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

sangeet259 updated this revision to Diff 6324.Mar 2 2018, 2:07 AM
sangeet259 edited the summary of this revision. (Show Details)Mar 2 2018, 2:46 AM

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

hgext/histedit.py
433–437

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:

@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

@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?

@durin42 Should I make any changes?

pulkit added a subscriber: pulkit.Mar 10 2018, 4:02 AM

@durin42 Should I make any changes?

Yep, you should try fixing the failures you mentioned.

@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!

durin42 accepted this revision.Mar 24 2018, 3:02 PM

queued, many thanks

This revision is now accepted and ready to land.Mar 24 2018, 3:02 PM
This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.Mar 25 2018, 1:13 AM
yuja added inline comments.
hgext/histedit.py
439

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

440

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.

441

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

Can you send a followup?