This is an archive of the discontinued Mercurial Phabricator instance.

Differential D159

undo: localbranch revset
ClosedPublic

Authored by felixmerk on Jul 20 2017, 6:20 PM.
Tags
None
Subscribers
quark, stash

Details

Reviewers
stash
Group Reviewers
Restricted Project
Commits
rFBHGXac8013486e5a: undo: localbranch revset
Summary

Adds localbranch revset to be used for localbranch undos. A localbranch is a
set of draft commits that are connected via draft commits. Any draft commit
within a branch, or a public commit at the base of the branch, can be passed as
an argument to localbranch(). This will be necessary for selecting the scope
for a localbranch undo. Also fixes oldraft() output when used with other
revsets. Previously olddraft() would ignore operators such as and because of a
missing the necessary handling.

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

felixmerk created this revision.Jul 20 2017, 6:20 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 20 2017, 6:20 PM
stash requested changes to this revision.Jul 21 2017, 5:11 AM
stash added a subscriber: stash.
stash added inline comments.
hgext3rd/undo.py
389

This is recomputed many times, I think it's a good idea to compute it only once

398–408

Doc says that you are accepting changeset, but you are really accepting arbitrary revset. Something needs to be changed, I think doc

417–420

I'm not really sure what you are trying to achieve. Are you trying to remove duplicates? In that case using set is faster and simpler to understand

This revision now requires changes to proceed.Jul 21 2017, 5:11 AM
felixmerk added inline comments.Jul 21 2017, 6:30 PM
hgext3rd/undo.py
398–408

The code does support either, but I wasn't sure how to accept both string arguments (such as 27cb) and a revset (such as draft()) while still using the revsetlang.get commands and so I restricted the input to what I needed and what I tested while allowing for future expansion of allowed inputs. Do we have some more powerful input handling commands?

417–420

Good point! That's definitely cleaner.

felixmerk edited edge metadata.Jul 21 2017, 7:08 PM
felixmerk updated this revision to Diff 384.
stash accepted this revision.Jul 24 2017, 6:20 AM

I don't want to block you so I'm going to accept the change. But before landing please do two things:

  1. Update the docstring of the revset describing the behavior of localbranch(PUBLIC)
  2. Add tests for localbranch(PUBLIC) and maybe localbranch(HASH1 + HASH2)
hgext3rd/undo.py
385–408

Do I understand correctly it's a bug fix?
If yes can you please add it to the commit message?

394

This can also be cached

398–408

No need to change the code, I'd just change the doc string from localbranch(changeset) to localbranch(revset) or something like that.

416

This case is not actually covered with tests, please do.
Also your commit message says
`Any draft commit
within a branch, or a public commit at the base of the branch, can be passed as
an argument to localbranch()`, but your docstring doesn't say it. Can you please update the docstring before landing the commit?

tests/test-undo.t
180–203

Why did you add this change? Is it related to the fix in olddraft() revset?

444

To verify your test I did arc patch and changed the test a bit:

{P57679225}

I think it's a bit clearer.

Also please add tests with public commits.

This revision is now accepted and ready to land.Jul 24 2017, 6:20 AM
felixmerk added inline comments.Jul 24 2017, 6:15 PM
hgext3rd/undo.py
385–408

It allows for logical operations such as "and" to work. I tried covering it in the commit message with "fixes oldraft() output when used with other revsets," but I can be more explicit about the nature of the bug.

tests/test-undo.t
180–203

Yes. olddraft() did not respond to "and" and some other revset operators. I hadn't realized that the revset itself had to cover that logic and I still don't know why "and" didn't work while "-" did work. I now use the same "subset &" logic that other revsets use.

444

Line 504

I guess I should add a newline and comment before

felixmerk added inline comments.Jul 24 2017, 6:33 PM
hgext3rd/undo.py
398–408

Maybe I'm misunderstanding, but "tip()" for example is a revset but not a string and so will raise an error. If you look at a revset that takes both as arguments, such as branch in revset.py, there is a lot of logic that goes with it.

felixmerk updated this revision to Diff 397.Jul 24 2017, 6:56 PM
felixmerk edited the summary of this revision. (Show Details)Jul 24 2017, 6:58 PM
stash added inline comments.Jul 25 2017, 12:58 PM
hgext3rd/undo.py
398–408

Ok, thanks for explanation

felixmerk updated this revision to Diff 414.Jul 26 2017, 2:41 PM
quark added a subscriber: quark.Jul 26 2017, 7:28 PM
quark added inline comments.
hgext3rd/undo.py
416

You can use revs = revset.getset(repo, fullreposet(repo), x) here so it works with arbitrary revset.

417–427

