Page MenuHomePhabricator

sebhtml (Sébastien Boisvert)
User

Projects

User does not belong to any projects.

User Details

User Since
Aug 30 2020, 9:52 PM (21 w, 23 h)

Recent Activity

Nov 23 2020

sebhtml added a comment to D9341: log: add bookmark option to "hg log".

Do test-lfs-serve.t and test-transaction-rollback-on-sigpipe.t consistently fail with this patch? If they do, you should fix them :) If they don't (i.e. it was some flakiness you were experiencing), just don't mention that in the commit message and instead ask on #mercurial or send an email to mercurial-devel to see if others have seen the flakiness and ask how to fix it.

Nov 23 2020, 7:14 PM
sebhtml closed D9341: log: add bookmark option to "hg log".
Nov 23 2020, 6:52 PM
sebhtml committed rHG0aa118f18d4b: log: add bookmark option to "hg log".
log: add bookmark option to "hg log"
Nov 23 2020, 6:51 PM
sebhtml added a comment to D9341: log: add bookmark option to "hg log".

The test failure has a diff. I still think that's just noise that will confuse much more than it helps. Okay if I drop the test plan from the commit message in flight?

Nov 23 2020, 6:21 PM
sebhtml added a comment to D9341: log: add bookmark option to "hg log".

That's what I meant: you need to indent diffs that you include in the commit message, otherwise tools can't figure out where the commit message ends and the diff starts. But I still think you can just remove that whole thing from the commit message; it's implied that the test plan is to run the test suite, so you only need to describe your test plan if you did some other testing and/or did not run the test suite.

Nov 23 2020, 5:22 PM
sebhtml updated the test plan for D9341: log: add bookmark option to "hg log".
Nov 23 2020, 5:22 PM
sebhtml added a comment to D9341: log: add bookmark option to "hg log".

Also, including diff output (such as from failing tests) without indentation makes the patch fail to apply:
unable to find 'tank/home/sebhtml/projects/mercurial/sebhtml-hg-log-bookmark/hg-committed/tests/test-lfs-serve.t' for patching

Nov 23 2020, 12:40 PM
sebhtml added a comment to D9341: log: add bookmark option to "hg log".

Do test-lfs-serve.t and test-transaction-rollback-on-sigpipe.t consistently fail with this patch? If they do, you should fix them :) If they don't (i.e. it was some flakiness you were experiencing), just don't mention that in the commit message and instead ask on #mercurial or send an email to mercurial-devel to see if others have seen the flakiness and ask how to fix it.

Nov 23 2020, 12:35 PM

Nov 18 2020

sebhtml updated the summary of D9341: log: add bookmark option to "hg log".
Nov 18 2020, 6:15 PM
sebhtml added a comment to D9316: log: add argument -B/--bookmark for bookmarks.

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.

Nov 18 2020, 6:10 PM
sebhtml updated the test plan for D9341: log: add bookmark option to "hg log".
Nov 18 2020, 6:07 PM
sebhtml created D9341: log: add bookmark option to "hg log".
Nov 18 2020, 5:59 PM

Nov 12 2020

sebhtml added reviewers for D9315: test: test 'hg log' using a bookmark: martinvonz, pulkit, marmoute.
Nov 12 2020, 8:21 PM
sebhtml added reviewers for D9317: log: use scmutil.bookmarkrevs to get the revisions of a bookmark: martinvonz, pulkit, marmoute.
Nov 12 2020, 8:20 PM
sebhtml added a reviewer for D9316: log: add argument -B/--bookmark for bookmarks: marmoute.
Nov 12 2020, 8:20 PM
sebhtml updated the summary of D9316: log: add argument -B/--bookmark for bookmarks.
Nov 12 2020, 8:17 PM
sebhtml updated the summary of D9316: log: add argument -B/--bookmark for bookmarks.
Nov 12 2020, 8:15 PM
sebhtml created D9317: log: use scmutil.bookmarkrevs to get the revisions of a bookmark.
Nov 12 2020, 8:12 PM
sebhtml created D9316: log: add argument -B/--bookmark for bookmarks.
Nov 12 2020, 8:12 PM
sebhtml created D9315: test: test 'hg log' using a bookmark.
Nov 12 2020, 8:08 PM

Nov 11 2020

sebhtml added a comment to D8973: log: add support for bookmarks.

As I said before, hg export, hg email, hg strip all rely on scmutil.bookmarkrevs(). I don't see how you get the same semantics with the current revset.
This patch was queued and then dropped. I don't think Phabricator has a way of indicating that. I think you'll have to send a new patch (with a new D-number) if you want to pursue this.

Nov 11 2020, 11:50 AM

Nov 8 2020

sebhtml added a comment to D8973: log: add support for bookmarks.

Marking as "change requested" since Pulkit (righfully) requested for a test case.

Nov 8 2020, 12:23 PM

Oct 3 2020

sebhtml added a comment to D8973: log: add support for bookmarks.

For the record, I'm -1 on this, for the following reasons:

Comparing with hg push/pull doesn't seem right. Those command *have to* exchange ancestors (e.g. hg push -r abc123 doesn't exchange just abc123). Let's look for other examples. Oh, hg export has a -B flag (I had no idea). Interestingly, it turns out to use semantics similar to what my guess about your hg log -B was :) It apparently relies no scmutil.bookmarkrevs(). Looking at other callers of that function, I found that hg email and hg strip also have -B options with the same semantics. I'd be much more comfortable taking a patch that used the same semantics for hg log -B. It's a little unfortunate that that won't match the bookmark() revset, but that's still much better than adding a third way of matching revisions (I count hg push/pull -B as matching the revset semantics of giving you only the commits pointed to).

Oct 3 2020, 10:09 AM
sebhtml added a comment to D8973: log: add support for bookmarks.

Folded next couple of patches in this one and queued. Thank you!

Oct 3 2020, 10:06 AM

Sep 26 2020

sebhtml added a comment to D8973: log: add support for bookmarks.

For the record, I'm -1 on this, for the following reasons:

Sep 26 2020, 11:47 AM

Sep 24 2020

sebhtml closed D8973: log: add support for bookmarks.
Sep 24 2020, 3:36 AM
sebhtml committed rHG2517df770648: log: add support for bookmarks.
log: add support for bookmarks
Sep 24 2020, 3:35 AM

Sep 19 2020

sebhtml created D9061: test: add bookmark for 'log' in test-completion.
Sep 19 2020, 11:35 AM
sebhtml added a comment to D9060: test: test 'hg log' using a bookmark.

I also ran all the tests.

Sep 19 2020, 10:08 AM
sebhtml added a comment to D9060: test: test 'hg log' using a bookmark.

It seems to work as expected:

Sep 19 2020, 9:20 AM
sebhtml added a comment to D8973: log: add support for bookmarks.

I added a test here https://phab.mercurial-scm.org/D9060

Sep 19 2020, 9:19 AM
sebhtml created D9060: test: test 'hg log' using a bookmark.
Sep 19 2020, 9:17 AM

Sep 3 2020

sebhtml added a comment to D8973: log: add support for bookmarks.

Thanks for the patch. Can you add some test case for this?

Sep 3 2020, 9:01 PM

Aug 30 2020

sebhtml created D8973: log: add support for bookmarks.
Aug 30 2020, 10:07 PM