Patchwork [2,of,2] bookmarks: on a bare merge use config to choose target or choose @

login
register
mail settings
Submitter Stephen Lee
Date March 11, 2014, 11:20 a.m.
Message ID <44db1ede5c65a0265203.1394536811@slee-desktop>
Download mbox | patch
Permalink /patch/3911/
State Deferred
Headers show

Comments

Stephen Lee - March 11, 2014, 11:20 a.m.
# HG changeset patch
# User Stephen Lee <sphen.lee@gmail.com>
# Date 1394536638 -39600
#      Tue Mar 11 22:17:18 2014 +1100
# Node ID 44db1ede5c65a0265203f5df063049813d2da8c8
# Parent  ff0ffb9855d8e88fbcce967867f2a373eaac581f
bookmarks: on a bare merge use config to choose target or choose @

When running "hg merge" if there is an active and current bookmark
look in config for a key "bookmarks.<name>.track" and it will name
the bookmark we should merge with.  If the key is missing, and an
@ bookmark exists, merge with it instead.

This is only performed after checking for divergent bookmarks which
are given preference when merging.
Matt Mackall - March 11, 2014, 10:16 p.m.
On Tue, 2014-03-11 at 22:20 +1100, Stephen Lee wrote:
> # HG changeset patch
> # User Stephen Lee <sphen.lee@gmail.com>
> # Date 1394536638 -39600
> #      Tue Mar 11 22:17:18 2014 +1100
> # Node ID 44db1ede5c65a0265203f5df063049813d2da8c8
> # Parent  ff0ffb9855d8e88fbcce967867f2a373eaac581f
> bookmarks: on a bare merge use config to choose target or choose @
> 
> When running "hg merge" if there is an active and current bookmark
> look in config for a key "bookmarks.<name>.track" and it will name
> the bookmark we should merge with.  If the key is missing, and an
> @ bookmark exists, merge with it instead.

I have no idea what the rationale here is, but it sounds like a behavior
change that's going to cause me to accidentally do horrible horrible
things like merge @ into stable without realizing it. This is worrisome.

Also, this sort of patch should probably be split into two patches:

- create surprise merge accidents
- do some mysterious tracking thing
Stephen Lee - March 12, 2014, 9:27 a.m.
On Wed, Mar 12, 2014 at 9:16 AM, Matt Mackall <mpm@selenic.com> wrote:
> On Tue, 2014-03-11 at 22:20 +1100, Stephen Lee wrote:
>> # HG changeset patch
>> # User Stephen Lee <sphen.lee@gmail.com>
>> # Date 1394536638 -39600
>> #      Tue Mar 11 22:17:18 2014 +1100
>> # Node ID 44db1ede5c65a0265203f5df063049813d2da8c8
>> # Parent  ff0ffb9855d8e88fbcce967867f2a373eaac581f
>> bookmarks: on a bare merge use config to choose target or choose @
>>
>> When running "hg merge" if there is an active and current bookmark
>> look in config for a key "bookmarks.<name>.track" and it will name
>> the bookmark we should merge with.  If the key is missing, and an
>> @ bookmark exists, merge with it instead.
>
> I have no idea what the rationale here is, but it sounds like a behavior
> change that's going to cause me to accidentally do horrible horrible
> things like merge @ into stable without realizing it. This is worrisome.

I will try to explain the rationale (sorry, these two patches should
have been marked RFC).

Firstly this patch is the less important of the two - the update patch
is fixing a real functionality hole.
This patch was mostly to make the same changes to merge as I made to update.

Currently there is no way to move the active bookmark to a descendant
rev, and update to that rev in a single command _unless that rev
happens to be tip_.
(What Git calls a fast-forward merge).
This is a common thing to want to do in a bookmark-as-feature-branch workflow.
It is a sore point for Git people coming to Mercurial and causes
confusion for beginners: "hg merge @" Just-Works, but "hg up @"
usually is not what you meant...