Could you check if the revset is equivalent to the below one?

querystring = revsetlang.formatspec('((::%ld) & draft())::', revs)
return subset & smartset.baseset(repo.revs(querystring))

And that might still have issues with:

$ hg update a-public-commit
$ hg commit
$ hg strip .
$ hg undo # ??

But we can figure out it later.

quark added inline comments.Jul 26 2017, 8:00 PM
hgext3rd/undo.py
417–427

I see the strip case was handled by the public() and %s test. It might be possible to handle it first, like, we can assume len(revs) == 1 and test repo[revs.first()].phase(), if it's phases.public, use an alternative revset:

(children(%d) & draft())::   # executed on an filtered repo
felixmerk updated this revision to Diff 420.Jul 26 2017, 8:27 PM
felixmerk updated this revision to Diff 507.Aug 2 2017, 4:31 PM
Closed by commit rFBHGXac8013486e5a: undo: localbranch revset (authored by felixmerk). · Explain WhyAug 4 2017, 9:19 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext3rd/undo.py (26 lines)
Mtests/test-undo.t (127 lines)

Diff 538

hgext3rd/undo.py

dispatch, dispatch,
error, error,
extensions, extensions,
hg, hg,
localrepo, localrepo,
lock as lockmod, lock as lockmod,
obsolete, obsolete,
obsutil, obsutil,
phases,
registrar, registrar,
revlog, revlog,
revset, revset,
revsetlang, revsetlang,
smartset, smartset,
transaction, transaction,
util, util,
) )
an undoable command is one that changed draft heads, bookmarks an undoable command is one that changed draft heads, bookmarks
and or working copy parent and or working copy parent
Note: this revset may include hidden commits Note: this revset may include hidden commits
""" """
args = revset.getargsdict(x, 'olddraftrevset', 'reverseindex') args = revset.getargsdict(x, 'olddraftrevset', 'reverseindex')
reverseindex = revsetlang.getinteger(args.get('reverseindex'), reverseindex = revsetlang.getinteger(args.get('reverseindex'),
_('index must be a positive integer'), 1) _('index must be a positive integer'), 1)
revs = _getolddrafts(repo, reverseindex) revs = _getolddrafts(repo, reverseindex)
return smartset.baseset(revs) return subset & smartset.baseset(revs)
@revsetpredicate('_localbranch')
def _localbranch(repo, subset, x):
"""``_localbranch(changectx)``
stashUnsubmitted
Not Done

This is recomputed many times, I think it's a good idea to compute it only once

stash: This is recomputed many times, I think it's a good idea to compute it only once
localbranch changesets
Returns all commits within the same localbranch as the changeset(s). A local
branch is all draft changesets that are connected, uninterupted by public
changesets. Any draft commit within a branch, or a public commit at the
stashUnsubmitted
Not Done

This can also be cached

stash: This can also be cached
base of the branch, can be passed used to identify localbranches.
"""
# executed on an filtered repo
args = revset.getargsdict(x, 'branchrevset', 'changectx')
revstring = revsetlang.getstring(args.get('changectx'),
_('localbranch argument must be a changectx'))
revs = repo.revs(revstring)
# we assume that there is only a single rev
if repo[revs.first()].phase() == phases.public:
querystring = revsetlang.formatspec('(children(%d) & draft())::',
revs.first())
else:
querystring = revsetlang.formatspec('((::%ld) & draft())::', revs)
return subset & smartset.baseset(repo.revs(querystring))
stashUnsubmitted
Not Done

Doc says that you are accepting changeset, but you are really accepting arbitrary revset. Something needs to be changed, I think doc

stash: Doc says that you are accepting changeset, but you are really accepting arbitrary revset.
felixmerkAuthorUnsubmitted
Not Done

The code does support either, but I wasn't sure how to accept both string arguments (such as 27cb) and a revset (such as draft()) while still using the revsetlang.get commands and so I restricted the input to what I needed and what I tested while allowing for future expansion of allowed inputs. Do we have some more powerful input handling commands?

felixmerk: The code does support either, but I wasn't sure how to accept both string arguments (such as…
stashUnsubmitted
Not Done

No need to change the code, I'd just change the doc string from localbranch(changeset) to localbranch(revset) or something like that.

stash: No need to change the code, I'd just change the doc string from `localbranch(changeset)` to…
felixmerkAuthorUnsubmitted
Not Done

Maybe I'm misunderstanding, but "tip()" for example is a revset but not a string and so will raise an error. If you look at a revset that takes both as arguments, such as branch in revset.py, there is a lot of logic that goes with it.

felixmerk: Maybe I'm misunderstanding, but "tip()" for example is a revset but not a string and so will…
stashUnsubmitted
Not Done

