| Submitter | phabricator |
|---|---|
| Date | July 1, 2018, 9:53 a.m. |
| Message ID | <differential-rev-PHID-DREV-habmbceszf3rkewmqe2m-req@phab.mercurial-scm.org> |
| Download | mbox | patch |
| Permalink | /patch/32543/ |
| State | Superseded |
| Headers | show |
Comments
> def _dryrunrebase(ui, repo, opts): > rbsrt = rebaseruntime(repo, ui, inmemory=True, opts=opts) > - ui.status(_('starting dry-run rebase; repository will not be changed\n')) > + confirm = opts.get('confirm') > + if confirm: > + ui.status(_('starting rebase...\n')) > + else: > + ui.status(_('starting dry-run rebase; repository will not be ' > + 'changed\n')) Nit: Perhaps --dry-run should precede --confirm, or "--dryrun --confirm" should be rejected. > with repo.wlock(), repo.lock(): > try: > overrides = {('rebase', 'singletransaction'): True} > with ui.configoverride(overrides, 'rebase'): > _origrebase(ui, repo, opts, rbsrt, inmemory=True, > leaveunfinished=True, supptrwarns=True) > except error.InMemoryMergeConflictsError: > + conflict = True > ui.status(_('hit a merge conflict\n')) > return 1 > else: > - ui.status(_('dry-run rebase completed successfully; run without ' > - '-n/--dry-run to perform this rebase\n')) > + conflict = False > + if confirm: > + ui.status(_('rebase completed successfully\n')) > + else: > + ui.status(_('dry-run rebase completed successfully; run without' > + ' -n/--dry-run to perform this rebase\n')) > return 0 > finally: > # no need to store backup in case of dryrun > rbsrt._prepareabortorcontinue(isabort=True, backup=False, > suppwarns=True) > + if confirm: > + if not ui.promptchoice(_(b'apply changes (yn)?' > + b'$$ &Yes $$ &No')): > + if not conflict: > + inmemory = ui.configbool('rebase', > + 'experimental.inmemory') > + return _dorebase(ui, repo, opts, inmemory=inmemory) > + return _dorebase(ui, repo, opts) Can't we just "finish" the last in-memory rebase if "apply changes" selected?
yuja added a comment. > def _dryrunrebase(ui, repo, opts): > rbsrt = rebaseruntime(repo, ui, inmemory=True, opts=opts) > > - ui.status(_('starting dry-run rebase; repository will not be changed\n')) + confirm = opts.get('confirm') + if confirm: + ui.status(_('starting rebase...\n')) + else: + ui.status(_('starting dry-run rebase; repository will not be ' + 'changed\n')) Nit: Perhaps --dry-run should precede --confirm, or "--dryrun --confirm" should be rejected. > with repo.wlock(), repo.lock(): > try: > overrides = {('rebase', 'singletransaction'): True} > with ui.configoverride(overrides, 'rebase'): > _origrebase(ui, repo, opts, rbsrt, inmemory=True, > leaveunfinished=True, supptrwarns=True) > except error.InMemoryMergeConflictsError: > > + conflict = True > > ui.status(_('hit a merge conflict\n')) > return 1 > else: > > - ui.status(_('dry-run rebase completed successfully; run without ' > - '-n/--dry-run to perform this rebase\n')) + conflict = False + if confirm: + ui.status(_('rebase completed successfully\n')) + else: + ui.status(_('dry-run rebase completed successfully; run without' + ' -n/--dry-run to perform this rebase\n')) return 0 finally: > 1. no need to store backup in case of dryrun rbsrt._prepareabortorcontinue(isabort=True, backup=False, suppwarns=True) + if confirm: + if not ui.promptchoice(_(b'apply changes (yn)?' + b'$$ &Yes $$ &No')): + if not conflict: + inmemory = ui.configbool('rebase', + 'experimental.inmemory') + return _dorebase(ui, repo, opts, inmemory=inmemory) + return _dorebase(ui, repo, opts) Can't we just "finish" the last in-memory rebase if "apply changes" selected? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3870 To: khanchi97, #hg-reviewers Cc: yuja, mercurial-devel
khanchi97 added a comment. In https://phab.mercurial-scm.org/D3870#60813, @yuja wrote: > > def _dryrunrebase(ui, repo, opts): > > rbsrt = rebaseruntime(repo, ui, inmemory=True, opts=opts) > > > > - ui.status(_('starting dry-run rebase; repository will not be changed\n')) + confirm = opts.get('confirm') + if confirm: + ui.status(_('starting rebase...\n')) + else: + ui.status(_('starting dry-run rebase; repository will not be ' + 'changed\n')) > > Nit: Perhaps --dry-run should precede --confirm, or "--dryrun --confirm" should > be rejected. I think later option is good. > > >> with repo.wlock(), repo.lock(): >> try: >> overrides = {('rebase', 'singletransaction'): True} >> with ui.configoverride(overrides, 'rebase'): >> _origrebase(ui, repo, opts, rbsrt, inmemory=True, >> leaveunfinished=True, supptrwarns=True) >> except error.InMemoryMergeConflictsError: >> >> + conflict = True >> >> ui.status(_('hit a merge conflict\n')) >> return 1 >> else: >> >> - ui.status(_('dry-run rebase completed successfully; run without ' >> - '-n/--dry-run to perform this rebase\n')) + conflict = False + if confirm: + ui.status(_('rebase completed successfully\n')) + else: + ui.status(_('dry-run rebase completed successfully; run without' + ' -n/--dry-run to perform this rebase\n')) return 0 finally: >> 1. no need to store backup in case of dryrun rbsrt._prepareabortorcontinue(isabort=True, backup=False, suppwarns=True) + if confirm: + if not ui.promptchoice(_(b'apply changes (yn)?' + b'$$ &Yes $$ &No')): + if not conflict: + inmemory = ui.configbool('rebase', + 'experimental.inmemory') + return _dorebase(ui, repo, opts, inmemory=inmemory) + return _dorebase(ui, repo, opts) > > Can't we just "finish" the last in-memory rebase if "apply changes" selected? Ah, I think we can :) will update it, thanks! REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3870 To: khanchi97, #hg-reviewers Cc: yuja, mercurial-devel
khanchi97 added a comment. @yuja Rebased, removed variable "needsabort/conflict" and moved that code to "except" and "else" clause accordingly. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3870 To: khanchi97, #hg-reviewers Cc: yuja, mercurial-devel
khanchi97 added a comment. In https://phab.mercurial-scm.org/D3870#61079, @yuja wrote: > Still can't apply. Which revision is this patch based on? Ah, this patch is based on https://phab.mercurial-scm.org/D3830 which is not reviewed. Do I need to rebase this to hg-commited current tip? > > >> with repo.wlock(), repo.lock(): >> try: >> overrides = {('rebase', 'singletransaction'): True} >> with ui.configoverride(overrides, 'rebase'): >> _origrebase(ui, repo, opts, rbsrt, inmemory=True, >> leaveunfinished=True, supptrwarns=True) >> except error.InMemoryMergeConflictsError: >> ui.status(_('hit a merge conflict\n')) >> >> + if confirm: >> + # abort as in-memory merge doesn't support conflict >> + rbsrt._prepareabortorcontinue(isabort=True, backup=False, >> + suppwarns=True) >> + if not ui.promptchoice(_(b'apply changes (yn)?' >> + b'$$ &Yes $$ &No')): >> + _dorebase(ui, repo, opts, inmemory=False) >> >> return 1 >> else: >> >> - ui.status(_('dry-run rebase completed successfully; run without ' >> - '-n/--dry-run to perform this rebase\n')) + if confirm: + ui.status(_('rebase completed successfully\n')) + if not ui.promptchoice(_(b'apply changes (yn)?' + b'$$ &Yes $$ &No')): > > If KeyboardInterrupt is raised here for example, abort() wouldn't be called. okay, can I update this patch or send a follow-up to handle KeyboardInterrupt? > > >> finally: >> >> - # no need to store backup in case of dryrun >> - rbsrt._prepareabortorcontinue(isabort=True, backup=False, >> - suppwarns=True) + if not confirm: + # no need to store backup in case of dryrun + rbsrt._prepareabortorcontinue(isabort=True, backup=False, + suppwarns=True) REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3870 To: khanchi97, #hg-reviewers Cc: yuja, mercurial-devel
> Ah, this patch is based on https://phab.mercurial-scm.org/D3830 which is > not reviewed. Do I need to rebase this to hg-commited current tip? Can you rewrite this to not depend on D3830 if that's easy? It appears nobody reviewed D3830 yet, and I can't trivially say it's good to go. > > If KeyboardInterrupt is raised here for example, abort() wouldn't be called. > > okay, can I update this patch or send a follow-up to handle KeyboardInterrupt? Can you fix that in this patch? KeyboardInterrupt is just an example. The abort in the finally clause can't be gated by the confirm flag.
yuja added a comment. > Ah, this patch is based on https://phab.mercurial-scm.org/D3830 which is > not reviewed. Do I need to rebase this to hg-commited current tip? Can you rewrite this to not depend on https://phab.mercurial-scm.org/D3830 if that's easy? It appears nobody reviewed https://phab.mercurial-scm.org/D3830 yet, and I can't trivially say it's good to go. > > If KeyboardInterrupt is raised here for example, abort() wouldn't be called. > > okay, can I update this patch or send a follow-up to handle KeyboardInterrupt? Can you fix that in this patch? KeyboardInterrupt is just an example. The abort in the finally clause can't be gated by the confirm flag. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3870 To: khanchi97, #hg-reviewers Cc: yuja, mercurial-devel
khanchi97 added inline comments. INLINE COMMENTS > rebase.py:895 > finally: > - # no need to store backup in case of dryrun > - rbsrt._prepareabortorcontinue(isabort=True, backup=False, > - suppwarns=True) > + if needsabort: > + # no need to store backup in case of dryrun @yuja will this work? Now instead of using "confirm" flag, I added "needsabort" variable. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3870 To: khanchi97, #hg-reviewers Cc: yuja, mercurial-devel
khanchi97 added inline comments. INLINE COMMENTS > khanchi97 wrote in rebase.py:895 > @yuja will this work? Now instead of using "confirm" flag, I added "needsabort" variable. And I rebased this revision to not depend on https://phab.mercurial-scm.org/D3830. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3870 To: khanchi97, #hg-reviewers Cc: yuja, mercurial-devel
khanchi97 added a comment. >> except error.InMemoryMergeConflictsError: >> ui.status(_('hit a merge conflict\n')) >> >> + if confirm: >> + # abort as in-memory merge doesn't support conflict >> + rbsrt._prepareabortorcontinue(isabort=True, backup=False, >> + suppwarns=True) >> + needsabort = False >> + if not ui.promptchoice(_(b'apply changes (yn)?' >> + b'$$ &Yes $$ &No')): > > Nit: This isn't actually "apply changes". Hmm, then what it should be? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3870 To: khanchi97, #hg-reviewers Cc: yuja, mercurial-devel
> >> + if confirm: > >> + # abort as in-memory merge doesn't support conflict > >> + rbsrt._prepareabortorcontinue(isabort=True, backup=False, > >> + suppwarns=True) > >> + needsabort = False > >> + if not ui.promptchoice(_(b'apply changes (yn)?' > >> + b'$$ &Yes $$ &No')): > > > > Nit: This isn't actually "apply changes". > > Hmm, then what it should be? I think we'll need to tell that we'll rerun the rebase that's known to have at least one merge conflict. "apply changes" sounds like it would just finalize the rebase up to the previous revision of the merge conflict. Alternatively, it can just abort without the prompt since the dry-run was incomplete.
yuja added a comment. > >> + if confirm: > >> + # abort as in-memory merge doesn't support conflict > >> + rbsrt._prepareabortorcontinue(isabort=True, backup=False, > >> + suppwarns=True) > >> + needsabort = False > >> + if not ui.promptchoice(_(b'apply changes (yn)?' > >> + b'$$ &Yes $$ &No')): > > > > Nit: This isn't actually "apply changes". > > Hmm, then what it should be? I think we'll need to tell that we'll rerun the rebase that's known to have at least one merge conflict. "apply changes" sounds like it would just finalize the rebase up to the previous revision of the merge conflict. Alternatively, it can just abort without the prompt since the dry-run was incomplete. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D3870 To: khanchi97, #hg-reviewers Cc: yuja, mercurial-devel
Patch
diff --git a/tests/test-rebase-inmemory.t b/tests/test-rebase-inmemory.t --- a/tests/test-rebase-inmemory.t +++ b/tests/test-rebase-inmemory.t @@ -4,6 +4,7 @@ > amend= > rebase= > debugdrawdag=$TESTDIR/drawdag.py + > strip= > [rebase] > experimental.inmemory=1 > [diff] @@ -156,8 +157,8 @@ o 0: b173517d0057 'a' Test dry-run rebasing - $ hg init skrepo - $ cd skrepo + $ hg init repo3 + $ cd repo3 $ echo a>a $ hg ci -Aqma $ echo b>b @@ -321,3 +322,237 @@ merging e hit a merge conflict [1] + +========================== +Test for --confirm option| +========================== + $ cd .. + $ hg clone repo3 repo4 -q + $ cd repo4 + $ hg strip 7 -q + $ hg log -G --template "{rev}:{short(node)} {person(author)}\n{firstline(desc)} {topic}\n\n" + @ 6:baf10c5166d4 test + | g + | + o 5:6343ca3eff20 test + | f + | + | o 4:e860deea161a test + | | e + | | + | o 3:055a42cdd887 test + | | d + | | + | o 2:177f92b77385 test + |/ c + | + o 1:d2ae7f538514 test + | b + | + o 0:cb9a9f314b8b test + a + + $ hg rebase -s 2 -d . --keep --config ui.interactive=True --confirm << EOF + > n + > EOF + starting rebase... + rebasing 2:177f92b77385 "c" + rebasing 3:055a42cdd887 "d" + rebasing 4:e860deea161a "e" + rebase completed successfully + apply changes (yn)? n + $ hg log -G --template "{rev}:{short(node)} {person(author)}\n{firstline(desc)} {topic}\n\n" + @ 6:baf10c5166d4 test + | g + | + o 5:6343ca3eff20 test + | f + | + | o 4:e860deea161a test + | | e + | | + | o 3:055a42cdd887 test + | | d + | | + | o 2:177f92b77385 test + |/ c + | + o 1:d2ae7f538514 test + | b + | + o 0:cb9a9f314b8b test + a + + $ hg rebase -s 2 -d . --keep --config ui.interactive=True --confirm << EOF + > y + > EOF + starting rebase... + rebasing 2:177f92b77385 "c" + rebasing 3:055a42cdd887 "d" + rebasing 4:e860deea161a "e" + rebase completed successfully + apply changes (yn)? y + rebasing 2:177f92b77385 "c" + rebasing 3:055a42cdd887 "d" + rebasing 4:e860deea161a "e" + $ hg log -G --template "{rev}:{short(node)} {person(author)}\n{firstline(desc)} {topic}\n\n" + o 9:9fd28f55f6dc test + | e + | + o 8:12cbf031f469 test + | d + | + o 7:c83b1da5b1ae test + | c + | + @ 6:baf10c5166d4 test + | g + | + o 5:6343ca3eff20 test + | f + | + | o 4:e860deea161a test + | | e + | | + | o 3:055a42cdd887 test + | | d + | | + | o 2:177f92b77385 test + |/ c + | + o 1:d2ae7f538514 test + | b + | + o 0:cb9a9f314b8b test + a + +Test --confirm option when there is a conflict + $ hg up tip -q + $ echo ee>e + $ hg ci --amend -m "conflict with e" -q + $ hg log -G --template "{rev}:{short(node)} {person(author)}\n{firstline(desc)} {topic}\n\n" + @ 9:906d72f66a59 test + | conflict with e + | + o 8:12cbf031f469 test + | d + | + o 7:c83b1da5b1ae test + | c + | + o 6:baf10c5166d4 test + | g + | + o 5:6343ca3eff20 test + | f + | + | o 4:e860deea161a test + | | e + | | + | o 3:055a42cdd887 test + | | d + | | + | o 2:177f92b77385 test + |/ c + | + o 1:d2ae7f538514 test + | b + | + o 0:cb9a9f314b8b test + a + + $ hg rebase -s 4 -d . --keep --config ui.interactive=True --confirm << EOF + > n + > EOF + starting rebase... + rebasing 4:e860deea161a "e" + merging e + hit a merge conflict + apply changes (yn)? n + [1] + $ hg log -G --template "{rev}:{short(node)} {person(author)}\n{firstline(desc)} {topic}\n\n" + @ 9:906d72f66a59 test + | conflict with e + | + o 8:12cbf031f469 test + | d + | + o 7:c83b1da5b1ae test + | c + | + o 6:baf10c5166d4 test + | g + | + o 5:6343ca3eff20 test + | f + | + | o 4:e860deea161a test + | | e + | | + | o 3:055a42cdd887 test + | | d + | | + | o 2:177f92b77385 test + |/ c + | + o 1:d2ae7f538514 test + | b + | + o 0:cb9a9f314b8b test + a + + + $ hg rebase -s 4 -d . --keep --config ui.interactive=True --confirm << EOF + > y + > EOF + starting rebase... + rebasing 4:e860deea161a "e" + merging e + hit a merge conflict + apply changes (yn)? y + rebasing 4:e860deea161a "e" + merging e + warning: conflicts while merging e! (edit, then use 'hg resolve --mark') + unresolved conflicts (see hg resolve, then hg rebase --continue) + [1] + + $ echo e>e + $ hg resolve --mark --all + (no more unresolved files) + continue: hg rebase --continue + $ hg rebase --continue + rebasing 4:e860deea161a "e" + $ hg log -G --template "{rev}:{short(node)} {person(author)}\n{firstline(desc)} {topic}\n\n" + o 10:9fa3731dd6df test + | e + | + @ 9:906d72f66a59 test + | conflict with e + | + o 8:12cbf031f469 test + | d + | + o 7:c83b1da5b1ae test + | c + | + o 6:baf10c5166d4 test + | g + | + o 5:6343ca3eff20 test + | f + | + | o 4:e860deea161a test + | | e + | | + | o 3:055a42cdd887 test + | | d + | | + | o 2:177f92b77385 test + |/ c + | + o 1:d2ae7f538514 test + | b + | + o 0:cb9a9f314b8b test + a + diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py --- a/mercurial/cmdutil.py +++ b/mercurial/cmdutil.py @@ -62,6 +62,11 @@ _('do not perform actions, just print output')), ] +confirmopts = [ + ('', 'confirm', None, + _('ask before applying actions')), +] + remoteopts = [ ('e', 'ssh', '', _('specify ssh command to use'), _('CMD')), diff --git a/hgext/rebase.py b/hgext/rebase.py --- a/hgext/rebase.py +++ b/hgext/rebase.py @@ -671,7 +671,7 @@ ('a', 'abort', False, _('abort an interrupted rebase')), ('', 'auto-orphans', '', _('automatically rebase orphan revisions ' 'in the specified revset (EXPERIMENTAL)')), - ] + cmdutil.dryrunopts + cmdutil.formatteropts, + ] + cmdutil.dryrunopts + cmdutil.formatteropts + cmdutil.confirmopts, _('[-s REV | -b REV] [-d REV] [OPTION]')) def rebase(ui, repo, **opts): """move changeset (and descendants) to a different branch @@ -802,6 +802,8 @@ raise error.Abort(_('cannot specify both --dry-run and --abort')) if opts.get('continue'): raise error.Abort(_('cannot specify both --dry-run and --continue')) + if opts.get('confirm'): + dryrun = True if (opts.get('continue') or opts.get('abort') or repo.currenttransaction() is not None): @@ -838,24 +840,42 @@ def _dryrunrebase(ui, repo, opts): rbsrt = rebaseruntime(repo, ui, inmemory=True, opts=opts) - ui.status(_('starting dry-run rebase; repository will not be changed\n')) + confirm = opts.get('confirm') + if confirm: + ui.status(_('starting rebase...\n')) + else: + ui.status(_('starting dry-run rebase; repository will not be ' + 'changed\n')) with repo.wlock(), repo.lock(): try: overrides = {('rebase', 'singletransaction'): True} with ui.configoverride(overrides, 'rebase'): _origrebase(ui, repo, opts, rbsrt, inmemory=True, leaveunfinished=True, supptrwarns=True) except error.InMemoryMergeConflictsError: + conflict = True ui.status(_('hit a merge conflict\n')) return 1 else: - ui.status(_('dry-run rebase completed successfully; run without ' - '-n/--dry-run to perform this rebase\n')) + conflict = False + if confirm: + ui.status(_('rebase completed successfully\n')) + else: + ui.status(_('dry-run rebase completed successfully; run without' + ' -n/--dry-run to perform this rebase\n')) return 0 finally: # no need to store backup in case of dryrun rbsrt._prepareabortorcontinue(isabort=True, backup=False, suppwarns=True) + if confirm: + if not ui.promptchoice(_(b'apply changes (yn)?' + b'$$ &Yes $$ &No')): + if not conflict: + inmemory = ui.configbool('rebase', + 'experimental.inmemory') + return _dorebase(ui, repo, opts, inmemory=inmemory) + return _dorebase(ui, repo, opts) def _dorebase(ui, repo, opts, inmemory=False): rbsrt = rebaseruntime(repo, ui, inmemory, opts)