Bug 4700 - pulling a bookmark using 'hg pull --rev BOOKMARK' is racy
Summary: pulling a bookmark using 'hg pull --rev BOOKMARK' is racy
Status: RESOLVED FIXED
Alias: None
Product: Mercurial
Classification: Unclassified
Component: bookmarks (show other bugs)
Version: default branch
Hardware: PC Linux
: normal bug
Assignee: Bugzilla
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-06-01 21:03 UTC by Pierre-Yves David
Modified: 2019-01-15 00:00 UTC (History)
1 user (show)

See Also:
Python Version: ---


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Pierre-Yves David 2015-06-01 21:03 UTC
Similar to issue4689 but --rev deserve its own case.


I've a repo whose current behavior is For some reason, the current behavior is "crash" for unknown reason.
Comment 1 Pierre-Yves David 2015-06-02 00:46 UTC
The race is confirmed, the crash is yet another issue related to --rev.
Comment 2 Pierre-Yves David 2015-06-03 19:14 UTC
The crash was caused by 4707.

The following test repro the issue:

diff --git a/tests/test-bookmarks-pushpull.t b/tests/test-bookmarks-pushpull.t
--- a/tests/test-bookmarks-pushpull.t
+++ b/tests/test-bookmarks-pushpull.t
@@ -342,10 +342,65 @@ Update a bookmark right after the initia
    * @                         1:0d2164f0ce0d
      X                         1:0d2164f0ce0d
      Y                         5:35d1ef0a8d1b
      Z                         1:0d2164f0ce0d
 
+Update a bookmark right after the initial lookup -r (issue4700)
+
+  $ echo c7 > ../pull-race/f3 # to be committed during the race
+  $ cat <<EOF > ../lookuphook.py
+  > """small extensions adding a hook after wireprotocol lookup to test race"""
+  > from mercurial import extensions, wireproto
+  > 
+  > oldlookup = wireproto.lookup
+  > @wireproto.wireprotocommand('lookup', 'key')
+  > def lookup(repo, proto, key):
+  >     ret= oldlookup(repo, proto, key)
+  >     repo.hook('lookup')
+  >     return ret
+  > EOF
+  $ cat <<EOF > ../pull-race/.hg/hgrc
+  > [extensions]
+  > lookuphook=$TESTTMP/lookuphook.py
+  > [hooks]
+  > # if anything to commit, commit it right after the first key listing used #
+  > # during lookup. This make the commit appears before the actual getbundle
+  > # call.
+  > lookup.makecommit= ((hg st | grep -q M) && (hg commit -m race; echo commited in pull-race)) || exit 0
+  > EOF
+
+(new config need server restart)
+
+  $ "$TESTDIR/killdaemons.py" $DAEMON_PIDS
+  $ hg -R ../pull-race serve -p $HGPORT -d --pid-file=../pull-race.pid -E main-error.log
+  $ cat ../pull-race.pid >> $DAEMON_PIDS
+
+  $ hg -R $TESTTMP/pull-race book
+     @                         1:0d2164f0ce0d
+     X                         1:0d2164f0ce0d
+   * Y                         6:0d60821d2197
+     Z                         1:0d2164f0ce0d
+  $ hg pull -r Y
+  pulling from http://localhost:$HGPORT/
+  searching for changes
+  adding changesets
+  adding manifests
+  adding file changes
+  added 1 changesets with 1 changes to 1 files
+  updating bookmark Y
+  (run 'hg update' to get a working copy)
+  $ hg book
+   * @                         1:0d2164f0ce0d
+     X                         1:0d2164f0ce0d
+     Y                         6:0d60821d2197
+     Z                         1:0d2164f0ce0d
+  $ hg -R $TESTTMP/pull-race book
+     @                         1:0d2164f0ce0d
+     X                         1:0d2164f0ce0d
+   * Y                         7:714424d9e8b8
+     Z                         1:0d2164f0ce0d
+
 (done with this section of the test)
 
   $ "$TESTDIR/killdaemons.py" $DAEMON_PIDS
   $ cd ../b



It fails with the following output:


--- /home/pyd/src/mercurial-dev/tests/test-bookmarks-pushpull.t
+++ /home/pyd/src/mercurial-dev/tests/test-bookmarks-pushpull.t.err
@@ -386,12 +386,11 @@
   adding manifests
   adding file changes
   added 1 changesets with 1 changes to 1 files
