bzr.dev smart client fails on log

Do you have a specific revision where this is occurring? I see the assert in a slightly different place (line 862), with bzr.dev revno 3373

If I change the assert in question to:
        for key in keys:
            assert type(key) is str, \
                "Key type is %s not a plain string" % (type(key),)

I can see that it is failing because it is getting a key of None, rather than a real value.

Tracing it back up the stack, it seems to be because _get_mainline_revs() is returning [None, all, the, branch, mainline] for 'mainline_revs'.

I can't figure out why, but it is very explicitly adding a revision at the beginning:
    # override the mainline to look like the revision history.
    mainline_revs = [revision_id for index, revision_id in cut_revs]
    if cut_revs[0][0] == 1:
        mainline_revs.insert(0, None)
    else:
        mainline_revs.insert(0, which_revs[start_revno-2][1])

It seems that if you *don't* insert something there, and you do:

bzr log ---short -r ..10

you end up getting 2-10 instead of 1-10, this seems to be because of this line:

def get_view_revisions(mainline_revs, rev_nos, branch, direction,
                       include_merges=True):

...
    if include_merges is False:
        revision_ids = mainline_revs[1:]
        if direction == 'reverse':
            revision_ids.reverse()
        for revision_id in revision_ids:
            yield revision_id, str(rev_nos[revision_id]), 0

So get_view_revisions explicitly assumes that there will be an extra entry at the beginning of 'mainline_revs'.

This seems to be required, because the code used "merge_sort(..., tip=mainline_revs[-1], mainline_revs=mainline_revs...).
And 'merge_sort' seems designed to stop at the first revision in the list:
            self._mainline_revisions = list(mainline_revisions)
            self._stop_revision = self._mainline_revisions[0]

So I think the real fix is just:
    parent_map = dict(((key, value) for key, value in
        graph.iter_ancestry(mainline_revs[1:]) if value is not None))

Because 'mainline_revs' seems to be defined as including 1 more node that the whole graph has. It certainly could be considered a bug that 'graph.iter_ancestry()' is allowing None to be passed, though arguably it normally just ignores it as a node that isn't in the graph.

Anyway, I'll post the fix to the list.

I sent a patch to the list.

marking it critical since it is a regression and it is breaking a plain 'bzr log bzr://"

> Do you have a specific revision where this is occurring?

Any. I first saw it on a branch I was looking at during a discussion
in #bzr. If I make a scratch branch with one rev and
bzr+ssh://localhost/... it, it dies in the place you listed below.

--
Matthew Fuller
<email address hidden>

> ** Changed in: bzr
> Status: Fix Committed => Fix Released

Are you sure? I still see it with bzr.dev r3405 (BZR_REMOTE_PATH used
to be sure I'm using bzr.dev on the far side as well as the near side;
.bzr.log confirms).

% env BZR_REMOTE_PATH=dbzr dbzr log bzr+ssh://localhost/tmp/bzr/t/repo/A/
[...]
  File "/home/fullermd/src/bzr/bzr.dev/bzrlib/graph.py", line 442, in iter_ancestry
    next_map = self.get_parent_map(pending)
  File "/home/fullermd/src/bzr/bzr.dev/bzrlib/remote.py", line 802, in get_parent_map
    parent_map = self._get_parent_map(missing_revisions)
  File "/home/fullermd/src/bzr/bzr.dev/bzrlib/remote.py", line 878, in _get_parent_map
    "key %r not a plain string" % (key,))
ValueError: key None not a plain string

There's nothing special at all about .../repo/A/; it's just a scratch
branch with 2 revs of nothing I made to test something else. It's
reproducible on any branch I have around or can make.

--
Matthew Fuller
<email address hidden>

It looks like you are right Matthew. I was thrown off because Martin said:
Thanks very much John!

I've merged this for 1.4rc2, with Andrew's tweak to the assertion.

We should if possible add tests for this before merging it into bzr.dev.

It seems he was lying about 1.4rc2, or he was lying later when he said he merged 1.4 rc2 into bzr.dev.

I'm submitting it now because it was approved (and I added tests anyway).

> I've merged this for 1.4rc2, with Andrew's tweak to the assertion.

It does seem to happen with 1.4 final (installed system-wise) too :(

  File "/usr/local/lib/python2.5/site-packages/bzrlib/remote.py", line 878, in _get_parent_map
    "key %r not a plain string" % (key,))
ValueError: key None not a plain string

bzr 1.4 on python 2.5.2 (freebsd8)
[...]

--
Matthew Fuller
<email address hidden>

> It seems he was lying about 1.4rc2, or he was lying later when he
> said he merged 1.4 rc2 into bzr.dev.

That seems to have happened in revid
<email address hidden> (r3360.2.7 in
bzr.dev). Not a merge, but changes seem to be there, and they're in
current .dev (mod the remove-assert's change). So it may be that the
change isn't catching all the ways it breaks...

--
Matthew Fuller
<email address hidden>

Re-open and shift milestone; still an issue.

The complete fix was merged for bzr 1.6, it should be in bzr.dev as revno 3434.