Patchwork [4,of,9,RFC] localrepo: add a method to return markers in a namespace

login
register
mail settings
Submitter Sean Farley
Date March 30, 2014, 11:09 p.m.
Message ID <0c5c36fe392d9dfbc56a.1396220942@laptop.local>
Download mbox | patch
Permalink /patch/4151/
State Deferred
Headers show

Comments

Sean Farley - March 30, 2014, 11:09 p.m.
# HG changeset patch
# User Sean Farley <sean.michael.farley@gmail.com>
# Date 1396218396 18000
#      Sun Mar 30 17:26:36 2014 -0500
# Node ID 0c5c36fe392d9dfbc56ac466e312a62bdeb2bf93
# Parent  1bb668ecda482ee83f31d505c2094e6402668931
localrepo: add a method to return markers in a namespace
David Soria Parra - March 31, 2014, 5:41 p.m.
Sean Farley <sean.michael.farley@gmail.com> writes:

> # HG changeset patch
> # User Sean Farley <sean.michael.farley@gmail.com>
> # Date 1396218396 18000
> #      Sun Mar 30 17:26:36 2014 -0500
> # Node ID 0c5c36fe392d9dfbc56ac466e312a62bdeb2bf93
> # Parent  1bb668ecda482ee83f31d505c2094e6402668931
> localrepo: add a method to return markers in a namespace
>
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -671,10 +671,37 @@ class localrepository(object):
>          RFC: should we allow tags and bookmarks to be set this way?
>          '''
>          self._createmarkernamespace(namespace)
>          self._markers[namespace][name] = node
>  
> +    def markers(self, namespace=None, name=None):
> +        '''Return markers in the namespace, otherwise return all markers.
> +
> +        If namespace is None, this will return a dictionary of dictionaries.
> +
> +        If namespace is passed but name is None, this function will return a
> +        dictionary.
> +
> +        If both namespace and name are passed, this will return a value.
> +
> +        RFC: should this allow namespace=None and name='foo' to find all 'foo'
> +        no matter the namespace?
> +        '''
> +        if namespace is None:
> +            # create a read-only copy that adds tags and bookmarks
> +            allmarks = self._markers.copy()
> +            allmarks["tag"] = self.tags()
> +            allmarks["bookmark"] = self._bookmarks
> +            return allmarks
> +
> +        self._createmarkernamespace(namespace)
> +
> +        if name is not None:
> +            return self._markers[namespace][name]
> +
> +        return self._markers[namespace]
> +

I personally think this is a bit error prone as you can accidentally
have a name = None when passing to markers and then you suddenly get a
dictionary instead of an expected value. I would recommend to only
return self._markers[namespace] so people can use it as
self.markers(namespace).get(name, default).
Sean Farley - March 31, 2014, 7:30 p.m.
David Soria Parra <davidsp@fb.com> writes:

> Sean Farley <sean.michael.farley@gmail.com> writes:
>
>> # HG changeset patch
>> # User Sean Farley <sean.michael.farley@gmail.com>
>> # Date 1396218396 18000
>> #      Sun Mar 30 17:26:36 2014 -0500
>> # Node ID 0c5c36fe392d9dfbc56ac466e312a62bdeb2bf93
>> # Parent  1bb668ecda482ee83f31d505c2094e6402668931
>> localrepo: add a method to return markers in a namespace
>>
>> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>> --- a/mercurial/localrepo.py
>> +++ b/mercurial/localrepo.py
>> @@ -671,10 +671,37 @@ class localrepository(object):
>>          RFC: should we allow tags and bookmarks to be set this way?
>>          '''
>>          self._createmarkernamespace(namespace)
>>          self._markers[namespace][name] = node
>>  
>> +    def markers(self, namespace=None, name=None):
>> +        '''Return markers in the namespace, otherwise return all markers.
>> +
>> +        If namespace is None, this will return a dictionary of dictionaries.
>> +
>> +        If namespace is passed but name is None, this function will return a
>> +        dictionary.
>> +
>> +        If both namespace and name are passed, this will return a value.
>> +
>> +        RFC: should this allow namespace=None and name='foo' to find all 'foo'
>> +        no matter the namespace?
>> +        '''
>> +        if namespace is None:
>> +            # create a read-only copy that adds tags and bookmarks
>> +            allmarks = self._markers.copy()
>> +            allmarks["tag"] = self.tags()
>> +            allmarks["bookmark"] = self._bookmarks
>> +            return allmarks
>> +
>> +        self._createmarkernamespace(namespace)
>> +
>> +        if name is not None:
>> +            return self._markers[namespace][name]
>> +
>> +        return self._markers[namespace]
>> +
>
> I personally think this is a bit error prone as you can accidentally
> have a name = None when passing to markers and then you suddenly get a
> dictionary instead of an expected value. I would recommend to only
> return self._markers[namespace] so people can use it as
> self.markers(namespace).get(name, default).

