Page MenuHomePhabricator

New bookflow extension for bookmark-based branching
ClosedPublic

Authored by idlsoft on Aug 16 2018, 7:16 PM.

Details

Summary

This extension should be helpful for feature branches - based workflows.
At my company we first considered branches, but weren't sure about creating a lot of permanent objects.
We tried bookmarks, but found some scenarios to be difficult to control.

The main problem, was the active bookmark being moved on update.
Disabling that made everything a lot more predictable.

Bookmarks move on commit, and updating means switching between them.

The extension also implements a few minor features to better guide the workflow:

  • hg bookmark NAME can be ambiguous (create or move), unlike hg branch. The extension requires -rev to move.
  • require an active bookmark on commit.
  • some bookmarks can be protected (like @), useful for teams, that require code reviews.
  • block creation of new branches.

The initial implementation requires no changes in the core, but it does rely on some implementation details.
I thought it may be useful to discuss the functionality first, and then focus making the code more robust.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

idlsoft created this revision.Aug 16 2018, 7:16 PM
idlsoft edited the summary of this revision. (Show Details)Aug 17 2018, 12:40 AM
pulkit added a subscriber: pulkit.Aug 22 2018, 6:49 AM
pulkit added inline comments.
hgext/bookflow.py
26

can we get rid of this hack and just use 'bookflow'?

54

We also don't allow double new lines in core.

61

_() missing here. Also, we don't start words with capital letters in error messages.

71

please add r'' prefix to 'clean' and 'rev' for py3 compatibility.

72

_() missing here too.

tests/test-bookflow.t
3

You can enable the extension by putting [extensions] bookflow= in .hgrc. Aliasing does not looks neat.

I like the functionality it introduces and having this as an extension will be good.

idlsoft updated this revision to Diff 10506.Aug 22 2018, 10:12 AM
idlsoft updated this revision to Diff 10507.Aug 22 2018, 10:16 AM
idlsoft updated this revision to Diff 10508.Aug 22 2018, 11:56 AM
pulkit added a comment.Sep 1 2018, 5:51 PM

I have looked at the things more thoroughly and commented things inline. Certain things can be treated as opinions and it's perfectly fine to now fix them.

I found following cases missing in tests:

  • Testing of creation of bookmark name which already exists
  • Testing active bookmark is not moved on update (I might be wrong)

Also, mercurial core tests mainly test-check-code.t and test-check-commit.t fails with this patch. Please make them happy.

Also, the functionality this extension implemented is somewhat similar to https://www.mercurial-scm.org/wiki/BookmarkUpdatePlan.

hgext/bookflow.py
3

Can we add some help text here that this extension helps implement feature branches workflow using bookamrks by providing config knobs to change the default behavior.

7

Since the above two are enabled by default when we enabled this extension, they deserve a separate paragraph before the config options.

15

Can we switch the way help is described here. From 'cmdname: help' to

To create a new bookmark: hg book mark.

42

We generally use cannot instead of can't in core messages.

44

The Run: hg up {} part should be in the help text. Also run 'hg up {}' matches the core style more.

73

We can better say that "the active bookmark is updated to <commit>. use hg update to update to it."

83

This can be improved to "creating named branches is disabled and you should use bookmarks. see hg help bookmarks."

Also maybe since we are defining a workflow using this extension, documenting that workflow will be great!

88

We should move this couple of wrapfunctions call to uisetup() too.

90

If I understand correctly, the above couple of lines can be deleted?

tests/test-bookflow.t
3

I am not good at bash I am certain you make it simpler by removing the logic related to pwd, I am not sure why do we need that.

4

sorry, but is it like hg status?

30

output of push to /dev/null, that seems a bit weird, let's not do that.

111

I suggest printing the complete output of hg pull and remove the grep.

124

Opinion: these assert_clean and make_changes are not clean ways. Maybe the global -R flag can be used to commit without changing directories.

256

nit: -R flag can be used.

259

Can we just show the whole output. We are completely fine with more redundant lines.

272

Same, let's not push output to /dev/null.

276

Same, let's not push output to /dev/null.

I added some inline comments.
A test for hg book EXISTING is indeed missing, there is, however, one for not moving the bookmark on update, it's line 52: (leaving bookmark X).
Hopefully, I'll be able to make some changes later in the week.
Was not aware about the wiki page, this indeed seems to be addressing the same problem. At least that's how it started., and later evolved into something that helps you follow the workflow.

hgext/bookflow.py
15

I uppercased it to distinguish the parameter from the command.
I thought hg uses this convention:

hg bookmarks [OPTIONS]... [NAME]...
73

For me personally "out of sync" is more obvious, it underlines the fact that you won't be able to do commits.

83

Did you mean hg help bookflow?

90

They can, but I left them to make sure people aren't tempted to add a hook.

tests/test-bookflow.t
3

oh, that was done to see where the change was made, you'd have lines like:

test a
test a
test b
...
4

You're probably right :)

30

Yes, I do this in a few other places, when I don't care about the output, only the exit code.
It helps shorten the testcase to just the relevant pieces.

jwatt added a subscriber: jwatt.Sep 3 2018, 1:33 PM
idlsoft added inline comments.Sep 4 2018, 11:36 AM
hgext/bookflow.py
15

Also, this way it looks more like a cheatsheet

idlsoft updated this revision to Diff 10754.Sep 4 2018, 1:34 PM
idlsoft updated this revision to Diff 10789.Sep 5 2018, 10:04 AM
pulkit set the repository for this revision to rHG Mercurial.Sep 12 2018, 12:40 PM
pulkit accepted this revision.Sep 20 2018, 5:32 PM

The code looks mostly good. Since this is something which changes existing bookmark behavior, I will let one more pair of eyes to look before getting pushed.

I've poked some hg hosting providers to see if this would potentially improve bookmarks for their users. I intend to look at it today or Tuesday, with an eye towards landing it as part of 4.8.

marcink accepted this revision.Oct 15 2018, 6:12 AM
marcink added a subscriber: marcink.

This looks fine to me. Works very well with RhodeCode pull-requests based model with bookmark support.

At some point RhodeCode was checking if the destination bookmark was a descendant of the source, and not allowing such pull requests to be created.
That would have to be handled as a fast-forward, in other words just moving the destination bookmark.

If this is accepted we might want to look into changing the behavior of hg pull -u.
It should update the working directory only if the active bookmark was moved remotely.
I didn't find an easy way to do this without changes to core.

markand added inline comments.
idlsoft updated this revision to Diff 12149.Oct 15 2018, 9:48 AM
smf added a subscriber: smf.Oct 18 2018, 1:43 AM

Thanks for submitting your patch, it’s clearly a work of passion and I appreciate that. I know feature branches is a much sought-after behavior. However, I must express my objection to this direction in bookmarks. (apologies in advance)

I think the core issue at hand is the difference between data models. In particular, there are two huge differences:

  • mapping (one-to-many vs one-to-one) and
  • garbage collection

A bit of background: I've written many versions of this ref-like behavior over the years, including one during my time at Bitbucket. My experience with each of these extensions never felt right and I ended up throwing them all away.

As programmers, we like to abstract and create functional mappings around data. But we can't hack our way around the difference between a one-to-many relation vs a one-to-one relation. At a fundamental mathematical level, the two relations are not the same. For the last ten years, millions of Mercurial repos have been created that have (named) branches that are stored in the metadata. This is pretty ingrained in each developer's mind: one commit <-> one branch name. I think that changing this at this point would cause unnecessary frustration for our users.

For hosting sites like Bitbucket, this is even worse. Having two branching models is just impossible. A simple example is writing the branch api: should it return named branches? bookmarks? both? What happens when there is a call to create a branch? Create a branch? bookmark? both??? There are other rough edges as well: documentation, user tutorials, UI/UX, etc. The lessons I learned at Bitbucket helped me understand that core Mercurial should not recommend bookmarks as feature branches.

It doesn't stop there, though. Possibly the biggest misunderstanding is what and how anonymous heads work and why they don't disappear like they do in git. It is straight-up impossible to remove anonymous heads in Mercurial and trying to paper over them just makes things worse. I've tried assigning auto-generated bookmarks, auto-marking obsolescence markers, auto-hiding commits locally, etc; anything I could think of. Nothing ever really worked in my experience.

Perhaps the show stopper, though, is evolve. Much like Mercurial, evolve views the world through changesets. Changesets and their successors / predecessors are related by obsolescence markers and form a DAG, just like changesets of a repo are in a DAG. These relations have a starting point and an ending point and the most natural fit to that is, of course, storing the branch name in the actual changeset. When needing to evolve changsets, the only practical (perhaps even only possible) way to determine which branch it lived on is with named branches. Bookmarks / refs lose that information. Therefore, trying to shoehorn that model into evolve or even core is reinventing what branches already achieve.

