[PATCH 01 of 12 V2] bookmarks: add function to centralize the logic to compare bookmarks

[Kevin Bullock]

On 22 Sep 2013, at 9:05 AM, FUJIWARA Katsunori wrote:

> # HG changeset patch
> # User FUJIWARA Katsunori <foozy at lares.dti.ne.jp>
> # Date 1379858556 -32400
> #      Sun Sep 22 23:02:36 2013 +0900
> # Node ID 2bb09ac15facc79ae3e926317d250d4f2bf50bfc
> # Parent  ea35caf324bb04cbc9ab5e2328367bc50f558cfb
> bookmarks: add function to centralize the logic to compare bookmarks
> 
> This patch adds "compare()" function to centralize the logic to
> compare bookmarks between two repositories.
> 
> This patch also adds utility function "_execactions()" to execute
> action corresponded to comparison result for each bookmarks.
> 
> diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
> --- a/mercurial/bookmarks.py
> +++ b/mercurial/bookmarks.py
> @@ -239,6 +239,84 @@
>     finally:
>         w.release()
> 
> +def compare(localrepo, srcmarks, dstmarks,
> +            srchex=None, dsthex=None, targets=None):
> +    '''Compare bookmarks between srcmarks and dstmarks, and create result list
> +
> +    Each elements in result list are tuple "(kind, bookmark name,
> +    changeset ID on source side, changeset ID on destination
> +    side)". "kind" is one of "addsrc", "adddst", "advsrc", "advdst",
> +    "diverge", "differ" or "invalid". Each changeset IDs are 40
> +    hexadecimal digit string or None.
> +
> +    Changeset IDs in result tuples may be unknown for localrepo, if
> +    "kind" is one of "addsrc", "adddst", "differ" or "invalid".

Seems like it would be better to return a tuple of sets, like we do from localrepo.status(). I like the clarity of the dispatch-table ("actionmap") you have _execactions() take, but it would be just as clear (and _execactions would be unnecessary) if we had a call pattern like:

    addsrc, adddst, advsrc, advdst, diverge, differ, invalid =
        bookmarks.compare(...)

    for mark, srcnode, dstnode in addsrc | advsrc | advdst | diverge | differ:
        ...
    for mark, srcnode, dstnode in adddst:
        ...
    etc.

We would also avoid making callers know the "magic strings" that are returned in these tuples, and callers could only look at the return sets they're interested in (cf. patch 4, where updateremote() is only interested in the advsrc tuples).

pacem in terris / мир / शान्ति / ‎‫سَلاَم‬ / 平和
Kevin R. Bullock