This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8489

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

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

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
mharbison72 added a comment.Apr 30 2020, 1:25 AM

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.

durin42 added subscribers: marmoute, durin42.May 14 2020, 4:27 PM
durin42 accepted this revision as: durin42.

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

PathPackages
Mhgext/rebase.py (28 lines)
Mtests/test-rebase-named-branches.t (18 lines)

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