On Thu, Apr 25, 2013 at 1:19 PM, Ramkumar Ramachandra
<artagnon@xxxxxxxxx> 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?
has_key is deprecated.
>> @@ -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?
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.
It's not an argument.
>> @@ -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!
And nobody has noticed any problem.
>> @@ -643,6 +643,7 @@ def do_export(parser):
>> wt = repo.bzrdir.open_workingtree()
>> wt.update()
>> print "ok %s" % ref
>> +
>
> Whitespace?
Aka. trivial.
> 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?
There are no miscellaneous changes other than the *possible* fix for
deleted files. Which we don't know if it would actually fix anything,
but as far as we know if it's a bug, nobody has seen it, and if it
isn't, it's very unlikely that anybody is relying on the current
behavior.
Plus the change seems to be obviously correct, as it comes from
remote-hg, where somebody appeared to have found a bug.
That being said, I do remember writing an explanation for this in the
commit message:
--
commit ca8c02dc7ea6395b1c864296f2500b718892fab8
Reflog: HEAD@{143} (Felipe Contreras <felipe.contreras@xxxxxxxxx>)
Reflog message: rebase -i (fixup): remote-bzr: trivial cleanups
Author: Felipe Contreras <felipe.contreras@xxxxxxxxx>
Date: Tue Apr 23 18:29:49 2013 -0500
remote-bzr: trivial cleanups
Mostly from remote-hg. It's possible that there's a fix to delete files
with spaces.
Signed-off-by: Felipe Contreras <felipe.contreras@xxxxxxxxx>
---
Yeap, there it is. It was just squashed by mistake.
But I do not care that much really. The patch is good either way, if
you don't like it, you go ahead and fix it, because I won't. I have
174 remote-helper related patches in my queue, and nobody benefits
from rambling about a one liner that is obviously correct, not you,
not me, not the users, not the developers.
Junio of course might disagree and drop this patch, but then he would
need to deal with the fallout of possible conflicts. Or he can do the
sensible thing and pick the commit message above. I have real issues
to deal with, and I think the less-than-perfect commit messages in a
*contrib* script that is extremely recent is a small price to pay for
having nice and workable bzr and mercurial remote-helpers as soon as
possible; our users would thank us, and in fact, they already are.
In my hurry to reorganize all the commits of my fourteen remote-helper
branches, I squashed the commit message of a trivial fix into trivial
cleanups. Big whooping deal.
> Why don't you just squash everything into one
> "miscellaneous changes" patch?
Hyperbole much?
Cheers.
--
Felipe Contreras
--
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