This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1044

bisect: add --abort flag
AbandonedPublic

Authored by quark on Oct 12 2017, 8:11 PM.

Details

Reviewers
ryanmce
Group Reviewers
hg-reviewers
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.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Oct 12 2017, 8:11 PM
quark updated this revision to Diff 2655.Oct 13 2017, 2:13 AM
dlax added a subscriber: dlax.Oct 13 2017, 4:28 AM
dlax added inline comments.
mercurial/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.

ryanmce requested changes to this revision.Oct 13 2017, 10:50 AM
ryanmce added a subscriber: ryanmce.
ryanmce added inline comments.
mercurial/commands.py
800

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
804

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).

809

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

This revision now requires changes to proceed.Oct 13 2017, 10:50 AM
quark updated this revision to Diff 2705.Oct 13 2017, 8:20 PM
quark added inline comments.Oct 13 2017, 8:21 PM
mercurial/commands.py
809

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

durin42 added a subscriber: durin42.Oct 14 2017, 12:11 AM

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.)

quark added a subscriber: kulshrax.EditedOct 14 2017, 8:50 AM
In 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 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?

(I still think it's extremely weird that ~everyone at Facebook sees bisect as a modal command like histedit or rebase.)

I don't treat it as a modal command. Otherwise I would have sent something like --restore: *must use together* with --command, update back to the original changeset.

durin42 added a comment.Oct 14 2017, 10:35 AM
In D1044#17970, @quark wrote:
In 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 D950 for background. I cannot find a suitable flag name.

We can do 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.

quark added a comment.EditedOct 16 2017, 7:50 PM

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 (D947+D948+D949)? They are safe and solve a real issue when log template is changed.

durin42 added a comment.Oct 16 2017, 9:00 PM
In 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 (D947+D948+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?

quark abandoned this revision.Oct 17 2017, 1:00 AM
In 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.

Revision Contents
Changeset List

PathPackages
Mmercurial/commands.py (26 lines)
Mmercurial/hbisect.py (7 lines)
AMtests/test-bisect-abort.t (36 lines)

Diff 2705

mercurial/commands.py

@command('bisect', @command('bisect',
[('r', 'reset', False, _('reset bisect state')), [('r', 'reset', False, _('reset bisect state')),
('g', 'good', False, _('mark changeset good')), ('g', 'good', False, _('mark changeset good')),
('b', 'bad', False, _('mark changeset bad')), ('b', 'bad', False, _('mark changeset bad')),
('s', 'skip', False, _('skip testing changeset')), ('s', 'skip', False, _('skip testing changeset')),
('e', 'extend', False, _('extend the bisect range')), ('e', 'extend', False, _('extend the bisect range')),
('c', 'command', '', _('use command to check changeset state'), _('CMD')), ('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]")) _("[-gbsr] [-U] [-c CMD] [REV]"))
def bisect(ui, repo, rev=None, extra=None, command=None, def bisect(ui, repo, rev=None, extra=None, command=None,
reset=None, good=None, bad=None, skip=None, extend=None, reset=None, good=None, bad=None, skip=None, extend=None,
noupdate=None): noupdate=None, abort=None):
"""subdivision search of changesets """subdivision search of changesets
This command helps to find changesets which introduce problems. To This command helps to find changesets which introduce problems. To
use, mark the earliest changeset you know exhibits the problem as use, mark the earliest changeset you know exhibits the problem as
bad, then mark the latest changeset which is free from the problem bad, then mark the latest changeset which is free from the problem
as good. Bisect will update your working directory to a revision as good. Bisect will update your working directory to a revision
for testing (unless the -U/--noupdate option is specified). Once for testing (unless the -U/--noupdate option is specified). Once
you have performed tests, mark the working directory as good or you have performed tests, mark the working directory as good or
elif cmd == "bad": elif cmd == "bad":
bad = True bad = True
else: else:
reset = True reset = True
elif extra: elif extra:
raise error.Abort(_('incompatible arguments')) raise error.Abort(_('incompatible arguments'))
incompatibles = { incompatibles = {
'--abort': abort,
'--bad': bad, '--bad': bad,
'--command': bool(command), '--command': bool(command),
'--extend': extend, '--extend': extend,
'--good': good, '--good': good,
'--reset': reset, '--reset': reset,
'--skip': skip, '--skip': skip,
} }
enabled = [x for x in incompatibles if incompatibles[x]] enabled = [x for x in incompatibles if incompatibles[x]]
if len(enabled) > 1: if len(enabled) > 1:
raise error.Abort(_('%s and %s are incompatible') % raise error.Abort(_('%s and %s are incompatible') %
tuple(sorted(enabled)[0:2])) tuple(sorted(enabled)[0:2]))
if reset: if reset:
hbisect.resetstate(repo) hbisect.resetstate(repo)
return return
firsttime = not hbisect.stateexists(repo)
state = hbisect.load_state(repo) state = hbisect.load_state(repo)
if abort:
if firsttime:
raise error.Abort(_('not bisect in progress'))
ryanmceUnsubmitted
Not Done

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
ryanmce: Histedit does this: ``` abort: no histedit in progress ``` That wording sounds more natural…
cmdutil.checkunfinished(repo)
cmdutil.bailifchanged(repo)
if not state['original']:
ui.warn(_('warning: not updating since bisect state did not '
ryanmceUnsubmitted
Not Done

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).

ryanmce: I think we can be more clear here. It's not that the original is an unknown node (which is how…
'include original changeset\n'))
else:
node = state['original'][0]
hg.clean(repo, node, show_stats=False)
hbisect.resetstate(repo)
dlaxUnsubmitted
Not Done

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.

dlax: This "bisect aborted" message is not very useful (especially at "warning" level): either the…
ryanmceUnsubmitted
Not Done

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

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

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

quark: rebase uses `ui.warn`. histedit does not have this output. `ui.write` will cause inconsistency.
return
if firsttime:
state['original'] = [repo['.'].node()]
# update state # update state
if good or bad or skip: if good or bad or skip:
if rev: if rev:
nodes = [repo.lookup(i) for i in scmutil.revrange(repo, [rev])] nodes = [repo.lookup(i) for i in scmutil.revrange(repo, [rev])]
else: else:
nodes = [repo.lookup('.')] nodes = [repo.lookup('.')]
if good: if good:
state['good'] += nodes state['good'] += nodes

mercurial/hbisect.py

else: else:
side = state['good'] side = state['good']
num = len(set(i.node() for i in parents) & set(side)) num = len(set(i.node() for i in parents) & set(side))
if num == 1: if num == 1:
return parents[0].ancestor(parents[1]) return parents[0].ancestor(parents[1])
return None return None
def load_state(repo): def load_state(repo):
state = {'current': [], 'good': [], 'bad': [], 'skip': []} state = {'current': [], 'good': [], 'bad': [], 'skip': [], 'original': []}
for l in repo.vfs.tryreadlines("bisect.state"): for l in repo.vfs.tryreadlines("bisect.state"):
kind, node = l[:-1].split() kind, node = l[:-1].split()
node = repo.lookup(node) node = repo.lookup(node)
if kind not in state: if kind not in state:
raise error.Abort(_("unknown bisect kind %s") % kind) raise error.Abort(_("unknown bisect kind %s") % kind)
state[kind].append(node) state[kind].append(node)
return state return state
def save_state(repo, state): def save_state(repo, state):
f = repo.vfs("bisect.state", "w", atomictemp=True) f = repo.vfs("bisect.state", "w", atomictemp=True)
with repo.wlock(): with repo.wlock():
for kind in sorted(state): for kind in sorted(state):
for node in state[kind]: for node in state[kind]:
f.write("%s %s\n" % (kind, hex(node))) f.write("%s %s\n" % (kind, hex(node)))
f.close() f.close()
def stateexists(repo):
"""return True if bisect state exists, False otherwise"""
return repo.vfs.exists("bisect.state")
def resetstate(repo): def resetstate(repo):
"""remove any bisect state from the repository""" """remove any bisect state from the repository"""
if repo.vfs.exists("bisect.state"): if repo.vfs.exists("bisect.state"):
repo.vfs.unlink("bisect.state") repo.vfs.unlink("bisect.state")
def checkstate(state): def checkstate(state):
"""check we have both 'good' and 'bad' to define a range """check we have both 'good' and 'bad' to define a range

tests/test-bisect-abort.t

  • This file was added.
$ cat >> $HGRCPATH <<EOF
> [extensions]
> drawdag=$TESTDIR/drawdag.py
> EOF
$ hg init
$ hg debugdrawdag <<'EOS'
> D
> |
> C
> |
> B
> |
> A
> EOS
$ hg update -q C
$ hg bisect --abort
abort: not bisect in progress
[255]
$ hg bisect --good D
$ hg bisect --bad A
Testing changeset 1:112478962961 (3 changesets remaining, ~1 tests)
0 files updated, 0 files merged, 1 files removed, 0 files unresolved
$ echo 1 >> A
$ hg bisect --abort
abort: uncommitted changes
[255]
$ hg update -q -C .
$ hg bisect --abort
$ hg log -r . -T '{desc}\n'
C