Code review comment for lp:~vila/bzr/320119-exclude-ancestry

>>>>> 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.