That's a good point. I'll rework it as you suggest. Other points about
this patch series are the two scenarios:

Where can I inject to provide a custom namespace? Should we promote
wrapping _createmarkernamespace? Or load them during during reposetup?

Where can I inject to save this custom namespace? Following the logic
from local tags, an extension could wrap localrepo.mark and write out a
file each time. What if an extension wants to add 10 marks at once? I
can't imagine writing a file 10 times in a row is good.
Matt Mackall - April 2, 2014, 3:18 p.m.
On Mon, 2014-03-31 at 17:41 +0000, David Soria Parra wrote:
> Sean Farley <sean.michael.farley@gmail.com> writes:
> 
> > # HG changeset patch
> > # User Sean Farley <sean.michael.farley@gmail.com>
> > # Date 1396218396 18000
> > #      Sun Mar 30 17:26:36 2014 -0500
> > # Node ID 0c5c36fe392d9dfbc56ac466e312a62bdeb2bf93
> > # Parent  1bb668ecda482ee83f31d505c2094e6402668931
> > localrepo: add a method to return markers in a namespace
> >
> > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> > --- a/mercurial/localrepo.py
> > +++ b/mercurial/localrepo.py
> > @@ -671,10 +671,37 @@ class localrepository(object):
> >          RFC: should we allow tags and bookmarks to be set this way?
> >          '''
> >          self._createmarkernamespace(namespace)
> >          self._markers[namespace][name] = node
> >  
> > +    def markers(self, namespace=None, name=None):
> > +        '''Return markers in the namespace, otherwise return all markers.
> > +
> > +        If namespace is None, this will return a dictionary of dictionaries.
> > +
> > +        If namespace is passed but name is None, this function will return a
> > +        dictionary.
> > +
> > +        If both namespace and name are passed, this will return a value.

This is a bit too polymorphic an API: it has three return types (dict of
dicts, dict, and value).
Sean Farley - April 2, 2014, 5:01 p.m.
Matt Mackall <mpm@selenic.com> writes:

> On Mon, 2014-03-31 at 17:41 +0000, David Soria Parra wrote:
>> Sean Farley <sean.michael.farley@gmail.com> writes:
>> 
>> > # HG changeset patch
>> > # User Sean Farley <sean.michael.farley@gmail.com>
>> > # Date 1396218396 18000
>> > #      Sun Mar 30 17:26:36 2014 -0500
>> > # Node ID 0c5c36fe392d9dfbc56ac466e312a62bdeb2bf93
>> > # Parent  1bb668ecda482ee83f31d505c2094e6402668931
>> > localrepo: add a method to return markers in a namespace
>> >
>> > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>> > --- a/mercurial/localrepo.py
>> > +++ b/mercurial/localrepo.py
>> > @@ -671,10 +671,37 @@ class localrepository(object):
>> >          RFC: should we allow tags and bookmarks to be set this way?
>> >          '''
>> >          self._createmarkernamespace(namespace)
>> >          self._markers[namespace][name] = node
>> >  
>> > +    def markers(self, namespace=None, name=None):
>> > +        '''Return markers in the namespace, otherwise return all markers.
>> > +
>> > +        If namespace is None, this will return a dictionary of dictionaries.
>> > +
>> > +        If namespace is passed but name is None, this function will return a
>> > +        dictionary.
>> > +
>> > +        If both namespace and name are passed, this will return a value.
>
> This is a bit too polymorphic an API: it has three return types (dict of
> dicts, dict, and value). 

