[PATCH 1 of 3] bookmarks: simplify iscurrent to isactivecurrent (API)

[Martin von Zweigbergk]

On Thu, May 7, 2015 at 3:16 PM Ryan McElroy <rm at fb.com> wrote:

> On 5/7/2015 2:56 PM, Martin von Zweigbergk wrote:
>
>
>
> On Thu, May 7, 2015 at 1:59 PM Ryan McElroy <rmcelroy at fb.com> wrote:
>
>> # HG changeset patch
>> # User Ryan McElroy <rmcelroy at fb.com>
>> # Date 1429040715 25200
>> #      Tue Apr 14 12:45:15 2015 -0700
>> # Node ID ccb7b2b9a2f0dc5ac57e2e13e48180a03ad6d175
>> # Parent  17ba4ccd20b48511b3d06ab47fb1b2faf31410d7
>> bookmarks: simplify iscurrent to isactivecurrent (API)
>>
>> Previously this function accepted two optional parameters that were
>> unused by
>> any callers and complicated the function.
>>
>> Today, the terms 'active' and 'current' are interchangeably used
>> throughout the
>> codebase in reference to the active bookmark (the bookmark that will be
>> updated
>> with the next commit). This leads to confusion among developers and users.
>> This patch is part of a series to standardize the usage to 'active'
>> throughout
>> the mercurial codebase and user interface.
>>
>> diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
>> --- a/mercurial/bookmarks.py
>> +++ b/mercurial/bookmarks.py
>> @@ -165,17 +165,14 @@ def deactivate(repo):
>>      finally:
>>          wlock.release()
>>
>> -def iscurrent(repo, mark=None, parents=None):
>> -    '''Tell whether the current bookmark is also active
>> -
>> -    I.e., the bookmark listed in .hg/bookmarks.current also points to a
>> -    parent of the working directory.
>> -    '''
>> -    if not mark:
>> -        mark = repo._activebookmark
>> -    if not parents:
>> -        parents = [p.node() for p in repo[None].parents()]
>> +def isactivecurrent(repo):
>> +    """
>> +    Tell whether the 'active' bookmark is also 'current' -- eg, points to
>> +    working directory parent(s).
>
>
>  In the description, you said that 'active' and 'current' are
> uses interchangeably and now you're standardizing on 'active', but here it
> seems like they now start to mean different things, or the above docstring
> would be pointless ("whether the active bookmark is also the active
> bookmark"). Assuming that you want them to mean different things, could you
> describe the difference in the change description?
>
>  Also, I'm almost certain you meant "i.e.", not "e.g.".
>
>
> You've caught me in a Latin mixup! Indeed, I should have used i.e.
>
> This function alone is the epitome of what is wrong with the "active" vs
> "current" terminology.
> "active" refers to up to one bookmark that will be updated when we make a
> new commit. Lots of people have it show up in their command prompt too.
> "current" refers to whatever the current working directory's parents are,
> eg, the '.' commit (technically, p1() and p2())
>

I think my problem was that I didn't realize that a bookmark can still be
"active" after a pull (and maybe other operations). Please mention that too
if you don't mind. Thanks!


>
> The whole thing is messed up and this just clarifies the messiness. A
> future (BC) patch in this mega-series will eliminate this command
> altogether.
> Regardless, I'll clean up the description and the latin and send a V2. Are
> the other two patches to your liking though?
>

I've pushed 2/3. It seems like 3/3 makes sense without 1/3, so I can push
that too.


>
>
>
>
>> +    """
>> +    mark = repo._activebookmark
>>      marks = repo._bookmarks
>> +    parents = [p.node() for p in repo[None].parents()]
>>      return (mark in marks and marks[mark] in parents)
>>
>>  def updatecurrentbookmark(repo, oldnode, curbranch):
>> @@ -210,7 +207,7 @@ def calculateupdate(ui, repo, checkout):
>>      movemarkfrom = None
>>      if checkout is None:
>>          curmark = repo._activebookmark
>> -        if iscurrent(repo):
>> +        if isactivecurrent(repo):
>>              movemarkfrom = repo['.'].node()
>>          elif curmark:
>>              ui.status(_("updating to active bookmark %s\n") % curmark)
>> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
>> --- a/mercurial/cmdutil.py
>> +++ b/mercurial/cmdutil.py
>> @@ -2721,7 +2721,7 @@ def buildcommittext(repo, ctx, subs, ext
>>          edittext.append(_("HG: branch merge"))
>>      if ctx.branch():
>>          edittext.append(_("HG: branch '%s'") % ctx.branch())
>> -    if bookmarks.iscurrent(repo):
>> +    if bookmarks.isactivecurrent(repo):
>>          edittext.append(_("HG: bookmark '%s'") % repo._activebookmark)
>>      edittext.extend([_("HG: subrepo %s") % s for s in subs])
>>      edittext.extend([_("HG: added %s") % f for f in added])
>> diff --git a/mercurial/templatekw.py b/mercurial/templatekw.py
>> --- a/mercurial/templatekw.py
>> +++ b/mercurial/templatekw.py
>> @@ -226,7 +226,7 @@ def showcurrentbookmark(**args):
>>      associated with the changeset"""
>>      import bookmarks as bookmarks # to avoid circular import issues
>>      repo = args['repo']
>> -    if bookmarks.iscurrent(repo):
>> +    if bookmarks.isactivecurrent(repo):
>>          current = repo._activebookmark
>>          if current in args['ctx'].bookmarks():
>>              return current
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel at selenic.com
>> http://selenic.com/mailman/listinfo/mercurial-devel
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20150507/8f98a30f/attachment.html>