This is an archive of the discontinued Mercurial Phabricator instance.

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

Authored by pulkit on Oct 4 2017, 5:52 PM.

Download Raw Diff

Details

Reviewers

dlax
ryanmce

Group Reviewers

hg-reviewers

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.

Diff Detail

Repository

rHG Mercurial

Lint

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

pulkit created this revision.Oct 4 2017, 5:52 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptOct 4 2017, 5:52 PM

Herald added a subscriber: mercurial-devel. · View Herald Transcript

pulkit added a child revision: D938: remotenames: move utility functions to join and split names to core.Oct 4 2017, 5:53 PM

dlax requested changes to this revision.Oct 5 2017, 3:18 AM

dlax added a subscriber: dlax.

dlax added inline comments.

mercurial/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".

This revision now requires changes to proceed.Oct 5 2017, 3:18 AM

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.

In 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.

mercurial/remotenames.py
28	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.

ryanmce requested changes to this revision.Oct 12 2017, 7:51 AM

ryanmce added a subscriber: ryanmce.

ryanmce added inline comments.

mercurial/remotenames.py
10	It might be better to call this `fetchremotenames` or similar since it might be called both on push and on pull
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 """
22–28	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.

dlax added inline comments.Oct 12 2017, 8:19 AM

mercurial/remotenames.py
11–16	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.

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?

In 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.

In 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.

pulkit (Pulkit Goyal) <phabricator@mercurial-scm.org> 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:

the base layer: bugfixes and storage format

the remote tracking layer: simple wrappers that sit in-between push /

pull and write out the remote info

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&fileviewer=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.asc832 BDownload

pulkit mentioned this in D1547: remotenames: move function to pull remotenames from the remoterepo to core.Nov 29 2017, 4:57 PM

pulkit mentioned this in rHG5a62910948d2: remotenames: move function to pull remotenames from the remoterepo to core.Dec 5 2017, 5:22 PM

Revision Contents
Changeset List

			Path	Packages
A	M		mercurial/remotenames.py (28 lines)

Status	Author	Revision
Abandoned	pulkit	D1358 remotenames: store journal entry for bookmarks if journal is loaded
Abandoned	pulkit	D1357 remotenames: consider existing data while storing newer data
Abandoned	pulkit	D941 remotenames: add a function to return the vfs object
Abandoned	pulkit	D940 remotenames: add functions to read remotenames data from .hg/remotenames/
Abandoned	pulkit	D1356 remotenames: add test showing overwriting on remotenames data
Abandoned	pulkit	D939 remotenames: add functionality to store remotenames under .hg/hgremotenames/
Abandoned	pulkit	D938 remotenames: move utility functions to join and split names to core
Abandoned	pulkit	D937 remotenames: move function to pull remotenames from the remoterepo to core

Diff 2434

mercurial/remotenames.py

This file was added.

				# 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):
				ryanmceUnsubmitted Not Done It might be better to call this `fetchremotenames` or similar since it might be called both on push and on pull ryanmce: It might be better to call this `fetchremotenames` or similar since it might be called both on…
				""" 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
				"""
				ryanmceUnsubmitted Not Done 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 """ ryanmce: nit: for multiline docblocks, I prefer to leave the first line empty: ``` """ pulls…
				dlaxUnsubmitted Not Done 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. dlax: While we are nitpicking on docstring, verbs should be in imperative form (so "pull" instead of…
				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)
				dlaxUnsubmitted Not Done 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". dlax: I don't get the point of the function as it is in this patch. First it does not do what the…
				pulkitAuthorUnsubmitted Not Done 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. pulkit: Yeah I agree with you. This series is porting things from hgremotenames extension to core. The…
				ryanmceUnsubmitted Not Done 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. ryanmce: It might be worth refactoring this function into: (1) `fetchremotenames()`, a top-level…

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	2434			Oct 4 2017, 5:52 PM	★	★