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.
Tags
None
Subscribers

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
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
dlax added a comment.Oct 5 2017, 3:44 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.

pulkit added a comment.Oct 5 2017, 6:13 AM
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.

pulkit abandoned this revision.Oct 16 2017, 1:05 PM
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.

smf added a subscriber: smf.Nov 1 2017, 3:29 PM

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:

  1. the base layer: bugfixes and storage format
  1. the remote tracking layer: simple wrappers that sit in-between push /

pull and write out the remote info

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