Patchwork D1044: bisect: add --abort flag

login
register
mail settings
Submitter phabricator
Date Oct. 13, 2017, 12:12 a.m.
Message ID <differential-rev-PHID-DREV-wjvruqni6zjjcyz2ovix-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/24807/
State Superseded
Headers show

Comments

phabricator - Oct. 13, 2017, 12:12 a.m.
quark created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Other commands that support a state usually have an `--abort` flag that
  restores the repo state to before the state. Let's add a similar one for
  bisect. It will be used in the next patch.
  
  .. feature:: bisect now supports an --abort flag
  
    `hg bisect --abort` will update the working copy back to the place where
    the bisect started, in addition to what `hg bisect --reset` does.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1044

AFFECTED FILES
  mercurial/commands.py
  mercurial/hbisect.py

CHANGE DETAILS




To: quark, #hg-reviewers
Cc: mercurial-devel
phabricator - Oct. 13, 2017, 8:28 a.m.
dlax added inline comments.

INLINE COMMENTS

> commands.py:809
> +        hbisect.resetstate(repo)
> +        ui.warn(_('bisect aborted\n'))
> +        return

This "bisect aborted" message is not very useful (especially at "warning" level): either the command fails with a message or it succeeds silently. Also maybe it'd be more useful to display the new working directory parent.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1044

To: quark, #hg-reviewers
Cc: dlax, mercurial-devel
phabricator - Oct. 13, 2017, 2:50 p.m.
ryanmce requested changes to this revision.
ryanmce added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> commands.py:800
> +        if firsttime:
> +            raise error.Abort(_('not in a bisect'))
> +        cmdutil.checkunfinished(repo)

Histedit does this:

  abort: no histedit in progress

That wording sounds more natural to me, so I'd suggest something similar here, like:

  abort: no bisect in progress

> commands.py:804
> +        if not state['original']:
> +            ui.warn(_('warning: unknown original changeset\n'))
> +        else:

I think we can be more clear here. It's not that the original is an unknown node (which is how I parse this wording), but that that state file didn't have one, so perhaps "warning: not updating since bisect state did not include original changeset" would be more clear (though it's a bit wordy).

> dlax wrote in commands.py:809
> This "bisect aborted" message is not very useful (especially at "warning" level): either the command fails with a message or it succeeds silently. Also maybe it'd be more useful to display the new working directory parent.

I agree. This can be a status or even an info. +1 on the idea of mentioning the parent.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1044

To: quark, #hg-reviewers, ryanmce
Cc: ryanmce, dlax, mercurial-devel
phabricator - Oct. 14, 2017, 12:21 a.m.
quark added inline comments.

INLINE COMMENTS

> ryanmce wrote in commands.py:809
> I agree. This can be a status or even an info. +1 on the idea of mentioning the parent.

rebase uses `ui.warn`. histedit does not have this output. `ui.write` will cause inconsistency. Maybe it's "safer" to just not output anything.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1044

To: quark, #hg-reviewers, ryanmce
Cc: ryanmce, dlax, mercurial-devel
phabricator - Oct. 14, 2017, 4:11 a.m.
durin42 added a comment.


  I'm -0 on the behavior. I'd be indifferent about adding a --abort to bisect that just is an alias for --reset.
  
  (I still think it's extremely weird that ~everyone at Facebook sees bisect as a modal command like histedit or rebase.)

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1044

To: quark, #hg-reviewers, ryanmce
Cc: durin42, ryanmce, dlax, mercurial-devel
phabricator - Oct. 14, 2017, 12:50 p.m.
quark added a subscriber: kulshrax.
quark added a comment.


  In https://phab.mercurial-scm.org/D1044#17757, @durin42 wrote:
  
  > I'm -0 on the behavior. I'd be indifferent about adding a --abort to bisect that just is an alias for --reset.
  >
  > (I still think it's extremely weird that ~everyone at Facebook sees bisect as a modal command like histedit or rebase.)
  
  
  What I care more about is  `--update-back-to-original-changeset-before-bisect`. See the next patch https://phab.mercurial-scm.org/D950 for background. I cannot find a suitable flag name.
  
  I don't really care about the `--reset` behavior. But when @kulshrax mentioned `--abort`, I thought it was better than other flags I could think of.
  
  Alternatively, we can add a revset like `bisect("original")`. Would that sound good to you?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1044

To: quark, #hg-reviewers, ryanmce
Cc: kulshrax, durin42, ryanmce, dlax, mercurial-devel
phabricator - Oct. 14, 2017, 2:35 p.m.
durin42 added a comment.


  In https://phab.mercurial-scm.org/D1044#17970, @quark wrote:
  
  > In https://phab.mercurial-scm.org/D1044#17757, @durin42 wrote:
  >
  > > I'm -0 on the behavior. I'd be indifferent about adding a --abort to bisect that just is an alias for --reset.
  >
  >
  > What I care more about is  `--update-back-to-original-changeset-before-bisect`. See the next patch https://phab.mercurial-scm.org/D950 for background. I cannot find a suitable flag name.
  
  
  We can do https://phab.mercurial-scm.org/D950 without having to teach bisect to undo its checkout modifications: just have run-tests make note of its starting rev, and then restore it when it finishes. I've actually often found it helpful to *not* lose the bisect state immediately on finishing the bisect, so --abort doesn't quite feel right to me personally. That'd let us deliver the specific needed functionality without losing too much time on this feature discussion.
  
  Longer-term, the thing to do is probably to make sure that it's easy to go through `hg journal` and find the last revision you were on prior to running bisect? I think?
  
  > I don't really care about the `--reset` behavior. But when @kulshrax mentioned `--abort`, I thought it was better than other flags I could think of.
  > 
  > Alternatively, we can add a revset like `bisect("original")`. Would that sound good to you?
  
  I like the flag more than a bisect revset thing, I guess.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1044

To: quark, #hg-reviewers, ryanmce
Cc: kulshrax, durin42, ryanmce, dlax, mercurial-devel
phabricator - Oct. 16, 2017, 11:50 p.m.
quark added a comment.


  I think with `--command`, an option to restore to the original commit is a useful feature in `bisect` itself. If we cannot reach agreement, could we push the first 3 commits (https://phab.mercurial-scm.org/D947+https://phab.mercurial-scm.org/D948+https://phab.mercurial-scm.org/D949)? They are safe.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1044

To: quark, #hg-reviewers, ryanmce
Cc: kulshrax, durin42, ryanmce, dlax, mercurial-devel
phabricator - Oct. 17, 2017, 1 a.m.
durin42 added a comment.


  In https://phab.mercurial-scm.org/D1044#18780, @quark wrote:
  
  > I think with `--command`, an option to restore to the original commit is a useful feature in `bisect` itself. If we cannot reach an agreement before freeze, could we push the first 3 commits (https://phab.mercurial-scm.org/D947+https://phab.mercurial-scm.org/D948+https://phab.mercurial-scm.org/D949)? They are safe and solve a real issue when log template is changed.
  
  
  I'm confused what you think the state of this is: the proximate goal of this series (as I understood it!) was to get run-tests able to restore to an original revision after doing its bisection, a goal which I wholeheartedly endorse.
  
  While I also agree that some built-in-to-bisect mechanism for "take be back where I was before bisection began" probably makes sense, I disagree that it should be named --abort. I've not looked at any of the earlier diffs in the series because @ryanmce requested changes, so much like a commented V1 on the list, I wasn't going to spend time on them until you sent a V2 (aka updated the diff, or otherwise cleared the "changes requested" state.) Does that make sense? I'm happy to land 947-949 assuming they're ready to go, but it looks like at least 948 needs some minor indentation cleanup?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1044

To: quark, #hg-reviewers, ryanmce
Cc: kulshrax, durin42, ryanmce, dlax, mercurial-devel
phabricator - Oct. 17, 2017, 5 a.m.
quark abandoned this revision.
quark added a comment.


  In https://phab.mercurial-scm.org/D1044#18824, @durin42 wrote:
  
  > I'm confused what you think the state of this is: the proximate goal of this series (as I understood it!) was to get run-tests able to restore to an original revision after doing its bisection, a goal which I wholeheartedly endorse.
  
  
  This series fixes other things, like the run-tests.py bisect does not work with a customized `log` template.
  
  > While I also agree that some built-in-to-bisect mechanism for "take be back where I was before bisection began" probably makes sense, 
  >  I disagree that it should be named --abort.
  
  I'd like to make it a built-in bisect feature so `run-tests.py` does not need to implement the feature using a less reliable (shell script) way.
  
  It's unlikely to reach an agreement about how (what flags or revsets) to implement it before the freeze. Therefore I'm abandoning these two patches.
  
  > I've not looked at any of the earlier diffs in the series because @ryanmce requested changes, so much like a commented V1 on the list, I wasn't going to spend time on them until you sent a V2 (aka updated the diff, or otherwise cleared the "changes requested" state.) Does that make sense? I'm happy to land 947-949 assuming they're ready to go, but it looks like at least 948 needs some minor indentation cleanup?
  
  I don't think @ryanmce requested changes fairly for the 1st and 3rd patch. I've pushed back.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1044

To: quark, #hg-reviewers, ryanmce
Cc: kulshrax, durin42, ryanmce, dlax, mercurial-devel

Patch

diff --git a/mercurial/hbisect.py b/mercurial/hbisect.py
--- a/mercurial/hbisect.py
+++ b/mercurial/hbisect.py
@@ -154,24 +154,27 @@ 
     return None
 
 def load_state(repo):
-    state = {'current': [], 'good': [], 'bad': [], 'skip': []}
+    state = {'current': [], 'good': [], 'bad': [], 'skip': [], 'original': []}
     for l in repo.vfs.tryreadlines("bisect.state"):
         kind, node = l[:-1].split()
         node = repo.lookup(node)
         if kind not in state:
             raise error.Abort(_("unknown bisect kind %s") % kind)
         state[kind].append(node)
     return state
 
-
 def save_state(repo, state):
     f = repo.vfs("bisect.state", "w", atomictemp=True)
     with repo.wlock():
         for kind in sorted(state):
             for node in state[kind]:
                 f.write("%s %s\n" % (kind, hex(node)))
         f.close()
 
+def stateexists(repo):
+    """return True if bisect state exists, False otherwise"""
+    return repo.vfs.exists("bisect.state")
+
 def resetstate(repo):
     """remove any bisect state from the repository"""
     if repo.vfs.exists("bisect.state"):
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -670,11 +670,14 @@ 
     ('s', 'skip', False, _('skip testing changeset')),
     ('e', 'extend', False, _('extend the bisect range')),
     ('c', 'command', '', _('use command to check changeset state'), _('CMD')),
-    ('U', 'noupdate', False, _('do not update to target'))],
+    ('U', 'noupdate', False, _('do not update to target')),
+    ('', 'abort', False,
+     _('abort bisect and update to the original changeset')),
+    ],
     _("[-gbsr] [-U] [-c CMD] [REV]"))
 def bisect(ui, repo, rev=None, extra=None, command=None,
                reset=None, good=None, bad=None, skip=None, extend=None,
-               noupdate=None):
+               noupdate=None, abort=None):
     """subdivision search of changesets
 
     This command helps to find changesets which introduce problems. To
@@ -770,6 +773,7 @@ 
         raise error.Abort(_('incompatible arguments'))
 
     incompatibles = {
+        '--abort': abort,
         '--bad': bad,
         '--command': bool(command),
         '--extend': extend,
@@ -788,8 +792,26 @@ 
         hbisect.resetstate(repo)
         return
 
+    firsttime = not hbisect.stateexists(repo)
     state = hbisect.load_state(repo)
 
+    if abort:
+        if firsttime:
+            raise error.Abort(_('not in a bisect'))
+        cmdutil.checkunfinished(repo)
+        cmdutil.bailifchanged(repo)
+        if not state['original']:
+            ui.warn(_('warning: unknown original changeset\n'))
+        else:
+            node = state['original'][0]
+            hg.clean(repo, node, show_stats=False)
+        hbisect.resetstate(repo)
+        ui.warn(_('bisect aborted\n'))
+        return
+
+    if firsttime:
+        state['original'] = [repo['.'].node()]
+
     # update state
     if good or bad or skip:
         if rev: