This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9316

log: add argument -B/--bookmark for bookmarks
AbandonedPublic

Authored by pulkit on Nov 12 2020, 8:11 PM.

Details

Reviewers
martinvonz
marmoute
sebhtml
Group Reviewers
hg-reviewers
Summary

This is the v2 of my patch series for adding bookmark support to "hg log".

v2

Previous version:
v1

Mercurial upstream version for v2:
hg-committed ede4a1bf14bd (@ bookmark)

Operating system:
FreeBSD atlantis 12.2-RELEASE FreeBSD 12.2-RELEASE r366954 GENERIC amd64

CPU: AMD Ryzen 5 2600 Six-Core Processor

RAM: 8 GB ECC

Python: Python 3.7.6

Shell:
GNU bash, version 5.0.18(3)-release (amd64-portbld-freebsd12.1)

Test Plan

Test command:
python3.7 ./tests/run-tests.py

Failed tests:

Ran 871 tests, 71 skipped, 20 failed.

Failed test-bookmarks-pushpull.t#b2-binary: timed out
Failed test-bookmarks-pushpull.t#b2-pushkey: timed out
Failed test-duplicateoptions.py: output changed ("failed to import extension git: No module named 'git'")
Failed test-glog-beautifygraph.t: timed out
Failed test-glog.t: timed out
Failed test-import.t: timed out
Failed test-largefiles-misc.t: timed out
Failed test-largefiles.t: timed out
Failed test-lfs-serve.t#lfsremote-on: output changed ("error: conflicting output format options.")
Failed test-log.t: timed out
Failed test-merge-combination.t: timed out
Failed test-mq.t: timed out
Failed test-obsmarker-template.t: timed out
Failed test-obsolete-bundle-strip.t: timed out
Failed test-obsolete.t: timed out
Failed test-persistent-nodemap.t: timed out
Failed test-rebase-obsolete.t: timed out
Failed test-revset.t: timed out
Failed test-subrepo.t: timed out
Failed test-template-functions.t: timed out

I don't know the why some of the tests are doing a time out.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

sebhtml created this revision.Nov 12 2020, 8:11 PM
sebhtml edited the summary of this revision. (Show Details)Nov 12 2020, 8:15 PM
sebhtml edited the test plan for this revision. (Show Details)
sebhtml added reviewers: martinvonz, pulkit.
sebhtml edited the summary of this revision. (Show Details)Nov 12 2020, 8:17 PM
sebhtml edited the test plan for this revision. (Show Details)
martinvonz added a comment.Nov 16 2020, 1:07 PM

IMO, these three commits should be folded/squashed into a single commit. One way to think of it is this: if we decide to roll it back, would it be okay to roll back only one of them?

Also, most of the commit message is noise to me. You can safely remove all of it, IMO. I would instead like the commit message to tell me what the feature does and why it's useful. That includes explaining that it shows the commits that are reachable only by the specified bookmark. Also explain that that matches the semantics of hg export (and whatever other commands it was that I pointed you to last time). Thanks.

I don't know the why some of the tests are doing a time out.

Some of them are just really slow. I usually pass -t 600 to give them more time to finish.

martinvonz requested changes to this revision.Nov 16 2020, 1:08 PM
This revision now requires changes to proceed.Nov 16 2020, 1:08 PM
sebhtml added a comment.Nov 18 2020, 6:10 PM
In D9316#140172, @martinvonz wrote:

IMO, these three commits should be folded/squashed into a single commit. One way to think of it is this: if we decide to roll it back, would it be okay to roll back only one of them?
Also, most of the commit message is noise to me. You can safely remove all of it, IMO. I would instead like the commit message to tell me what the feature does and why it's useful. That includes explaining that it shows the commits that are reachable only by the specified bookmark. Also explain that that matches the semantics of hg export (and whatever other commands it was that I pointed you to last time). Thanks.

I don't know the why some of the tests are doing a time out.

Some of them are just really slow. I usually pass -t 600 to give them more time to finish.

Got it.

So... have a question, regarding the process to follow.

Since you recommend that I put my 3 patches in 1 commit, and that I rewrite the commit message as you guided, would it make sense for me to

In D9316#140172, @martinvonz wrote:

IMO, these three commits should be folded/squashed into a single commit. One way to think of it is this: if we decide to roll it back, would it be okay to roll back only one of them?
Also, most of the commit message is noise to me. You can safely remove all of it, IMO. I would instead like the commit message to tell me what the feature does and why it's useful. That includes explaining that it shows the commits that are reachable only by the specified bookmark. Also explain that that matches the semantics of hg export (and whatever other commands it was that I pointed you to last time). Thanks.

I don't know the why some of the tests are doing a time out.

Some of them are just really slow. I usually pass -t 600 to give them more time to finish.

Hello,

I addressed your comments, see my V3 here https://phab.mercurial-scm.org/D9341

pulkit edited reviewers, added: sebhtml; removed: pulkit.Dec 4 2020, 2:30 PM
pulkit commandeered this revision.

Okay seems functionality implemented by this and later patches got pushed as D9341. Moving this out of review.

pulkit abandoned this revision.Dec 4 2020, 2:30 PM

Revision Contents
Changeset List

PathPackages
Mmercurial/commands.py (6 lines)
Mtests/test-completion.t (2 lines)

Diff 23486

mercurial/commands.py

( (
b'b', b'b',
b'branch', b'branch',
[], [],
_(b'show changesets within the given named branch'), _(b'show changesets within the given named branch'),
_(b'BRANCH'), _(b'BRANCH'),
), ),
( (
b'B',
b'bookmark',
[],
_(b"show changesets within the given bookmark"),
_(b'BOOKMARK')),
(
b'P', b'P',
b'prune', b'prune',
[], [],
_(b'do not display revision or any of its ancestors'), _(b'do not display revision or any of its ancestors'),
_(b'REV'), _(b'REV'),
), ),
] ]
+ logopts + logopts

tests/test-completion.t

grep: print0, all, diff, text, follow, ignore-case, files-with-matches, line-number, rev, all-files, user, date, template, include, exclude grep: print0, all, diff, text, follow, ignore-case, files-with-matches, line-number, rev, all-files, user, date, template, include, exclude
heads: rev, topo, active, closed, style, template heads: rev, topo, active, closed, style, template
help: extension, command, keyword, system help: extension, command, keyword, system
identify: rev, num, id, branch, tags, bookmarks, ssh, remotecmd, insecure, template identify: rev, num, id, branch, tags, bookmarks, ssh, remotecmd, insecure, template
import: strip, base, secret, edit, force, no-commit, bypass, partial, exact, prefix, import-branch, message, logfile, date, user, similarity import: strip, base, secret, edit, force, no-commit, bypass, partial, exact, prefix, import-branch, message, logfile, date, user, similarity
incoming: force, newest-first, bundle, rev, bookmarks, branch, patch, git, limit, no-merges, stat, graph, style, template, ssh, remotecmd, insecure, subrepos incoming: force, newest-first, bundle, rev, bookmarks, branch, patch, git, limit, no-merges, stat, graph, style, template, ssh, remotecmd, insecure, subrepos
init: ssh, remotecmd, insecure init: ssh, remotecmd, insecure
locate: rev, print0, fullpath, include, exclude locate: rev, print0, fullpath, include, exclude
log: follow, follow-first, date, copies, keyword, rev, line-range, removed, only-merges, user, only-branch, branch, prune, patch, git, limit, no-merges, stat, graph, style, template, include, exclude log: follow, follow-first, date, copies, keyword, rev, line-range, removed, only-merges, user, only-branch, branch, bookmark, prune, patch, git, limit, no-merges, stat, graph, style, template, include, exclude
manifest: rev, all, template manifest: rev, all, template
merge: force, rev, preview, abort, tool merge: force, rev, preview, abort, tool
outgoing: force, rev, newest-first, bookmarks, branch, patch, git, limit, no-merges, stat, graph, style, template, ssh, remotecmd, insecure, subrepos outgoing: force, rev, newest-first, bookmarks, branch, patch, git, limit, no-merges, stat, graph, style, template, ssh, remotecmd, insecure, subrepos
parents: rev, style, template parents: rev, style, template
paths: template paths: template
phase: public, draft, secret, force, rev phase: public, draft, secret, force, rev
pull: update, force, confirm, rev, bookmark, branch, ssh, remotecmd, insecure pull: update, force, confirm, rev, bookmark, branch, ssh, remotecmd, insecure
push: force, rev, bookmark, branch, new-branch, pushvars, publish, ssh, remotecmd, insecure push: force, rev, bookmark, branch, new-branch, pushvars, publish, ssh, remotecmd, insecure