This is an archive of the discontinued Mercurial Phabricator instance.

remotenames: add functionality to store remotenames under .hg/hgremotenames/
AbandonedPublic

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

Download Raw Diff

Details

Reviewers

dlax
ryanmce

Group Reviewers

hg-reviewers

Summary

This patch moves the functionality from remotenames extension to store
remotenames in .hg/remotenames file to core by changing following things:

Each type of remotename will be stored in it's own specific file, hence we

are now storing bookmarks in .hg/remotenames/bookmarks and branches in
.hg/remotenames/branches.

Since each type of remotename is stored in specific file, the format of the

data stored is dropped to emit the nametype thing.

The logic to sync with existing remotenames file and saving journals and other
related things will be moved to core in next patches incrementally.

Thanks to Ryan McElroy to suggesting to store each type in their own file which
makes the data format more better and makes the storage extensible.

The functions saveremotebookmarks and saveremotebranches looks very much same
for now but it's preferred not to merge them because there will be bookmark
specific logic like storing journal and will be easy to plug in more type
specific logic.

Diff Detail

Repository

rHG Mercurial

Lint

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

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

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

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

pulkit added a child revision: D940: remotenames: add functions to read remotenames data from .hg/remotenames/.Oct 4 2017, 5:53 PM

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

dlax added a subscriber: dlax.

dlax added inline comments.

mercurial/remotenames.py
31	`repo` parameter is not used
45	Incorrect indentation.
48	ditto for `repo`
64	Functions `saveremotebranches` and `saveremotebookmarks` are very similar, only differing by the vfs file name. Perhaps there could be a single function taking the filename as a parameter?

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

I think we need to take a step back and think through how to implement this more robustly than we have currently done in the remotenames extension:

storage file names
storage format

I like that you've split branches and bookmarks, but I suspect that storing all paths in the same file isn't the best option.

mercurial/remotenames.py
35	As per the issue you raised about paths containing '/', this probably is insufficient.
64	+1
67–69	nit: start multiline docblocks on next line

In D939#17129, @ryanmce wrote:

I think we need to take a step back and think through how to implement this more robustly than we have currently done in the remotenames extension:

storage file names

storage format

I like that you've split branches and bookmarks, but I suspect that storing all paths in the same file isn't the best option.

I like the JSON idea you mentioned in a previous patch. Can you explain your concern about "storing all paths in the same file"?

Will revisit after the freeze.

pulkit edited the summary of this revision. (Show Details)Nov 10 2017, 2:01 PM

pulkit added a child revision: D1356: remotenames: add test showing overwriting on remotenames data.

pulkit updated this revision to Diff 3393.

pulkit added a subscriber: durin42.Nov 13 2017, 12:45 PM

pulkit added inline comments.

tests/test-remotenames.t
69 ↗	(On Diff #3393)	@durin42 related to your comment on list[0], is space separated format okay or null separated is preferred? [0]: https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-November/107528.html

I'm confused by the state of this diff - it appears to be modifying a file which doesn't yet exist in hg?

tests/test-remotenames.t
69 ↗	(On Diff #3393)	This is sort of okay, but won't scale well. I'd rather we used something we know we'll never see in a path like NUL rather than space separated, but this is probably fine. I might bias towards making the first line an int followed by a newline so we can easily iterate on the file format and migrate things in the future.

pulkit mentioned this in D1548: remotenames: add functionality to store remotenames under .hg/hgremotenames/.Nov 29 2017, 4:57 PM

Updated the series with new differentials D1547 to D1551.

pulkit (Pulkit Goyal) a écrit :

Updated the series with new differentials D1547
https://phab.mercurial-scm.org/D1547 to D1551
https://phab.mercurial-scm.org/D1551.

Why creating new differentials? I thought the very point of Phabricator
was to make it possible to review several versions of a given patch on
the same entity. By abandoning and creating new differentials, we loose
previous review feedback and it breaks the subscription group (which,
given how handy it is subscribe to a series of differentials, is not
really nice).

In D939#26379, @dlax wrote:

pulkit (Pulkit Goyal) a écrit :

Updated the series with new differentials D1547
https://phab.mercurial-scm.org/D1547 to D1551
https://phab.mercurial-scm.org/D1551.

Why creating new differentials? I thought the very point of Phabricator
was to make it possible to review several versions of a given patch on
the same entity. By abandoning and creating new differentials, we loose
previous review feedback and it breaks the subscription group (which,
given how handy it is subscribe to a series of differentials, is not
really nice).

Indeed, but something happened with this series such taht I can no longer figure out what's going on. Pulkit created the new series at my request so I can review something coherent. It's a mess, but it'll unblock the review. :(

pulkit mentioned this in rHG8df8ce2cc5dd: remotenames: add functionality to store remotenames under .hg/hgremotenames/.Dec 5 2017, 5:23 PM

Revision Contents
Changeset List

			Path	Packages
M			mercurial/remotenames.py (62 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 2436

mercurial/remotenames.py

	# remotenames.py			# remotenames.py
	#			#
	# Copyright 2017 Facebook, Inc.			# Copyright 2017 Facebook, Inc.
	#			#
	# This software may be used and distributed according to the terms of the			# This software may be used and distributed according to the terms of the
	# GNU General Public License version 2 or any later version.			# GNU General Public License version 2 or any later version.

	from __future__ import absolute_import			from __future__ import absolute_import

				from .node import (
				hex,
				)

				from . import (
				vfs as vfsmod,
				)

				remotenamedir = 'remotenames'

	def splitremotename(remote):			def splitremotename(remote):
	name = ''			name = ''
	if '/' in remote:			if '/' in remote:
	remote, name = remote.split('/', 1)			remote, name = remote.split('/', 1)
	return remote, name			return remote, name

	def joinremotename(remote, ref):			def joinremotename(remote, ref):
	if ref:			if ref:
	remote += '/' + ref			remote += '/' + ref
	return remote			return remote

				def saveremotebookmarks(repo, vfs, remotepath, bookmarks):
				dlaxUnsubmitted Not Done `repo` parameter is not used dlax: `repo` parameter is not used
				""" save remote bookmarks in .hg/remotenames/bookmarks.
				The format of the data stored will be:

				`node remotepath/bookmarkname`
				ryanmceUnsubmitted Not Done As per the issue you raised about paths containing '/', this probably is insufficient. ryanmce: As per the issue you raised about paths containing '/', this probably is insufficient.

				bookmarks is a dictionary of remote bookmarks.
				"""

				f = vfs('bookmarks', 'w', atomictemp=True)

				for bookmark, node in bookmarks.iteritems():
				if node:
				f.write('%s %s\n' %
				(node, joinremotename(remotepath, bookmark)))
				dlaxUnsubmitted Not Done Incorrect indentation. dlax: Incorrect indentation.
				f.close()

				def saveremotebranches(repo, vfs, remotepath, branches):
				dlaxUnsubmitted Not Done ditto for `repo` dlax: ditto for `repo`
				""" save remote branches is .hg/remotenames/branches.
				The format of the data stored will be:

				`node remotepath/branchname`

				branches is a dictionary of remote branches.
				"""

				f = vfs('branches', 'w', atomictemp=True)

				for branch, nodes in branches.iteritems():
				for n in nodes:
				rname = joinremotename(remotepath, branch)
				f.write('%s %s\n' % (hex(n), rname))

				f.close()
				dlaxUnsubmitted Not Done Functions `saveremotebranches` and `saveremotebookmarks` are very similar, only differing by the vfs file name. Perhaps there could be a single function taking the filename as a parameter? dlax: Functions `saveremotebranches` and `saveremotebookmarks` are very similar, only differing by…
				ryanmceUnsubmitted Not Done +1 ryanmce: +1

				def saveremotenames(repo, remotepath, branches=None, bookmarks=None):
				""" save remotenames i.e. remotebookmarks and remotebranches in their
				respective files under ".hg/remotenames/" directory.
				"""
				ryanmceUnsubmitted Not Done nit: start multiline docblocks on next line ryanmce: nit: start multiline docblocks on next line

				vfs = vfsmod.vfs(repo.vfs.join(remotenamedir))
				wlock = repo.wlock()
				try:
				if bookmarks:
				saveremotebookmarks(repo, vfs, remotepath, bookmarks)
				if branches:
				saveremotebranches(repo, vfs, remotepath, branches)
				finally:
				wlock.release()

	def pullremotenames(localrepo, remoterepo, remotepath):			def pullremotenames(localrepo, remoterepo, remotepath):
	""" pulls bookmarks and branches information of the remote repo during a			""" pulls bookmarks and branches information of the remote repo during a
	pull or clone operation.			pull or clone operation.
	localrepo is our local repository			localrepo is our local repository
	remoterepo is the repo from which we are pulling or cloning			remoterepo is the repo from which we are pulling or cloning
	remotepath is the path of the remote repository			remotepath is the path of the remote repository
	"""			"""
	bookmarks = remoterepo.listkeys('bookmarks')			bookmarks = remoterepo.listkeys('bookmarks')
	# on a push, we don't want to keep obsolete heads since			# 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			# they won't show up as heads on the next pull, so we
	# remove them here otherwise we would require the user			# remove them here otherwise we would require the user
	# to issue a pull to refresh the storage			# to issue a pull to refresh the storage
	bmap = {}			bmap = {}
	repo = localrepo.unfiltered()			repo = localrepo.unfiltered()
	for branch, nodes in remoterepo.branchmap().iteritems():			for branch, nodes in remoterepo.branchmap().iteritems():
	bmap[branch] = []			bmap[branch] = []
	for node in nodes:			for node in nodes:
	if node in repo and not repo[node].obsolete():			if node in repo and not repo[node].obsolete():
	bmap[branch].append(node)			bmap[branch].append(node)

				saveremotenames(localrepo, remotepath, bmap, bookmarks)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	2436		Oct 4 2017, 5:53 PM	★	★
Diff 2	3393		Nov 10 2017, 2:00 PM	★	★