Re: D3827: rebase: no need to store backup during dry-run while aborting

[Yuya Nishihara]

>   > > - 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.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: D3827: rebase: no need to store backup during dry-run while aborting

[sushil khanchi]

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

On Mon, Jun 25, 2018 at 6:18 PM, khanchi97 (Sushil khanchi) <
phabrica...@mercurial-scm.org> wrote:

> khanchi97 added a comment.
>
>
>   In https://phab.mercurial-scm.org/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?
>
> REPOSITORY
>   rHG Mercurial
>
> REVISION DETAIL
>   https://phab.mercurial-scm.org/D3827
>
> To: khanchi97, #hg-reviewers
> Cc: yuja, mercurial-devel
> ___
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: D3827: rebase: no need to store backup during dry-run while aborting

[Yuya Nishihara]

> -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.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: D3827: rebase: no need to store backup during dry-run while aborting

[Yuya Nishihara]

> -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 @@
>  
>  # 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`.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel