Review Request #1537: patch for perforce. If changing the security level of perforce server, perhaps fail to login perforce when creating diffs.

Information
Owner:	seagle0128
Repository:	Review Board
Branch:	master, 1.0.x
Bugs:	1623
Depends On:
Groups:	reviewboard
People:	chipx86

SE

Branch:

-	1.0.x
+	master

I really don't understand the purpose of this change or why it's done the way it is. It concerns me, since we're calling out to an external program and commenting out code known to work in some environments. Please add a more detailed description discussing the rationale for the change and how this fixes the problem.

SE

seagle0128 May 14, 2010, 5:27 a.m.

This patch if to to solve this issue:

Traceback (most recent call last):
  File "c:\python25\lib\site-packages\ReviewBoard-1.0.8-py2.5.egg\reviewboard\diffviewer\views.py", line 153, in view_diff
    interdiffset, highlighting, True)
  File "c:\python25\lib\site-packages\ReviewBoard-1.0.8-py2.5.egg\reviewboard\diffviewer\diffutils.py", line 623, in get_diff_files
    large_data=True)
  File "c:\python25\lib\site-packages\Djblets-0.6.1-py2.5.egg\djblets\util\misc.py", line 166, in cache_memoize
    data = lookup_callable()
  File "c:\python25\lib\site-packages\ReviewBoard-1.0.8-py2.5.egg\reviewboard\diffviewer\diffutils.py", line 622, in <lambda>
    enable_syntax_highlighting),
  File "c:\python25\lib\site-packages\ReviewBoard-1.0.8-py2.5.egg\reviewboard\diffviewer\diffutils.py", line 344, in get_chunks
    old = get_original_file(filediff)
  File "c:\python25\lib\site-packages\ReviewBoard-1.0.8-py2.5.egg\reviewboard\diffviewer\diffutils.py", line 251, in get_original_file
    large_data=True)[0]
  File "c:\python25\lib\site-packages\Djblets-0.6.1-py2.5.egg\djblets\util\misc.py", line 166, in cache_memoize
    data = lookup_callable()
  File "c:\python25\lib\site-packages\ReviewBoard-1.0.8-py2.5.egg\reviewboard\diffviewer\diffutils.py", line 250, in <lambda>
    data = cache_memoize(key, lambda: [fetch_file(file, revision)],
  File "c:\python25\lib\site-packages\ReviewBoard-1.0.8-py2.5.egg\reviewboard\diffviewer\diffutils.py", line 229, in fetch_file
    data = tool.get_file(file, revision)
  File "C:\Python25\lib\site-packages\reviewboard-1.0.8-py2.5.egg\reviewboard\scmtools\perforce.py", line 101, in get_file
    raise SCMError('\n'.join(line.lstrip("\t") for line in error))
SCMError: Password not allowed at this server security level, use 'p4 login'.

Now I change to use p4 API.

reviewboard/scmtools/perforce.py (Diff revision 1)

We can't use echo like this. We might not be running on a system where it works. Please use the p4 api if at all possible, too.

SE

Description:

		patch for perforce. If changing the security level of perforce server, perhaps fail to login perforce when creating diffs.
	+
	+	To solve this issue:
	+
	+	Traceback (most recent call last):
	+	File "c:\python25\lib\site-packages\ReviewBoard-1.0.8-py2.5.egg\reviewboard\diffviewer\views.py", line 153, in view_diff
	+	interdiffset, highlighting, True)
	+	File "c:\python25\lib\site-packages\ReviewBoard-1.0.8-py2.5.egg\reviewboard\diffviewer\diffutils.py", line 623, in get_diff_files
	+	large_data=True)
	+	File "c:\python25\lib\site-packages\Djblets-0.6.1-py2.5.egg\djblets\util\misc.py", line 166, in cache_memoize
	+	data = lookup_callable()
	+	File "c:\python25\lib\site-packages\ReviewBoard-1.0.8-py2.5.egg\reviewboard\diffviewer\diffutils.py", line 622, in <lambda>
	+	enable_syntax_highlighting),
	+	File "c:\python25\lib\site-packages\ReviewBoard-1.0.8-py2.5.egg\reviewboard\diffviewer\diffutils.py", line 344, in get_chunks
	+	old = get_original_file(filediff)
	+	File "c:\python25\lib\site-packages\ReviewBoard-1.0.8-py2.5.egg\reviewboard\diffviewer\diffutils.py", line 251, in get_original_file
	+	large_data=True)[0]
	+	File "c:\python25\lib\site-packages\Djblets-0.6.1-py2.5.egg\djblets\util\misc.py", line 166, in cache_memoize
	+	data = lookup_callable()
	+	File "c:\python25\lib\site-packages\ReviewBoard-1.0.8-py2.5.egg\reviewboard\diffviewer\diffutils.py", line 250, in <lambda>
	+	data = cache_memoize(key, lambda: [fetch_file(file, revision)],
	+	File "c:\python25\lib\site-packages\ReviewBoard-1.0.8-py2.5.egg\reviewboard\diffviewer\diffutils.py", line 229, in fetch_file
	+	data = tool.get_file(file, revision)
	+	File "C:\Python25\lib\site-packages\reviewboard-1.0.8-py2.5.egg\reviewboard\scmtools\perforce.py", line 101, in get_file
	+	raise SCMError('\n'.join(line.lstrip("\t") for line in error))
	+	SCMError: Password not allowed at this server security level, use 'p4 login'.

SE

Diff:

Revision 2 (+2)

Show changes

reviewboard/scmtools/perforce.py

SE

Description:

		patch for perforce. If changing the security level of perforce server, perhaps fail to login perforce when creating diffs.
-
-		To solve this issue:
-
-		Traceback (most recent call last):
-		File "c:\python25\lib\site-packages\ReviewBoard-1.0.8-py2.5.egg\reviewboard\diffviewer\views.py", line 153, in view_diff
-		interdiffset, highlighting, True)
-		File "c:\python25\lib\site-packages\ReviewBoard-1.0.8-py2.5.egg\reviewboard\diffviewer\diffutils.py", line 623, in get_diff_files
-		large_data=True)
-		File "c:\python25\lib\site-packages\Djblets-0.6.1-py2.5.egg\djblets\util\misc.py", line 166, in cache_memoize
-		data = lookup_callable()
-		File "c:\python25\lib\site-packages\ReviewBoard-1.0.8-py2.5.egg\reviewboard\diffviewer\diffutils.py", line 622, in <lambda>
-		enable_syntax_highlighting),
-		File "c:\python25\lib\site-packages\ReviewBoard-1.0.8-py2.5.egg\reviewboard\diffviewer\diffutils.py", line 344, in get_chunks
-		old = get_original_file(filediff)
-		File "c:\python25\lib\site-packages\ReviewBoard-1.0.8-py2.5.egg\reviewboard\diffviewer\diffutils.py", line 251, in get_original_file
-		large_data=True)[0]
-		File "c:\python25\lib\site-packages\Djblets-0.6.1-py2.5.egg\djblets\util\misc.py", line 166, in cache_memoize
-		data = lookup_callable()
-		File "c:\python25\lib\site-packages\ReviewBoard-1.0.8-py2.5.egg\reviewboard\diffviewer\diffutils.py", line 250, in <lambda>
-		data = cache_memoize(key, lambda: [fetch_file(file, revision)],
-		File "c:\python25\lib\site-packages\ReviewBoard-1.0.8-py2.5.egg\reviewboard\diffviewer\diffutils.py", line 229, in fetch_file
-		data = tool.get_file(file, revision)
-		File "C:\Python25\lib\site-packages\reviewboard-1.0.8-py2.5.egg\reviewboard\scmtools\perforce.py", line 101, in get_file
-		raise SCMError('\n'.join(line.lstrip("\t") for line in error))
-		SCMError: Password not allowed at this server security level, use 'p4 login'.

Diff:

Revision 3 (+9 -2)

Show changes

reviewboard/scmtools/perforce.py

SE

reviewboard/scmtools/perforce.py (Diff revision 3)

Must restore password after executing p4.run_login(), or will fail to post requests. Don't know the reason. it seems to be a bug of p4svn.

SE

Change Summary:

Hope to fix in 1.0.x branch ASAP.

Branch:

-	master
+	master, 1.0.x

AR

Using "p4 login" is what my patch (http://reviews.reviewboard.org/r/1445/) also does. It should now include all of the changes Christian requested some time ago.
What it does different is that it won't enforce "p4 login" but makes it optional depending on a configuration setting.

What I don't understand is why you call the login command with each connect. Running the login command should replace both user and password attributes of the p4 class with the ticket credentials so those will be used for further p4 calls. I don't see the necessity to redo that step with each connect.

SE

seagle0128 May 30, 2010, 1:35 a.m.

Yes, your changes look good and including my changes. My patch is just used for my team. Thanks!

You have a pending review.

Review Board 5.0.2 alpha 0 (dev)

patch for perforce. If changing the security level of perforce server, perhaps fail to login perforce when creating diffs.

Branch:

Description:

Diff:

Description:

Diff:

Change Summary:

Branch: