This is an archive of the discontinued Mercurial Phabricator instance.

rebase: avoid clobbering wdir() with --dry-run or --confirm (issue6291)
ClosedPublic

Authored by mharbison72 on Apr 30 2020, 1:20 AM.

Download Raw Diff

Details

Reviewers

martinvonz
durin42

Group Reviewers

hg-reviewers

Commits

rHG1f114c797961: rebase: avoid clobbering wdir() with --dry-run or --confirm (issue6291)

Summary

See 56d3e0b499df for the source of adding originalwd to the list of things that
cause wdir to be updated. That change didn't come with tests, and attempts to
recreate the scenario described have thus far failed.

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

mharbison72 created this revision.Apr 30 2020, 1:20 AM

Herald added a reviewer: martinvonz. · View Herald TranscriptApr 30 2020, 1:20 AM

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

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

I'm not real familiar with how rebase works, and seems like it's pretty intricate code. I'd also feel better if there were tests for the original commit referenced. But it's very unlikely that I'll have any time before the weekend to look at this again, so I thought I'd send it out now. Maybe it's narrow enough or maybe somebody can backfill the tests, and we can avoid a known data shredder in the next release.

Seems fine to me, but I want one of @martinvonz or @marmoute to look at this.

martinvonz added inline comments.May 16 2020, 2:04 AM

tests/test-rebase-named-branches.t
342–344	It seems to me that it also shouldn't affect a dirty working directory that is related to the source or destination. Right?

mharbison72 added inline comments.May 16 2020, 12:08 PM

tests/test-rebase-named-branches.t
342–344	56d3e0b499df seems to imply "yes", unless I'm either misunderstanding or your recent cleanups around rebases in progress changed that.

mharbison72 added a commit: rHG1f114c797961: rebase: avoid clobbering wdir() with --dry-run or --confirm (issue6291).Jun 5 2020, 11:13 PM

This revision was not accepted when it landed; it landed in state Needs Review.

Closed by commit rHG1f114c797961: rebase: avoid clobbering wdir() with --dry-run or --confirm (issue6291) (authored by mharbison72). · Explain Why

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			hgext/rebase.py (28 lines)
M			tests/test-rebase-named-branches.t (18 lines)

	Status	Author	Revision
	Closed	mharbison72	D8489 rebase: avoid clobbering wdir() with --dry-run or --confirm (issue6291)
	Closed	mharbison72	D8488 tests: show that rebase --dry-run and --confirm wipeout uncommitted changes

Diff 21569

