MYSQL/BZR P3: "bzr log -rX..Y" should fail if X is not an ancestor of Y

[GuilhemBichot]

This is a Sun/MySQL - Canonical escalation imported into Launchpad by the Canonical Support Team (internal case 6611 )

Original escalation: 2009-11-16 10:12 UTC

Hello. Another report, after 474807 filed recently.
My initial intention here was too see whether bzr can be used to tell if a revision Y is a child of X; I expected that "bzr log -rX..Y" would fail if Y is not a child of X. So I did experiments, below.
I grab this branch: https://code.launchpad.net/~mysql/mysql-server/mysql-6.0-codebase-bugfixing
and I do
bzr log -n0 --show-ids -r tag:clone-5.2..tag:mysql-5.1.39
I expected an error, as mysql-5.1.39 is not a child of clone-5.2. I got no error, and the displaid log contains lots of revisions, including tag:clone-5.2 and tag:mysql-5.1.39. I wonder if "no error" is normal, I expected to trigger "bzr: ERROR: Start revision must be older than the end revision" (but when I look at the code which can emit such error, I see it compares revnos; with revnos being dotted numbers, I cannot see how they can reliably be compared).
Then I tried the reverse order:
bzr log -n0 --show-ids -r tag:mysql-5.1.39..tag:clone-5.2
I expected an error, as clone-5.2 is not a child of mysql-5.1.39. I got no error, and this time the displaid log contains tag:clone-5.2 but NOT tag:mysql-5.1.39, which sounds weird (how come the start bound is not included?).
Bazaar (bzr) 2.1.0dev3
  from bzr checkout /home/mysql_src/logiciels/bzr_versions/bzr.dev
    revision: 4782
    revid: <email address hidden>
    branch nick: bzr.dev

[Vincent Ladeuil (vila)]

We have a regression here as the order of the revisions wasn't the criteria to decide how they were presented.
--forward was needed to reverse the default order.
I'll look into it.

[Vincent Ladeuil (vila)]

In other words, doing 'bzr log -r tag:a..tag:b' used to produce the same output as 'bzr log -r tab:b..tag:a', hence the
--forward parameter.

[Vincent Ladeuil (vila)]

I've implemented a simple plugin (lp:~vila/+junk/bzr-graph) to answer the original question:
is a revision an ancestor of another one.

Contrary to my comments, 'bzr log -rX..Y' now requires X to be older than Y.
The case exposed here is a genuine bug as 'bzr log -n0 --show-ids -r tag:mysql-5.1.39..tag:clone-5.2'
should fail instead of displaying irrelevant revisions.

[John A Meinel (jameinel)]

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

Vincent Ladeuil wrote:
> I've implemented a simple plugin (lp:~vila/+junk/bzr-graph) to answer the original question:
> is a revision an ancestor of another one.
>
> Contrary to my comments, 'bzr log -rX..Y' now requires X to be older than Y.
> The case exposed here is a genuine bug as 'bzr log -n0 --show-ids -r tag:mysql-5.1.39..tag:clone-5.2'
> should fail instead of displaying irrelevant revisions.
>

I think Ian mentioned that it was a design decision to start logging
what we could, when we couldn't (yet) determine whether the arguments
were correct. So rather than searching all of history to say that they
args are invalid, and *then* displaying revs, start displaying revs
until we can determine whether it is valid or not.

I thought he had put in code to say "not valid" in some form once the
range had been determined...

John
=:->

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

iEYEARECAAYFAktVxyYACgkQJdeBCYSNAAPWZQCguOjbwthP12kos0wtwo/nTOre
niAAoNmxgVmk5pl70rB1HBFX+WvQliQV
=hbTE
-----END PGP SIGNATURE-----

[GuilhemBichot (guilhem-bichot)]

Hello Vincent. Thanks a lot for the plugin! I renamed it to "graph" because bzr was complaining about "bzr-graph" (not a valid plugin name). After that, I tried the plugin with bzr 2.0.3 which gave ("bzr is-ancestor -r tag:mysql-6.0.9"):
 bzr: ERROR: exceptions.AttributeError: 'cmd_is_ancestor' object has no attribute 'add_cleanup'
but with bzr.dev it works fine. It seems that to know whether 6.0.9 is an ancestor of 6.0.10 I now can do:
 bzr is-ancestor -r tag:mysql-6.0.9..tag:mysql-6.0.10 (which says "yes"),