Ok, thanks for explanation

stash: Ok, thanks for explanation
stashUnsubmitted
Not Done

Do I understand correctly it's a bug fix?
If yes can you please add it to the commit message?

stash: Do I understand correctly it's a bug fix? If yes can you please add it to the commit message?
felixmerkAuthorUnsubmitted
Not Done

It allows for logical operations such as "and" to work. I tried covering it in the commit message with "fixes oldraft() output when used with other revsets," but I can be more explicit about the nature of the bug.

felixmerk: It allows for logical operations such as "and" to work. I tried covering it in the commit…
# Undo: # Undo:
@command('undo', [ @command('undo', [
('a', 'absolute', False, _("absolute based on command index instead of " ('a', 'absolute', False, _("absolute based on command index instead of "
"relative undo")), "relative undo")),
('f', 'force', False, _("undo across missing undo history (ADVANCED)")), ('f', 'force', False, _("undo across missing undo history (ADVANCED)")),
('k', 'keep', False, _("keep working copy changes")), ('k', 'keep', False, _("keep working copy changes")),
stashUnsubmitted
Not Done

This case is not actually covered with tests, please do.
Also your commit message says
`Any draft commit
within a branch, or a public commit at the base of the branch, can be passed as
an argument to localbranch()`, but your docstring doesn't say it. Can you please update the docstring before landing the commit?

stash: This case is not actually covered with tests, please do. Also your commit message says `Any…
quarkUnsubmitted
Not Done

You can use revs = revset.getset(repo, fullreposet(repo), x) here so it works with arbitrary revset.

quark: You can use `revs = revset.getset(repo, fullreposet(repo), x)` here so it works with arbitrary…
('n', 'index', 1, _("how many steps to undo back")), ('n', 'index', 1, _("how many steps to undo back")),
]) ])
def undo(ui, repo, *args, **opts): def undo(ui, repo, *args, **opts):
"""perform an undo """perform an undo
stashUnsubmitted
Not Done

I'm not really sure what you are trying to achieve. Are you trying to remove duplicates? In that case using set is faster and simpler to understand

stash: I'm not really sure what you are trying to achieve. Are you trying to remove duplicates? In…
felixmerkAuthorUnsubmitted
Not Done

Good point! That's definitely cleaner.

felixmerk: Good point! That's definitely cleaner.
Undoes an undoable command. An undoable command is one that changed at Undoes an undoable command. An undoable command is one that changed at
least one of the following three: bookmarks, working copy parent or least one of the following three: bookmarks, working copy parent or
changesets. Note that this specifically does not include commands like log. changesets. Note that this specifically does not include commands like log.
It will include update if update changes the working copy parent (you update It will include update if update changes the working copy parent (you update
to a changeset that isn't the current one). Note that commands that edit to a changeset that isn't the current one). Note that commands that edit
public repos can't be undone (specifically push). public repos can't be undone (specifically push).
quarkUnsubmitted
Not Done

Could you check if the revset is equivalent to the below one?

querystring = revsetlang.formatspec('((::%ld) & draft())::', revs)
return subset & smartset.baseset(repo.revs(querystring))

And that might still have issues with:

$ hg update a-public-commit
$ hg commit
$ hg strip .
$ hg undo # ??

But we can figure out it later.

quark: Could you check if the revset is equivalent to the below one? querystring = revsetlang.
quarkUnsubmitted
Not Done

I see the strip case was handled by the public() and %s test. It might be possible to handle it first, like, we can assume len(revs) == 1 and test repo[revs.first()].phase(), if it's phases.public, use an alternative revset:

(children(%d) & draft())::   # executed on an filtered repo
quark: I see the strip case was handled by the `public() and %s` test. It might be possible to handle…
Undo does not preserve the working copy changes. Undo does not preserve the working copy changes.
To undo to a specific state use the --index and --absolute flags. To undo to a specific state use the --index and --absolute flags.
See hg debugundohistory to get a list of indeces and commands run. See hg debugundohistory to get a list of indeces and commands run.
By undoing to a specific index you undo to the state after that command. By undoing to a specific index you undo to the state after that command.
For example, hg undo --index 0 --absolute won't do anything, while For example, hg undo --index 0 --absolute won't do anything, while
hg undo -n 1 -a will bring you back to the repo state before the current hg undo -n 1 -a will bring you back to the repo state before the current

tests/test-undo.t

workingparent: workingparent:
aa430c8afedf9b2ec3f0655d39eef6b6b0a2ddb6 aa430c8afedf9b2ec3f0655d39eef6b6b0a2ddb6
Test 'olddraft([NUM])' revset Test 'olddraft([NUM])' revset
$ hg log -G -r 'olddraft(0) - olddraft(1)' --hidden -T compact $ hg log -G -r 'olddraft(0) - olddraft(1)' --hidden -T compact
@ 10[tip][master] aa430c8afedf 1970-01-01 00:00 +0000 test @ 10[tip][master] aa430c8afedf 1970-01-01 00:00 +0000 test
| a5 | a5
~ ~
$ hg log -G -r 'olddraft(1) and draft()' -T compact
o 9 1dafc0b43612 1970-01-01 00:00 +0000 test
| cmiss
|
o 8:4 0a3dd3e15e65 1970-01-01 00:00 +0000 test
| words
|
| o 7[feature2]:4 296fda51a303 1970-01-01 00:00 +0000 test
|/ d
|
o 4 38d85b506754 1970-01-01 00:00 +0000 test
| c2
|
o 3:1 ec7553f7b382 1970-01-01 00:00 +0000 test
| c1
|
| o 2[feature1] 49cdb4091aca 1970-01-01 00:00 +0000 test
|/ b
|
o 1 b68836a6e2ca 1970-01-01 00:00 +0000 test
| a2
|
o 0 df4fd610a3d6 1970-01-01 00:00 +0000 test
a1
stashUnsubmitted
Not Done

Why did you add this change? Is it related to the fix in olddraft() revset?

stash: Why did you add this change? Is it related to the fix in `olddraft()` revset?
felixmerkAuthorUnsubmitted
Not Done

Yes. olddraft() did not respond to "and" and some other revset operators. I hadn't realized that the revset itself had to cover that logic and I still don't know why "and" didn't work while "-" did work. I now use the same "subset &" logic that other revsets use.

felixmerk: Yes. olddraft() did not respond to "and" and some other revset operators. I hadn't realized…
$ hg log -G -r 'olddraft(1) and public()' -T compact
Test undolog lock Test undolog lock
$ hg log --config hooks.duringundologlock="sleep 1" > /dev/null & $ hg log --config hooks.duringundologlock="sleep 1" > /dev/null &
$ sleep 0.1 $ sleep 0.1
$ hg st --time $ hg st --time
time: real [1-9]*\..* (re) time: real [1-9]*\..* (re)
hg undo command tests hg undo command tests
#if chg #if chg
(note: chg has issues with the below test) (note: chg has issues with the below test)
#else #else
$ hg st --debug $ hg st --debug
caught revlog error. undolog/index.i was probably corrupted caught revlog error. undolog/index.i was probably corrupted
#endif #endif
$ hg debugundohistory -l $ hg debugundohistory -l
0: -- gap in log -- 0: -- gap in log --
_localbranch revset tests
stashUnsubmitted
Not Done

To verify your test I did arc patch and changed the test a bit:

{P57679225}

I think it's a bit clearer.

Also please add tests with public commits.

stash: To verify your test I did arc patch and changed the test a bit: {P57679225} I think it's a…
felixmerkAuthorUnsubmitted
Not Done

Line 504

I guess I should add a newline and comment before

felixmerk: Line 504 I guess I should add a newline and comment before
$ hg log -r '_localbranch(75f63379f12b)'
changeset: 0:df4fd610a3d6
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: a1
changeset: 1:b68836a6e2ca
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: a2
changeset: 2:49cdb4091aca
bookmark: feature1
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: b
changeset: 3:ec7553f7b382
parent: 1:b68836a6e2ca
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: c1
changeset: 4:38d85b506754
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: c2
changeset: 7:296fda51a303
bookmark: feature2
parent: 4:38d85b506754
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: d
changeset: 8:0a3dd3e15e65
parent: 4:38d85b506754
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: words
changeset: 9:1dafc0b43612
bookmark: master
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: cmiss
changeset: 17:d0fdb9510dbf
parent: 9:1dafc0b43612
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: newfiles
changeset: 18:75f63379f12b
tag: tip
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: newfiles
Test with public commit
$ hg phase -r 0a3dd3e15e65 -p
$ hg log -r '_localbranch(75f63379f12b)'
changeset: 9:1dafc0b43612
bookmark: master
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: cmiss
changeset: 17:d0fdb9510dbf
parent: 9:1dafc0b43612
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: newfiles
changeset: 18:75f63379f12b
tag: tip
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: newfiles
$ hg log -r '_localbranch(0a3dd3e15e65)'
changeset: 9:1dafc0b43612
bookmark: master
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: cmiss
changeset: 17:d0fdb9510dbf
parent: 9:1dafc0b43612
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: newfiles
changeset: 18:75f63379f12b
tag: tip
user: test
date: Thu Jan 01 00:00:00 1970 +0000
summary: newfiles