Bazaar

Code review comment for lp:~spiv/bzr/per-file-merge-hook-491711

per-file-merge-hook-491711
Merge into bzr.dev

Revision history for this message

Aaron Bentley (abentley) wrote on 2010-01-16:

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

Andrew Bennetts wrote:
>> ^- Are we sure this is correct? If there is a contents_conflict we mark
>> the file as unversioned?

That's correct. For a contents conflict that is not a text conflict,
bzr does not leave a file in place with the original filename, therefore
we also unversion it, and version the .OTHER, .THIS or .BASE--
whatever's there.

>> + elif hook_status == 'conflicted':
>> + # XXX: perhaps the hook should be able to provide
>> + # the BASE/THIS/OTHER files?
>> + self.tt.create_file(lines, trans_id)
>> + self._raw_conflicts.append(('text conflict', trans_id))
>> + name = self.tt.final_name(trans_id)
>> + parent_id = self.tt.final_parent(trans_id)
>> + file_group = self._dump_conflicts(name, parent_id, file_id)
>> + file_group.append(trans_id)
>>
>> ^- Do you know why we are appending the trans_id here? file_group isn't
>> mentioned again.

> Hmm. Again, this is something that lp:bzr does that I've
> preserved/copied. On closer inspection, I don't think it has any effect
> (either in lp:bzr or in my branch), so I'll just delete that.

This code doesn't appear to have ever made sense. It was introduced in
<email address hidden>. I
suspect it was meant to help with treating a file and its conflict files
as a group for conflict handling.

>> + try:
>> + self.tt.tree_kind(trans_id)
>> + self.tt.delete_contents(trans_id)
>> + except errors.NoSuchFile:
>> + pass
>>
>> ^- I don't quite understand the unconditional "delete_contents" if the
>> contents exist. Especially given that:
>>
>> + elif hook_status == 'success':
>> + self.tt.create_file(lines, trans_id)
>>
>> Is called *before* this.

Operations on a TreeTransform simply schedule things to happen when you
call 'apply'. Before and after are irrelevant (except when you cancel
an operation you had previously scheduled). All that matters is that
the tree transform is consistent when you call apply.

>> Is it that TreeTransform.delete_contents() indicates we want to delete
>> the content in the current tree, and "create_file()" is creating
>> contents in the "next" tree?

delete_contents means that applying the transform should delete these
contents, and create_file means that applying the transform should
create a file with specified contents.

> Exactly how TreeTransform isn't clearly documented that I could find.

How it what? Works?

> If someone that knows precisely what the operations on it means and
> could document that it would be a great help, I think.

I've certainly tried in the past, and I'm happy to enhance the
documentation. Could you start by explaining what is confusing or
missing from the TreeTransform docstring?

> AFAICS there
> isn't even a description of what a trans_id is...

There are three paragraphs of description:

> Transform/Transaction ids
> -------------------------
> trans_ids are temporary ids assigned to all files involved in a transform.
> It's possible, even common, that not all files in the Tree have trans_ids.
>
> trans_ids are used because filenames and file_ids are not good enough
> identifiers; filenames change, and not all files have file_ids. File-ids
> are also associated with trans-ids, so that moving a file moves its
> file-id.
>
> trans_ids are only valid for the TreeTransform that generated them.

is an abbreviation of
> “transform id” or “transaction id”

Both, as you can see above.

>, are they file_ids, or something else
> but like file_ids?

> What's the difference between unversion_file()
> and delete_contents()?

One schedules the deletion of the contents of a file. The other
schedules removes the removal of file id of the file. The docstring for
delete_contents is:

> """Schedule the contents of a path entry for deletion"""

The docstring for unversion_file is:
> """Schedule a path entry to become unversioned"""

If this is unclear, could you say what about it is unclear?

You can do both at the same time by calling delete_versioned.

>> A comment of this might clarify:
>> # Now get rid of the contents in the working tree, so that any new
>> # content (even conflicts) has a place to go.

No TreeTransform operation except apply will touch the working tree.

I would comment it:
# However the merge has been resolved, the old contents should not be
# retained.

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

iEYEARECAAYFAktRAgsACgkQ0F+nu1YWqI1w+wCdGTnIXrkWCM02MAHifchhKoCf
wukAniYMJrt7mBRdkZN5fmeBYMr2h5kf
=sHo7
-----END PGP SIGNATURE-----

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

