-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 ... >> + 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. > > Yes, this certainly puzzled me when I came across it in trunk! (Again, > this oddity is definitely present in trunk, just that my patch has > perhaps made it more visible.) > >> 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? > > Exactly how TreeTransform isn't clearly documented that I could find. > If someone that knows precisely what the operations on it means and > could document that it would be a great help, I think. AFAICS there > isn't even a description of what a trans_id is... is an abbreviation of > “transform id” or “transaction id”, are they file_ids, or something else > but like file_ids? What's the difference between unversion_file() > and delete_contents()? etc. I can help with this. A trans_id is a handle for the TreeTransform. Similar in concept to a file_id mapping to a path in an inventory. However, TreeTransform can talk about files that aren't versioned (have no trans_id), and multiple copies of the same logical file (two people introduce the same file_id at different locations, etc). As such, trans_id are an abstraction of a specific file/directory content during the transaction. AFAICT they are always of the form "new-%d". unversion_file is saying that we need to remove the file_id from the inventory. delete_contents says we want to call 'os.remove()' on the object. Note that you don't get a new trans_id for content being removed versus content being added. I checked and _removed_contents indicates files that are to be deleted from the wt, but you can *also* have the same trans_id in _new_contents. So I think trans_id is like file_id, in that it is how TT maps this file should be replacing that file. I think part of it is that TT doesn't think about replacements. It only has a remove phase and an add phase. 1) All new content is staged into .bzr/checkout/limbo, generally with names like 'new-1'. Though we do try to create files in their final directories with their final names if we know in advance. (So we don't rename all files, just the containing dirs for 'bzr co'.) 2) All old files are renamed into .bzr/checkout/pending_deletion 3) All new files are renamed from .bzr/checkout/limbo into the working tree 4) All old files in .bzr/checkout/pending_deletion are removed That is how we get atomicity of updates, so that if something fails in (3) we can still revert back to the original tree. > > So, for this specific puzzle... > > The code I'm guessing is there to ensure that the file is marked as > versioned, but that the temporary file used for the new contents is > removed? Staring again at the treetransform.py code isn't bringing me > enlightenment. > >> 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. >> ... > > I really wish I knew if that was right. delete_contents first calls "tree_kind()" which requires that the path exists on disk in the working tree (file_kind(self._tree.abspath(path)). Though it also means that calling: self.tt.tree_kind() self.tt.delete_contents() is redundant and double statting the file. So we should clean that up anyway. And 'create_file' is certainly scheduling new content to be added to the tree. So the comment looks correct to my understanding. > > All I'm certain of is that I'm doing the same as trunk in this case. > >> + def _default_other_winner_merge(self, merge_hook_params): >> + """Replace this contents with other.""" >> + file_id = merge_hook_params.file_id >> + trans_id = merge_hook_params.trans_id >> + file_in_this = file_id in self.this_tree >> + if file_id in self.other_tree: >> + # OTHER changed the file >> + wt = self.this_tree >> + if wt.supports_content_filtering(): >> + # We get the path from the working tree if it exists. >> + # That fails though when OTHER is adding a file, so >> + # we fall back to the other tree to find the path if >> + # it doesn't exist locally. >> + try: >> + filter_tree_path = wt.id2path(file_id) >> + except errors.NoSuchId: >> + filter_tree_path = self.other_tree.id2path(file_id) >> + else: >> + # Skip the id2path lookup for older formats >> + filter_tree_path = None >> >> ^- Something seems missing if we are doing this only in this code path. >> What about all the other code paths that will be creating file contents? >> How do the MergeHooks interact with content filtering? > > That's a good question! I don't know, but it's interesting that even in > trunk I only see content filtering involved in this case (OTHER is > winner) and when generating conflicts. When both sides have modified > the file no content filtering appears to be involved, which doesn't > sound right. > > So I *think* my patch doesn't make this worse, unless a plugin decides > to perform the merge for the winner == 'other' case. > > [...] >> +"""Merge hook for bzr's NEWS file. >> + >> +Install this as a plugin, e.g: >> + >> + cp contrib/news-file-merge-hook.py ~/.bazaar/plugins/news_merge.py >> +""" >> >> ^- This is no longer true, since you changed it to "news_merge/__init__.py". >> >> +from .parser import simple_parse >> >> ^- ???? >> >> I'm guessing ".parser" is python 2.6 syntax, but this certainly isn't >> 2.4 syntax. > > Right. Fixed. > >> Given the discussion, I would probably move this into bzrlib/plugins/* >> and change the trigger to look at a config variable to determine whether >> it is active. > > I haven't done this yet, but will soon. > >> Which makes me think that MergeHookParams is incomplete if we don't have >> semi-easy access to what Branch/Tree this hook is being run on. I >> suppose we can use "params.merger.this_tree.branch.get_config()" ? > > I suppose so... I'll find out when I move the plugin to bzrlib.plugins I > guess! :) > >> How does this interact with "bzr merge --preview". > > It seems to work as I expect: any registered hook, such as news_merge's, > is used. Do you have some specific concerns in mind? If I set up location.conf to say: [/home/jameinel/dev/bzr] news_merger_files = NEWS news_merge_files:policy = recurse Will that match when 'bzr merge --preview' is invoked (what Branch is given to the PreviewTree, etc.) I'm not 100% sure that PreviewTree *has* a .branch property. (Given that RevisionTree does not, but has a _repository attribute. Even better it is passed in as 'branch'... :) > >> I would also move the bulk of the code into a separate file that is only >> imported with the merge hook is actually triggered. > > I haven't yet moved the code, although there isn't much of it, but I > have added some lazy_import magic. > > -Andrew. It is probably reasonable to merge soon. I'd probably like to see the news plugin in 'plugins' first, but that is not critical. John =:-> -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (Cygwin) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAktQgzsACgkQJdeBCYSNAAOAIgCdE7l5qa1A5T9ReBu7lUCU5n8G TskAnRXmCz77OKU7EZJCl73O+iYEFWXN =SBY8 -----END PGP SIGNATURE-----