threads / patch / 33722
Subject: [PATCH v2 1/2] remote-bzr: convert all unicode keys to str
Felipe Contreras[PATCH v2 0/2] remote-bzr: couple of fixes
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
Felipe ContrerasOtherwise 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
Felipe Contreras[PATCH v2 2/2] remote-bzr: avoid bad refs
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
Stefano LattariniRe: [PATCH v2 2/2] remote-bzr: avoid bad refs
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
Junio C HamanoRe: [PATCH v2 0/2] remote-bzr: couple of fixes
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(-)
Felipe ContrerasRe: [PATCH v2 0/2] remote-bzr: couple of fixes
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
Junio C HamanoRe: [PATCH v2 0/2] remote-bzr: couple of fixes
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'?
Felipe ContrerasRe: [PATCH v2 0/2] remote-bzr: couple of fixes
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
Junio C HamanoRe: [PATCH v2 0/2] remote-bzr: couple of fixes
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.
Felipe ContrerasRe: [PATCH v2 0/2] remote-bzr: couple of fixes
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