[PATCH V3] bundle: warn when update to revision existing only in a bundle (issue5004)

[Pierre-Yves David]

On 03/31/2016 02:24 AM, liscju wrote:
> # HG changeset patch
> # User liscju <piotr.listkiewicz at gmail.com>
> # Date 1458719722 -3600
> #      Wed Mar 23 08:55:22 2016 +0100
> # Node ID 095b93f07d18fa13d5f284d19d9133bf576fc1cc
> # Parent  345f4fa4cc8912bb722ad3e35d68858487420bc6
> bundle: warn when update to revision existing only in a bundle (issue5004)
>
> Now its done silently, so unless user really knows what he is doing
> will be suprised to find that after update 'hg status' doesn't work.
> This commit makes also merge operation warns about missing parent when
> revision to merge exists only in the bundle.

The overall logic seems fine. I've a group of various code feedback, 
sorry for not giving them earlier.


>
> diff -r 345f4fa4cc89 -r 095b93f07d18 mercurial/bundlerepo.py
> --- a/mercurial/bundlerepo.py	Fri Mar 25 16:23:23 2016 -0500
> +++ b/mercurial/bundlerepo.py	Wed Mar 23 08:55:22 2016 +0100
> @@ -32,6 +32,7 @@ from . import (
>       localrepo,
>       manifest,
>       mdiff,
> +    node as nodemod,
>       pathutil,
>       phases,
>       revlog,
> @@ -385,6 +386,18 @@ class bundlerepository(localrepo.localre
>       def getcwd(self):
>           return os.getcwd() # always outside the repo
>
> +    def setparents(self, p1, p2=nullid):

It would be useful to have a small '#' comment explaining what the extra 
logic is aimed at.


> +        c = changelog.changelog(self.svfs)
> +        nodemap = c.nodemap

The bundlerevlog have a 'repotiprev' attribute that containt the tipmost 
revnum in the repository. We should use it for our testing instead of 
going through a full creation and parsing of a changelog.

> +        if p1 not in nodemap:
> +            self.ui.warn(_("setting parent to node %s that "
> +                           "only exists in the bundle\n")
> +                         % nodemod.hex(p1))

lets put the msg in a temporary variable, this will avoid a line 
wrapping and make the code clearer:

msg = _("setting parent to node %s that only exists in the bundle\n")
if …:

> +        if p2 and p2 not in nodemap:
> +            self.ui.warn(_("setting parent to node %s that "
> +                           "only exists in the bundle\n")
> +                         % nodemod.hex(p2))

And this will avoid duplicated it too ;-)

> +        return localrepo.localrepository.setparents(self, p1, p2)

The canonical python way to do this is:

return super(bundlerepository, self).setparents(p1, p2)

>   def instance(ui, path, create):
>       if create:
> diff -r 345f4fa4cc89 -r 095b93f07d18 tests/test-bundle.t
> --- a/tests/test-bundle.t	Fri Mar 25 16:23:23 2016 -0500
> +++ b/tests/test-bundle.t	Wed Mar 23 08:55:22 2016 +0100
> @@ -733,3 +733,67 @@ bundle single branch
>     $ hg bundle -r 'public()' no-output.hg
>     abort: no commits to bundle
>     [255]
> +
> +  $ cd ..
> +
> +When user merges to the revision existing only in the bundle,
> +it should show warning that second parent of the working
> +directory does not exist
> +
> +  $ hg init updates_to_bundle_revision
> +  $ cd updates_to_bundle_revision

We usually avoid '_' in mercurial (Matt Mackall hate them, I assume on 
of them stole his lunch when he was a kid or something (that or little 
finger damages)). Lets find a shorter name: "update2bundled"?

> +  $ cat <<EOF >> .hg/hgrc
> +  > [extensions]
> +  > strip =
> +  > EOF
> +  $ hg --config ui.allowemptycommit=true commit -m 0
> +  $ hg --config ui.allowemptycommit=true commit -m 1
> +  $ hg --config ui.allowemptycommit=true commit -m 2

Why are we using empty commit here? If does not seems necessary. Can we 
use normal commit with content ?

> +  $ hg update -r 1
> +  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  $ hg --config ui.allowemptycommit=true commit -m 3
> +  created new head
> +  $ hg update -r 2
> +  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  $ hg log -G
> +  o  changeset:   3:2aa7a5ec0bcc
> +  |  tag:         tip
> +  |  parent:      1:49086794b4c6
> +  |  user:        test
> +  |  date:        Thu Jan 01 00:00:00 1970 +0000
> +  |  summary:     3
> +  |
> +  | @  changeset:   2:5aa9a7cd3632
> +  |/   user:        test
> +  |    date:        Thu Jan 01 00:00:00 1970 +0000
> +  |    summary:     2
> +  |
> +  o  changeset:   1:49086794b4c6
> +  |  user:        test
> +  |  date:        Thu Jan 01 00:00:00 1970 +0000
> +  |  summary:     1
> +  |
> +  o  changeset:   0:65545109f7cb
> +     user:        test
> +     date:        Thu Jan 01 00:00:00 1970 +0000
> +     summary:     0
> +
> +  $ hg strip -r 3
> +  saved backup bundle to $TESTTMP/updates_to_bundle_revision/.hg/strip-backup/2aa7a5ec0bcc-322d4c1d-backup.hg (glob)
> +  $ hg merge -R .hg/strip-backup/*.hg -r 3
> +  setting parent to node 2aa7a5ec0bcc8462a34dcaa3f4f923f2c45415b3 that only exists in the bundle
> +  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  (branch merge, don't forget to commit)

It would seems more robust to explicitly bundle the changeset (using hg 
bundle) to a specific file and use that. (we would still strip and not 
rely on its implementation details.

> +When user updates to the revision existing only in the bundle,
> +it should show warning
> +
> +  $ hg update -R .hg/strip-backup/*.hg --clean -r 3
> +  setting parent to node 2aa7a5ec0bcc8462a34dcaa3f4f923f2c45415b3 that only exists in the bundle
> +  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +
> +When user updates to the revision existing in the local repository as well,
> +the warning shouldn't be emitted
> +
> +  $ hg update -R .hg/strip-backup/*.hg -r 0
> +  0 files updated, 0 files merged, 0 files removed, 0 files unresolved

Beside that, all the logic seems solid, good job. Looking forward to 
accept the V4 :)

Cheers,

-- 
Pierre-Yves David