[PATCH] strip: use repo._bookmarks.recordchange instead of repo._bookmarks.write

[Pierre-Yves David]

On 11/16/2015 05:20 PM, Laurent Charignon wrote:
> # HG changeset patch
> # User Laurent Charignon <lcharignon at fb.com>
> # Date 1447709407 28800
> #      Mon Nov 16 13:30:07 2015 -0800
> # Node ID 7f86786cd6692d5e4bccb946125e343bf7cf6589
> # Parent  2da6a2dbfc42bdec4bcaf47da947c64ff959a59c
> strip: use repo._bookmarks.recordchange instead of repo._bookmarks.write
>
> Before this patch, strip was using repo._bookmarks.write.
> This patch replaces this code with the recommended way of saving bookmarks
> changes: repo._bookmarks.recordchange.

It feel a bi strange to have this dedicated transaction after the 
bookmarked changesets are actually removed. However this is strip so 
this is expected to be strange (aka, I do not think we should the logic 
in this patch).

However, can we get a more accurate transaction name? eg: 'strip'?

Also, this changes will collide with Shubhanshu Agrawal series. I've 
CCed him.


>
> diff --git a/hgext/strip.py b/hgext/strip.py
> --- a/hgext/strip.py
> +++ b/hgext/strip.py
> @@ -64,11 +64,17 @@
>
>           marks = repo._bookmarks
>           if bookmark:
> -            if bookmark == repo._activebookmark:
> -                bookmarks.deactivate(repo)
> -            del marks[bookmark]
> -            marks.write()
> -            ui.write(_("bookmark '%s' deleted\n") % bookmark)
> +            tr = None
> +            try:
> +                tr = repo.transaction('bookmark')
> +                if bookmark == repo._activebookmark:
> +                    bookmarks.deactivate(repo)
> +                del marks[bookmark]
> +                marks.recordchange(tr)
> +                tr.close()
> +                ui.write(_("bookmark '%s' deleted\n") % bookmark)
> +            finally:
> +                release(tr)
>       finally:
>           release(lock, wlock)

small neat: you can skip the try except and use tr in the last release. 
(if you ensure it is None at some point).

(+ feedback about the transaction name)

>
> @@ -145,9 +151,16 @@
>                   rsrevs = repair.stripbmrevset(repo, mark)
>                   revs.update(set(rsrevs))
>               if not revs:
> -                del marks[mark]
> -                marks.write()
> -                ui.write(_("bookmark '%s' deleted\n") % mark)
> +                lock = tr = None
> +                try:
> +                    lock = repo.lock()
> +                    tr = repo.transaction('bookmark')
> +                    del marks[mark]
> +                    marks.recordchange(tr)
> +                    tr.close()
> +                    ui.write(_("bookmark '%s' deleted\n") % mark)
> +                finally:
> +                    release(lock, tr)

(+ feedback about the transaction name)

-- 
Pierre-Yves David