Merge lp://staging/~vila/bzr/320119-exclude-ancestry into lp://staging/bzr

This seems like something other commands than log will want; perhaps we
should tackle it using an algreba, like git does?

You have a few conflicts because this branch waited so long for a review. Sorry :(

You define a new stop_rule for iter_merge_sorted_revisions, but you haven't added it to the docstring. Please fix that.

It's a shame that make_branch_with_alternate_ancestries is duplicated in test_log and per_branch.test_iter_merge_sorted_revisions. Perhaps make it a function and have the latter import it from the former? Please at least add comments in both places noting the duplication.

(And further, it's a shame that the description of the ancestry is duplicated within that code... if only there were some way to do "make_branch_from_ascii_art_ancestry".)

Other than those, this seems ok to me. Once you fix those issues you can consider my vote upgraded to Approve.

I get the feeling that the next time we are tempted to another parameter to log we should think about refactoring the way we pass log options through the various layers (maybe with a LogOptions object? Or maybe Robert's idea about an algebra...), but this is ok for now.

>>>>> Andrew Bennetts <email address hidden> writes:

    > Review: Needs Fixing

    > You have a few conflicts because this branch waited so long for a review. Sorry :(

    > You define a new stop_rule for iter_merge_sorted_revisions, but
    > you haven't added it to the docstring. Please fix that.

Done.

    > It's a shame that make_branch_with_alternate_ancestries is
    > duplicated in test_log and
    > per_branch.test_iter_merge_sorted_revisions. Perhaps make it a
    > function and have the latter import it from the former? Please at
    > least add comments in both places noting the duplication.

Since there is a valuable slight variation, I went with adding comments.

    > (And further, it's a shame that the description of the ancestry is
    > duplicated within that code... if only there were some way to do
    > "make_branch_from_ascii_art_ancestry".)

Hehe, ideally I'd prefer some GUI to define the graph and get both the
ascii art and the branchbuilder stuff from that. ascii art is *not* fun
to write ;)

    > Other than those, this seems ok to me. Once you fix those issues
    > you can consider my vote upgraded to Approve.

Thanks.

    > I get the feeling that the next time we are tempted to another
    > parameter to log we should think about refactoring the way we pass
    > log options through the various layers (maybe with a LogOptions
    > object?

Definitely, but we also need to wire that up to the command line as many
log formatters will want their own options, I've punted on that one so
far for lack of time to research how to catch the options not recognized
by the parser and allow plugins or any external code to give it a try.

    > Or maybe Robert's idea about an algebra...), but this is ok for
    > now.

Successful steps
Failure output:
All lines of log output:
Executing star-merge lp:~vila/bzr/320119-exclude-ancestry at Wed Apr 28 10:41:57 2010
['Nothing to merge.']

Meh, where is that pqm failure coming from ?
I sent a single submission with feed-pqm and it was merged.... (I didn't get a success email for it though).

This is probably due to lp taking too long to mark the branch as
merged: if it takes longer than pqm takes to try again, then pqm will
see it as still pending. Possibly we should:
mark things we succeed at as approved
adding a comment that it landed ok

or mark it as merged.

I'm a little worried about triggering launchpadlib errors though,
because lp is going to be updating the status at the same time - we
can collide.

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

Robert Collins wrote:
> This is probably due to lp taking too long to mark the branch as
> merged: if it takes longer than pqm takes to try again, then pqm will
> see it as still pending. Possibly we should:
> mark things we succeed at as approved
> adding a comment that it landed ok
>
> or mark it as merged.
>
> I'm a little worried about triggering launchpadlib errors though,
> because lp is going to be updating the status at the same time - we
> can collide.

I think it would be reasonable to have your script notice that "Nothing
to be Merged" obviously means that the branch is already merged...

John
=:->

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

iEYEARECAAYFAkvY+TsACgkQJdeBCYSNAAPIMQCgqjpPMmQSCREDGp9/5OdQKQVN
V8YAn34mPZMKIzBYQqYSNCR5lVb6+31+
=hVtp
-----END PGP SIGNATURE-----