Re: D937: remotenames: move function to pull remotenames from the remoterepo to core

[Sean Farley]

pulkit (Pulkit Goyal)  writes:

> pulkit created this revision.
> Herald added a subscriber: mercurial-devel.
> Herald added a reviewer: hg-reviewers.
>
> REVISION SUMMARY
>   This patch moves the functionality to pull branches and bookmarks from a
>   server from which we are pulling to core. The function used to exist in
>   remotenames repo.

Bringing all of remotenames into core is a slippery slope to a confusing
user experience. As a whole it’s unnecessarily complicated. I recommend
splitting remotenames up and only bringing in parts (1) & (2) (below) that
were discussed at the Montreal sprint.

The current remotenames code is composed of roughly three layers:

1) the base layer: bugfixes and storage format

2) the remote tracking layer: simple wrappers that sit in-between push /
pull and write out the remote info

3) behavior changes: push and pull changes, --anon, update -B, etc.

(1) and (2) were previously discussed in Montreal and were ok’d by Matt.

Recommendations:

For (1), most of these patches are suitable for core. It’s bugfixes and
refactoring to help all extensions. There are a number of things to
still fix.

The easiest to start tackling would be schemes:

https://bitbucket.org/seanfarley/hgremotenames/src/c7454740c212d405b4e9cbb2704823a1cd9b476c/remotenames.py?at=default=file-view-default#remotenames.py-1433

Next, there is strengthening path logic (perhaps refactoring a bit) so
that working with paths is less fragile and error-prone (e.g. see
activepath function). Fixing this in core will benefit all extensions.

Third, there is a long-standing issue with multiple extensions
displaying bookmarks. I’ve done some refactoring in core to help with
this issue but there is still work to be done. In remotenames, you can
see how much copy-pasta there is in displaylocalbookmarks and
exbookmarks. Currently, this breaks with any other extension that also
tries to display bookmarks (e.g. hg-git).

Lastly, for (1), there is the issue with storage format selection. We
have many storage formats in Mercurial, and we should put some thought
into the remotenames’ format. I hesitate to use json because bookmarks
can be unicode.

There is a lot of work to be done for (1). I have recommendations for
(2) that I can address in a follow-up, if desired. I hope this
illustrates how much work needs to be done for (1) and why we should
reconsider this approach submitted so far.


signature.asc
Description: PGP signature
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D937: remotenames: move function to pull remotenames from the remoterepo to core

[pulkit (Pulkit Goyal)]

pulkit abandoned this revision.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D937#17855, @durin42 wrote:
  
  > Could we try and get this series ready to land before the freeze on 
Wednesday? or should we be aiming to get this early in the 4.5 cycle?
  
  
  Me and @ryanmce were discussing how we can implement the storage robustly in 
core and there are some design issues around this. He suggests use of JSON, a 
file for each path(remote) which has lot of benefits including fixing existing 
bugs. But I think we need to discuss this with community more and reaching to a 
consensus will be difficult in couple of days. So let's discuss this during the 
freeze and we can have the functionality early in 4.5 cycle.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D937

To: pulkit, #hg-reviewers, dlax, ryanmce
Cc: durin42, ryanmce, dlax, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D937: remotenames: move function to pull remotenames from the remoterepo to core

[pulkit (Pulkit Goyal)]

pulkit added a comment.


  In https://phab.mercurial-scm.org/D937#17855, @durin42 wrote:
  
  > Could we try and get this series ready to land before the freeze on 
Wednesday? or should we be aiming to get this early in the 4.5 cycle?
  
  
  Yes, I will be trying to get this series ready to land before the freeze.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D937

To: pulkit, #hg-reviewers, dlax, ryanmce
Cc: durin42, ryanmce, dlax, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D937: remotenames: move function to pull remotenames from the remoterepo to core

[durin42 (Augie Fackler)]

durin42 added a comment.


  Could we try and get this series ready to land before the freeze on 
Wednesday? or should we be aiming to get this early in the 4.5 cycle?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D937

To: pulkit, #hg-reviewers, dlax, ryanmce
Cc: durin42, ryanmce, dlax, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D937: remotenames: move function to pull remotenames from the remoterepo to core

[dlax (Denis Laxalde)]

dlax added inline comments.

INLINE COMMENTS

> ryanmce wrote in remotenames.py:11-16
> nit: for multiline docblocks, I prefer to leave the first line empty:
> 
>   """
>   pulls bookmarks and branches information of the remote repo during a
>   pull or clone operation.
>   
>   localrepo is our local repository
>   remoterepo is the repo from which we are pulling or cloning
>   remotepath is the path of the remote repository
>   """

While we are nitpicking on docstring, verbs should be in imperative form (so 
"pull" instead of "pulls") as the purpose is to document the effect of the 
function as a command (quoting PEP 257).
Also capitalizing sentences first word would be nice.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D937

To: pulkit, #hg-reviewers, dlax, ryanmce
Cc: ryanmce, dlax, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D937: remotenames: move function to pull remotenames from the remoterepo to core

[ryanmce (Ryan McElroy)]

ryanmce requested changes to this revision.
ryanmce added inline comments.

INLINE COMMENTS

