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

Revision history for this message
Jason Spashett (jspashett) wrote :

Alexander Belchenko wrote:
> I don't think that removing my fix for bug 588277 is the right thing. And btw, have you tested your branch manually with --profile-imports in the real command line?
>
Yes I did test it with "python bzr --profile-imports status". Perhaps
it's not right as you say but It does get rid of the message "bzr:
ERROR: no such option: --profile-imports" though, and it does print out
the profile to stdout.

Like so:

C:\bzr\bazaar\587868_args_handling_cant_debug>python bzr.py
--profile-imports status 2>&1 | more
unknown:
  .project
  .pydevproject
  bzr.py
  cum inline name @ file:line
 92.2 1.5 bzrlib.commands @ __main__:133
 80.8 1.0 + [disable_plugins, load_plugins]bzrlib.plugin @
bzrlib.commands:52
 79.8 61.1 ++ [osutils]bzrlib @ bzrlib.plugin:36
  ...etc

It works because --profile-imports is removed from sys.argv, and that is
the length used in my fix, so it is also removed from the string
returned by GetCommandLineW
However this isn't correct because if you run python bzr status
--profile-imports 2>&1 | more with --profile-imports last, it does not
work. So your code is required. I could uncomit my change and do it
again. But since sys.argv is modified I don't think my fix will work
properly with as it assumes the arguments to the front should be removed
not arbitrary ones in the middle.

bzr, line 55: sys.argv.remove('--profile-imports')

I think therefore I will scrap this branch as I don't think there is a
way around this.

« Back to merge proposal