My idea is to allow a per-bookmark config setting to name the bookmark
that should be used as the target of a bare "hg up".
I called it a 'tracking' bookmark (which is also the term Git uses)
because it implies that your bookmark is 'following behind' some other
bookmark.

A secondary idea is to assume @ if the setting is missing.
In a bookmark workflow where @ is the mainline branch this is a
sensible default, and much less confusing that the current behaviour.
Also @ is already used as the update target of a clone.

In a non-bookmark workflow, the new logic only triggers if you have an
active bookmark and an @ bookmark (which should be rare in a
non-bookmark workflow).

The logic is this patch (for merge) is the same. If you have an active
bookmark and run a bare merge, currently it will abort unless there is
a divergent bookmark.
In this patch it will select the 'tracked' bookmark, or @.
If you have no active bookmark there is no behaviour change (it won't
magically merge @ into stable).

>
> Also, this sort of patch should probably be split into two patches:
>
> - create surprise merge accidents
> - do some mysterious tracking thing

I can split the patch apart if you would prefer - but before that I
would like to get comments/suggestions from other contributors about
the entire idea.
There were lots of people commenting on the thread last time I brought
up this issue!
The only conclusion I came to was "make a patch and go from there".

Steve
Jordi Gutiérrez Hermoso - March 18, 2014, 2:05 p.m.
On Wed, 2014-03-12 at 20:27 +1100, Stephen Lee wrote:

> In a non-bookmark workflow, the new logic only triggers if you have an
> active bookmark and an @ bookmark (which should be rare in a
> non-bookmark workflow).

