This is an archive of the discontinued Mercurial Phabricator instance.

Differential D168

smartlog: do not draw public commits between draft commits and master
ClosedPublic

Authored by stash on Jul 21 2017, 8:41 AM.
Tags
None
Subscribers
ryanmce

Details

Reviewers
ryanmce
Group Reviewers
Restricted Project
Commits
rFBHGXf03173d49447: smartlog: do not draw public commits between draft commits and master
Summary

We've seen a couple of issues when there are lots of public commits between a
head and master bookmark. Smartlog tries to draw all of the public commits
and drawing it can be too slow. See example below

   o - some commit that smartlog wants to draw
  /
o
.
.    -- many commits
.
o  - master bookmarks

This issue happens often while using infinitepush extension.
Let's fix it by drawing just one public commit between a head and master.
We could of course avoid drawing this public commit at all, but in that case
output would be smth like

   o - some commit that smartlog wants to draw
  /
o
|     - many commits in betwee, but user doesn't see it because there are no dots
o  - master bookmarks
Test Plan

Slowly run tests

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

stash created this revision.Jul 21 2017, 8:41 AM
ryanmce edited the summary of this revision. (Show Details)Jul 21 2017, 8:48 AM
ryanmce accepted this revision.Jul 21 2017, 8:53 AM
ryanmce added a subscriber: ryanmce.

Nice! This will also help a lot with release branches.

hgext3rd/smartlog.py
546–548

grammar-nit: s/found/find (alternatively, use "have found")

I could go into the grammar details of why, but just trust me on this one :-)

This revision is now accepted and ready to land.Jul 21 2017, 8:53 AM
stash added inline comments.Jul 21 2017, 9:26 AM
hgext3rd/smartlog.py
546–548

Trust you and will fix. But still curious to know the details :)

stash updated this revision to Diff 363.Jul 21 2017, 9:29 AM

addressed comments

ryanmce added inline comments.Jul 21 2017, 10:38 AM
hgext3rd/smartlog.py
546–548

We should probably also add an indefinite article in here:

stop as soon as we find a public commit

As for the english lesson (I'm not an expert here, but this is my understanding):

"found" is the past participle of "to find", and generally, you need to use "have" in front of the past participle.

Unfortunately, it's a little more complicated (as always in English) because "found" is also the past-tense of "to find".

It's easier to see if you use a verb that doesn't have these overlapping conjugations. Take "to eat" as an example.

  • Infinitive: to eat
  • Past tense: ate
  • Part participle: eaten

So, it sounds okay to say "we ate that" but "we eaten that" sounds very weird. Instead, you'd say "we have eaten that".

Now, coming back to your use of "found": if it's both a past participle and a past tense, why did it sound weird to me? To figure this out, I had to look at the full phrase (Note that I'm adding the "a" in here to make it more correct-sounding as well):

stop as soon as we found a public commit

The issue is that "stop" is the present imperative (command) tense, and it's a tense mismatch to combine the present-tense "stop" with the past-tense "found". Thus, even though "we found a public commit" sounds fine, it doesn't work as the dependent clause to "stop as soon as".

Yet, it's okay to use the past participle form "have found" because the present-tense "stop" happens only after we have found the public commit.

So, going back to the "to eat" example:

jump after we ate the food

Whereas these work:

EXAMPLE
jump after we have eaten the food
jump after we eat the food

Also you can make everything past-tense to sound okay as well:

EXAMPLE
you jumped after we ate the food
stash added inline comments.Jul 21 2017, 10:43 AM
hgext3rd/smartlog.py
546–548

Hm, probably I get it. Since stop is in present tense, then we should use present or present perfect in the other clause. But then stopped as soon as we found a new commit is correct, isn't it?

Closed by commit rFBHGXf03173d49447: smartlog: do not draw public commits between draft commits and master (authored by stash). · Explain WhyJul 21 2017, 11:10 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext3rd/smartlog.py (9 lines)
Mtests/test-smartlog.t (40 lines)

Diff 375

hgext3rd/smartlog.py

# local-only (draft) branch # local-only (draft) branch
rs = 'branch("%s")' % branch rs = 'branch("%s")' % branch
branchmaster = repo.revs(rs).first() branchmaster = repo.revs(rs).first()
except Exception: except Exception:
branchmaster = repo.revs('tip').first() branchmaster = repo.revs('tip').first()
else: else:
branchmaster = masterrev branchmaster = masterrev
# Find ancestors of heads that are not in master # Find all draft ancestors and latest public ancestor of heads
# that are not in master.
# We don't want to draw all public commits because there can be too
# many of them.
# Don't use revsets, they are too slow # Don't use revsets, they are too slow
for head in heads: for head in heads:
if branchinfo(head)[0] != branch: if branchinfo(head)[0] != branch:
continue continue
anc = rev(ancestor(node(head), node(branchmaster))) anc = rev(ancestor(node(head), node(branchmaster)))
queue = [head] queue = [head]
while queue: while queue:
current = queue.pop(0) current = queue.pop(0)
if current not in revs: if current not in revs:
revs.add(current) revs.add(current)
if current != anc: # stop as soon as we find public commit
ispublic = repo[current].phase() == phases.public
if current != anc and not ispublic:
ryanmceUnsubmitted
Not Done

grammar-nit: s/found/find (alternatively, use "have found")

I could go into the grammar details of why, but just trust me on this one :-)

ryanmce: grammar-nit: s/found/find (alternatively, use "have found") I could go into the grammar…
stashAuthorUnsubmitted
Not Done

Trust you and will fix. But still curious to know the details :)

stash: Trust you and will fix. But still curious to know the details :)
ryanmceUnsubmitted
Not Done

We should probably also add an indefinite article in here:

stop as soon as we find a public commit

As for the english lesson (I'm not an expert here, but this is my understanding):

"found" is the past participle of "to find", and generally, you need to use "have" in front of the past participle.

Unfortunately, it's a little more complicated (as always in English) because "found" is also the past-tense of "to find".

It's easier to see if you use a verb that doesn't have these overlapping conjugations. Take "to eat" as an example.

  • Infinitive: to eat
  • Past tense: ate
  • Part participle: eaten

So, it sounds okay to say "we ate that" but "we eaten that" sounds very weird. Instead, you'd say "we have eaten that".

Now, coming back to your use of "found": if it's both a past participle and a past tense, why did it sound weird to me? To figure this out, I had to look at the full phrase (Note that I'm adding the "a" in here to make it more correct-sounding as well):

stop as soon as we found a public commit

The issue is that "stop" is the present imperative (command) tense, and it's a tense mismatch to combine the present-tense "stop" with the past-tense "found". Thus, even though "we found a public commit" sounds fine, it doesn't work as the dependent clause to "stop as soon as".

Yet, it's okay to use the past participle form "have found" because the present-tense "stop" happens only after we have found the public commit.

So, going back to the "to eat" example:

jump after we ate the food

Whereas these work:

EXAMPLE
jump after we have eaten the food
jump after we eat the food

Also you can make everything past-tense to sound okay as well:

EXAMPLE
you jumped after we ate the food
ryanmce: We should probably also add an indefinite article in here: > stop as soon as we find **a**…
stashAuthorUnsubmitted
Not Done

Hm, probably I get it. Since stop is in present tense, then we should use present or present perfect in the other clause. But then stopped as soon as we found a new commit is correct, isn't it?

stash: Hm, probably I get it. Since `stop` is in present tense, then we should use present or present…
parents = parentrevs(current) parents = parentrevs(current)
for p in parents: for p in parents:
if p > anc: if p > anc:
queue.append(p) queue.append(p)
# add context: master, current commit, and the common ancestor # add context: master, current commit, and the common ancestor
revs.add(branchmaster) revs.add(branchmaster)

tests/test-smartlog.t

| r1 | r1
| |
@ 0 1ea73414a91b 1970-01-01 00:00 +0000 debugbuilddag @ 0 1ea73414a91b 1970-01-01 00:00 +0000 debugbuilddag
| r0 | r0
| |
o -1[curr] 000000000000 1970-01-01 00:00 +0000 o -1[curr] 000000000000 1970-01-01 00:00 +0000
Make sure public commits that are descendants of master are not drawn
$ cd ..
$ hg init repo3
$ cd repo3
$ hg debugbuilddag '+5'
$ hg bookmark master -r 1
$ hg phase --public -r 1
$ hg smartlog -T compact --all --config smartlog.indentnonpublic=1
o 4[tip] bebd167eb94d 1970-01-01 00:00 +0000 debugbuilddag
| r4
|
o 3 2dc09a01254d 1970-01-01 00:00 +0000 debugbuilddag
| r3
|
o 2 01241442b3c2 1970-01-01 00:00 +0000 debugbuilddag
/ r2
|
o 1[master] 66f7d451a68b 1970-01-01 00:00 +0000 debugbuilddag
| r1
|
$ hg phase -r 3 --public --force
$ hg up -q 4
$ hg smartlog -T compact --all --config smartlog.indentnonpublic=1
@ 4[tip] bebd167eb94d 1970-01-01 00:00 +0000 debugbuilddag
/ r4
|
o 3 2dc09a01254d 1970-01-01 00:00 +0000 debugbuilddag
. r3
.
o 1[master] 66f7d451a68b 1970-01-01 00:00 +0000 debugbuilddag
| r1
|
$ hg phase -r 4 --public --force
$ hg smartlog -T compact --all --config smartlog.indentnonpublic=1
@ 4[tip] bebd167eb94d 1970-01-01 00:00 +0000 debugbuilddag
. r4
.
o 1[master] 66f7d451a68b 1970-01-01 00:00 +0000 debugbuilddag
| r1
|
Make sure the template keywords are documented correctly Make sure the template keywords are documented correctly
$ hg help templates | grep -A1 successor | grep -v predecessors $ hg help templates | grep -A1 successor | grep -v predecessors
amendsuccessors amendsuccessors
Return all of the node's successors created as a result of Return all of the node's successors created as a result of
amend amend
-- --
foldsuccessors foldsuccessors