Andrew Bennetts wrote:
>> ^- Are we sure this is correct? If there is a contents_conflict we mark
>> the file as unversioned?

That's correct.  For a contents conflict that is not a text conflict,
bzr does not leave a file in place with the original filename, therefore
we also unversion it, and version the .OTHER, .THIS or .BASE--
whatever's there.

>> +        elif hook_status == 'conflicted':
>> +            # XXX: perhaps the hook should be able to provide
>> +            # the BASE/THIS/OTHER files?
>> +            self.tt.create_file(lines, trans_id)
>> +            self._raw_conflicts.append(('text conflict', trans_id))
>> +            name = self.tt.final_name(trans_id)
>> +            parent_id = self.tt.final_parent(trans_id)
>> +            file_group = self._dump_conflicts(name, parent_id, file_id)
>> +            file_group.append(trans_id)
>>
>> ^- Do you know why we are appending the trans_id here? file_group isn't
>> mentioned again.

> Hmm.  Again, this is something that lp:bzr does that I've
> preserved/copied.  On closer inspection, I don't think it has any effect
> (either in lp:bzr or in my branch), so I'll just delete that.

This code doesn't appear to have ever made sense.  It was introduced in
abentley@panoramicfeedback.com-20060117212354-511f293f10a80062.  I
suspect it was meant to help with treating a file and its conflict files
as a group for conflict handling.

>> +        try:
>> +            self.tt.tree_kind(trans_id)
>> +            self.tt.delete_contents(trans_id)
>> +        except errors.NoSuchFile:
>> +            pass
>>
>> ^- I don't quite understand the unconditional "delete_contents" if the
>> contents exist. Especially given that:
>>
>> +        elif hook_status == 'success':
>> +            self.tt.create_file(lines, trans_id)
>>
>> Is called *before* this.

Operations on a TreeTransform simply schedule things to happen when you
call 'apply'.  Before and after are irrelevant (except when you cancel
an operation you had previously scheduled).  All that matters is that
the tree transform is consistent when you call apply.

>> Is it that TreeTransform.delete_contents() indicates we want to delete
>> the content in the current tree, and "create_file()" is creating
>> contents in the "next" tree?

delete_contents means that applying the transform should delete these
contents, and create_file means that applying the transform should
create a file with specified contents.

> Exactly how TreeTransform isn't clearly documented that I could find.

How it what?  Works?

> If someone that knows precisely what the operations on it means and
> could document that it would be a great help, I think.

I've certainly tried in the past, and I'm happy to enhance the
documentation.  Could you start by explaining what is confusing or
missing from the TreeTransform docstring?

> AFAICS there
> isn't even a description of what a trans_id is...

There are three paragraphs of description:

>     Transform/Transaction ids
>     -------------------------
>     trans_ids are temporary ids assigned to all files involved in a transform.
>     It's possible, even common, that not all files in the Tree have trans_ids.
> 
>     trans_ids are used because filenames and file_ids are not good enough
>     identifiers; filenames change, and not all files have file_ids.  File-ids
>     are also associated with trans-ids, so that moving a file moves its
>     file-id.
> 
>     trans_ids are only valid for the TreeTransform that generated them.

is an abbreviation of
> “transform id” or “transaction id”

Both, as you can see above.

>, are they file_ids, or something else
> but like file_ids?

> What's the difference between unversion_file()
> and delete_contents()?

One schedules the deletion of the contents of a file.  The other
schedules removes the removal of file id of the file.  The docstring for
delete_contents is:

>         """Schedule the contents of a path entry for deletion"""

The docstring for unversion_file is:
>         """Schedule a path entry to become unversioned"""

If this is unclear, could you say what about it is unclear?

You can do both at the same time by calling delete_versioned.

>> A comment of this might clarify:
>>   # Now get rid of the contents in the working tree, so that any new
>>   # content (even conflicts) has a place to go.

No TreeTransform operation except apply will touch the working tree.

I would comment it:
# However the merge has been resolved, the old contents should not be
# retained.

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

iEYEARECAAYFAktRAgsACgkQ0F+nu1YWqI1w+wCdGTnIXrkWCM02MAHifchhKoCf
wukAniYMJrt7mBRdkZN5fmeBYMr2h5kf
=sHo7
-----END PGP SIGNATURE-----

« Back to merge proposal