-  updating bookmark Y
   (run 'hg update' to get a working copy)
   $ hg book
    * @                         1:0d2164f0ce0d
      X                         1:0d2164f0ce0d
-     Y                         6:0d60821d2197
+     Y                         5:35d1ef0a8d1b
      Z                         1:0d2164f0ce0d
   $ hg -R $TESTTMP/pull-race book
      @                         1:0d2164f0ce0d

ERROR: test-bookmarks-pushpull.t output changed
Comment 3 HG Bot 2015-06-15 14:45 UTC
Fixed by https://selenic.com/repo/hg/rev/847fce27effc
Pierre-Yves David <pierre-yves.david@fb.com>
bookmark: informs of failure to upgrade a bookmark

When we explicitly requested to update a bookmark but the bookmark location was
missing locally, we used to silently ignore the case. We now issue a message
about it to point that something wrong is going on.

By chance, we fixed all the cases where is case happened (for explicit pulling
only, issue4700 is still open). But I think it is still valuable to have a
warning in place in case such issue is reintroduced.

This patch have been tested against issue4689 test (but without issue4689 fix).
It give the better but expected failure seen below:

> --- /home/pyd/src/mercurial-dev/tests/test-bookmarks-pushpull.t
> +++ /home/pyd/src/mercurial-dev/tests/test-bookmarks-pushpull.t.err
> @@ -337,12 +337,12 @@
>    adding manifests
>    adding file changes
>    added 1 changesets with 1 changes to 1 files
> -  updating bookmark Y
> +  remote bookmark Y point to locally missing 0d60821d2197
>    (run 'hg update' to get a working copy)
>    $ hg book
>     * @                         1:0d2164f0ce0d
>       X                         1:0d2164f0ce0d
> -     Y                         5:35d1ef0a8d1b
> +     Y                         4:b0a5eff05604
>       Z                         1:0d2164f0ce0d
>
>  Update a bookmark right after the initial lookup -r (issue4700)
> @@ -387,12 +387,11 @@
>    adding manifests
>    adding file changes
>    added 1 changesets with 1 changes to 1 files
> -  updating bookmark Y
>    (run 'hg update' to get a working copy)
>    $ hg book
>     * @                         1:0d2164f0ce0d
>       X                         1:0d2164f0ce0d
> -     Y                         6:0d60821d2197
> +     Y                         4:b0a5eff05604
>       Z                         1:0d2164f0ce0d
>    $ hg -R $TESTTMP/pull-race book
>       @                         1:0d2164f0ce0d

(please test the fix)
Comment 4 Bugzilla 2015-06-23 16:35 UTC
Bug was set to TESTING for 7 days, resolving
Comment 5 HG Bot 2019-01-07 18:35 UTC
Fixed by https://mercurial-scm.org/repo/hg/rev/c236a491ab7b
Valentin Gatien-Baron <valentin.gatienbaron@gmail.com>
test-bookmarks-pushpull: add failing test of issue4700

Differential Revision: https://phab.mercurial-scm.org/D5447

(please test the fix)
Comment 6 HG Bot 2019-01-07 18:37 UTC
Fixed by https://mercurial-scm.org/repo/hg/rev/bad05a6afdc8
Valentin Gatien-Baron <valentin.gatienbaron@gmail.com>
pull: fix inconsistent view of bookmarks during pull (issue4700)

I had a share where a pull apparently pulled a bookmark but not the
revision pointed to by the bookmark, which I suspect is due to this
(and if not, we might as well remove known issues in this area).

I do this by combining doing all the queries that could read the
bookmarks in one round trip.

I had to change the handling of the case where the server doesn't
support the lookup query, because if it fails, it would otherwise make
fremotebookmark.result() block forever. This is due to
wireprotov1peer.peerexecutor.sendcommands's behavior (it fills a
single future if any query fails synchronously and leaves all other
futures unchanged), but I don't know if the fix is to cancel all other
futures, or to keep going with the other queries.

Differential Revision: https://phab.mercurial-scm.org/D5449

(please test the fix)
Comment 7 Bugzilla 2019-01-15 00:00 UTC
Bug was set to TESTING for 7 days, resolving