This is an archive of the discontinued Mercurial Phabricator instance.

rebase: no need to store backup in case of dryrun
ClosedPublic

Authored by khanchi97 on Jun 22 2018, 9:21 AM.

Details

Summary

While aborting an unfinished rebase in case of dryrun
there is no need to store backup for those rebased csets.

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

khanchi97 created this revision.Jun 22 2018, 9:21 AM
khanchi97 updated this revision to Diff 9260.Jun 22 2018, 1:10 PM
khanchi97 updated this revision to Diff 9262.Jun 23 2018, 8:48 AM
yuja added a subscriber: yuja.Jun 23 2018, 11:12 PM
  • def _prepareabortorcontinue(self, isabort):

+ def _prepareabortorcontinue(self, isabort, opts={}):

Mutable default value should be avoided. It can be backup=True since we
aren't processing command-level thingy here.

And please make sure tests pass before sending out patches.

@@ -828,7 +829,8 @@

else:
    ui.status(_('there will be no conflict, you can rebase\n'))
finally:
  • _origrebase(ui, repo, abort=True)

+ opts = {'abort':True, 'no_backup':True}
+ _origrebase(ui, repo, **opts)

Maybe we can refactor the dryrun handling in a way we don't have to pass around
the no_backup flag. IIUC, _origrebase() is a high-level function which has
many parameter/state checks, but what we want is to cancel the in-memory
session we've made just before. No user error should occur.

@@ -1588,7 +1590,10 @@

    1. Strip from the first rebased revision if rebased:
  • repair.strip(repo.ui, repo, strippoints)

+ if nobackup:
+ repair.strip(repo.ui, repo, strippoints, backup=False)
+ else:
+ repair.strip(repo.ui, repo, strippoints)

This can be written as backup=not nobackup or backup=backup.

khanchi97 updated this revision to Diff 9265.Jun 24 2018, 5:30 AM
yuja added a comment.Jun 25 2018, 8:14 AM
  • retcode = rbsrt._prepareabortorcontinue(abortf)

+ # If in-memory, means aborting during dry-run, no need to backup
+ backup = not rbsrt.inmemory
+ retcode = rbsrt._prepareabortorcontinue(abortf, backup=backup)

This seems confusing and is probably wrong since we wouldn't overwrite
inmemory to False if in-memory rebase were resumable.

I think explicit backup flag is less bad.

In D3827#59873, @yuja wrote:
  • retcode = rbsrt._prepareabortorcontinue(abortf)

+ # If in-memory, means aborting during dry-run, no need to backup
+ backup = not rbsrt.inmemory
+ retcode = rbsrt._prepareabortorcontinue(abortf, backup=backup)

This seems confusing and is probably wrong since we wouldn't overwrite
inmemory to False if in-memory rebase were resumable.
I think explicit backup flag is less bad.

@yuja Can I pass a indicator type flag to _origrebase() just to indicate that we are in dryrun and then I can use that flag to set values for backup and suppwarns (suppress warning when aborting rebase). I think it would be better than explicitly passing flags for each case. What do you say?

If we pass that indicator flag for dryrun then we don't have pass that
leaveunfinished flag too.

yuja added a comment.Jun 25 2018, 10:18 AM
> > - retcode = rbsrt._prepareabortorcontinue(abortf) +            # If in-memory, means aborting during dry-run, no need to backup +            backup = not rbsrt.inmemory +            retcode = rbsrt._prepareabortorcontinue(abortf, backup=backup)
>
> This seems confusing and is probably wrong since we wouldn't overwrite
>  `inmemory` to `False` if in-memory rebase were resumable.
>
> I think explicit `backup` flag is less bad.
@yuja Can I pass a indicator type flag to _origrebase() just to indicate that we are in dryrun and then I can use that flag to set values for `backup` and `suppwarns` (suppress warning when aborting rebase). I think it would be better than explicitly passing flags for each case. What do you say?

Sounds good.

A cleaner (but not simple) approach would be to stop calling _origrebase()
twice, and instead refactor _origrebase() and/or rebaseruntime to support
dry-run operation.

In D3827#59878, @yuja wrote:
> > - retcode = rbsrt._prepareabortorcontinue(abortf) +            # If in-memory, means aborting during dry-run, no need to backup +            backup = not rbsrt.inmemory +            retcode = rbsrt._prepareabortorcontinue(abortf, backup=backup)
>
> This seems confusing and is probably wrong since we wouldn't overwrite
>  `inmemory` to `False` if in-memory rebase were resumable.
>
> I think explicit `backup` flag is less bad.
@yuja Can I pass a indicator type flag to _origrebase() just to indicate that we are in dryrun and then I can use that flag to set values for `backup` and `suppwarns` (suppress warning when aborting rebase). I think it would be better than explicitly passing flags for each case. What do you say?

Sounds good.
A cleaner (but not simple) approach would be to stop calling _origrebase()
twice, and instead refactor _origrebase() and/or rebaseruntime to support
dry-run operation.

Sounds interesting. Let me try this.
Thanks for review :)

khanchi97 edited the summary of this revision. (Show Details)Jun 28 2018, 4:10 PM
khanchi97 retitled this revision from rebase: no need to store backup during dry-run while aborting to rebase: no need to store backup in case of dryrun.
khanchi97 updated this revision to Diff 9345.
khanchi97 updated this revision to Diff 9357.Jun 29 2018, 2:09 PM
This revision was automatically updated to reflect the committed changes.