Page MenuHomePhabricator

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

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

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:

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

  1. 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
dlax requested changes to this revision.Oct 5 2017, 3:57 AM
dlax added a subscriber: dlax.
dlax added inline comments.
mercurial/remotenames.py
13

repo parameter is not used

27

Incorrect indentation.

30

ditto for repo

46

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
ryanmce requested changes to this revision.Oct 12 2017, 8:39 AM
ryanmce added a subscriber: ryanmce.

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
17

As per the issue you raised about paths containing '/', this probably is insufficient.

46

+1

49–51

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"?

pulkit abandoned this revision.Oct 17 2017, 7:20 PM

Will revisit after the freeze.

pulkit edited the summary of this revision. (Show Details)Nov 10 2017, 2:01 PM
pulkit updated this revision to Diff 3393.
pulkit added inline comments.
tests/test-remotenames.t
69

@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

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 abandoned this revision.Nov 29 2017, 5:04 PM

Updated the series with new differentials D1547 to D1551.

dlax added a comment.Nov 30 2017, 3:12 AM

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. :(