It seems to me that you're failing to account the situation of both
bookmarks and named branches being used intentionally, which is a
common thing to do with Mercurial (named branches aren't anathema).
When on a named branch, `hg update` keeps you on that branch and moves
you to it head, if unique. I think this is what mpm is complaining
about, what if you're on the stable named branch with no active
bookmark?

- Jordi G. H.
Stephen Lee - March 21, 2014, 3:29 a.m.
On Wed, Mar 19, 2014 at 1:05 AM, Jordi Gutiérrez Hermoso
<jordigh@octave.org> wrote:
> On Wed, 2014-03-12 at 20:27 +1100, Stephen Lee wrote:
>
>> In a non-bookmark workflow, the new logic only triggers if you have an
>> active bookmark and an @ bookmark (which should be rare in a
>> non-bookmark workflow).
>
> It seems to me that you're failing to account the situation of both
> bookmarks and named branches being used intentionally, which is a
> common thing to do with Mercurial (named branches aren't anathema).
> When on a named branch, `hg update` keeps you on that branch and moves
> you to it head, if unique. I think this is what mpm is complaining
> about, what if you're on the stable named branch with no active
> bookmark?

As I said before if you have no active bookmark, there is no behaviour
change - you will end up on the tip of the current branch.
If you are using bookmarks with branches then that usually makes no
sense - which branch head is the 'real' head and which are feature
heads? The 'real' head is often not even at a branch head!

Also note that if the destination selected by the new logic is not a
descendent, then update will abort anyway.

My patch currently requires bookmarks on a non-default branch to
explicitly name the bookmark they want to track.
By convention the @ bookmark is usually on default, so for other
branches you would need a different bookmark for the 'real' head
("<branch name>@" has been suggested in the past).
I can incorporate that behaviour into the patch(es).

I know named branches are not anathema. The workflow I advocated at my
work uses named branches for release branches and bookmarks for
feature branches.
I'm trying to improve bookmarks in general - with or without named branches.

Steve
Matt Mackall - April 15, 2014, 5:47 p.m.
On Fri, 2014-03-21 at 14:29 +1100, Stephen Lee wrote:
> On Wed, Mar 19, 2014 at 1:05 AM, Jordi Gutiérrez Hermoso
> <jordigh@octave.org> wrote:
> > On Wed, 2014-03-12 at 20:27 +1100, Stephen Lee wrote:
> >
> >> In a non-bookmark workflow, the new logic only triggers if you have an
> >> active bookmark and an @ bookmark (which should be rare in a
> >> non-bookmark workflow).
> >
> > It seems to me that you're failing to account the situation of both
> > bookmarks and named branches being used intentionally, which is a
> > common thing to do with Mercurial (named branches aren't anathema).
> > When on a named branch, `hg update` keeps you on that branch and moves
> > you to it head, if unique. I think this is what mpm is complaining
> > about, what if you're on the stable named branch with no active
> > bookmark?
> 
> As I said before if you have no active bookmark, there is no behaviour
> change - you will end up on the tip of the current branch.
> If you are using bookmarks with branches then that usually makes no
> sense - which branch head is the 'real' head and which are feature
> heads? The 'real' head is often not even at a branch head!
> 
> Also note that if the destination selected by the new logic is not a
> descendent, then update will abort anyway.
> 
> My patch currently requires bookmarks on a non-default branch to
> explicitly name the bookmark they want to track.
> By convention the @ bookmark is usually on default, so for other
> branches you would need a different bookmark for the 'real' head
> ("<branch name>@" has been suggested in the past).
> I can incorporate that behaviour into the patch(es).
> 
> I know named branches are not anathema. The workflow I advocated at my
> work uses named branches for release branches and bookmarks for
> feature branches.
> I'm trying to improve bookmarks in general - with or without named branches.

Sadly, this is all too complicated to discuss in this format because
there are too many cases and it's not clear that we're covering them
all. We really need to move this discussion into some summary table that
covers all the possible cases and shows both the old and new behavior.

I'm also wary of assigning any semantics to '@' beyond 'this is the
default checkout on clone if it exists'.

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -4248,9 +4248,11 @@ 
                 "please merge with an explicit rev or bookmark"),
                 hint=_("run 'hg heads' to see all heads"))
         elif len(bmheads) <= 1:
-            raise util.Abort(_("no matching bookmark to merge - "
-                "please merge with an explicit rev or bookmark"),
-                hint=_("run 'hg heads' to see all heads"))
+            node = bookmarks.trackingbookmark(repo)
+            if node is None:
+                raise util.Abort(_("no matching bookmark to merge - "
+                    "please merge with an explicit rev or bookmark"),
+                    hint=_("run 'hg heads' to see all heads"))
 
     if not node and not repo._bookmarkcurrent:
         branch = repo[None].branch()
diff --git a/tests/test-bookmarks-merge.t b/tests/test-bookmarks-merge.t
--- a/tests/test-bookmarks-merge.t
+++ b/tests/test-bookmarks-merge.t
@@ -177,3 +177,53 @@ 
      g                         8:04dd21731d95
   $ hg id
   26bee9c5bcf3 @/c
+
+# test bookmark tracking for merge
+
+  $ hg book -d @
+  $ hg up -q g
+  $ hg --config bookmarks.g.track=e merge
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+  $ hg parents
+  changeset:   8:04dd21731d95
+  bookmark:    g
+  tag:         tip
+  parent:      6:be381d1126a0
+  user:        test
+  date:        Thu Jan 01 00:00:00 1970 +0000
+  summary:     g
+  
+  changeset:   7:ca784329f0ba
+  bookmark:    e
+  parent:      5:26bee9c5bcf3
+  parent:      4:a0546fcfe0fb
+  user:        test
+  date:        Thu Jan 01 00:00:00 1970 +0000
+  summary:     merge
+  
+
+# test bookmarks implicitly tracking @ for merge
+
+  $ hg up -C -q g
+  $ hg book -r5 @
+  $ hg merge
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+  $ hg parents
+  changeset:   8:04dd21731d95
+  bookmark:    g
+  tag:         tip
+  parent:      6:be381d1126a0
+  user:        test
+  date:        Thu Jan 01 00:00:00 1970 +0000
+  summary:     g
+  
+  changeset:   5:26bee9c5bcf3
+  bookmark:    @
+  bookmark:    c
+  parent:      3:b8f96cf4688b
+  user:        test
+  date:        Thu Jan 01 00:00:00 1970 +0000
+  summary:     e
+