hgext/rebase.py

	self.obsoletewithoutsuccessorindestination,			self.obsoletewithoutsuccessorindestination,
	obsoleteextinctsuccessors,			obsoleteextinctsuccessors,
	) = _computeobsoletenotrebased(self.repo, obsoleteset, destmap)			) = _computeobsoletenotrebased(self.repo, obsoleteset, destmap)
	skippedset = set(self.obsoletenotrebased)			skippedset = set(self.obsoletenotrebased)
	skippedset.update(self.obsoletewithoutsuccessorindestination)			skippedset.update(self.obsoletewithoutsuccessorindestination)
	skippedset.update(obsoleteextinctsuccessors)			skippedset.update(obsoleteextinctsuccessors)
	_checkobsrebase(self.repo, self.ui, obsoleteset, skippedset)			_checkobsrebase(self.repo, self.ui, obsoleteset, skippedset)

	def _prepareabortorcontinue(self, isabort, backup=True, suppwarns=False):			def _prepareabortorcontinue(
				self, isabort, backup=True, suppwarns=False, dryrun=False, confirm=False
				):
	self.resume = True			self.resume = True
	try:			try:
	self.restorestatus()			self.restorestatus()
	self.collapsemsg = restorecollapsemsg(self.repo, isabort)			self.collapsemsg = restorecollapsemsg(self.repo, isabort)
	except error.RepoLookupError:			except error.RepoLookupError:
	if isabort:			if isabort:
	clearstatus(self.repo)			clearstatus(self.repo)
	clearcollapsemsg(self.repo)			clearcollapsemsg(self.repo)
	self.repo.ui.warn(			self.repo.ui.warn(
	_(			_(
	b'rebase aborted (no revision is removed,'			b'rebase aborted (no revision is removed,'
	b' only broken state is cleared)\n'			b' only broken state is cleared)\n'
	)			)
	)			)
	return 0			return 0
	else:			else:
	msg = _(b'cannot continue inconsistent rebase')			msg = _(b'cannot continue inconsistent rebase')
	hint = _(b'use "hg rebase --abort" to clear broken state')			hint = _(b'use "hg rebase --abort" to clear broken state')
	raise error.Abort(msg, hint=hint)			raise error.Abort(msg, hint=hint)

	if isabort:			if isabort:
	backup = backup and self.backupf			backup = backup and self.backupf
	return self._abort(backup=backup, suppwarns=suppwarns)			return self._abort(
				backup=backup,
				suppwarns=suppwarns,
				dryrun=dryrun,
				confirm=confirm,
				)

	def _preparenewrebase(self, destmap):			def _preparenewrebase(self, destmap):
	if not destmap:			if not destmap:
	return _nothingtorebase()			return _nothingtorebase()

	rebaseset = destmap.keys()			rebaseset = destmap.keys()
	if not self.keepf:			if not self.keepf:
	try:			try:

	if (			if (
	self.activebookmark			self.activebookmark
	and self.activebookmark in repo._bookmarks			and self.activebookmark in repo._bookmarks
	and repo[b'.'].node() == repo._bookmarks[self.activebookmark]			and repo[b'.'].node() == repo._bookmarks[self.activebookmark]
	):			):
	bookmarks.activate(repo, self.activebookmark)			bookmarks.activate(repo, self.activebookmark)

	def _abort(self, backup=True, suppwarns=False):			def _abort(self, backup=True, suppwarns=False, dryrun=False, confirm=False):
	'''Restore the repository to its original state.'''			'''Restore the repository to its original state.'''

	repo = self.repo			repo = self.repo
	try:			try:
	# If the first commits in the rebased set get skipped during the			# If the first commits in the rebased set get skipped during the
	# rebase, their values within the state mapping will be the dest			# rebase, their values within the state mapping will be the dest
	# rev id. The rebased list must must not contain the dest rev			# rev id. The rebased list must must not contain the dest rev
	# (issue4896)			# (issue4896)
	if cleanup:			if cleanup:
	if rebased:			if rebased:
	strippoints = [			strippoints = [
	c.node() for c in repo.set(b'roots(%ld)', rebased)			c.node() for c in repo.set(b'roots(%ld)', rebased)
	]			]

	updateifonnodes = set(rebased)			updateifonnodes = set(rebased)
	updateifonnodes.update(self.destmap.values())			updateifonnodes.update(self.destmap.values())

				if not dryrun and not confirm:
	updateifonnodes.add(self.originalwd)			updateifonnodes.add(self.originalwd)

	shouldupdate = repo[b'.'].rev() in updateifonnodes			shouldupdate = repo[b'.'].rev() in updateifonnodes

	# Update away from the rebase if necessary			# Update away from the rebase if necessary
	if shouldupdate:			if shouldupdate:
	mergemod.clean_update(repo[self.originalwd])			mergemod.clean_update(repo[self.originalwd])

	# Strip from the first rebased revision			# Strip from the first rebased revision
	if rebased:			if rebased:
	else:			else:
	if confirm:			if confirm:
	ui.status(_(b'rebase completed successfully\n'))			ui.status(_(b'rebase completed successfully\n'))
	if not ui.promptchoice(_(b'apply changes (yn)?$$ &Yes $$ &No')):			if not ui.promptchoice(_(b'apply changes (yn)?$$ &Yes $$ &No')):
	# finish unfinished rebase			# finish unfinished rebase
	rbsrt._finishrebase()			rbsrt._finishrebase()
	else:			else:
	rbsrt._prepareabortorcontinue(			rbsrt._prepareabortorcontinue(
	isabort=True, backup=False, suppwarns=True			isabort=True,
				backup=False,
				suppwarns=True,
				confirm=confirm,
	)			)
	needsabort = False			needsabort = False
	else:			else:
	ui.status(			ui.status(
	_(			_(
	b'dry-run rebase completed successfully; run without'			b'dry-run rebase completed successfully; run without'
	b' -n/--dry-run to perform this rebase\n'			b' -n/--dry-run to perform this rebase\n'
	)			)
	)			)
	return 0			return 0
	finally:			finally:
	if needsabort:			if needsabort:
	# no need to store backup in case of dryrun			# no need to store backup in case of dryrun
	rbsrt._prepareabortorcontinue(			rbsrt._prepareabortorcontinue(
	isabort=True, backup=False, suppwarns=True			isabort=True,
				backup=False,
				suppwarns=True,
				dryrun=opts.get(b'dry_run'),
	)			)


	def _dorebase(ui, repo, action, opts, inmemory=False):			def _dorebase(ui, repo, action, opts, inmemory=False):
	rbsrt = rebaseruntime(repo, ui, inmemory, opts)			rbsrt = rebaseruntime(repo, ui, inmemory, opts)
	return _origrebase(ui, repo, action, opts, rbsrt, inmemory=inmemory)			return _origrebase(ui, repo, action, opts, rbsrt, inmemory=inmemory)

