Patchwork [1,of,8,py3] bundle2: raise a more helpful error if building a bundle part header fails

login
register
mail settings
Submitter Augie Fackler
Date Sept. 15, 2017, 11:14 p.m.
Message ID <662bbd6d96952985eff8.1505517244@augie-macbookpro2.roam.corp.google.com>
Download mbox | patch
Permalink /patch/23924/
State Accepted
Headers show

Comments

Augie Fackler - Sept. 15, 2017, 11:14 p.m.
# HG changeset patch
# User Augie Fackler <raf@durin42.com>
# Date 1505515049 14400
#      Fri Sep 15 18:37:29 2017 -0400
# Node ID 662bbd6d96952985eff807f424dd128663724672
# Parent  209120041d12b524648fa856732aa404dfedd91d
bundle2: raise a more helpful error if building a bundle part header fails

I've tripped on this several times now, and am tired of debugging. Now
the header parts are part of the error message when the ''.join()
fails, which makes debugging obvious.
Yuya Nishihara - Sept. 16, 2017, 11:54 a.m.
On Fri, 15 Sep 2017 19:14:04 -0400, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <raf@durin42.com>
> # Date 1505515049 14400
> #      Fri Sep 15 18:37:29 2017 -0400
> # Node ID 662bbd6d96952985eff807f424dd128663724672
> # Parent  209120041d12b524648fa856732aa404dfedd91d
> bundle2: raise a more helpful error if building a bundle part header fails
> 
> I've tripped on this several times now, and am tired of debugging. Now
> the header parts are part of the error message when the ''.join()
> fails, which makes debugging obvious.
> 
> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
> --- a/mercurial/bundle2.py
> +++ b/mercurial/bundle2.py
> @@ -1050,7 +1050,11 @@ class bundlepart(object):
>              header.append(key)
>              header.append(value)
>          ## finalize header
> -        headerchunk = ''.join(header)
> +        try:
> +            headerchunk = ''.join(header)
> +        except TypeError:
> +            raise TypeError(u'Found a non-bytes trying to '
> +                            u'build bundle part header: %r' % header)

I was making it a r'' string, but probably ProgrammingError would be better.
Augie Fackler - Sept. 16, 2017, 3:08 p.m.
> On Sep 16, 2017, at 7:54 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> On Fri, 15 Sep 2017 19:14:04 -0400, Augie Fackler wrote:
>> # HG changeset patch
>> # User Augie Fackler <raf@durin42.com>
>> # Date 1505515049 14400
>> #      Fri Sep 15 18:37:29 2017 -0400
>> # Node ID 662bbd6d96952985eff807f424dd128663724672
>> # Parent  209120041d12b524648fa856732aa404dfedd91d
>> bundle2: raise a more helpful error if building a bundle part header fails
>> 
>> I've tripped on this several times now, and am tired of debugging. Now
>> the header parts are part of the error message when the ''.join()
>> fails, which makes debugging obvious.
>> 
>> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
>> --- a/mercurial/bundle2.py
>> +++ b/mercurial/bundle2.py
>> @@ -1050,7 +1050,11 @@ class bundlepart(object):
>>             header.append(key)
>>             header.append(value)
>>         ## finalize header
>> -        headerchunk = ''.join(header)
>> +        try:
>> +            headerchunk = ''.join(header)
>> +        except TypeError:
>> +            raise TypeError(u'Found a non-bytes trying to '
>> +                            u'build bundle part header: %r' % header)
> 
> I was making it a r'' string, but probably ProgrammingError would be better.

I was conflicted on this one. Preserving the type of the exception seemed reasonable, since this can only happen with weird bugs, but it’s also something I can just sit on for a while as a debugging aid.
Yuya Nishihara - Sept. 16, 2017, 11:31 p.m.
On Sat, 16 Sep 2017 11:08:07 -0400, Augie Fackler wrote:
> 
> > On Sep 16, 2017, at 7:54 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> > 
> > On Fri, 15 Sep 2017 19:14:04 -0400, Augie Fackler wrote:
> >> # HG changeset patch
> >> # User Augie Fackler <raf@durin42.com>
> >> # Date 1505515049 14400
> >> #      Fri Sep 15 18:37:29 2017 -0400
> >> # Node ID 662bbd6d96952985eff807f424dd128663724672
> >> # Parent  209120041d12b524648fa856732aa404dfedd91d
> >> bundle2: raise a more helpful error if building a bundle part header fails
> >> 
> >> I've tripped on this several times now, and am tired of debugging. Now
> >> the header parts are part of the error message when the ''.join()
> >> fails, which makes debugging obvious.
> >> 
> >> diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
> >> --- a/mercurial/bundle2.py
> >> +++ b/mercurial/bundle2.py
> >> @@ -1050,7 +1050,11 @@ class bundlepart(object):
> >>             header.append(key)
> >>             header.append(value)
> >>         ## finalize header
> >> -        headerchunk = ''.join(header)
> >> +        try:
> >> +            headerchunk = ''.join(header)
> >> +        except TypeError:
> >> +            raise TypeError(u'Found a non-bytes trying to '
> >> +                            u'build bundle part header: %r' % header)
> > 
> > I was making it a r'' string, but probably ProgrammingError would be better.
> 
> I was conflicted on this one. Preserving the type of the exception seemed reasonable, since this can only happen with weird bugs, but it’s also something I can just sit on for a while as a debugging aid.

Okay, queued with s/u'/r'/ change in flight, thanks.

Patch

diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
--- a/mercurial/bundle2.py
+++ b/mercurial/bundle2.py
@@ -1050,7 +1050,11 @@  class bundlepart(object):
             header.append(key)
             header.append(value)
         ## finalize header
-        headerchunk = ''.join(header)
+        try:
+            headerchunk = ''.join(header)
+        except TypeError:
+            raise TypeError(u'Found a non-bytes trying to '
+                            u'build bundle part header: %r' % header)
         outdebug(ui, 'header chunk size: %i' % len(headerchunk))
         yield _pack(_fpartheadersize, len(headerchunk))
         yield headerchunk