[MERGE] pre_commit hook (was: [RFC] pre_commit hook)

[John Arbash Meinel]

John Arbash Meinel has voted approve.
Status is now: Semi-approved
Comment:
To start with, I think this is reasonable as is, I just have a few 
comments to discuss.

+        if change != 'unchanged' and path != '':
+            self.affected_ids.setdefault(change, []).append(ie.file_id)

^- I wonder if it makes more sense to fill the dictionary with the 
different types pointing to empty lists at the start. That way we don't 
have to worry about setdefault().
I'm just thinking that an initial add of 10,000 files is going to have a 
bit of overhead here. Though I suppose it should be profiled before we 
worry about it.
But since there are only a few possible cases, it seems easier to do it 
that way.

+    def test_pre_commit_ids(self):
+        self.build_tree(['rootfile', 'dir/', 'dir/subfile'])
+        tree = self.make_branch_and_tree('.')
+        tree.lock_write()
+        tree.set_root_id('root_id')
+        tree.add('rootfile', 'rootfile_id')
+        tree.put_file_bytes_non_atomic('rootfile_id', 'abc')
+        tree.add('dir', 'dir_id')
+        tree.add('dir/subfile', 'dir_subfile_id')
+        tree.put_file_bytes_non_atomic('dir_subfile_id', 'def')
+        Branch.hooks.install_hook("pre_commit", 
self.capture_pre_commit_hook)
+        rev1 = tree.commit('first revision')
+        tree.unversion(['dir_id'])
+        rev2 = tree.commit('second revision')
+        tree.put_file_bytes_non_atomic('rootfile_id', 'ghi')
+        rev3 = tree.commit('third revision')
+        tree.unlock()
+        tree.lock_write()
+        tree.rename_one('rootfile', 'renamed')
+        rev4 = tree.commit('fourth revision')
+        tree.unlock()
+        tree.lock_write()
+        tree.put_file_bytes_non_atomic('rootfile_id', 'jkl')
+        tree.rename_one('renamed', 'rootfile')
+        rev5 = tree.commit('fifth revision')
+        tree.unlock()
+        self.assertEqual([
+            ('pre_commit', 0, NULL_REVISION, 1, rev1,
+             {'added': ['dir_id', 'dir_subfile_id', 'rootfile_id']} ),
+            ('pre_commit', 1, rev1, 2, rev2,
+             {'deleted': ['dir_id', 'dir_subfile_id']} ),
+            ('pre_commit', 2, rev2, 3, rev3,
+             {'modified': ['rootfile_id']} ),
+            ('pre_commit', 3, rev3, 4, rev4,
+             {'renamed': ['rootfile_id']} ),
+            ('pre_commit', 4, rev4, 5, rev5,
+             {'modified and renamed': ['rootfile_id']} )
+            ],
+            self.hook_calls)

^- I would have preferred these as separate tests, because as it stands 
the test is a little bit hard to read.
Also, you really should have try/finally for:
tree.lock_write():
try:
   do stuff
finally:
   tree.unlock()

Otherwise, if something fails in the middle, you will get a mixed 
traceback, when it also warns that the tree wasn't unlocked. Oh, and if 
you are going to do them one after the other, you should be able to just 
lock the tree across the whole set. No need to keep unlocking it. (If 
there is, let us know, because that would be a sign of a problem).

+            ('pre_commit', 4, rev4, 5, rev5,
+             {'modified and renamed': ['rootfile_id']} )
+            ],

^- This one specifically makes me wonder if we want to be using the 
simple "change" value. 'modified', 'renamed', 'added', 'deleted' seem 
reasonable, 'modified and renamed' seems a little weird to use as a key. 
Not enough to say we shouldn't do it. But just a little.

Anyway, these are just thoughts, not anything that I would merge 
without. Someone else can decide if they want to be more strict about 
it. Good job overall.

For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3Caac8d3110708150229q23b6eba5l3c30d445be0a9862%40mail.gmail.com%3E