ValueError parsing git-unpeel-map

[Jelmer Vernooij]

Eli writes in 734145:

I tried that on another Windows XP machine, and this time sat tight and looked at the progress display. This attempt progressed further than the first one (why?), but still crashed. This crash happened during the "Adding files" phase (I think). Here's the full trace from .bzr.log:

0.172 bazaar version: 2.3.0
0.172 bzr arguments: [u'branch', u'git://git.savannah.gnu.org/gnulib.git', u'gnulib']
0.250 looking for plugins in C:/Documents and Settings/p0009057/Application Data/bazaar/2.0/plugins
0.250 looking for plugins in C:/Program Files/Bazaar/plugins
0.687 encoding stdout as sys.stdout encoding 'cp862'
1.969 creating repository in file:///D:/usr/eli/bzr/gnulib/.bzr/.
4776.563 Auto-packing repository GCRepositoryPackCollection(CHKInventoryRepository('file:///D:/usr/eli/bzr/gnulib/.bzr/repository/')), which has 10 pack files, containing 10000 revisions. Packing 10 files into 1 affecting 10000 revisions
4777.032 repacking 10000 revisions
4805.042 repacking 10000 inventories
4821.821 repacking chk: 9996 id_to_entry roots, 1367 p_id_map roots, 34202 total keys
4965.157 repacking 31402 texts
5222.928 repacking 0 signatures
5235.941 Auto-packing repository GCRepositoryPackCollection(CHKInventoryRepository('file:///D:/usr/eli/bzr/gnulib/.bzr/repository/')) completed
5986.415 checking remap as size shrunk by 33 to be 3766
11504.501 Packing repository GCRepositoryPackCollection(CHKInventoryRepository('file:///D:/usr/eli/bzr/gnulib/.bzr/repository/')), which has 6 pack files, containing 14209 revisions with hint ['c79de7dd918fdc13b993e076cc304930', 'c5262f497d1d2ef891abfdd4e081f78f', '44e334ecb978accce329a6869f16ba0a', '5bc887639994a43ffb41398ac364cf62', 'a1f0e2b578cf6cc997f612bb18449d9b', '92ff782999555875a58a42a006a73568', '1bb733c88ad0365ec65652b452f00eb5', '0ac4fe4c63fdf5562d81b960e3478bf5', '6845b86e17dc8696b823643f778676f6', 'ba338f71996ff1d4d6a05b5c3908ba1b', 'd7bf1202fc064565672808fa36904f2f', '468d78af6244482fcf198acd2af0f5bf', 'c534f399ecef73aa2b7f9b9a031e67b9', '4e5303b693dab42a8ee82c68e4818885', '1b328727266c999551430a67774c903b'].
11505.063 repacking 14209 revisions
11550.681 repacking 14209 inventories
11574.271 repacking chk: 14203 id_to_entry roots, 2242 p_id_map roots, 62418 total keys
11763.256 repacking 65256 texts
12245.507 repacking 0 signatures
12268.925 creating branch <bzrlib.branch.BzrBranchFormat7 object at 0x00CE9BB0> in file:///D:/usr/eli/bzr/gnulib/
12272.128 created new branch BzrBranch7(file:///D:/usr/eli/bzr/gnulib/)
12272.300 opening working tree 'D:/usr/eli/bzr/gnulib'
12441.304 Traceback (most recent call last):
  File "bzrlib\commands.pyo", line 923, in exception_to_return_code
  File "bzrlib\commands.pyo", line 1123, in run_bzr
  File "bzrlib\commands.pyo", line 691, in run_argv_aliases
  File "bzrlib\commands.pyo", line 710, in run
  File "bzrlib\cleanup.pyo", line 135, in run_simple
  File "bzrlib\cleanup.pyo", line 165, in _do_with_cleanups
  File "bzrlib\builtins.pyo", line 1234, in run
  File "bzrlib\tag.pyo", line 298, in _merge_tags_if_possible
  File "C:/Program Files/Bazaar/plugins\git\branch.py", line 149, in merge_to
  File "C:/Program Files/Bazaar/plugins\git\branch.py", line 128, in _merge_to_non_git
  File "C:/Program Files/Bazaar/plugins\git\refs.py", line 237, in from_repository
  File "C:/Program Files/Bazaar/plugins\git\refs.py", line 202, in load
ValueError: need more than 1 value to unpack

I have this session sitting in the debugger; let me know if I can supply more information.

[Jelmer Vernooij (jelmer)]

Unpeel map contents on that machine:

unpeel map version 1
d5daff19f4a26e67e7c410cefa9ddc13e036162a: d11be07eab194fc5e039c8d14eb423207409ff27
692e80d36f3d6aea3e812600815c97389d6bc4fb: e53ea8ba3ca2f871dee6b3550eda061aba64b87d

(looks reasonable)

[Jelmer Vernooij (jelmer)]

In the debugger, can you run:

print repr(l)

Looking at this again I suspect it's got something to do with line endings.

[Eli Zaretskii (eliz)]

I did that on another repository (git://git.savannah.gnu.org/findutils.git) which didn't require a 4-hour wait for the bug to happen ;-). In the meantime, I upgraded to bzr 2.3.1, so a traceback is slightly different. I show the traceback below, together with the data you asked for.

(Pdb) print repr(l)
'unpeel map version 1\n'

The .bzr/repository/git-unpeel-map file in the resulting repo is in Unix EOL format (only a newline character at the end of each line), and it says this:

unpeel map version 1
564859bff86173d28a51158edbba3e671fcc7ef5: 62870fdf2a80c63219d5b90de42e17d094cb0d04
0fb6853f15b3cdd8e3a9130002acd283d821115b: 267bcfff46dcea8c4cb3c0c8196eaba34872af74
1a1eec87dd90ebf8b6cd76196bc36c576f04e76c: a5043b436cb8abac9c84fa699b09f8bbe5bd77e4
177b8d6f9009680163d5cb324b04ddf4293609c3: 51849bfb4224f5641b738ea7f0af8cb785a1da3e
6dbe9ec45388d3fa8e8c7e3c8bd05b16080cc1ee: 984f8228176f61a63536852ba36e97170edf212c
cfe7068a20c65f1e9435be3dda94a5d7fe6fc924: e6e051d8a053f8c10a9c22d40a151a1f4f62f215
63363c95de6af1087abeafa9e68b273a96deea0a: b7b591dae88d4fb20d65f52b994efc2f6c2061e6
c05472245048dce78fd4c59365c2f8133cdc4030: a22323e08d8589751832b1adc0f4eebb66340608
e0ba22eac7393efabfaeaf183232559fd72c160b: 4714d4606ac86d1f7fdf23543dd2e5e9d8f1741c
0d2407529718dc16784a41736dfb174f2419d2ce: a9ca7c9af27d8dc88c8a10a36d80187e82a769eb
8ff39397dd84aeab696a51b7e33a653ff3d9d394: 99fc4015a111a6d34ce1f828240e20b4e22b20ba
59b48c9b1b1860954aa196cfbe26e1d68c629654: 58fdab00bce029ca5e2a84c0a7718f9abd74e8d4
137303374938627b41dede10bac9c536a121b498: 53cd9cf30f5b68e218b0346ea6edf7bf66b7caef
1f5cd46311947d0a17a845cbef790491e4da96ae: 713e93fb4c85b0e4a87dd555142f361769cce1e2
7048a7292596c10f618c3e6826245c7316394468: bba124bb147ed00ff0ae0ea1b22766c876a11460
613827844c581e6178fb5390fcf16bb1ff848705: 87fbc1b7627b749083b602b38336c4da687ad3a9
2bf4a6f6d7efd84427231131b2626769c0592237: 9fe4354aaec0a50c0bbf5ed8c021d3c679bb74d4
59eda1be931ec18cf9c67035c703f9fcd22d4a11: 3f63c5d1620f5dce43f216d94ae2299dd6fa653e
8212ddb768a95cc60809be2347228934d8da698e: c0c91efb2c7c7aa903de058568280b3a57d950d5
5faa9572bf59489b9c9737ef622f7adda7bf5f09: 673ddf6656649f5e47f0aab11146ff63107e76b4
cd0a24a4b6785269fce855040e31e8f05d6e4106: 9cdcfbc21e1f330c1f33f9ef21068a960ceda52c
85b257c424058db5f4edf8d3748602e8ca9426a6: 5c9081dfcdc36618d9e3ca0d0453d9162f74617d
998aa375a516641f6981a24961b739737f3d464f: 83d18d2ab6cdab1f00a7b9a0dfdddfe5a3df779c
dd5c0dc60fcac286205ada5d25b2b157b28d4720: 7654e0ec10332243f13cfaa5b493dd7cebefb77a
688146c9a39985b0cd615b300a12af5ffe234e28: 3a7cf1eca5484dcc14983466f65ad37ca49649b1
ed3acf4e27472abc9ee087145f8d6dadfaee2e5a: 07bd2b26d8fd8ba412ba3735b29c7741814731be
d63a927543c366605274cca09afc9dbafbe35267: 1dd53a3dc03f8ea8ca7ebdc1a199093097f47c09
8b57fb9fae81041f28cadec1fb9fcdfb5ae74e7f: e8decc0dfac27f4c18757839743c684faa294b28
10308a8b706825bb43dc78ae3a565913dcb2f008: ab1b4aeb3c8fd165ae78ea794b64a0cc8988cf2b
4348c9fcf5d67a53d3358dec835bfd6aa607aa2b: 7c7504047726a156422056ba584560ced33840a5
81a573407222d88809e7cc03320a93f41861ec22: 5ed503109b996d4bbb6410d9306bbaa5a7e9d344
8b35d67929496c8e2970a88842830762774286b9: 321d488dc9cf4b06ddb7d444f272e208fea3f511
8b35d67929496c8e2970a88842830762774...

[Jelmer Vernooij (jelmer)]

On Sat, 2011-03-19 at 08:54 +0000, Eli Zaretskii wrote:
> I did that on another repository
> (git://git.savannah.gnu.org/findutils.git) which didn't require a 4-hour
> wait for the bug to happen ;-). In the meantime, I upgraded to bzr
> 2.3.1, so a traceback is slightly different. I show the traceback
> below, together with the data you asked for.
Should be fixed in current bzr-git trunk (or at least one aspect of it
is).

  status fixcommitted

Cheers,

Jelmer

[Eli Zaretskii (eliz)]

Thanks, I tried that with findutils and with git://git.savannah.gnu.org/acl.git (which previously would crash like that), and both now run flawlessly to completion.

[Jelmer Vernooij (jelmer)]

Great, thanks for confirming.

This wasn't actually related to windows, but rather to the fact that some code didn't run when python was used in -O mode. It skipped some "assert" statements, one of which had a side-effect that was actually required.

[Martin Pool (mbp)]

On 20 March 2011 09:02, Jelmer Vernooij <email address hidden> wrote:
> Great, thanks for confirming.
>
> This wasn't actually related to windows, but rather to the fact that
> some code didn't run when python was used in -O mode. It skipped some
> "assert" statements, one of which had a side-effect that was actually
> required.

Maybe you should stop using (or even ban) assert, like we do in bzr?
I don't find that separate debug builds work as well in Python as they
sometimes can in C. (Especially because users can turn it on or off
without being aware of the consequences.)

[Jelmer Vernooij (jelmer)]

On Tue, 2011-03-22 at 09:02 +0000, Martin Pool wrote:
> On 20 March 2011 09:02, Jelmer Vernooij <email address hidden> wrote:
> > Great, thanks for confirming.
> >
> > This wasn't actually related to windows, but rather to the fact that
> > some code didn't run when python was used in -O mode. It skipped some
> > "assert" statements, one of which had a side-effect that was actually
> > required.
>
> Maybe you should stop using (or even ban) assert, like we do in bzr?
> I don't find that separate debug builds work as well in Python as they
> sometimes can in C. (Especially because users can turn it on or off
> without being aware of the consequences.)
There are a few places where I do actually do fairly expensive things in
assert statements that I would like to disable for production users. How
does Bazaar handle that sort of situation?

Cheers,

Jelmer

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 3/22/2011 1:02 PM, Jelmer Vernooij wrote:
> On Tue, 2011-03-22 at 09:02 +0000, Martin Pool wrote:
>> On 20 March 2011 09:02, Jelmer Vernooij <email address hidden> wrote:
>>> Great, thanks for confirming.
>>>
>>> This wasn't actually related to windows, but rather to the fact that
>>> some code didn't run when python was used in -O mode. It skipped some
>>> "assert" statements, one of which had a side-effect that was actually
>>> required.
>>
>> Maybe you should stop using (or even ban) assert, like we do in bzr?
>> I don't find that separate debug builds work as well in Python as they
>> sometimes can in C. (Especially because users can turn it on or off
>> without being aware of the consequences.)
> There are a few places where I do actually do fairly expensive things in
> assert statements that I would like to disable for production users. How
> does Bazaar handle that sort of situation?
>
> Cheers,
>
> Jelmer
>

If you had to, you can use:

if __debug__:

That is actually the python magic that assert uses.

That said, the whole point of Martin's comment is that:

 a) It generally isn't useful to do stuff in __debug__ and not
    elsewhere. Because often you won't realize that a critical section
    is in __debug__.
 b) Nobody else really does that, so you don't save on anything but
    your own code.
 c) If you *really* want it, consider using a "-D" style flag.
    (debug.debug_flags)
 d) Most of that sort of stuff should be covered in a test case, rather
    than in production code.
    So the expensive 'really validate things are correct', should be
    turned into a test case that is run infrequently, rather than
    something that slows down production code.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2IqnAACgkQJdeBCYSNAAMKbwCfVxB5CMBm22fs/TfSSFQ7TTJT
