bzr branch fails to set parent

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

John A Meinel wrote:
> Public bug reported:
>
> After my changes to make the contents of '.bzr/branch/parent' be a
> relative path, we now have a new bug that shows up.
>
> Where when I try to branch someone else's branch, their parent may not be accessible to me.
> But 'bzr branch' uses 'sprout', which uses 'copy_content_into' which defaults to copying the parent over.

That doesn't seem like something sprout should do.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFEt7nY0F+nu1YWqI0RAtnAAJ0dL2rSzwLMhYpG3Qyiq/01mZ9S4wCfbhQ/
cJ9Sko7fMph5P8poph8qPcs=
=IOrJ
-----END PGP SIGNATURE-----

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

Aaron Bentley wrote:
> John A Meinel wrote:
>>> Public bug reported:
>>>
>>> After my changes to make the contents of '.bzr/branch/parent' be a
>>> relative path, we now have a new bug that shows up.
>>>
>>> Where when I try to branch someone else's branch, their parent may not be accessible to me.
>>> But 'bzr branch' uses 'sprout', which uses 'copy_content_into' which defaults to copying the parent over.
>
> That doesn't seem like something sprout should do.
>
> Aaron

Well, if you look at sprout, it does:

result = self._format.initialize(to_bzrdir)
self.copy_content_into(result, revision_id=revision_id)
result.set_parent(self.bzrdir.root_transport.base)
return result

So the first thing it does after the copy finishes is to overwrite the
parent.

And if you dig through clone (which had an api change, thus being
difficult to read):

result = self._format.initialize(to_bzrdir)
self.copy_content_into(result, revision_id=revision_id)
return result

copy_content_into is doing:

destination.set_revision_history(new_history)
parent = self.get_parent()
if parent:
    destination.set_parent(parent)

So one difference would be to move the 'get_parent()' call into clone()
instead of being in copy_content. However, I'm pretty sure that
'copy_content_into' has an api that says it is cloning the object.

So while I agree sprout() shouldn't be copying the parent, it is kind of
a layering issue, and I don't think it is super critical. And I think we
need to fix branch.get_parent() anyway, since it shouldn't raise a
InvalidURLJoin. It could raise a different exception rather than
returning None, but it shouldn't raise *that* exception.

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

iD8DBQFEt8SdJdeBCYSNAAMRAkF9AKCxxEhOSDbw57E8jeQngBG/zkWi/ACfRkqc
crPLKHtZ4rJ80S5Xtw4CjuE=
=MtSC
-----END PGP SIGNATURE-----

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

John A Meinel wrote:
> copy_content_into is doing:
>
> destination.set_revision_history(new_history)
> parent = self.get_parent()
> if parent:
> destination.set_parent(parent)
>
>
> So one difference would be to move the 'get_parent()' call into clone()
> instead of being in copy_content. However, I'm pretty sure that
> 'copy_content_into' has an api that says it is cloning the object.

So, copy_content_into is supposed to be a clone sort of operation that
does an exact copy. So if it tries to copy a branch with an invalid
parent, the result should be a branch with an invalid parent. (Because
you may later use the branch from a location where the parent *is* valid.)

e.g. copy_content_into should do
parent = self._get_raw_parent()
if parent:
    destination._set_raw_parent(parent)

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFEt8+O0F+nu1YWqI0RAvqNAJ9URjnzY9SaWHXTm6i5GUA1c+FPeQCghsHD
kV58ugcBZ983QGDtLwGVkTo=
=HR17
-----END PGP SIGNATURE-----

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

Aaron Bentley wrote:
> John A Meinel wrote:
>>> copy_content_into is doing:
>>>
>>> destination.set_revision_history(new_history)
>>> parent = self.get_parent()
>>> if parent:
>>> destination.set_parent(parent)
>>>
>>>
>>> So one difference would be to move the 'get_parent()' call into clone()
>>> instead of being in copy_content. However, I'm pretty sure that
>>> 'copy_content_into' has an api that says it is cloning the object.
>
> So, copy_content_into is supposed to be a clone sort of operation that
> does an exact copy. So if it tries to copy a branch with an invalid
> parent, the result should be a branch with an invalid parent. (Because
> you may later use the branch from a location where the parent *is* valid.)
>
> e.g. copy_content_into should do
> parent = self._get_raw_parent()
> if parent:
> destination._set_raw_parent(parent)
>
> Aaron

Maybe. The issue is that if you clone from remote into local, the path
might actually need to change. Say I have:

/branch-one
/branch-two (parent is ../branch-one)
and then I clone branch-two into /sub/branch-3

/sub/branch-3 (if parent is branch-one, it needs to be
               ../../branch-one).

A raw clone would give just ../branch-one which doesn't point anywhere.

Now we could make _get_raw_parent() try to create an absolute path, and
if it fails, it just copies the raw parent over. But I do believe that
is not a whole lot better, and not worth a new api layer.

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

iD8DBQFEt9aMJdeBCYSNAAMRAn6NAJ0T1MEYpmad4xeURAmnnMoqykBJ9wCg1ohi
FTzQIIrrFSlWD38ZdPfW30g=
=0Rdn
-----END PGP SIGNATURE-----

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

John A Meinel wrote:
> Maybe. The issue is that if you clone from remote into local, the path
> might actually need to change. Say I have:

If this API is a smart API, it will do the wrong thing sometimes. It
will no longer be suitable for making backup copies of branches with,
and it will no longer be suitable for cloning branches remotely.

So I propose making it a dumb API, and doing exactly what's requested.
Higher-level logic can decide how to handle these cases. I don't expect
copy_content_into to be smart.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFEt+8F0F+nu1YWqI0RAs11AJ9QcG8cG/ac2PFEAsnEoySileg+AwCffJcV
vne8PLPHOZxEHPHGio0Qcqo=
=M3kq
-----END PGP SIGNATURE-----

On Fri, 2006-07-14 at 19:22 +0000, Aaron Bentley wrote:
>
>
> So I propose making it a dumb API, and doing exactly what's requested.
> Higher-level logic can decide how to handle these cases. I don't
> expect
> copy_content_into to be smart.

Agreed - this was the original intent of it.

That is, clone() and sprout() are smart, copy_content_into is the dumb
workhorse.

Rob
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.

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

Robert Collins wrote:
> On Fri, 2006-07-14 at 19:22 +0000, Aaron Bentley wrote:
>>
>> So I propose making it a dumb API, and doing exactly what's requested.
>> Higher-level logic can decide how to handle these cases. I don't
>> expect
>> copy_content_into to be smart.
>
> Agreed - this was the original intent of it.
>
> That is, clone() and sprout() are smart, copy_content_into is the dumb
> workhorse.
>
> Rob

Then I would propose that 'copy_content_to' should not do anything with
the branch parent.
There are currently 2 callers of 'copy_content_to'.

1) clone - This needs to be smart about the branch parent, so that the
final object contains a pointer to the same location.
2) sprout - This is going to set the parent to itself, so it doesn't
care what the old parent was.

Is there plans to have more callers of 'copy_content_to' which would
even want the branch parent to be pointing to an incorrect location? If
you 'clone' to anywhere but a sibling directory (with an identical
parent) the parent pointer is going to be wrong if you copy the raw
pointer location.

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

iD8DBQFEuE/2JdeBCYSNAAMRAu/ZAKCUJJHH/O2Pj9C8n9t2+AiXA+GH9wCeKEk5
83RAtVltkZvxFCRhzo71ODo=
=XoFx
-----END PGP SIGNATURE-----

This is in bzr-0.9