[MERGE][bug #172657] Graph.find_differences() and status after merge

[John Arbash Meinel]

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

Ian Clatworthy wrote:
| John Arbash Meinel wrote:
|> Attached is an updated form of my previous submission, which I now
|> consider at least ready for review.

Attached is the "final form" of the patch. With the updates to find_differences,
and a new find_unique_ancestors function (which status uses). I believe I've
addressed all of your concerns here. There is one test I'm still working on, to
trigger a particular race condition I fixed. But the code itself should be
correct, so I would rather submit it, and get it reviewed/merged rather than
waiting on that to happen.


|
| I'm still working my way through the changes to graph.py. So far, so
| good in that file. I posted some comments yesterday about some requested
| changes to the tests. I'd also like quite a few changes made to status.py.
|
| bb:resubmit
|
|> +        width = osutils.terminal_width()
|>          try:
|>              from bzrlib.osutils import terminal_width
|>              width = terminal_width() - 1    # we need one extra space to avoid
|>                                              # extra blank lines
|>              m_revision = branch.repository.get_revision(merge)
|> +            revisions = dict((rev.revision_id, rev) for rev in
|> +                             branch.repository.get_revisions(merge_extra))
|> +        except errors.NoSuchRevision:
|> +            # If we are missing a revision, just print out the revision id
|> +            if short:
|> +                prefix = 'P  '
|> +            else:
|> +                prefix = ' '
|> +            to_file.write(prefix + ' ' + merge)
|> +            to_file.write('\n')
|> +        else:
|> +            rev_id_iterator = sorter.iter_topo_order()
|> +            num, first, depth, eom = rev_id_iterator.next()
|> +            assert first == merge
|> +            m_revision = revisions[merge]
|
| Hmm, show_pending_merges was looking pretty crufty before your changes.
| I appreciate that the interesting/important part of this patch is the
| graph.py changes but show_pending_merges needs some refactoring love
| before I'd be happy to accept the new version. Here's what I'd like:
|
| * width calculation to be pulled out of the 'for merge in pending' loop
| * ditto for setting the prefix strings (prefix and mprefix say)
| * replace the assert with an if ... raise
| * don't set m_revision twice - the first one will do (##)
| * put the sorting logic in a separate function (returning the iterator
|   say) and only call it in the else of the try block

I revamped it quite a bit, for a few reasons.

1) I pulled out the common stuff like you mentioned
2) I did factor out the sorting code into a helper function, and I also added
some more tests to see how the sorting gets triggered.
3) I wanted to display the message for the pending merge before I did the graph
search. It is faster than it was, but it still is a little slower than I would
like. This way you at least get to see the tip information.
4) Otherwise, I didn't really want to change the output of status until we
decide as a group to do so. I certainly don't want the rest of the code to block
on that.

|
| ## You could just as easily drop the first one. Looking at
| https://lists.ubuntu.com/archives/bazaar/2008q1/038430.html though and
| Andrew's input in particular ...
|
|> pending merges:
|>   merge 1:
|>     sequence #1 commit #3
|>     sequence #1 commit #2
|>     sequence #1 commit #1
|>   merge 2:
|>     sequence #2 commit #2
|>     sequence #2 commit #1
|>
|> Or more radically, I'd probably be satisfied with this output by default:
|>
|> pending merges:
|>   sequence #1 commit #3
|>     (and 2 others)
|>   sequence #2 commit #2
|>     (and 1 other)
|
| much of the sorting code could be skipped altogether if we only
| displayed the full list in verbose mode. WE can left that bridge to
| later though.
|
| Ian C.
|

I actually would still sort them, even if we didn't indent them in the same way.
At least, IMO the sort order is still relevant.

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

iEYEARECAAYFAkgaLRkACgkQJdeBCYSNAAN/3QCfZ45jJ/G3+bvA8lbriu/qB3JZ
KzQAoIM11+KiRFaUPpf8aA5xiNLWh6Rr
=bxmA
-----END PGP SIGNATURE-----
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: find_unique_ancestors.patch
Url: https://lists.ubuntu.com/archives/bazaar/attachments/20080501/d1a5f55d/attachment-0001.diff