( )⚙ D1551 remotenames: consider existing data while storing newer data

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1551

remotenames: consider existing data while storing newer data
ClosedPublic

Authored by pulkit on Nov 29 2017, 4:58 PM.
Tags
None
Subscribers
grim, smf, durin42 and 2 others

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Commits
rHG2ea6e42ed15e: remotenames: consider existing data while storing newer data
Summary

Previously reviewed as D1357.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pulkit created this revision.Nov 29 2017, 4:58 PM
dlax added a subscriber: dlax.Dec 1 2017, 3:23 AM
durin42 added a subscriber: durin42.Dec 5 2017, 5:15 PM

I'm landing this series as-is, even though there are some good ideas for how we could improve the storage layer. My reasoning is this: we've had this as a semi-popular extension for literally years, it's functionality that we know is useful, and it'd be better to land something good rather than never land it in the name of wishing for something better. We've got almost two full months left in the cycle before this even ships as an experimental feature, so if anyone wants to try and iterate aggressively on the format I'd be delighted to talk over goals and review patches (but I'm far too busy to do any coding work, alas.)

durin42 accepted this revision.Dec 5 2017, 5:17 PM
This revision is now accepted and ready to land.Dec 5 2017, 5:17 PM
Closed by commit rHG2ea6e42ed15e: remotenames: consider existing data while storing newer data (authored by pulkit). · Explain WhyDec 5 2017, 5:23 PM
This revision was automatically updated to reflect the committed changes.
smf added a subscriber: smf.Dec 6 2017, 1:36 PM

durin42 (Augie Fackler) <phabricator@mercurial-scm.org> writes:

durin42 added a comment.

I'm landing this series as-is, even though there are some good ideas for how we could improve the storage layer. My reasoning is this: we've had this as a semi-popular extension for literally years, it's functionality that we know is useful, and it'd be better to land something good rather than never land it in the name of wishing for something better. We've got almost two full months left in the cycle before this even ships as an experimental feature, so if anyone wants to try and iterate aggressively on the format I'd be delighted to talk over goals and review patches (but I'm far too busy to do any coding work, alas.)

I hadn't finished writing my review of this patch, so that's awesome. I
have many concerns for this as-is. We need to have some kind of
measuring stick for quality in core instead of saying, "perfect is the
enemy of good," and letting everything through.

I'm not asking for perfect. I'm asking for this to be reworked to
something consistent for core. We haven't had a file format so external
pushed to core so quickly before. Histedit, journal, schemes, and even
mq have their own file formats which have been ironed out in hgext/.

For this patch as-is, it should at the very least be named
logexchange.py or something similar. "remotenames" is weird name to have
in core and already conflicts with the name of the extension it comes
from. I plan to back this out because there are so many things that need
to be worked out before this lands.

What happened to our quality for core?

grim added a subscriber: grim.Dec 6 2017, 1:53 PM

As an outsider looking in, I can see both points of view here. But as someone maintaining a large code base here I have to say I agree with Sean. The whole "we'll fix it later thing" is crap. It never happens as long as the code "works good enough". Case in point, I'm currently systematically removing 228 #if 0's from the Pidgin code base, most of which were put there more than a decade ago.

In my experience there are two times when code in this situation can/will be cleaned up. Before it's merged or when someone working on a bug fix/feature get's completely frustrated with it's state and ends up refactoring the crap out of it. Right now this feature is at the former but this discussion seems to be leading to the later, which is actively buying into the long term maintenance cost. Perhaps Mercurial can afford that cost, but this is not something I would advise/condone on my own projects.

durin42 added a comment.Dec 6 2017, 1:55 PM
In D1551#27331, @grim wrote:

As an outsider looking in, I can see both points of view here. But as someone maintaining a large code base here I have to say I agree with Sean. The whole "we'll fix it later thing" is crap. It never happens as long as the code "works good enough". Case in point, I'm currently systematically removing 228 #if 0's from the Pidgin code base, most of which were put there more than a decade ago.

That is, frankly, the point: smf has been requesting reasonable improvements in a thing that is already working and widely deployed that are then resulting in us shipping *nothing*.

In my experience there are two times when code in this situation can/will be cleaned up. Before it's merged or when someone working on a bug fix/feature get's completely frustrated with it's state and ends up refactoring the crap out of it. Right now this feature is at the former but this discussion seems to be leading to the later, which is actively buying into the long term maintenance cost. Perhaps Mercurial can afford that cost, but this is not something I would advise/condone on my own projects.

My expectation is that I have two realistic choices: reject the feature I want, or take it with a hope that someday someone improves it to be better than it is today. I don't believe there's a path (as you seem to?) that involves someone meeting a higher bar to get this feature shared to all hg users in the next year.

durin42 added a comment.Dec 6 2017, 2:20 PM
In D1551#27325, @smf wrote:

durin42 (Augie Fackler) <phabricator@mercurial-scm.org> writes:

durin42 added a comment.

I'm landing this series as-is, even though there are some good ideas for how we could improve the storage layer. My reasoning is this: we've had this as a semi-popular extension for literally years, it's functionality that we know is useful, and it'd be better to land something good rather than never land it in the name of wishing for something better. We've got almost two full months left in the cycle before this even ships as an experimental feature, so if anyone wants to try and iterate aggressively on the format I'd be delighted to talk over goals and review patches (but I'm far too busy to do any coding work, alas.)

I hadn't finished writing my review of this patch, so that's awesome. I
have many concerns for this as-is. We need to have some kind of
measuring stick for quality in core instead of saying, "perfect is the
enemy of good," and letting everything through.

Sean, the last state I had from you[0] was requiring three-step refactor that involved not-small work on at least blackbox and journal, as well as on remotenames. I don't think that's realistic (and so kills remotenames as something we can ship any fraction of in core), and it's been weeks since we had any action on that last round of patches. As far as I can tell, everyone that was part of that discussion agrees in broad strokes with your notions, but nobody is willing to do the work, certainly not as a blocker for landing this functionality in core.

0: https://phab.mercurial-scm.org/D1358#23866

I'm not asking for perfect. I'm asking for this to be reworked to
something consistent for core. We haven't had a file format so external
pushed to core so quickly before. Histedit, journal, schemes, and even
mq have their own file formats which have been ironed out in hgext/.

I'm confused. histedit landed in core in a single go, with no review on the file format (it was pickle! the worst possible choice!). The file format we're looking at here for remotenames is substantially similar to the one that's been in remotebranches for nearly a decade, so forgive me if I'm dubious that a file format refactor should be a blocker here.

For this patch as-is, it should at the very least be named
logexchange.py or something similar. "remotenames" is weird name to have
in core and already conflicts with the name of the extension it comes
from.

Per irc, Pulkit already has a patch ready to fix that.

I plan to back this out because there are so many things that need
to be worked out before this lands.

Do not back this out without getting someone else to agree with you first. As of now I see a new complaint in this message from you (please rename this) which is fine, and is something we can roll forward on.

What happened to our quality for core?

Patience. If I'd known about the naming concern I would have waited to land this, but I never saw that complaint in the almost a month this patch (or its precursor stack related to D1358) have been visible. I can't read minds, and it's consistent with past decisions to bring things in without name changes (we did that on histedit!), but I see why we should behave differently here (since we're only bringing parts of remotenames to core.)

Does that help clarify things?

smf added a comment.Dec 8 2017, 3:02 AM

durin42 (Augie Fackler) <phabricator@mercurial-scm.org> writes:

durin42 added a comment.

In https://phab.mercurial-scm.org/D1551#27325, @smf wrote:
> durin42 (Augie Fackler) <phabricator@mercurial-scm.org> writes:
>
> > durin42 added a comment.
> >
> >   I'm landing this series as-is, even though there are some good ideas for how we could improve the storage layer. My reasoning is this: we've had this as a semi-popular extension for literally years, it's functionality that we know is useful, and it'd be better to land something good rather than never land it in the name of wishing for something better. We've got almost two full months left in the cycle before this even ships as an experimental feature, so if anyone wants to try and iterate aggressively on the format I'd be delighted to talk over goals and review patches (but I'm far too busy to do any coding work, alas.)
>
> I hadn't finished writing my review of this patch, so that's awesome. I
>  have many concerns for this as-is. We need to have some kind of
>  measuring stick for quality in core instead of saying, "perfect is the
>  enemy of good," and letting everything through.
Sean, the last state I had from you[0] was requiring three-step refactor that involved not-small work on at least blackbox and journal, as well as on remotenames. I don't think that's realistic (and so kills remotenames as something we can ship any fraction of in core), and it's been weeks since we had any action on that last round of patches. As far as I can tell, everyone that was part of that discussion agrees in broad strokes with your notions, but nobody is willing to do the work, certainly not as a blocker for landing this functionality in core.
0: https://phab.mercurial-scm.org/D1358#23866
> I'm not asking for perfect. I'm asking for this to be reworked to
>  something consistent for core. We haven't had a file format so external
>  pushed to core so quickly before. Histedit, journal, schemes, and even
>  mq have their own file formats which have been ironed out in hgext/.
I'm confused. histedit landed in core in a single go, with no review on the file format (it was pickle! the worst possible choice!). The file format we're looking at here for remotenames is substantially similar to the one that's been in remotebranches for nearly a decade, so forgive me if I'm dubious that a file format refactor should be a blocker here.

histedit landed in hgext/. My push back is to this putting remotenames
in mercurial/. In fact, I can't think of an extension that has skipped
going into hgext/ like this has.

As I've brought up before, there are scheme fixes, path patches, and
other minor things that could be improved before even getting to this
patch.

> For this patch as-is, it should at the very least be named
>  logexchange.py or something similar. "remotenames" is weird name to have
>  in core and already conflicts with the name of the extension it comes
>  from.
Per irc, Pulkit already has a patch ready to fix that.
> I plan to back this out because there are so many things that need
>  to be worked out before this lands.
Do not back this out without getting someone else to agree with you first. As of now I see a new complaint in this message from you (please rename this) which is fine, and is something we can roll forward on.
> What happened to our quality for core?
Patience. If I'd known about the naming concern I would have waited to land this, but I never saw that complaint in the almost a month this patch (or its precursor stack related to https://phab.mercurial-scm.org/D1358) have been visible. I can't read minds, and it's consistent with past decisions to bring things in without name changes (we did that on histedit!), but I see why we should behave differently here (since we're only bringing parts of remotenames to core.)
Does that help clarify things?

Again, histedit landed in hgext/. As for my delay in responding to this
list: I've taken a break because it doesn't seem that healthy debate is
encouraged or welcomed. For a while now, I've observed more
strong-arming and less collaboration.

Revision Contents
Changeset List

Diff 4142

mercurial/remotenames.py

def writeremotenamefile(repo, remotepath, names, nametype): def writeremotenamefile(repo, remotepath, names, nametype):
vfs = vfsmod.vfs(repo.vfs.join(remotenamedir)) vfs = vfsmod.vfs(repo.vfs.join(remotenamedir))
f = vfs(nametype, 'w', atomictemp=True) f = vfs(nametype, 'w', atomictemp=True)
# write the storage version info on top of file # write the storage version info on top of file
# version '0' represents the very initial version of the storage format # version '0' represents the very initial version of the storage format
f.write('0\n\n') f.write('0\n\n')
olddata = set(readremotenamefile(repo, nametype))
# re-save the data from a different remote than this one.
for node, oldpath, rname in sorted(olddata):
if oldpath != remotepath:
f.write('%s\0%s\0%s\n' % (node, oldpath, rname))
for name, node in sorted(names.iteritems()): for name, node in sorted(names.iteritems()):
if nametype == "branches": if nametype == "branches":
for n in node: for n in node:
f.write('%s\0%s\0%s\n' % (n, remotepath, name)) f.write('%s\0%s\0%s\n' % (n, remotepath, name))
elif nametype == "bookmarks": elif nametype == "bookmarks":
if node: if node:
f.write('%s\0%s\0%s\n' % (node, remotepath, name)) f.write('%s\0%s\0%s\n' % (node, remotepath, name))

tests/test-remotenames.t

$ cd ../client/ $ cd ../client/
$ hg pull ../server2/ $ hg pull ../server2/
pulling from ../server2/ pulling from ../server2/
searching for changes searching for changes
no changes found no changes found
$ cat .hg/remotenames/bookmarks $ cat .hg/remotenames/bookmarks
0 0
62615734edd52f06b6fb9c2beb429e4fe30d57b8\x00file:$TESTTMP/server\x00foo (esc)
87d6d66763085b629e6d7ed56778c79827273022\x00file:$TESTTMP/server\x00bar (esc)
87d6d66763085b629e6d7ed56778c79827273022\x00file:$TESTTMP/server2\x00bar (esc) 87d6d66763085b629e6d7ed56778c79827273022\x00file:$TESTTMP/server2\x00bar (esc)
62615734edd52f06b6fb9c2beb429e4fe30d57b8\x00file:$TESTTMP/server2\x00foo (esc) 62615734edd52f06b6fb9c2beb429e4fe30d57b8\x00file:$TESTTMP/server2\x00foo (esc)
$ cat .hg/remotenames/branches $ cat .hg/remotenames/branches
0 0
3e1487808078543b0af6d10dadf5d46943578db0\x00file:$TESTTMP/server\x00wat (esc)
ec2426147f0e39dbc9cef599b066be6035ce691d\x00file:$TESTTMP/server\x00default (esc)
ec2426147f0e39dbc9cef599b066be6035ce691d\x00file:$TESTTMP/server2\x00default (esc) ec2426147f0e39dbc9cef599b066be6035ce691d\x00file:$TESTTMP/server2\x00default (esc)
3e1487808078543b0af6d10dadf5d46943578db0\x00file:$TESTTMP/server2\x00wat (esc) 3e1487808078543b0af6d10dadf5d46943578db0\x00file:$TESTTMP/server2\x00wat (esc)