This is an archive of the discontinued Mercurial Phabricator instance.

Differential D2394

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

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

Download Raw Diff

Details

Reviewers

durin42

Group Reviewers

hg-reviewers

Commits

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

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

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

sangeet259 created this revision.Feb 23 2018, 1:19 AM

Herald added a reviewer: durin42. · View Herald TranscriptFeb 23 2018, 1:19 AM

Herald added a reviewer: hg-reviewers. · View Herald Transcript

Herald added a subscriber: mercurial-devel. · View Herald Transcript

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!

tom.prince added a subscriber: tom.prince.Mar 1 2018, 6:11 PM

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

added the tests @krbullock

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

hgext/histedit.py
431–440	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

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

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

@durin42 Should I make any changes?

In D2394#44294, @sangeet259 wrote:

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

queued, many thanks

This revision is now accepted and ready to land.Mar 24 2018, 3:02 PM

Closed by commit rHG3d3cff1f6bde: histedit: make histedit's commands accept revsets (issue5746) (authored by sangeet259). · Explain WhyMar 24 2018, 5:21 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
437	This could be `rev = scmutil.revsingle(...).node()`.
438	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.
439	`_("invalid changeset %s")` for translation. Can you send a followup?

Revision Contents
Changeset List

			Path	Packages
M			hgext/histedit.py (13 lines)
M			tests/test-histedit-arguments.t (40 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	6007		Feb 23 2018, 1:19 AM	★	★
Diff 2	6008		Feb 23 2018, 1:43 AM	★	★
Diff 3	6324		Mar 2 2018, 2:07 AM	★	★
Diff 4	7272	rHG3d3cff1f6bdea90a620bd1bff87b459809bbcfb2	Feb 23 2018, 1:18 AM	★	★

Diff 7272

hgext/histedit.py

	self.state = state			self.state = state
	self.repo = state.repo			self.repo = state.repo
	self.node = node			self.node = node

	@classmethod			@classmethod
	def fromrule(cls, state, rule):			def fromrule(cls, state, rule):
	"""Parses the given rule, returning an instance of the histeditaction.			"""Parses the given rule, returning an instance of the histeditaction.
	"""			"""
	rulehash = rule.strip().split(' ', 1)[0]			ruleid = rule.strip().split(' ', 1)[0]
				# ruleid can be anything from rev numbers, hashes, "bookmarks" etc
				# Check for validation of rule ids and get the rulehash
	try:			try:
	rev = node.bin(rulehash)			rev = node.bin(ruleid)
	except TypeError:			except TypeError:
	raise error.ParseError("invalid changeset %s" % rulehash)			try:
				_ctx = scmutil.revsingle(state.repo, ruleid)
				rulehash = _ctx.hex()
				rev = node.bin(rulehash)
				yujaUnsubmitted Not Done This could be `rev = scmutil.revsingle(...).node()`. yuja: This could be `rev = scmutil.revsingle(...).node()`.
				except error.RepoLookupError:
				yujaUnsubmitted Not Done 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. yuja: Note that `error.LookupError` may be raised if `ruleid` is odd-length and if it is an ambiguous…
				raise error.ParseError("invalid changeset %s" % ruleid)
				yujaUnsubmitted Not Done `_("invalid changeset %s")` for translation. Can you send a followup? yuja: `_("invalid changeset %s")` for translation. Can you send a followup?
	return cls(state, rev)			return cls(state, rev)
				durin42Unsubmitted Not Done 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: I think you can replace this outer try with a revsingle call, something like this: try…

	def verify(self, prev, expected, seen):			def verify(self, prev, expected, seen):
	""" Verifies semantic correctness of the rule"""			""" Verifies semantic correctness of the rule"""
	repo = self.repo			repo = self.repo
	ha = node.hex(self.node)			ha = node.hex(self.node)
	try:			try:
	self.node = repo[ha].node()			self.node = repo[ha].node()
	except error.RepoError:			except error.RepoError:

tests/test-histedit-arguments.t

	hg: parse error: duplicated command for changeset eb57da33312f			hg: parse error: duplicated command for changeset eb57da33312f
	[255]			[255]

	Test bogus rev			Test bogus rev
	---------------------------------------			---------------------------------------

	$ HGEDITOR=cat hg histedit "tip^^" --commands - << EOF			$ HGEDITOR=cat hg histedit "tip^^" --commands - << EOF
	> pick eb57da33312f 2 three			> pick eb57da33312f 2 three
	> pick 0			> pick 0u98
	> pick 08d98a8350f3 4 five			> pick 08d98a8350f3 4 five
	> EOF			> EOF
	hg: parse error: invalid changeset 0			hg: parse error: invalid changeset 0u98
	[255]			[255]

	Test short version of command			Test short version of command
	---------------------------------------			---------------------------------------

	Note: we use varying amounts of white space between command name and changeset			Note: we use varying amounts of white space between command name and changeset
	short hash. This tests issue3893.			short hash. This tests issue3893.

	# p, pick = use commit			# p, pick = use commit
	# b, base = checkout changeset and apply further changesets from there			# b, base = checkout changeset and apply further changesets from there
	# d, drop = remove commit from history			# d, drop = remove commit from history
	# f, fold = use commit, but combine it with the one above			# f, fold = use commit, but combine it with the one above
	# r, roll = like fold, but discard this commit's description and date			# r, roll = like fold, but discard this commit's description and date
	#			#

	$ cd ..			$ cd ..

				Check that histedit's commands accept revsets
				$ hg init bar
				$ cd bar
				$ echo w >> a
				$ hg ci -qAm "adds a"
				$ echo x >> b
				$ hg ci -qAm "adds b"
				$ echo y >> c
				$ hg ci -qAm "adds c"
				$ echo z >> d
				$ hg ci -qAm "adds d"
				$ hg log -G -T '{rev} {desc}\n'
				@ 3 adds d
				\|
				o 2 adds c
				\|
				o 1 adds b
				\|
				o 0 adds a

				$ HGEDITOR=cat hg histedit "2" --commands - << EOF
				> base -4 adds c
				> pick 2 adds c
				> pick tip adds d
				> EOF
				$ hg log -G -T '{rev} {desc}\n'
				@ 5 adds d
				\|
				o 4 adds c
				\|
				\| o 1 adds b
				\|/
				o 0 adds a