tests/test-rebase-named-branches.t

	$ hg diff			$ hg diff
	diff -r 2b586e70108d A			diff -r 2b586e70108d A
	--- a/A Thu Jan 01 00:00:00 1970 +0000			--- a/A Thu Jan 01 00:00:00 1970 +0000
	+++ b/A Thu Jan 01 00:00:00 1970 +0000			+++ b/A Thu Jan 01 00:00:00 1970 +0000
	@@ -1,1 +1,1 @@			@@ -1,1 +1,1 @@
	-A			-A
	+A-mod			+A-mod

	BUG: This shouldn't affect wdir			--dry-run doesn't affect a dirty working directory that is unrelated to the
				source or destination.

				martinvonzUnsubmitted Not Done It seems to me that it also shouldn't affect a dirty working directory that is related to the source or destination. Right? martinvonz: It seems to me that it also shouldn't affect a dirty working directory that is related to the…
				mharbison72AuthorUnsubmitted Done 56d3e0b499df seems to imply "yes", unless I'm either misunderstanding or your recent cleanups around rebases in progress changed that. mharbison72: 56d3e0b499df seems to imply "yes", unless I'm either misunderstanding or your recent cleanups…
	$ hg rebase -s tip -d 4 --dry-run			$ hg rebase -s tip -d 4 --dry-run
	starting dry-run rebase; repository will not be changed			starting dry-run rebase; repository will not be changed
	rebasing 11:be1dea60f2a6 "D" (tip)			rebasing 11:be1dea60f2a6 "D" (tip)
	dry-run rebase completed successfully; run without -n/--dry-run to perform this rebase			dry-run rebase completed successfully; run without -n/--dry-run to perform this rebase
	$ hg diff			$ hg diff
				diff -r 2b586e70108d A
				--- a/A Thu Jan 01 00:00:00 1970 +0000
				+++ b/A Thu Jan 01 00:00:00 1970 +0000
				@@ -1,1 +1,1 @@
				-A
				+A-mod

	BUG: This shouldn't affect wdir			Bailing out on --confirm doesn't affect a dirty working directory that is
				unrelated to the source or destination.

	$ echo A-mod > A			$ echo A-mod > A
	$ echo n \| hg rebase -s tip -d 4 --confirm --config ui.interactive=True			$ echo n \| hg rebase -s tip -d 4 --confirm --config ui.interactive=True
	starting in-memory rebase			starting in-memory rebase
	rebasing 11:be1dea60f2a6 "D" (tip)			rebasing 11:be1dea60f2a6 "D" (tip)
	rebase completed successfully			rebase completed successfully
	apply changes (yn)? n			apply changes (yn)? n
	$ hg diff			$ hg diff
				diff -r 2b586e70108d A
				--- a/A Thu Jan 01 00:00:00 1970 +0000
				+++ b/A Thu Jan 01 00:00:00 1970 +0000
				@@ -1,1 +1,1 @@
				-A
				+A-mod

	$ echo A-mod > A			$ echo A-mod > A
	$ hg rebase -s tip -d 4 --confirm			$ hg rebase -s tip -d 4 --confirm
	starting in-memory rebase			starting in-memory rebase
	rebasing 11:be1dea60f2a6 "D" (tip)			rebasing 11:be1dea60f2a6 "D" (tip)
	rebase completed successfully			rebase completed successfully
	apply changes (yn)? y			apply changes (yn)? y
	saved backup bundle to $TESTTMP/a1/.hg/strip-backup/be1dea60f2a6-ca6d2dac-rebase.hg			saved backup bundle to $TESTTMP/a1/.hg/strip-backup/be1dea60f2a6-ca6d2dac-rebase.hg

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	21237		Apr 30 2020, 1:20 AM	★	★
Diff 2	21569	rHG1f114c797961733ea03d0de88390f9555a7edef4	Apr 30 2020, 12:33 AM	★	★