Agreed. What about the name, though?
Matt Mackall - April 2, 2014, 8:49 p.m.
On Wed, 2014-04-02 at 12:01 -0500, Sean Farley wrote:
> Matt Mackall <mpm@selenic.com> writes:
> 
> > On Mon, 2014-03-31 at 17:41 +0000, David Soria Parra wrote:
> >> Sean Farley <sean.michael.farley@gmail.com> writes:
> >> 
> >> > # HG changeset patch
> >> > # User Sean Farley <sean.michael.farley@gmail.com>
> >> > # Date 1396218396 18000
> >> > #      Sun Mar 30 17:26:36 2014 -0500
> >> > # Node ID 0c5c36fe392d9dfbc56ac466e312a62bdeb2bf93
> >> > # Parent  1bb668ecda482ee83f31d505c2094e6402668931
> >> > localrepo: add a method to return markers in a namespace
> >> >
> >> > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> >> > --- a/mercurial/localrepo.py
> >> > +++ b/mercurial/localrepo.py
> >> > @@ -671,10 +671,37 @@ class localrepository(object):
> >> >          RFC: should we allow tags and bookmarks to be set this way?
> >> >          '''
> >> >          self._createmarkernamespace(namespace)
> >> >          self._markers[namespace][name] = node
> >> >  
> >> > +    def markers(self, namespace=None, name=None):
> >> > +        '''Return markers in the namespace, otherwise return all markers.
> >> > +
> >> > +        If namespace is None, this will return a dictionary of dictionaries.
> >> > +
> >> > +        If namespace is passed but name is None, this function will return a
> >> > +        dictionary.
> >> > +
> >> > +        If both namespace and name are passed, this will return a value.
> >
> > This is a bit too polymorphic an API: it has three return types (dict of
> > dicts, dict, and value). 
> 
> Agreed. What about the name, though?

If we agree that the shape of this function is:

no args -> dictionary of name, hash pairs

..it's fine. But then we have questions about what to do with
collisions. Perhaps all we really need is marked() -> set of nodes that
have some form of marker.
Sean Farley - April 2, 2014, 9:20 p.m.
Matt Mackall <mpm@selenic.com> writes:

> On Wed, 2014-04-02 at 12:01 -0500, Sean Farley wrote:
>> Matt Mackall <mpm@selenic.com> writes:
>> 
>> > On Mon, 2014-03-31 at 17:41 +0000, David Soria Parra wrote:
>> >> Sean Farley <sean.michael.farley@gmail.com> writes:
>> >> 
>> >> > # HG changeset patch
>> >> > # User Sean Farley <sean.michael.farley@gmail.com>
>> >> > # Date 1396218396 18000
>> >> > #      Sun Mar 30 17:26:36 2014 -0500
>> >> > # Node ID 0c5c36fe392d9dfbc56ac466e312a62bdeb2bf93
>> >> > # Parent  1bb668ecda482ee83f31d505c2094e6402668931
>> >> > localrepo: add a method to return markers in a namespace
>> >> >
>> >> > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>> >> > --- a/mercurial/localrepo.py
>> >> > +++ b/mercurial/localrepo.py
>> >> > @@ -671,10 +671,37 @@ class localrepository(object):
>> >> >          RFC: should we allow tags and bookmarks to be set this way?
>> >> >          '''
>> >> >          self._createmarkernamespace(namespace)
>> >> >          self._markers[namespace][name] = node
>> >> >  
>> >> > +    def markers(self, namespace=None, name=None):
>> >> > +        '''Return markers in the namespace, otherwise return all markers.
>> >> > +
>> >> > +        If namespace is None, this will return a dictionary of dictionaries.
>> >> > +
>> >> > +        If namespace is passed but name is None, this function will return a
>> >> > +        dictionary.
>> >> > +
>> >> > +        If both namespace and name are passed, this will return a value.
>> >
>> > This is a bit too polymorphic an API: it has three return types (dict of
>> > dicts, dict, and value). 
>> 
>> Agreed. What about the name, though?
>
> If we agree that the shape of this function is:
>
> no args -> dictionary of name, hash pairs
>
> ..it's fine. But then we have questions about what to do with
> collisions. Perhaps all we really need is marked() -> set of nodes that
> have some form of marker.

