In D9341#140899, @martinvonz wrote:

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.

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?

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.

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

In D9341#140899, @martinvonz wrote:

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.

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.

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.

In D8973#134941, @marmoute wrote:

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

In D8973#136734, @martinvonz wrote:

In D8973#136435, @sebhtml wrote:

In D8973#136267, @martinvonz wrote:

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

In D8973#136174, @pulkit wrote:

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

In D8973#136267, @martinvonz wrote:

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

I also ran all the tests.

It seems to work as expected:

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

In D8973#134422, @pulkit wrote:

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