whereas the opposite order says "no", all fine.
Then I wonder about what should be done:
- if the plugin is the intended way to answer the child/parent question, how about moving it to bzrtools? It seems useful enough to land there...?
- if it is a bug that the current stock bzr commands cannot answer the question, what should be done?
The only way I had found (after seeing that "bzr log -rX..Y" wasn't the answer) was to do
 bzr branch work_branch -r Y tmp_Y --no-tree # relatively fast if branching into shared repo
 bzr log -rX tmp_Y # if this gives an error, X is not a parent of Y
 rm -rf tmp_Y
Regarding the genuine bug(s) you have found, I hope you could take care of keeping track of them, I get a bit lost in the end about what is and is not a bug here...

[Ian Clatworthy (ian-clatworthy)]

John A Meinel wrote:
> Vincent Ladeuil wrote:
>> I've implemented a simple plugin (lp:~vila/+junk/bzr-graph) to answer the original question:
>> is a revision an ancestor of another one.
>
>> Contrary to my comments, 'bzr log -rX..Y' now requires X to be older than Y.
>> The case exposed here is a genuine bug as 'bzr log -n0 --show-ids -r tag:mysql-5.1.39..tag:clone-5.2'
>> should fail instead of displaying irrelevant revisions.
>
>
> I think Ian mentioned that it was a design decision to start logging
> what we could, when we couldn't (yet) determine whether the arguments
> were correct. So rather than searching all of history to say that they
> args are invalid, and *then* displaying revs, start displaying revs
> until we can determine whether it is valid or not.

That was my thinking at the time. In hindsight, I think it's fine to
validate the lower bound before displaying revisions, given that
explicitly providing a lower bound is relatively rare.

Ian C.

[John A Meinel (jameinel)]

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

Ian Clatworthy wrote:
> John A Meinel wrote:
>> Vincent Ladeuil wrote:
>>> I've implemented a simple plugin (lp:~vila/+junk/bzr-graph) to answer the original question:
>>> is a revision an ancestor of another one.
>>> Contrary to my comments, 'bzr log -rX..Y' now requires X to be older than Y.
>>> The case exposed here is a genuine bug as 'bzr log -n0 --show-ids -r tag:mysql-5.1.39..tag:clone-5.2'
>>> should fail instead of displaying irrelevant revisions.
>>
>> I think Ian mentioned that it was a design decision to start logging
>> what we could, when we couldn't (yet) determine whether the arguments
>> were correct. So rather than searching all of history to say that they
>> args are invalid, and *then* displaying revs, start displaying revs
>> until we can determine whether it is valid or not.
>
> That was my thinking at the time. In hindsight, I think it's fine to
> validate the lower bound before displaying revisions, given that
> explicitly providing a lower bound is relatively rare.
>
> Ian C.
>

Well, I do it by default... (bzr alias log=log --short -r -10..-1)

I suppose giving a lower bound such that it is sufficiently old to cause
perf problems is rare...

John
=:->

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

iEYEARECAAYFAktXga0ACgkQJdeBCYSNAANzlwCdF/yhfSzALqB/0qm8Kt+yUuXV
ffoAoNcf1osuxmnbCdm38yVbKqObK1S/
=IIMe
-----END PGP SIGNATURE-----

[Ian Clatworthy (ian-clatworthy)]

John A Meinel wrote:

> Well, I do it by default... (bzr alias log=log --short -r -10..-1)
>
> I suppose giving a lower bound such that it is sufficiently old to cause
> perf problems is rare...

I expect so. We can always encourage users to favour -l N rather than -r
-N..-1 if performance (or memory usage) proves a problem in practice.

Ian C.

[GuilhemBichot (guilhem-bichot)]

I never use -l N; our histories are so complex that I wouldn't know what revisions to expect from -l N. On the other hand, -rX..Y at least tells me that I'll likely see revisions on the path from X to Y (including merged revisions or not), which is useful (to understand what was added between X and Y).
So I form the wish that -rX..Y is not made slower than it is. I see no problem if it starts printing revisions and ends up with an error message, instead of pre-validating parameters, if that allows to avoid a performance problem (here I refer to John's post of 2010-01-19).

[Vincent Ladeuil (vila)]

So, there shouldn't be a performance problem since the bug was in a code path activated only when merged revisions were involved which already implies1 loading the whole ancestry graph to calculate revnos.

I have a fix that now correctly raise an error for the two commands in this bug report:
    bzr log -n0 -rtag:mysql-5.1.39..tag:clone-5.2
    bzr: ERROR: Start revision not found in history of end revision.

    bzr log -n0 -rtag:clone-5.2..tag:mysql-5.1.39
    bzr: ERROR: Start revision not found in history of end revision.

[GuilhemBichot (guilhem-bichot)]

Thanks Vincent. By the way, I propose to make this bug public.