Everything I tried in the bookmarks world fought against the core design principles of Mercurial. For those reasons, I think it is best to stick to one branching model and keep bookmarks (as much as possible) in third-party extensions (or if my dreams come true: in hg-git). As I've witnessed over the years, while at both Mercurial and Bitbucket, users will assume an implicit blessing for behavior when a feature is included in hgext. See for example, the nightmare of largefiles (and that need causing Facebook's rewrite in lfs), eol, keyword, etc, etc. We can put as many "features of last resort" warnings that we want but the ultimate protection we have to save ourselves the headache is to keep things out of hgext.

All of this is not to say I don't want feature branches in Mercurial, I do. It's something Erik and I have thought a lot about and we both want to improve on (named) branches that allow feature workflows and work better with phases and evolve. I have even worked over the last few months on this exact approach and, on my side, I will use this as a fire to get my own work out the door. I would welcome and appreciate any kind of help in planning, coding, or even just spitballing.

First of all, thank you for reviewing the patch.

We switched to mercurial a few months ago, mainly because of the narrow extension. Feature branches workflow was something everyone in the company understood and adhered to, so trying to adopt something else wouldn't be practical.
Besides it was unclear what that something would be. Short lived branches were not recommended, evolve and topics aren't in core and take a while to wrap your mind around. Besides whatever we picked, it would need to be fully supported by Teamcity and IntelliJ. And Rhodecode.
Writing a proprietary extension wasn't our first choice, not by a long shot. But at some point it was just the lesser evil.

I see a lot of people focus on the DAG aspect of the repo, which for a core DVCS developer makes a lot of sense. You also mentioned anonymous heads, but this is not what we were trying to solve at all.
In fact, as we realized at some point, tip and heads weren't very consequential for our workflow, but maintaining DAG references reliably was.
A common problem we encountered was somebody starting their work, and before committing anything doing an hg pull -u. This would then move their bookmark to someone else's changes.
To this day I can't quite see why it would make sense to move bookmarks on update. What was the original workflow that lead to that design?

Basing a solution on bookmarks seemed like the least intrusive option. It would be client-side only, the only thing the server needs to do is protect certain bookmarks via the acl extension.
Tools already support bookmarks, they just need to be more predictable.

That's what the extension does. Really, if there was a config setting to not move bookmarks on update, I'm not sure we would have written anything at all (well, the fact that hg bookmark NAME can either create or move a bookmark is also not great).
All the other functionality is trivial, to help guide the workflow and avoid confusion. We don't autocreate bookmarks, we just make sure you have one when you commit.

While I understand the concern of endorsing too many branching models, I don't see this extension as introducing anything drastically new.
It just addresses issues in the default bookmark behavior, which made them unsuitable for a pretty established workflow.
Whether or not this is approved, I hope that can be revisited in core, perhaps made configurable.

First of all, thank you for reviewing the patch.
We switched to mercurial a few months ago, mainly because of the narrow extension.

Unrelated, do you use narrow with ellipses or without ellipses? Also I am sorry to say but in this cycle, narrow extension has under gone a lot of perf and correctness improvements and it won't be backward compatible in upcoming release. I will be writing some text about this in the releasenotes too, and you can contact me personally and I will be glad to help you with the BC changes.

pulkit set the repository for this revision to rHG Mercurial.Oct 18 2018, 1:36 PM
In D4312#77084, @pulkit wrote:

Unrelated, do you use narrow with ellipses or without ellipses? Also I am sorry to say but in this cycle, narrow extension has under gone a lot of perf and correctness improvements and it won't be backward compatible in upcoming release. I will be writing some text about this in the releasenotes too, and you can contact me personally and I will be glad to help you with the BC changes.

Hmm, I thought we did, but no, just this:

[experimental]
changegroup3=true

I'm actually kinda looking forward to see if lfs works better with all the new changes.

... Even more unrelated, IntelliJ 2018.3 will have a branch/bookmark compare dialog similar to the one for git.

I'm actually kinda looking forward to see if lfs works better with all the new changes.

Were there particular pain points before? The list of things to polish isn't short, but I don't mind reprioritizing things if it helps.

Were there particular pain points before? The list of things to polish isn't short, but I don't mind reprioritizing things if it helps.

I submitted a bug report https://bz.mercurial-scm.org/show_bug.cgi?id=5794
It has a zip file with the repo, although to be honest I'm testing with another repo locally. As of 4.7.2 I'm still seeing that stack trace

@smf I just noticed your name on https://www.mercurial-scm.org/wiki/BookmarkUpdatePlan, which puts your comments into a larger context.
This would definitely be an improvement, and reduce the scope of what this extension does.
Would you consider also addressing the hg bookmark NAME doing two very different things depending on the bookmark already existing?

smf added a comment.Oct 26 2018, 7:16 PM

@smf I just noticed your name on https://www.mercurial-scm.org/wiki/BookmarkUpdatePlan, which puts your comments into a larger context.
This would definitely be an improvement, and reduce the scope of what this extension does.

I think that page has a typo: it should be "discussed at the London sprint." Ryan and I were quite eager to change this behavior of not moving bookmarks. It was one of the experiments I mentioned in my previous comment. Over the next year or so, I learned there was a big difference between Ryan's use-case (the monorepo) and the everyday repo (average repo on Bitbucket): having one (and only one) centrally controlled repo is easy to set a policy like "don't move bookmarks."

In the wild, there are tons of unknown variables of how people are currently relying on the behavior; in particular, scripts. In fact, I have a few scripts that do rely on this behavior. Changing that at this point (by default, without a flag) is ill-advised in my opinion.

Would you consider also addressing the hg bookmark NAME doing two very different things depending on the bookmark already existing?

At this point, I would prefer not to change default behavior for similar reasons as above.

There's been some good discussion on this. I'm sympathetic to both arguments here, namely: "we could improve bookmarks and make them less bad" and "bookmarks are a dead end and nobody should use them and we shouldn't improve them" (or thereabouts - I'm summarizing complicated positions to less than a sentence, so bear with me.) I think not following up on our plans to at least make plausible incremental improvements to bookmarks serves our users poorly, and this extension merits landing as an experimental extension. We can always spin it back out if we get unhappy with it.

I'm going to land this patch largely as-is on default, with the following tweaks: I'm adding (EXPERIMENTAL) to the docstring of the extension so we can iterate its behavior, some misc. check-code fixes, tweak the commit message slightly to make check-commit happy.

Thanks! Let's try and collect thoughts with @krbullock at some point on what of this behavior should land where.

This revision was automatically updated to reflect the committed changes.
evzijst added a subscriber: evzijst.Dec 3 2018, 4:03 PM

There's been some good discussion on this. I'm sympathetic to both arguments here, namely: "we could improve bookmarks and make them less bad" and "bookmarks are a dead end and nobody should use them and we shouldn't improve them" (or thereabouts - I'm summarizing complicated positions to less than a sentence, so bear with me.) I think not following up on our plans to at least make plausible incremental improvements to bookmarks serves our users poorly, and this extension merits landing as an experimental extension. We can always spin it back out if we get unhappy with it.

I was asked to provide some context from the perspective of Bitbucket. We and our users have been struggling with bookmarks and adoption has remained extremely low since support was added ~8(?) years ago. A complicating factor on Bitbucket is that it's proven hard to do a good job at supporting both bookmarks and named branches as first class citizens, especially in pull requests and branch contexts. We feel that without pull request support it's hard to claim Bitbucket offers bookmark workflows.

Because of the low adoption of bookmarks and backend complexity, we've decided to officially drop bookmark support. They can of course still be pushed up, but they won't show up under branches anymore and we'll be guiding users towards a named branch workflow. We should be publishing a statement about that soon.

smf added a comment.Dec 3 2018, 7:57 PM

There's been some good discussion on this. I'm sympathetic to both arguments here, namely: "we could improve bookmarks and make them less bad" and "bookmarks are a dead end and nobody should use them and we shouldn't improve them" (or thereabouts - I'm summarizing complicated positions to less than a sentence, so bear with me.) I think not following up on our plans to at least make plausible incremental improvements to bookmarks serves our users poorly, and this extension merits landing as an experimental extension. We can always spin it back out if we get unhappy with it.

Obviously, I can't say I'm too happy with this. Allowing users to shoot themselves in the foot even more is pretty bad.

I'm going to land this patch largely as-is on default, with the following tweaks: I'm adding (EXPERIMENTAL) to the docstring of the extension so we can iterate its behavior, some misc. check-code fixes, tweak the commit message slightly to make check-commit happy.

When I bring up community issues (including at this sprint) I thought this project was more than just one person. Having to constantly put up the good fight is completely negated if no one is going to listen and just ship this anyways. Bookmark related features belong as a third-party extension just like hgsubversion, hg-git, etc.

In D4312#79573, @smf wrote:

There's been some good discussion on this. I'm sympathetic to both arguments here, namely: "we could improve bookmarks and make them less bad" and "bookmarks are a dead end and nobody should use them and we shouldn't improve them" (or thereabouts - I'm summarizing complicated positions to less than a sentence, so bear with me.) I think not following up on our plans to at least make plausible incremental improvements to bookmarks serves our users poorly, and this extension merits landing as an experimental extension. We can always spin it back out if we get unhappy with it.

Obviously, I can't say I'm too happy with this. Allowing users to shoot themselves in the foot even more is pretty bad.

That ship has sailed: bookmarks exist, and are more visible than features like rebase.

I'm going to land this patch largely as-is on default, with the following tweaks: I'm adding (EXPERIMENTAL) to the docstring of the extension so we can iterate its behavior, some misc. check-code fixes, tweak the commit message slightly to make check-commit happy.

When I bring up community issues (including at this sprint) I thought this project was more than just one person. Having to constantly put up the good fight is completely negated if no one is going to listen and just ship this anyways. Bookmark related features belong as a third-party extension just like hgsubversion, hg-git, etc.

...it is? You're the only voice of objection that I'm seeing. I see some mostly indifference from BitBucket and some pretty good enthusiasm from the contributor, rhodecode, Kevin, etc. And it's experimental, so we can dump it if I've made the wrong choice. If I've missed some other /objections/ (as contrasted with what I perceive to be indifference - maybe I'm misreading Erik?) please point out my error.

Obviously, I can't say I'm too happy with this. Allowing users to shoot themselves in the foot even more is pretty bad.

I don't think that's fair.
Everyone's experience is different, but it did precisely the opposite for us.

When we switched from git we couldn't find an official recommendation on how to do feature branches. Not in core.
Using short-lived branches is discouraged, and bookmarks don't do a good job for all the reasons I mentioned before.

For better or worse this is a solution, and after using it for a few months, I can at least say it works very well for us.
It was easy for everyone on the team to understand, and all our JetBrains tools work seamlessly.
Can it be done better? Sure, just like everything else. In fact, let's.

But whatever the implementation, I believe a recipe for feature branches does belong in core, only then you can hope to get some support from hosting tools.

On a completely unrelated note: having to explain the concept of multiple heads on a branch to a mortal sucks.

smf added a comment.Dec 6 2018, 5:39 PM
In D4312#79573, @smf wrote:

There's been some good discussion on this. I'm sympathetic to both arguments here, namely: "we could improve bookmarks and make them less bad" and "bookmarks are a dead end and nobody should use them and we shouldn't improve them" (or thereabouts - I'm summarizing complicated positions to less than a sentence, so bear with me.) I think not following up on our plans to at least make plausible incremental improvements to bookmarks serves our users poorly, and this extension merits landing as an experimental extension. We can always spin it back out if we get unhappy with it.

Obviously, I can't say I'm too happy with this. Allowing users to shoot themselves in the foot even more is pretty bad.

That ship has sailed: bookmarks exist, and are more visible than features like rebase.

I'm going to land this patch largely as-is on default, with the following tweaks: I'm adding (EXPERIMENTAL) to the docstring of the extension so we can iterate its behavior, some misc. check-code fixes, tweak the commit message slightly to make check-commit happy.

When I bring up community issues (including at this sprint) I thought this project was more than just one person. Having to constantly put up the good fight is completely negated if no one is going to listen and just ship this anyways. Bookmark related features belong as a third-party extension just like hgsubversion, hg-git, etc.

...it is? You're the only voice of objection that I'm seeing. I see some mostly indifference from BitBucket and some pretty good enthusiasm from the contributor, rhodecode, Kevin, etc. And it's experimental, so we can dump it if I've made the wrong choice. If I've missed some other /objections/ (as contrasted with what I perceive to be indifference - maybe I'm misreading Erik?) please point out my error.

I hung out with Erik yesterday and he said he was too frustrated and tired of explaining himself to reply to this thread. I can't say I really blame him. The previous discussion outlines why having (and encouraging) two branching models is bad for the ecosystem. This is based on Erik and mine’s years of experience working on Bitbucket.

The more important and pressing issues are exchanging obs markers and improving named branching. A cash donation in those directions has gone unused and it is frustrating that the weight of Bitbucket’s experience and resources has gone ignored.

This will not help the *average* user and sends a mixed (and dangerous) message that bookmarks should be used. For a team that truly wants this feature, hosting this extension as a package on pypi is the best solution.

In D4312#79938, @smf wrote:

This will not help the *average* user and sends a mixed (and dangerous) message that bookmarks should be used.

Bookmarks have been in core for some time now, and there is not one mention anywhere that they are not to be used.
They may not be trivial to use but it's certainly not officially discouraged anywhere.
Bookmarks is the first thing that comes up when you search for "mercurial light branching", although it's from a blog post, not the official wiki.
Meanwhile the official wiki plainly states that light branching is a different abstraction, and branches should not be used for for that.

So what IS the recommended way then?
Is there one?

I may have a horse in this race but I'd be more than happy to see it lose to a better one.