[PATCH] rebase: factor out nothing to rebase return code

[Ryan McElroy]

On 10/14/2015 7:22 AM, Augie Fackler wrote:
> On Tue, Oct 13, 2015 at 03:30:48PM -0700, Ryan McElroy wrote:
>> # HG changeset patch
>> # User Ryan McElroy <rmcelroy at fb.com>
>> # Date 1444731605 25200
>> #      Tue Oct 13 03:20:05 2015 -0700
>> # Node ID 73fac7cb3ee4c56df7256aba325fe004af2a15a5
>> # Parent  79d86ab65c9def3fdd65ec972bc5fa89688a19ff
>> rebase: factor out nothing to rebase return code
>>
>> A rebase call that results in nothing to rebase might be considered successful
>> in some contexts. This factors out the return code from places where hg
>> determines that there is nothing to rebase, so an extenion might change this
>> return code to be something that would allow scripts to run without seeing this
>> as an error.
> My gut is that we should categorize these cases and start returning
> different values anyway (eg 1 == "you asked me to rebase the null
> set", 2 == "you asked me to rebase these revisions onto their current
> parent", etc). See also a comment below.

I agree that this is a good long-term strategy, and beyond just rebase. 
We can generally specify different types of exits:

* everything went swimingly, nothing to worry about at all (returns 0 today)
* we didn't do anything, but it's probably fine (returns 1 today, to 
many people's chagrin)
* we didn't do anything, and maybe there's a problem (returns 1 today; 
indistinguishable from above; people disagree as to the severity)
* we didn't do anything because you give us a bad command (returns 255 
today)
* we did some stuff but now user intervention is required (returns 1 
today; oh noes -- but at least we have the UserInterventionRequired 
exception so this is easy to change)
...

However, I also feel that it's outside the scope of an incremental 
improvement to address all of these cases. We certainly have regressed 
no behavior here and I'd argue the code is cleaner and easier to 
maintain now.

I would like to eventually pursue the long-term plan here, but it would 
still be really nice to get this in as-is before the cut so our 
tweakdefaults extension that give people writing scripts the ability to 
run `hg pull && hg rebase -d master && hg push` for example without 
failing unexpectedly just because nothing was pulled.

>> diff --git a/hgext/rebase.py b/hgext/rebase.py
>> --- a/hgext/rebase.py
>> +++ b/hgext/rebase.py
>> @@ -36,6 +36,9 @@ command = cmdutil.command(cmdtable)
>>   # leave the attribute unspecified.
>>   testedwith = 'internal'
>>
>> +def _nothingtorebase():
>> +    return 1
>> +
>>   def _savegraft(ctx, extra):
>>       s = ctx.extra().get('source', None)
>>       if s is not None:
>> @@ -282,13 +285,13 @@ def rebase(ui, repo, **opts):
>>                   if not rebaseset:
>>                       ui.status(_('empty "rev" revision set - '
>>                                   'nothing to rebase\n'))
>> -                    return 1
>> +                    return _nothingtorebase()
>>               elif srcf:
>>                   src = scmutil.revrange(repo, [srcf])
>>                   if not src:
>>                       ui.status(_('empty "source" revision set - '
>>                                   'nothing to rebase\n'))
>> -                    return 1
>> +                    return _nothingtorebase()
>>                   rebaseset = repo.revs('(%ld)::', src)
>>                   assert rebaseset
>>               else:
>> @@ -296,7 +299,7 @@ def rebase(ui, repo, **opts):
>>                   if not base:
>>                       ui.status(_('empty "base" revision set - '
>>                                   "can't compute rebase set\n"))
>> -                    return 1
>> +                    return _nothingtorebase()
> This one and the one above it seem slightly different - the user
> managed to specify the null set, which seems more like an error than
> the other cases?

Agree, this one is more sketchy, but we still return the same result 
after this patch, the status message is unchanged, etc. I'd prefer not 
to block this refactor simply because there is more future work to do as 
well. Also, the function we use here is still not misnamed -- 
"nothingtorebase" is still very much the case here.

>
>>                   commonanc = repo.revs('ancestor(%ld, %d)', base, dest).first()
>>                   if commonanc is not None:
>>                       rebaseset = repo.revs('(%d::(%ld) - %d)::',
>> @@ -329,7 +332,7 @@ def rebase(ui, repo, **opts):
>>                       else: # can it happen?
>>                           ui.status(_('nothing to rebase from %s to %s\n') %
>>                                     ('+'.join(str(repo[r]) for r in base), dest))
>> -                    return 1
>> +                    return _nothingtorebase()
>>
>>               allowunstable = obsolete.isenabled(repo, obsolete.allowunstableopt)
>>               if (not (keepf or allowunstable)
>> @@ -357,7 +360,7 @@ def rebase(ui, repo, **opts):
>>               if not result:
>>                   # Empty state built, nothing to rebase
>>                   ui.status(_('nothing to rebase\n'))
>> -                return 1
>> +                return _nothingtorebase()
>>
>>               root = min(rebaseset)
>>               if not keepf and not repo[root].mutable():
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel at selenic.com
>> https://selenic.com/mailman/listinfo/mercurial-devel