> remotenames.py:10
> +
> +def pullremotenames(localrepo, remoterepo, remotepath):
> +""" pulls bookmarks and branches information of the remote repo during a

It might be better to call this `fetchremotenames` or similar since it might be 
called both on push and on pull

> remotenames.py:11-16
> +""" pulls bookmarks and branches information of the remote repo during a
> +pull or clone operation.
> +localrepo is our local repository
> +remoterepo is the repo from which we are pulling or cloning
> +remotepath is the path of the remote repository
> +"""

nit: for multiline docblocks, I prefer to leave the first line empty:

  """
  pulls bookmarks and branches information of the remote repo during a
  pull or clone operation.
  
  localrepo is our local repository
  remoterepo is the repo from which we are pulling or cloning
  remotepath is the path of the remote repository
  """

> remotenames.py:22-28
> +bmap = {}
> +repo = localrepo.unfiltered()
> +for branch, nodes in remoterepo.branchmap().iteritems():
> +bmap[branch] = []
> +for node in nodes:
> +if node in repo and not repo[node].obsolete():
> +bmap[branch].append(node)

It might be worth refactoring this function into:

(1) `fetchremotenames()`, a top-level function that loops through each 
supported namespace and then calls the appropriate fetching function for each 
name.
(2) `fetchremotebookmarks`, which does the appropriate stuff for bookmarks
(3) `fetchremotebranchheads`, which does the appropriate stuff for branch heads

That should make what's going on more clear.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D937

To: pulkit, #hg-reviewers, dlax, ryanmce
Cc: ryanmce, dlax, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D937: remotenames: move function to pull remotenames from the remoterepo to core

[pulkit (Pulkit Goyal)]

pulkit added a comment.


  In https://phab.mercurial-scm.org/D937#15824, @dlax wrote:
  
  > Also, it'd be useful to indicate where the code comes from (i.e. what is 
the "remoterepo" mentioned in the first line of the commit message).
  
  
  Yeah sure I will add that in next version.
  
  > Then, a module docstring explaining the concept at stake (starting by what 
is a "remotename") and the purpose of the extension would be really nice as a 
first step.
  
  The hgremotenames (https://bitbucket.org/seanfarley/hgremotenames/src) 
extension has very good module docstring which I plan to import once I ported 
enough functionality. If will port it just now, it will be like "it does not do 
what it says". Or I need to split up the docstring and to port some bit of it 
with each functionality added.
  And to mention, this is not an extension. This functionality is moving 
directly to core to improve bookmarks.

INLINE COMMENTS

> dlax wrote in remotenames.py:28
> I don't get the point of the function as it is in this patch. First it does 
> not do what the docstring says. Then it does not return anything nor does it 
> change any "state".

Yeah I agree with you. This series is porting things from hgremotenames 
extension to core. The extension has all these functionality which I tried 
breaking up in individual pieces and send as patches.  I will try to improve 
the ordering of how things are introduced in the next version of this series.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D937

To: pulkit, #hg-reviewers, dlax
Cc: dlax, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D937: remotenames: move function to pull remotenames from the remoterepo to core

[dlax (Denis Laxalde)]

dlax added a comment.


  Also, it'd be useful to indicate where the code comes from (i.e. what is the 
"remoterepo" mentioned in the first line of the commit message).
  
  Then, a module docstring explaining the concept at stake (starting by what is 
a "remotename") and the purpose of the extension would be really nice as a 
first step.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D937

To: pulkit, #hg-reviewers, dlax
Cc: dlax, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D937: remotenames: move function to pull remotenames from the remoterepo to core

[dlax (Denis Laxalde)]

dlax requested changes to this revision.
dlax added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> remotenames.py:28
> +if node in repo and not repo[node].obsolete():
> +bmap[branch].append(node)

I don't get the point of the function as it is in this patch. First it does not 
do what the docstring says. Then it does not return anything nor does it change 
any "state".

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D937

To: pulkit, #hg-reviewers, dlax
Cc: dlax, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D937: remotenames: move function to pull remotenames from the remoterepo to core

[pulkit (Pulkit Goyal)]

pulkit created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This patch moves the functionality to pull branches and bookmarks from a
  server from which we are pulling to core. The function used to exist in
  remotenames repo.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D937

AFFECTED FILES
  mercurial/remotenames.py

CHANGE DETAILS

diff --git a/mercurial/remotenames.py b/mercurial/remotenames.py
new file mode 100644
--- /dev/null
+++ b/mercurial/remotenames.py
@@ -0,0 +1,28 @@
+# remotenames.py
+#
+# Copyright 2017 Facebook, Inc.
+#
+# This software may be used and distributed according to the terms of the
+# GNU General Public License version 2 or any later version.
+
+from __future__ import absolute_import
+
+def pullremotenames(localrepo, remoterepo, remotepath):
+""" pulls bookmarks and branches information of the remote repo during a
+pull or clone operation.
+localrepo is our local repository
+remoterepo is the repo from which we are pulling or cloning
+remotepath is the path of the remote repository
+"""
+bookmarks = remoterepo.listkeys('bookmarks')
+# on a push, we don't want to keep obsolete heads since
+# they won't show up as heads on the next pull, so we
+# remove them here otherwise we would require the user
+# to issue a pull to refresh the storage
+bmap = {}
+repo = localrepo.unfiltered()
+for branch, nodes in remoterepo.branchmap().iteritems():
+bmap[branch] = []
+for node in nodes:
+if node in repo and not repo[node].obsolete():
+bmap[branch].append(node)



To: pulkit, #hg-reviewers
Cc: mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel