Bazaar

Code review comment for lp:~garyvdm/bzr/bug588698-criss-cross-null-base

bug588698-criss-cross-null-base
Merge into bzr.dev

Revision history for this message

Gary van der Merwe (garyvdm) wrote on 2010-11-18:

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

On 17/11/2010 23:05, John Arbash Meinel wrote:
> I think it usually is. However, you shouldn't need to pass
> [NULL_REVISION] either. Just pass the empty list (I think).
>
> + builder.build_snapshot('B-id', [],
> + [('add', ('', None, 'directory', None))])
>
> I'm not positive if that works, but it does actually match certain bits
> of code better. ('null:' isn't saved to the index files, for example).

Ok. I've updated the code to make it possible to pass [] to get a null
parent.

> graph.find_merge_order is not a cheap operation, because it has to walk
> the ancestry. Could we do it only in the 'if' block:
....
> I see that 'sorted_lca_keys' is used again right after this point. but
> is it used in all possible cases?

Previously, we always call graph.find_merge_order if we encounter a
criss-cross merge. It is used later to sort interesting_trees. I have
just moved it's execution earlier.

>
> interesting_revision_ids = [self.base_rev_id]
> interesting_revision_ids.extend(lcas)
> + interesting_revision_ids =
> list(set(interesting_revision_ids))
>
> ^- This doesn't preserve the ordering of interesting_revision_ids. But
> since it looks like we don't need the specific order, why not just use a
> set and not cast to a list in the first place?

Ah yes, when I wrote it that line, I meant to go back and change it, but
I forgot to. Fixed now.

> Anyway, the basic concept seems fine. Failing down to NULL_REVISION is
> bad, picking a 'best guess' LCA seems reasonable.

Great.

> We may want to inform the user that we did this, in case it would be
> confusing why X or Y didn't happen. At least a mutter() helps us
> debug it if it is happening.

I don't think that the behavior changes much compared to a criss-cross
merge, and so I don't think a warning is appropriate. I have added a mutter.

Regards,

Gary
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkzk5pYACgkQd/3EdwGKOh1IfACgnKiuHCi2+ra9ffOSiNgxus31
S9wAn1GtkQoM6nUyxMcGV7vbbUEKmhS/
=PvTF
-----END PGP SIGNATURE-----

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

On 17/11/2010 23:05, John Arbash Meinel wrote:
> I think it usually is. However, you shouldn't need to pass
> [NULL_REVISION] either. Just pass the empty list (I think).
> 
> +        builder.build_snapshot('B-id', [],
> +            [('add', ('', None, 'directory', None))])
> 
> I'm not positive if that works, but it does actually match certain bits
> of code better. ('null:' isn't saved to the index files, for example).

Ok. I've updated the code to make it possible to pass [] to get a null
parent.

Previously, we always call graph.find_merge_order if we encounter a
criss-cross merge. It is used later to sort interesting_trees. I have
just moved it's execution earlier.

> 
>                  interesting_revision_ids = [self.base_rev_id]
>                  interesting_revision_ids.extend(lcas)
> +                interesting_revision_ids =
> list(set(interesting_revision_ids))
> 
> ^- This doesn't preserve the ordering of interesting_revision_ids. But
> since it looks like we don't need the specific order, why not just use a
> set and not cast to a list in the first place?

Ah yes, when I wrote it that line, I meant to go back and change it, but
I forgot to. Fixed now.

> Anyway, the basic concept seems fine. Failing down to NULL_REVISION is
> bad, picking a 'best guess' LCA seems reasonable.

Great.

> We may want to inform the user that we did this, in case it would be
> confusing why X or Y didn't happen. At least a mutter() helps us
> debug it if it is happening.

I don't think that the behavior changes much compared to a criss-cross
merge, and so I don't think a warning is appropriate. I have added a mutter.

Regards,

Gary
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkzk5pYACgkQd/3EdwGKOh1IfACgnKiuHCi2+ra9ffOSiNgxus31
S9wAn1GtkQoM6nUyxMcGV7vbbUEKmhS/
=PvTF
-----END PGP SIGNATURE-----

« Back to merge proposal