CX0AoMhLePjFg3O8sFm8aQQ5F28vIi70
=cPoX
-----END PGP SIGNATURE-----

[Andrew Bennetts (spiv)]

Jelmer Vernooij wrote:
[…]
> There are a few places where I do actually do fairly expensive things in
> assert statements that I would like to disable for production users. How
> does Bazaar handle that sort of situation?

I agree with John's mail, but I think also the key issue is that
“users/environments that use python -O or not” is not very well
correlated with “is production user/environment or not”.

So triggering these extra runtime checks is probably better done by an
explicit debug flag, or perhaps by checking the version_info to see if
this is a dev/beta vs. final/pre release, etc.

The question of when it's appropriate to emit warnings intended for
developers (API deprecations and the like) is probably closely related.

[Martin Pool (mbp)]

I think a debug_flag is the best way to go, because there is fairly
little risk of users or distributions unintentionally un/setting it
(as there is with -O), and it's also easy to specifically turn it on
if you want debug data from a user situation, or if you always want to
have it turned on yourself.

Our infrastructure there is certainly not perfect, and if you can
think of what in particular it's lacking do say so. One thing that
comes to mind is that I think we're a little unclear about how debug
flags should/can be used in testing, which might be good for
assertion-type checks.

(I guess possibly some tests would actually want to run scenarios of
debug on vs debug off.)