[PATCH v2 0/2] remote-bzr: couple of fixes

[PATCH v2 0/2] remote-bzr: couple of fixes

[Felipe Contreras]

Hi,

The previous version had an indentation bug (did I mention I hate python?).

A few fixes to be applied on top of the massive changes already queued. Nothing
major.

Felipe Contreras (2):
  remote-bzr: convert all unicode keys to str
  remote-bzr: avoid bad refs

 contrib/remote-helpers/git-remote-bzr | 42 +++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 17 deletions(-)

-- 
1.8.3.rc0.401.g45bba44

[PATCH v2 1/2] remote-bzr: convert all unicode keys to str

[Felipe Contreras]

Otherwise some versions of bazaar might barf.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/git-remote-bzr | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr
index 161f831..bbaaa8f 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -95,7 +95,7 @@ class Marks:
         return self.marks[rev]
 
     def to_rev(self, mark):
-        return self.rev_marks[mark]
+        return str(self.rev_marks[mark])
 
     def next_mark(self):
         self.last_mark += 1
@@ -621,7 +621,7 @@ def parse_commit(parser):
         files[path] = f
 
     committer, date, tz = committer
-    parents = [str(mark_to_rev(p)) for p in parents]
+    parents = [mark_to_rev(p) for p in parents]
     revid = bzrlib.generate_ids.gen_revision_id(committer, date)
     props = {}
     props['branch-nick'] = branch.nick
-- 
1.8.3.rc0.401.g45bba44

[PATCH v2 2/2] remote-bzr: avoid bad refs

[Felipe Contreras]

Turns out fast-export throws bad 'reset' commands because of a behavior
in transport-helper that is not even needed.

We should ignore them, otherwise we will threat them as branches and
fail.

This was fixed in v1.8.2, but some people use this script in older
versions of git.

Also, check if the ref was a tag, and skip it for now.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/git-remote-bzr | 38 +++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr
index bbaaa8f..0ef30f8 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -682,23 +682,31 @@ def do_export(parser):
             die('unhandled export command: %s' % line)
 
     for ref, revid in parsed_refs.iteritems():
-        name = ref[len('refs/heads/'):]
-        branch = bzrlib.branch.Branch.open(branches[name])
-        branch.generate_revision_history(revid, marks.get_tip(name))
+        if ref.startswith('refs/heads/'):
+            name = ref[len('refs/heads/'):]
+            branch = bzrlib.branch.Branch.open(branches[name])
+            branch.generate_revision_history(revid, marks.get_tip(name))
 
-        if name in peers:
-            peer = bzrlib.branch.Branch.open(peers[name])
-            try:
-                peer.bzrdir.push_branch(branch, revision_id=revid)
-            except bzrlib.errors.DivergedBranches:
-                print "error %s non-fast forward" % ref
-                continue
+            if name in peers:
+                peer = bzrlib.branch.Branch.open(peers[name])
+                try:
+                    peer.bzrdir.push_branch(branch, revision_id=revid)
+                except bzrlib.errors.DivergedBranches:
+                    print "error %s non-fast forward" % ref
+                    continue
 
-        try:
-            wt = branch.bzrdir.open_workingtree()
-            wt.update()
-        except bzrlib.errors.NoWorkingTree:
-            pass
+            try:
+                wt = branch.bzrdir.open_workingtree()
+                wt.update()
+            except bzrlib.errors.NoWorkingTree:
+                pass
+        elif ref.startswith('refs/tags/'):
+            # TODO: implement tag push
+            print "error %s pushing tags not supported" % ref
+            continue
+        else:
+            # transport-helper/fast-export bugs
+            continue
 
         print "ok %s" % ref
 
-- 
1.8.3.rc0.401.g45bba44

Re: [PATCH v2 2/2] remote-bzr: avoid bad refs

[Stefano Lattarini]

On 05/04/2013 02:31 AM, Felipe Contreras wrote:
> Turns out fast-export throws bad 'reset' commands because of a behavior
> in transport-helper that is not even needed.
> 
> We should ignore them, otherwise we will threat
>
s/threat/treat/

> them as branches and fail.
> 
> This was fixed in v1.8.2, but some people use this script in older
> versions of git.
> 
> Also, check if the ref was a tag, and skip it for now.

Regards,
  Stefano

Re: [PATCH v2 0/2] remote-bzr: couple of fixes

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> The previous version had an indentation bug (did I mention I hate python?).
>
> A few fixes to be applied on top of the massive changes already queued. Nothing
> major.

[2/2] may not matter much in the context of my tree (people would
use post 1.8.2 fast-export if they are using remote-bzr from 1.8.3
from my tree ;-), but [1/2] sounds like it is a good thing to have
in 1.8.3 (not "on top of that 'massive' series").

Assuming the "otherwise some version of bzr might barf" problem is
that repo.generate_revision_history() in those versions may not
apply str() to its first parameter and the caller is expected to
pass a string there, or something?

Thanks.

>
> Felipe Contreras (2):
>   remote-bzr: convert all unicode keys to str
>   remote-bzr: avoid bad refs
>
>  contrib/remote-helpers/git-remote-bzr | 42 +++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 17 deletions(-)

Re: [PATCH v2 0/2] remote-bzr: couple of fixes

[Felipe Contreras]

On Sun, May 5, 2013 at 1:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> The previous version had an indentation bug (did I mention I hate python?).
>>
>> A few fixes to be applied on top of the massive changes already queued. Nothing
>> major.
>
> [2/2] may not matter much in the context of my tree (people would
> use post 1.8.2 fast-export if they are using remote-bzr from 1.8.3
> from my tree ;-),

Maybe, but if even if they have the latest git, pushing a tag will
fail miserably, and with the patch it would fail nicely :)

> but [1/2] sounds like it is a good thing to have
> in 1.8.3 (not "on top of that 'massive' series").
>
> Assuming the "otherwise some version of bzr might barf" problem is
> that repo.generate_revision_history() in those versions may not
> apply str() to its first parameter and the caller is expected to
> pass a string there, or something?

No, there's no change to repo.generate_revision_history(), because we
already convert the elements of the array to strings, it's the other
callers of Marks::to_rev() that see a change, namely code that pushes
to a remote, I think.

And BTW, they are already strings, but unicode strings, because they
come from a json file, somehow bazaar doesn't like that, but it works
fine in my machine without the patch. Shrugs.

Also, the emacs developers seem to be fine with all these changes,
there's only one patch pending that I need to cleanup.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v2 0/2] remote-bzr: couple of fixes

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Sun, May 5, 2013 at 1:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> The previous version had an indentation bug (did I mention I hate python?).
>>>
>>> A few fixes to be applied on top of the massive changes already queued. Nothing
>>> major.
>>
>> [2/2] may not matter much in the context of my tree (people would
>> use post 1.8.2 fast-export if they are using remote-bzr from 1.8.3
>> from my tree ;-),
>
> Maybe, but if even if they have the latest git, pushing a tag will
> fail miserably, and with the patch it would fail nicely :)
>
>> but [1/2] sounds like it is a good thing to have
>> in 1.8.3 (not "on top of that 'massive' series").
>>
>> Assuming the "otherwise some version of bzr might barf" problem is
>> that repo.generate_revision_history() in those versions may not
>> apply str() to its first parameter and the caller is expected to
>> pass a string there, or something?
>
> No, there's no change to repo.generate_revision_history(), because we
> already convert the elements of the array to strings, it's the other
> callers of Marks::to_rev() that see a change, namely code that pushes
> to a remote, I think.
>
> And BTW, they are already strings, but unicode strings, because they
> come from a json file, somehow bazaar doesn't like that, but it works
> fine in my machine without the patch. Shrugs.
>
> Also, the emacs developers seem to be fine with all these changes,
> there's only one patch pending that I need to cleanup.

So do you want to queue these on top of the "massive" in 'next', not
directly on 'master'?

Re: [PATCH v2 0/2] remote-bzr: couple of fixes

[Felipe Contreras]

