Merge lp:~gary/launchpad/bug643715 into lp:launchpad

Proposed by Gary Poster on 2010-09-20
Status: Work in progress
Proposed branch: lp:~gary/launchpad/bug643715
Merge into: lp:launchpad
Diff against target: 96 lines (+28/-9)
1 file modified
lib/devscripts/sourcecode.py (+28/-9)
To merge this branch: bzr merge lp:~gary/launchpad/bug643715
Reviewer Review Type Date Requested Status
Aaron Bentley (community) Needs Fixing on 2010-09-20
Jonathan Lange (community) 2010-09-20 Approve on 2010-09-20
Review via email: mp+36060@code.launchpad.net

Description of the change

This was just a quick itch to scratch as I was updating my tree after being away for a week. It makes calling update-sourcecode much faster.

To test, run

./utilities/update-sourcecode

If you want to play around, you can change utilities/sourcedeps.conf to revert a revision, then run update-sourcecode, and then change the revision back. I could optimize that too, but I don't think that's an important win for the common case, so I'm not worrying about it.

I made a variety of changes for lint, even though some of them didn't make a lot of sense to me. I was surprised in particular by the preference for ('foo', ) over ('foo',). It's alright.

Gary

To post a comment you must log in.
Jonathan Lange (jml) wrote :

Its preference for (foo, ) over (foo,) is wrong. Every linting tool that isn't pyflakes is terrible.

Thanks for doing this.

review: Approve
Aaron Bentley (abentley) wrote :

This branch treats revnos as if they were revision ids, which means that it may not notice cases where uncommit or push --overwrite have been used.

It also appears not to account for changes in the branch location.

It also assumes that "revision" is a revno, when other parts of the code treat it as a RevisionSpec instead.

review: Needs Fixing
Gary Poster (gary) wrote :

My change was an optimization for the common case. What we had already was "correct" for the edge cases you identified.

On one hand:

- I could make an additional change to handle the case of change in the branch location.

- We use revnos only, practically, so I think that's fine, myself.

However:

- I don't see how to handle an uncommit or an --overwrite.

I think those are pretty unlikely cases, but if we care about handling them, I think this is dead in the water. In that case, Aaron, isn't this actually a Rejected?

Aaron Bentley (abentley) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/20/2010 04:03 PM, Gary Poster wrote:
> My change was an optimization for the common case. What we had already was "correct" for the edge cases you identified.
>
> On one hand:
>
> - I could make an additional change to handle the case of change in the branch location.
>
> - We use revnos only, practically, so I think that's fine, myself.

That's as may be, but it's actually faster and requires less code to
convert from revision_spec to revision_id and then compare to
wt.last_revision().

> However:
>
> - I don't see how to handle an uncommit or an --overwrite.

You could handle both branch location changes and revno redefinition by
auto-generating a list of revision-ids from update-sourcecode. The
output of this would need to be versioned.

> I think those are pretty unlikely cases

Also, there's merge trunk, commit, push to trunk.

But given that these are all PQM-managed branches and everyone who has
write access to them is pretty sane, I guess I'd agree that those cases
are unlikely.

, but if we care about handling them, I think this is dead in the water.
 In that case, Aaron, isn't this actually a Rejected?

I think we can make it faster without making it less effective.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkyXzFAACgkQ0F+nu1YWqI1pcQCgiU2lEcyIcXnrvBZaAw1Izulj
jVkAn09Z9jlR13ZuWc5Wgv6plAm4nl0p
=YDgF
-----END PGP SIGNATURE-----

Unmerged revisions

11582. By Gary Poster on 2010-09-20

make lint happy

11581. By Gary Poster on 2010-09-20

import missing exception

11580. By Gary Poster on 2010-09-20

optimize case in sourcecode update of already being on the desired revision.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/devscripts/sourcecode.py'
2--- lib/devscripts/sourcecode.py 2010-04-19 11:05:07 +0000
3+++ lib/devscripts/sourcecode.py 2010-09-20 19:31:23 +0000
4@@ -16,7 +16,11 @@
5 import sys
6
7 from bzrlib.branch import Branch
8-from bzrlib.errors import BzrError, NotBranchError, IncompatibleRepositories
9+from bzrlib.errors import (
10+ BzrError,
11+ NotBranchError,
12+ IncompatibleRepositories,
13+ NoSuchRevision)
14 from bzrlib.plugin import load_plugins
15 from bzrlib.revisionspec import RevisionSpec
16 from bzrlib.trace import enable_default_logging, report_exception
17@@ -135,14 +139,13 @@
18 return spec.as_revision_id(from_branch)
19 # else return None
20
21-
22 def _format_revision_name(revision, tip=False):
23 """Formatting helper to return human-readable identifier for revision.
24
25 If ``tip`` is True, the revision value will be ignored.
26 """
27 if not tip and revision:
28- return 'revision %s' % (revision,)
29+ return 'revision %s' % (revision, )
30 else:
31 return 'tip'
32
33@@ -178,6 +181,17 @@
34 possible_transports=possible_transports)
35
36
37+def get_revno(wt):
38+ """Get the revno of a working tree."""
39+ # Ripped from bzrlib.builtins.cmd_revno.run.
40+ revid = wt.last_revision()
41+ try:
42+ revno_t = wt.branch.revision_id_to_dotted_revno(revid)
43+ except NoSuchRevision:
44+ revno_t = ('???', )
45+ return ".".join(str(n) for n in revno_t)
46+
47+
48 def update_branches(sourcecode_directory, update_branches,
49 possible_transports=None, tip=False, quiet=False):
50 """Update the existing branches in sourcecode."""
51@@ -193,6 +207,12 @@
52 print 'Updating %s to %s' % (
53 project, _format_revision_name(revision, tip))
54 local_tree = WorkingTree.open(destination)
55+ # See if we can shortcut: do we already have the desired revision?
56+ if not tip and revision == get_revno(local_tree):
57+ if not quiet:
58+ print ' (No change)'
59+ continue
60+ # Apparently we do not. Open the remote branch.
61 try:
62 remote_branch = Branch.open(
63 branch_url, possible_transports=possible_transports)
64@@ -210,10 +230,10 @@
65 remote_branch, stop_revision=revision_id, overwrite=True,
66 possible_transports=possible_transports)
67 except IncompatibleRepositories:
68- # XXX JRV 20100407: Ideally remote_branch.bzrdir._format
69+ # XXX JRV 20100407: Ideally remote_branch.bzrdir._format
70 # should be passed into upgrade() to ensure the format is the same
71- # locally and remotely. Unfortunately smart server branches
72- # have their _format set to RemoteFormat rather than an actual
73+ # locally and remotely. Unfortunately smart server branches
74+ # have their _format set to RemoteFormat rather than an actual
75 # format instance.
76 upgrade(destination)
77 # Upgraded, repoen working tree
78@@ -277,7 +297,6 @@
79 # differ from each other (because of developers fiddling with things), we can
80 # take a survey of all of them, and choose the most popular.
81
82-
83 def main(args):
84 parser = optparse.OptionParser("usage: %prog [options] [root [conffile]]")
85 parser.add_option(
86@@ -305,8 +324,8 @@
87 if len(args) > 3:
88 parser.error("Too many arguments.")
89 if not options.quiet:
90- print 'Sourcecode: %s' % (sourcecode_directory,)
91- print 'Config: %s' % (config_filename,)
92+ print 'Sourcecode: %s' % (sourcecode_directory, )
93+ print 'Config: %s' % (config_filename, )
94 enable_default_logging()
95 # Tell bzr to use the terminal (if any) to show progress bars
96 ui.ui_factory = ui.make_ui_for_terminal(