Code review comment for lp:~jspashett/bzr/587868_args_handling_cant_debug

Martin [gz] wrote:
> Hm, yes. Means move some code around, because the order must be:
> 1) Split line
> 2) Strip args from front
> 3) Glob unquoted args
>
> The unit tests for _command_line_to_argv aren't expecting step 2 so don't think you'll need to be looking at sys.argv there. You could change the tests, but it's good to have them reasonably isolated. Might be better to split the three steps into separate private functions and call 1 followed by 3 for those tests.
>
> I don't think assigning to sys.argv will really help anything, there's only one entry point for this code already, it's just been poked too many times and become rather gnarly.
>
>
Yes, well I have more questions than answers now. Presumably we have to
call GetCommandLineW on windows. So what happens on linux if the locale
is ascii but a file we try and open is utf with 'unicode' characters in
it? Does it fail? Sorry to be a pain but I am just trying to understand
why things are they way they are.

I was thinking of this sort of thing (not exactly this), so that the
command line is 'fixed' and out of the way. Of course there would need
to be changes in win32utils.py too, refactorings, which I've hacked in
but not shown.
C:\bzr\bazaar\587868_args_handling_cant_debug>bzr diff
=== modified file 'bzr'
--- bzr 2010-05-27 17:59:18 +0000
+++ bzr 2010-06-11 00:42:26 +0000
@@ -49,6 +49,16 @@
 if hasattr(os, "unsetenv"):
     os.unsetenv(REINVOKE)

+if sys.platform.startswith('win'):
+ from bzrlib.win32utils import get_unicode_argv
+ new_args = [u'bzr']
+ new_args.extend(get_unicode_argv())
+ if len(new_args) < len(sys.argv):
+ raise AssertError("len(GetCommandLineW(...)) < len(sys.argv)
should not be possible")
+ new_args = new_args[len(new_args) - len(sys.argv):]
+ sys.argv = new_args
+
+

In other words the work done to get the correct command line is called
once from bzr. Things appear to then work, including the unit tests.

> On the --profile-imports front, how about just removing the removal from from the bzr script and adding to bzrlib.commands.run_bzr something like:
> elif a == '--profile-imports':
> pass # already handled in startup script
>

Should probably do this anyway/aswell.