Merge lp:~philip-peitsch/bzr/annotate-ghost-revs into lp:bzr

Proposed by Philip Peitsch
Status: Work in progress
Proposed branch: lp:~philip-peitsch/bzr/annotate-ghost-revs
Merge into: lp:bzr
Diff against target: 22 lines (+5/-5)
1 file modified
bzrlib/annotate.py (+5/-5)
To merge this branch: bzr merge lp:~philip-peitsch/bzr/annotate-ghost-revs
Reviewer Review Type Date Requested Status
Martin Pool Needs Information
Review via email: mp+26460@code.launchpad.net

Description of the change

I modified the indenting from line 232-238 so that if the origin is not in the revisions, it returns '?' for the rev number, author and date. This prevents it crashing if ghost revisions are involved... though this doesn't really help if one is interested in the data in these ghost revisions.

To post a comment you must log in.
Revision history for this message
Martin Pool (mbp) wrote :

Thanks very much, that's a nice simple fix for this bug. I might incline towards unfolding the first "if" to the higher level so this does not get too indented.

Could you please complete the contributor agreement in <http://www.canonical.com/contributors> so that we can merge this?

review: Needs Information
4848. By Philip Peitsch

* Unfolded if clause in annotate.py up a level to reduce nesting slightly
* assign last_origin after all if clauses as this seems easier to read (and less error prone to being missed :))

4849. By Philip Peitsch

Merging bzr dev branch

4850. By Philip Peitsch

Cleaning up whitespace diffs and putting last_origin back inside elif and else clauses

4851. By Philip Peitsch

Fixing up bzr +x bit

Revision history for this message
Philip Peitsch (philip-peitsch) wrote :

I've completed contributor agreement as requested, and cleaned up the diff by merging in latest bzr head. If you have any more tweaks you'd like, let me know :)

P.S., I can't actually find any docs on the matter in launchpad, but is it better to resubmit a merge proposal or just re-use the existing one like this?

Revision history for this message
John A Meinel (jameinel) wrote :

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

Philip Peitsch wrote:
> I've completed contributor agreement as requested, and cleaned up the diff by merging in latest bzr head. If you have any more tweaks you'd like, let me know :)
>
> P.S., I can't actually find any docs on the matter in launchpad, but is it better to resubmit a merge proposal or just re-use the existing one like this?

It has changed over time :). It used to be better to re-use one, because
otherwise you would lose comments.

However, it is now better to resubmit, as the comments get carried
across, and it resets any votes.

In this particular case, there is no need, as your changes are small,
and the diff will be updated.

I would also mention that we probably would want a test for this case,
to help prevent it from regressing.

I realize it is a bit hard to inject ghosts into the system, because it
is *designed* to not make it easy. (ATM, I think only bzr-svn is able to
label a text as coming from a ghost.)

So we can try to help you with writing that test, but we really should
have something so that we maintain support for it.

Did you at least test it interactively? (run 'bzr annotate' on something
you know has ghosts?)

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkwJCzIACgkQJdeBCYSNAAMRCwCgsFAMRXDuIjXM7uvFEAtryb0T
WlYAn2Hlhz6Iu8TYZYfjgVMxJP9ho7WE
=WdGm
-----END PGP SIGNATURE-----

Revision history for this message
Philip Peitsch (philip-peitsch) wrote :

Hi John,

Ahh... good to know :)

For testing, I have actually been using bzr-svn for generating these. I've attached a script that can produce this problem to bug #393905. I have been running this, then pointing it to the patched bzr and re-running to verify that it no longer crashes.

Do you know of any way to inject one using unit tests or similar? I hadn't looked closely enough at the unit-testing to figure out if there was a straightforward way of doing this, but if you can point me towards any similar tests that may exist (or docs that might help) I'm happy to research it more :)

Revision history for this message
John A Meinel (jameinel) wrote :

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

Philip Peitsch wrote:
> Hi John,
>
> Ahh... good to know :)
>
> For testing, I have actually been using bzr-svn for generating these. I've attached a script that can produce this problem to bug #393905. I have been running this, then pointing it to the patched bzr and re-running to verify that it no longer crashes.
>
> Do you know of any way to inject one using unit tests or similar? I hadn't looked closely enough at the unit-testing to figure out if there was a straightforward way of doing this, but if you can point me towards any similar tests that may exist (or docs that might help) I'm happy to research it more :)

See:

http://bazaar.launchpad.net/~jameinel/bzr/2.2-create-ghost/revision/5285

For a way to introduce ghosts into the history. It isn't particularly
pretty, but I tested that it works against all repository formats.

That should give you both a text with ghosts, and a revision that is a
ghost.

If you need more history, you can just create another revision (with a
new file text), etc.

This seems like something that would be good to put on a low-level
class, so that we can test ghosts at various levels easily. Mostly
because creating a ghost is ugly/difficult enough, that it is probably
best as a helper function.

Maybe Martin has a good idea of how to put it into a Factory setting.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkwNYlkACgkQJdeBCYSNAAML1QCfZgFR6P3dWKgGc9gCiBaoEKLC
d5UAoMJOR4mZKQ46/yoVcYLNrb7Jr5ZM
=YZt6
-----END PGP SIGNATURE-----

Revision history for this message
Martin Pool (mbp) wrote :

On 8 June 2010 07:19, John Arbash Meinel <email address hidden> wrote:
> If you need more history, you can just create another revision (with a
> new file text), etc.
>
>
> This seems like something that would be good to put on a low-level
> class, so that we can test ghosts at various levels easily. Mostly
> because creating a ghost is ugly/difficult enough, that it is probably
> best as a helper function.
>
> Maybe Martin has a good idea of how to put it into a Factory setting.
>

I'd like to. My next step there is to take some of Robert & others'
feedback on my previous attempt and to turn that into a patch to
testtools (from whence our base TestCase comes) that sets the standard
pattern.

For now I would suggest you just factor it out into a function or
method on whichever class wants to use it.

--
Martin

Revision history for this message
Vincent Ladeuil (vila) wrote :

@Philip: I'm setting the status to Work In Progress, if you need help to write the tests asked for by Martin and John, just say so and I'll try to help.

Unmerged revisions

4851. By Philip Peitsch

Fixing up bzr +x bit

4850. By Philip Peitsch

Cleaning up whitespace diffs and putting last_origin back inside elif and else clauses

4849. By Philip Peitsch

Merging bzr dev branch

4848. By Philip Peitsch

* Unfolded if clause in annotate.py up a level to reduce nesting slightly
* assign last_origin after all if clauses as this seems easier to read (and less error prone to being missed :))

4847. By zpp <zpp@launchpad>

Corrected indenting in annotate so that annotate won't crash if the file was modified in a merge

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/annotate.py'
2--- bzrlib/annotate.py 2010-06-04 03:09:35 +0000
3+++ bzrlib/annotate.py 2010-06-04 09:42:26 +0000
4@@ -227,13 +227,13 @@
5 text = text.rstrip('\r\n')
6 if origin == last_origin:
7 (revno_str, author, date_str) = ('','','')
8+ elif origin not in revisions:
9+ last_origin = origin
10+ (revno_str, author, date_str) = ('?','?','?')
11 else:
12 last_origin = origin
13- if origin not in revisions:
14- (revno_str, author, date_str) = ('?','?','?')
15- else:
16- revno_str = '.'.join(str(i) for i in
17- revision_id_to_revno[origin])
18+ revno_str = '.'.join(str(i) for i in
19+ revision_id_to_revno[origin])
20 rev = revisions[origin]
21 tz = rev.timezone or 0
22 date_str = time.strftime('%Y%m%d',