Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Stefano Lattarini <stefano.lattarini@gmail.com>]

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