Sorry, I didn't clarify what I meant. It seems that 'marker' is not a
good name because there is a clash with 'obsolete marker'. In another
thread, I present the word 'label':

http://www.selenic.com/pipermail/mercurial-devel/2014-March/057599.html
Matt Mackall - April 2, 2014, 9:31 p.m.
On Wed, 2014-04-02 at 16:20 -0500, Sean Farley wrote:
> Matt Mackall <mpm@selenic.com> writes:
> 
> > On Wed, 2014-04-02 at 12:01 -0500, Sean Farley wrote:
> >> Matt Mackall <mpm@selenic.com> writes:
> >> 
> >> > On Mon, 2014-03-31 at 17:41 +0000, David Soria Parra wrote:
> >> >> Sean Farley <sean.michael.farley@gmail.com> writes:
> >> >> 
> >> >> > # HG changeset patch
> >> >> > # User Sean Farley <sean.michael.farley@gmail.com>
> >> >> > # Date 1396218396 18000
> >> >> > #      Sun Mar 30 17:26:36 2014 -0500
> >> >> > # Node ID 0c5c36fe392d9dfbc56ac466e312a62bdeb2bf93
> >> >> > # Parent  1bb668ecda482ee83f31d505c2094e6402668931
> >> >> > localrepo: add a method to return markers in a namespace
> >> >> >
> >> >> > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> >> >> > --- a/mercurial/localrepo.py
> >> >> > +++ b/mercurial/localrepo.py
> >> >> > @@ -671,10 +671,37 @@ class localrepository(object):
> >> >> >          RFC: should we allow tags and bookmarks to be set this way?
> >> >> >          '''
> >> >> >          self._createmarkernamespace(namespace)
> >> >> >          self._markers[namespace][name] = node
> >> >> >  
> >> >> > +    def markers(self, namespace=None, name=None):
> >> >> > +        '''Return markers in the namespace, otherwise return all markers.
> >> >> > +
> >> >> > +        If namespace is None, this will return a dictionary of dictionaries.
> >> >> > +
> >> >> > +        If namespace is passed but name is None, this function will return a
> >> >> > +        dictionary.
> >> >> > +
> >> >> > +        If both namespace and name are passed, this will return a value.
> >> >
> >> > This is a bit too polymorphic an API: it has three return types (dict of
> >> > dicts, dict, and value). 
> >> 
> >> Agreed. What about the name, though?
> >
> > If we agree that the shape of this function is:
> >
> > no args -> dictionary of name, hash pairs
> >
> > ..it's fine. But then we have questions about what to do with
> > collisions. Perhaps all we really need is marked() -> set of nodes that
> > have some form of marker.
> 
> Sorry, I didn't clarify what I meant. It seems that 'marker' is not a
> good name because there is a clash with 'obsolete marker'. In another
> thread, I present the word 'label':
> 
> http://www.selenic.com/pipermail/mercurial-devel/2014-March/057599.html

Either is fine. I've suggested label in the past.

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -671,10 +671,37 @@  class localrepository(object):
         RFC: should we allow tags and bookmarks to be set this way?
         '''
         self._createmarkernamespace(namespace)
         self._markers[namespace][name] = node
 
+    def markers(self, namespace=None, name=None):
+        '''Return markers in the namespace, otherwise return all markers.
+
+        If namespace is None, this will return a dictionary of dictionaries.
+
+        If namespace is passed but name is None, this function will return a
+        dictionary.
+
+        If both namespace and name are passed, this will return a value.
+
+        RFC: should this allow namespace=None and name='foo' to find all 'foo'
+        no matter the namespace?
+        '''
+        if namespace is None:
+            # create a read-only copy that adds tags and bookmarks
+            allmarks = self._markers.copy()
+            allmarks["tag"] = self.tags()
+            allmarks["bookmark"] = self._bookmarks
+            return allmarks
+
+        self._createmarkernamespace(namespace)
+
+        if name is not None:
+            return self._markers[namespace][name]
+
+        return self._markers[namespace]
+
     def branchmap(self):
         '''returns a dictionary {branch: [branchheads]} with branchheads
         ordered by increasing revision number'''
         branchmap.updatecache(self)
         return self._branchcaches[self.filtername]