On Sun, May 5, 2013 at 2:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Sun, May 5, 2013 at 1:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>>> The previous version had an indentation bug (did I mention I hate python?).
>>>>
>>>> A few fixes to be applied on top of the massive changes already queued. Nothing
>>>> major.
>>>
>>> [2/2] may not matter much in the context of my tree (people would
>>> use post 1.8.2 fast-export if they are using remote-bzr from 1.8.3
>>> from my tree ;-),
>>
>> Maybe, but if even if they have the latest git, pushing a tag will
>> fail miserably, and with the patch it would fail nicely :)
>>
>>> but [1/2] sounds like it is a good thing to have
>>> in 1.8.3 (not "on top of that 'massive' series").
>>>
>>> Assuming the "otherwise some version of bzr might barf" problem is
>>> that repo.generate_revision_history() in those versions may not
>>> apply str() to its first parameter and the caller is expected to
>>> pass a string there, or something?
>>
>> No, there's no change to repo.generate_revision_history(), because we
>> already convert the elements of the array to strings, it's the other
>> callers of Marks::to_rev() that see a change, namely code that pushes
>> to a remote, I think.
>>
>> And BTW, they are already strings, but unicode strings, because they
>> come from a json file, somehow bazaar doesn't like that, but it works
>> fine in my machine without the patch. Shrugs.
>>
>> Also, the emacs developers seem to be fine with all these changes,
>> there's only one patch pending that I need to cleanup.
>
> So do you want to queue these on top of the "massive" in 'next', not
> directly on 'master'?

If they apply on master, master. But I'm confused, are the massive
changes not going to graduate to master? Because if not, I should
cherry-pick the safest changes, as there's a lot of good stuff there.

-- 
Felipe Contreras

Re: [PATCH v2 0/2] remote-bzr: couple of fixes

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

>> So do you want to queue these on top of the "massive" in 'next', not
>> directly on 'master'?
>
> If they apply on master, master. But I'm confused, are the massive
> changes not going to graduate to master? Because if not, I should
> cherry-pick the safest changes, as there's a lot of good stuff there.

I think we discussed and agreed that we would ship it in 1.8.3 if we
hear positive feedback from Emacs folks, and my understanding is
that I was waiting for you to give me a go-ahead once that happens.

It is entirely up to you to add these two on top of that "massive"
stuff, their fate decided by feedback from Emacs folks, or apply
these as "much safer than those we need to hear from them; we can
verify their validity and safety ourselves without knowing the real
world projects that use the program" patches.

The impression I was getting from your response "I hear it breaks
for some of them without the patch but I haven't seen the breakage
myself" is that it is safer to group 2/2 as part of the rest of the
series, but as I heard in the same message that you heard Emacs
folks are happy with the entire series, so it wouldn't make much of
a difference either way.

Will apply these two to the tip of the "massive" stuff, and merge
the result before the next -rc.

Thanks.

Re: [PATCH v2 0/2] remote-bzr: couple of fixes

[Felipe Contreras]

On Sun, May 5, 2013 at 3:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>> So do you want to queue these on top of the "massive" in 'next', not
>>> directly on 'master'?
>>
>> If they apply on master, master. But I'm confused, are the massive
>> changes not going to graduate to master? Because if not, I should
>> cherry-pick the safest changes, as there's a lot of good stuff there.
>
> I think we discussed and agreed that we would ship it in 1.8.3 if we
> hear positive feedback from Emacs folks, and my understanding is
> that I was waiting for you to give me a go-ahead once that happens.

Yeah, and I just said everything seems to be fine. There's only one
more patch that would be good to have that I still haven't cleaned up.

> It is entirely up to you to add these two on top of that "massive"
> stuff, their fate decided by feedback from Emacs folks, or apply
> these as "much safer than those we need to hear from them; we can
> verify their validity and safety ourselves without knowing the real
> world projects that use the program" patches.
>
> The impression I was getting from your response "I hear it breaks
> for some of them without the patch but I haven't seen the breakage
> myself" is that it is safer to group 2/2 as part of the rest of the
> series, but as I heard in the same message that you heard Emacs
> folks are happy with the entire series, so it wouldn't make much of
> a difference either way.
>
> Will apply these two to the tip of the "massive" stuff, and merge
> the result before the next -rc.

Cool, I think that's the best approach. I'll send the last patch later today.

Cheers.

-- 
Felipe Contreras