On 04/25/2013 08:19 PM, Ramkumar Ramachandra wrote:
> Felipe Contreras wrote:
>> diff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr
>> index aa7bc97..82bf7c7 100755
>> --- a/contrib/remote-helpers/git-remote-bzr
>> +++ b/contrib/remote-helpers/git-remote-bzr
>> @@ -94,7 +94,7 @@ class Marks:
>> return self.last_mark
>>
>> def is_marked(self, rev):
>> - return self.marks.has_key(rev)
>> + return rev in self.marks
>
> Why? Is the new form faster than the older one?
>
I think the has_key() method is "deprecated" in modern python,
and the 'key in dict' usage is more idiomatic.
>> @@ -224,7 +224,7 @@ def export_files(tree, files):
>> else:
>> mode = '100644'
>>
>> - # is the blog already exported?
>> + # is the blob already exported?
>
> What is this? Whitespace?
>
Typofix: s/blog/blob/
>> @@ -521,7 +521,7 @@ def c_style_unescape(string):
>> return string
>>
>> def parse_commit(parser):
>> - global marks, blob_marks, bmarks, parsed_refs
>> + global marks, blob_marks, parsed_refs
>
> How is this trivial? You just removed one argument.
>
Maybe bmarks was no longer used there as a global variable
(left-over from previous patches?), so there is no longer any
need to declare it global.
>> @@ -555,7 +555,7 @@ def parse_commit(parser):
>> mark = int(mark_ref[1:])
>> f = { 'mode' : m, 'data' : blob_marks[mark] }
>> elif parser.check('D'):
>> - t, path = line.split(' ')
>> + t, path = line.split(' ', 1)
>
> How on earth is this trivial? It changes the entire meaning!
>
Indeed, that should best go in a separate path with a proper
explanation in the commit message.
>> @@ -643,6 +643,7 @@ def do_export(parser):
>> wt = repo.bzrdir.open_workingtree()
>> wt.update()
>> print "ok %s" % ref
>> +
>
> Whitespace?
>
Isn't that obvious?
> I'm outraged by this. What kind of changes are you pushing to
> remote-hg? A "trivial cleanups" bundling miscellaneous changes, with
> no commit message? Why don't you just squash everything into one
> "miscellaneous changes" patch?
>
I have no opinion on this, so I won't comment.
Regard,
Stefano
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html