Have not had a chance to look at the whole patch yet, but here are some initial comments.
https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py File contrib/profile_merge.py (right): https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode53 contrib/profile_merge.py:53: data_file = open(path, 'rb') How about simpler version: with open(file, 'rb') as f: data = f.read() https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode59 contrib/profile_merge.py:59: def MergeCounters(objs, index, multipliers): Perhaps use function name that shows that it returns the value. https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode78 contrib/profile_merge.py:78: length: Length of the data on the disk Minor style. Make sure format is coherent (ends with . or not, starts with capital letter or not). https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode119 contrib/profile_merge.py:119: self.ident = 0 It is ever useful to have a function object with no reader? https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode138 contrib/profile_merge.py:138: return self.ArcCounters().counters[0] This will throw an exception if no counters exists. Is that expected? https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode209 contrib/profile_merge.py:209: class Lines(DataObject): Where is this used? https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1088 contrib/profile_merge.py:1088: dir: A path to the directory containing the gcda/gcno. A cleaner way could perhaps be to have a base profile archive class and then classes for ProfileArchieveDir and ProfileArchiveZip? https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1153 contrib/profile_merge.py:1153: os.chdir(direc) How about using absolute paths to avoid the chdir logic? https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1199 contrib/profile_merge.py:1199: outfile_path = output_path + '/' + f os.path.join() instead of +, here and other places where the pattern is used. https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1213 contrib/profile_merge.py:1213: def Merge(self, archives, multipliers): The code seems to handle both the case when 'self' is inside archives and when it is not. Does both happen when running the code? https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1233 contrib/profile_merge.py:1233: for i, a in enumerate(archives): It's hard to see what i is in this case. I would prefer to have information about arguments in the documentation at the top of the function, but more descriptive variable names and comments would also help. https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1288 contrib/profile_merge.py:1288: group = OptionGroup(parser, 'Arguments', You don't seem to add any options to the group? It seems not necessary to use a group at all in this program. https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1313 contrib/profile_merge.py:1313: input_profiles[0].Merge(input_profiles, profile_multipliers) Did you conside creating a new object instead of modifying the existing? https://codereview.appspot.com/8508048/diff/4001/contrib/profile_merge.py#newcode1317 contrib/profile_merge.py:1317: input_profiles[0].Write(output_profile) Warn before overwriting? https://codereview.appspot.com/8508048/
