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

Revision history for this message
John A Meinel (jameinel) wrote :

One problem I potentially see with the plugin is this:
585 +def filename_matches_config(params):
586 + config = params.merger.this_branch.get_config()
587 + affected_files = config.get_user_option_as_list('news_merge_files')
588 + if affected_files:
589 + filename = params.merger.this_tree.id2path(params.file_id)
590 + if filename in affected_files:
591 + return True
592 + return False

The issue is that "branch.get_config()" is not cached, and re-reads all config files. And you trigger this for *all* modified files. So if you are merging, say, bzr.dev after 20 revs, you'll get 50 changed files, and we will re-read branch.conf, bazaar.conf and locations.conf 50 times.

I wonder if we should have a place on MergeHookParams to store hook data...

Otherwise I like this revision.

review: Approve

« Back to merge proposal