Page MenuHomePhabricator

log: add support for bookmarks
ClosedPublic

Authored by sebhtml on Aug 30 2020, 10:06 PM.

Details

Summary

A bookmark can already be pushed with 'hg push origin -B topic-foo'.
The pull command also supports bookmarks.

This patch adds support for bookmarks using the '_opt2logrevset' code path.

The list of changesets for a bookmark can now be obtained simply with:
'hg log -B topic-foo'.

Reviewed-by: Elenie Godzaridis <arangradient@gmail.com>

Diff Detail

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

Event Timeline

sebhtml created this revision.Aug 30 2020, 10:06 PM
pulkit added a subscriber: pulkit.Sep 2 2020, 8:04 AM

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

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

Hi, where should i add the test case for this ?

pulkit added a comment.Sep 7 2020, 2:53 AM

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

Hi, where should i add the test case for this ?

tests/test-log.t seems like a good candidate.

https://www.mercurial-scm.org/wiki/WritingTests should help in writing tests.

marmoute requested changes to this revision.Sep 11 2020, 4:18 AM
marmoute added a subscriber: marmoute.

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

This revision now requires changes to proceed.Sep 11 2020, 4:18 AM
pulkit accepted this revision.Sep 24 2020, 2:59 AM

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

This revision was not accepted when it landed; it landed in state Needs Revision.
This revision was automatically updated to reflect the committed changes.
martinvonz added a subscriber: martinvonz.EditedSep 24 2020, 2:24 PM

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

  • We have revsets in order to avoid adding flags to hg log and other commands. However, I think it makes sense to have flags for the most common options (-k and -u are the ones I use most). I'm not sure -B will be common enough.
  • The semantics are unintuitive. When I first saw it, I assumed it would show just the bookmarked commit itself. When I saw some of the tests and saw that it also showed other commits, I figured it must be showing only($bookmark) or maybe only($bookmark, $all_other_bookmarks). It instead seems to do the same as hg log --follow $bookmark
  • We have a bookmark() revset, which matches exactly the bookmarked commit, not including ancestors. It seems unfortunate for that revset and this new flag to disagree.

PS. I'm sorry I didn't see this patch until now :(

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

  • We have revsets in order to avoid adding flags to hg log and other commands. However, I think it makes sense to have flags for the most common options (-k and -u are the ones I use most). I'm not sure -B will be common enough.
  • The semantics are unintuitive. When I first saw it, I assumed it would show just the bookmarked commit itself. When I saw some of the tests and saw that it also showed other commits, I figured it must be showing only($bookmark) or maybe only($bookmark, $all_other_bookmarks). It instead seems to do the same as hg log --follow $bookmark
  • We have a bookmark() revset, which matches exactly the bookmarked commit, not including ancestors. It seems unfortunate for that revset and this new flag to disagree.

Thanks for the explanation, it convinced me also that this may not be a good idea. Dropping this for now.

@sebhtml can you explain a bit more why existing revsets and hg log --follow $bookmark does not work for you?

PS. I'm sorry I didn't see this patch until now :(

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

Hi @martinvonz, thanks for taking the time to write your comments.

I used Mercurial at work and the -B option is to be able to work with non-permanent branches, like it is done in Git.

  • We have revsets in order to avoid adding flags to hg log and other commands.

Before adding the -B option in "hg log", I was using the following revset:

$ hg log -r "reverse(ancestors(bookmark(sebhtml/2020-08-30-hg-log-with-bookmark)))"

But, it was time-consuming to write, so I decided to contribute something back to the upstream Mercurial.

However, I think it makes sense to have flags for the most common options (-k and -u are the ones I use most). I'm not sure -B will be common enough.

The -B option is already common in Mercurial.
The commands "hg push" and "hg pull" both have the -B option for bookmarks already.

I just don't see why "hg log" can not have it too...

  • The semantics are unintuitive. When I first saw it, I assumed it would show just the bookmarked commit itself. When I saw some of the tests and saw that it also showed other commits, I figured it must be showing only($bookmark) or maybe only($bookmark, $all_other_bookmarks). It instead seems to do the same as hg log --follow $bookmark

To get the commit itself, this is the correct command: hg log -r $bookmark .

The semantics of "hg log -B $bookmark" is the same as the existing semantics for
"hg push -B $bookmark" and "hg pull -B $bookmark".

I did not know about --follow. That's interesting,

  • We have a bookmark() revset, which matches exactly the bookmarked commit, not including ancestors. It seems unfortunate for that revset and this new flag to disagree.

bookmark() gives me only the "HEAD" commit of the bookmark, which is not the goal here...

PS. I'm sorry I didn't see this patch until now :(

That's ok.

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

Hi @martinvonz, thanks for taking the time to write your comments.
I used Mercurial at work and the -B option is to be able to work with non-permanent branches, like it is done in Git.

  • We have revsets in order to avoid adding flags to hg log and other commands.

Before adding the -B option in "hg log", I was using the following revset:
$ hg log -r "reverse(ancestors(bookmark(sebhtml/2020-08-30-hg-log-with-bookmark)))"

I think hg log -r "reverse(::sebhtml/2020-08-30-hg-log-with-bookmark)" should be sufficient (bookmark() is only necessary if you need to disambiguate it from e.g. a branch name called sebhtml/2020-08-30-hg-log-with-bookmark, and :: is the shorter way of saying ancestors()). Also, if you use hg log -G to get a graphical log (I pretty much always do), then the reverse() is also unnecessary because it orders the commits in reverse order regardless. So hg log -G -r ::sebhtml/2020-08-30-hg-log-with-bookmark is what I would use.

But, it was time-consuming to write, so I decided to contribute something back to the upstream Mercurial.

Thanks!

However, I think it makes sense to have flags for the most common options (-k and -u are the ones I use most). I'm not sure -B will be common enough.

The -B option is already common in Mercurial.
The commands "hg push" and "hg pull" both have the -B option for bookmarks already.
I just don't see why "hg log" can not have it too...

  • The semantics are unintuitive. When I first saw it, I assumed it would show just the bookmarked commit itself. When I saw some of the tests and saw that it also showed other commits, I figured it must be showing only($bookmark) or maybe only($bookmark, $all_other_bookmarks). It instead seems to do the same as hg log --follow $bookmark

To get the commit itself, this is the correct command: hg log -r $bookmark .
The semantics of "hg log -B $bookmark" is the same as the existing semantics for
"hg push -B $bookmark" and "hg pull -B $bookmark".

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

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

Hi, when will this land in https://www.mercurial-scm.org/repo/hg ?

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

Hello,

After reading your post, I am not sure not if this means that my patch is approved or rejected.

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

Hello,
After reading your post, I am not sure not if this means that my patch is approved or rejected.

Sorry, I understand that the terminology is not obvious to new contributors. It was first accepted ("landed" and "Closed by commit rHG2517df770648" both indicate that), but then it was dropped before it became public (see hg help phases if you're not familiar with the term "public" in this context).

I'm not sure it's worth it to add a -B flag to hg log, but if we do, I think that it should use the same semantics as hg export, hg email, and hg strip. I don't know what other reviewers think.

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

Hello @marmoute @martinvonz @pulkit,

Regarding the current review https://phab.mercurial-scm.org/D8973 :

From my point of view, the -B option (for bookmarks) is already available in: hg pull, hg push, hg export, hg strip, hg email.

My patch series adds the -B to "hg log", with the same semantics found in the 5 Mercurial commands above.

All I can see is, at the top of the page, that @marmoute has a red "X".

Am I missing some information ?

Thank you

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

Hello @marmoute @martinvonz @pulkit,
Regarding the current review https://phab.mercurial-scm.org/D8973 :

From my point of view, the -B option (for bookmarks) is already available in: hg pull, hg push, hg export, hg strip, hg email.
My patch series adds the -B to "hg log", with the same semantics found in the 5 Mercurial commands above.

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.

All I can see is, at the top of the page, that @marmoute has a red "X".
Am I missing some information ?
Thank you

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.

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.

OK. That makes sense now.

I will start from scratch, as https://phab.mercurial-scm.org/D8973 is not adequate as you explained.

As for my 2 patches for tests https://phab.mercurial-scm.org/D9061 and https://phab.mercurial-scm.org/D9060 , will I have to resubmit them separately ?

I have used GitHub, GitLab and Review Board for reviews, and I don't know exactly how this is done in Phabricator.

Thank you for your time and clarification.

I will start from scratch, as https://phab.mercurial-scm.org/D8973 is not adequate as you explained.

Sounds good. Thanks.

As for my 2 patches for tests https://phab.mercurial-scm.org/D9061 and https://phab.mercurial-scm.org/D9060 , will I have to resubmit them separately ?

You can reuse them. However, we prefer to have tests in the same commit as the code you're testing, so I think you should only need one patch/review.