Hi David,
You are right. parse_diff_header is parsing the header incorrectly. I can
state from a perforce diff point of view that it is incorrect.
Reviewboard server code needs to be fixed
Scenario:
reviewboard/diffviewer/parser.py +169
def parse_diff_header
if linenum + 1 < len(self.lines) and \
((self.lines[linenum].startswith(b'--- ') and
self.lines[linenum + 1].startswith(b'+++ ')) or
(self.lines[linenum].startswith(b'*** ') and
self.lines[linenum + 1].startswith(b'--- ') and
not self.lines[linenum].endswith(b" ****"))):
# This is a unified or context diff header. Parse the
# file and extra info.
try:
info['origFile'], info['origInfo'] = \
self.parse_filename_header(self.lines[linenum][4:],
linenum)
Both the condition is getting satisfied here for the same file
One of the file has content as
*--- //prod/re_test/*/f1 //prod/re_test/*/f1#0+++ //prod/re_test/*/f1
2016-03-17 16:02:19*** make_com.c.orig Fri Jul 4 04:18:35 1987---
make_com.c Wed May 27 08:47:42 1992****************
Therefore both conditions are matching. The problem is the diff parser
treats make_commands.c.orig also as a file and generates dict value for
info['origFile] = make_commands.c.orig
info['origInfo'] = make_commands.c.orig
This further breaks the system when *function parse_diff_revision* in
*reviewboard/scmtools/perforce.py* tries to get revision of file through
code
*filename, revision = revision_str.rsplit('#', 1)*
revision_str value is "make_commands.c.orig" which doesn't have a "#" in it
and the code breaks.
So probably the parse_diff_header should also be implemented by the
specific repository.tool so that it's handled properly.
Thanks,
Subodh
On Wednesday, March 23, 2016 at 12:35:36 AM UTC+5:30, David Trowbridge
wrote:
>
> I'm not super aware of your situation, but following a "process" just
> because even though it's ridiculous in extreme cases like this is absurd.
>
> There's no good reason why --diff-filename doesn't work with Perforce
> other than that it just hasn't been used. Passing in a diff file with
> perforce is very rare because it would first require creating the diff with
> "rbt diff" (since p4 diff creates broken diff files), and if you're doing
> that, you might as well use "rbt post".
>
> Perhaps you can use "rbt post -X ..." or "rbt post -I ..." to pare down
> the size of the diff rather than hand-editing it?
>
> On Tue, Mar 22, 2016 at 11:48 AM Subodh Konhor <[email protected]
> <javascript:>> wrote:
>
>> I also do acknowledge that it is humanly impossible to review but we have
>> a process of having review done for certain products before checkin is
>> made. Therefore we post review. So yes we do post huge diffs and cannot do
>> away with it.
>>
>> Command I ran to post the changenum is
>> rbt post -d <changenum>
>>
>> Error in reviewboard log is
>>
>> 2016-03-21 09:47:58,313 - DEBUG - - Logging to
>> /var/www/rb/reviewboard/logs/reviewboard.log with a minimum level of DEBUG
>> 2016-03-21 09:52:07,315 - DEBUG - - DiffParser.parse: Beginning parse of
>> diff, size = 359762870
>> 2016-03-21 09:52:59,314 - INFO - - Reloading logging settings
>> 2016-03-21 09:52:59,314 - DEBUG - - Logging to
>> /var/www/rb/reviewboard/logs/reviewboard.log with a minimum level of DEBUG
>> 2016-03-21 09:58:00,333 - INFO - - Reloading logging settings
>> 2016-03-21 09:58:00,333 - DEBUG - - Logging to
>> /var/www/rb/reviewboard/logs/reviewboard.log with a minimum level of DEBUG
>> 2016-03-21 10:00:21,018 - DEBUG - - DiffParser.parse: Finished parsing
>> diff.
>>
>> 2016-03-21 10:00:55,337 - ERROR - - Unexpected error when validating
>> diff.
>> Traceback (most recent call last):
>> File
>> "/usr/local/lib/python2.7/site-packages/reviewboard/webapi/resources/validate_diff.py",
>>
>> line 156, in create
>> save=False)
>> File
>> "/usr/local/lib/python2.7/site-packages/reviewboard/diffviewer/managers.py",
>> line 417, in create_from_upload
>> save=save)
>> File
>> "/usr/local/lib/python2.7/site-packages/reviewboard/diffviewer/managers.py",
>> line 441, in create_from_data
>> check_existence=(not parent_diff_file_contents)))
>> File
>> "/usr/local/lib/python2.7/site-packages/reviewboard/diffviewer/managers.py",
>> line 562, in _process_files
>> copied=f.copied)
>> File
>> "/usr/local/lib/python2.7/site-packages/reviewboard/scmtools/perforce.py",
>> line 379, in parse_diff_revision
>> filename, revision = revision_str.rsplit('#', 1)
>> ValueError: need more than 1 value to unpack
>>
>>
>> Why doesn't rbt post --diff-filename work?
>>
>> Thanks,
>> Subodh
>>
>>
>> On Tuesday, March 22, 2016 at 9:47:06 PM UTC+5:30, David Trowbridge wrote:
>>
>>> I'm not sure what's going on in the first couple tracebacks--somehow the
>>> diff headers aren't right. It would help if you included what command you
>>> ran to get that output. For the last traceback, it looks like there's some
>>> issues with perforce and 'rbt post --diff-filename'
>>>
>>> That said, a change with 1000 or more files is going to be totally
>>> impossible for humans to review. What's your goal in putting it on Review
>>> Board?
>>>
>>> -David
>>>
>>> On Tue, Mar 22, 2016 at 4:28 AM Subodh Konhor <[email protected]> wrote:
>>>
>> Please donot go by the diff size, I am doing some testing as we do post
>>>> huge integration's to reviewboard.
>>>>
>>>> Since we are migrating from 1.0.9 to 2.5.x I am doing some round of
>>>> stress testing. I have generated merge diff which is all of type "branch"
>>>> category.
>>>> The diff size is whooping 359762870
>>>>
>>>> rbt error I get is
>>>> >>> Making HTTP POST request to
>>>> http://reviewboard-dev.datadomain.com/review/api/validation/diffs/
>>>> >>> Got API Error 224 (HTTP code 400): The specified diff file could
>>>> not be parsed.
>>>> >>> Error data: {u'stat': u'fail', u'reason': u'need more than 1 value
>>>> to unpack', u'err': {u'msg': u'The specified diff file could not be
>>>> parsed.', u'code': 224}}
>>>> Traceback (most recent call last):
>>>> File "/usr/local/bin/rbt", line 9, in <module>
>>>> load_entry_point('RBTools==0.7.5', 'console_scripts', 'rbt')()
>>>> File
>>>> "/usr/local/lib/python2.7/dist-packages/RBTools-0.7.5-py2.7.egg/rbtools/commands/main.py",
>>>>
>>>> line 133, in main
>>>> command.run_from_argv([RB_MAIN, command_name] + args)
>>>> File
>>>> "/usr/local/lib/python2.7/dist-packages/RBTools-0.7.5-py2.7.egg/rbtools/commands/__init__.py",
>>>>
>>>> line 622, in run_from_argv
>>>> exit_code = self.main(*args) or 0
>>>> File
>>>> "/usr/local/lib/python2.7/dist-packages/RBTools-0.7.5-py2.7.egg/rbtools/commands/post.py",
>>>>
>>>> line 857, in main
>>>> (msg_prefix, e))
>>>> rbtools.commands.CommandError: Error validating diff
>>>>
>>>> The specified diff file could not be parsed. (HTTP 400, API Error 224)
>>>>
>>>>
>>>>
>>>> on the reviewboard log I get the error as
>>>> DiffParser.parse: Beginning parse of diff, size = 359762870
>>>> 2016-03-21 10:00:55,337 - ERROR - - Unexpected error when validating
>>>> diff.
>>>> Traceback (most recent call last):
>>>> File
>>>> "/usr/local/lib/python2.7/site-packages/reviewboard/webapi/resources/validate_diff.py",
>>>>
>>>> line 156, in create
>>>> save=False)
>>>> File
>>>> "/usr/local/lib/python2.7/site-packages/reviewboard/diffviewer/managers.py",
>>>>
>>>> line 417, in create_from_upload
>>>> save=save)
>>>> File
>>>> "/usr/local/lib/python2.7/site-packages/reviewboard/diffviewer/managers.py",
>>>>
>>>> line 441, in create_from_data
>>>> check_existence=(not parent_diff_file_contents)))
>>>> File
>>>> "/usr/local/lib/python2.7/site-packages/reviewboard/diffviewer/managers.py",
>>>>
>>>> line 562, in _process_files
>>>> copied=f.copied)
>>>> File
>>>> "/usr/local/lib/python2.7/site-packages/reviewboard/scmtools/perforce.py",
>>>> line 379, in parse_diff_revision
>>>> filename, revision = revision_str.rsplit('#', 1)
>>>> ValueError: need more than 1 value to unpack
>>>>
>>>> For sure the error is not due to unpacking behavior as I broke my
>>>> changelist to a 1000 file changelist and posted which went through
>>>> smoothly. All the files in this change list are of type branch.
>>>>
>>>> So what can be the problem here?
>>>> 1) Is there a size limitation of rbt post diff that reviewboard is not
>>>> able to handle as I can see in reviewboard log the diff is transmitted.
>>>> 2) Should I look at apache file upload limits
>>>> 3) Should I look into mysql limits.
>>>>
>>>> Since generating diff on my system takes around 6hrs, I thought of
>>>> generating a diff file using the below command so that I can save time on
>>>> generating diff in each iteration
>>>> rbt diff 1234 > rb1234.diff
>>>>
>>>> Now if I try to post it as below
>>>> rbt post --diff-filename=rb1234.diff
>>>>
>>>> I get below error
>>>> >>> RBTools 0.7.5
>>>> >>> Python 2.7.3 (default, Feb 27 2014, 19:58:35)
>>>> [GCC 4.6.3]
>>>> >>> Running on Linux-3.5.0-39-generic-x86_64-with-Ubuntu-12.04-precise
>>>> >>> Home = /auto/home/konhos
>>>> >>> Current directory = /home/konhos/p4ws/test_re_reviewboard
>>>> >>> Checking for a Perforce repository...
>>>> >>> Running: p4 info
>>>> >>> Running: diff --version
>>>> >>> repository info: Path: p4broker.datadomain.com:1666, Base path:
>>>> None, Supports changesets: True
>>>> >>> Making HTTP GET request to
>>>> http://reviewboard-dev.datadomain.com/review/api/
>>>> >>> Making HTTP GET request to
>>>> http://reviewboard-dev.datadomain.com/review/api/validation/diffs/
>>>> >>> Cached response for HTTP GET
>>>> http://reviewboard-dev.datadomain.com/review/api/validation/diffs/
>>>> expired and was modified
>>>> >>> Making HTTP POST request to
>>>> http://reviewboard-dev.datadomain.com/review/api/validation/diffs/
>>>> Traceback (most recent call last):
>>>> File "/usr/local/bin/rbt", line 9, in <module>
>>>> load_entry_point('RBTools==0.7.5', 'console_scripts', 'rbt')()
>>>> File
>>>> "/usr/local/lib/python2.7/dist-packages/RBTools-0.7.5-py2.7.egg/rbtools/commands/main.py",
>>>>
>>>> line 133, in main
>>>> command.run_from_argv([RB_MAIN, command_name] + args)
>>>> File
>>>> "/usr/local/lib/python2.7/dist-packages/RBTools-0.7.5-py2.7.egg/rbtools/commands/__init__.py",
>>>>
>>>> line 622, in run_from_argv
>>>> exit_code = self.main(*args) or 0
>>>> File
>>>> "/usr/local/lib/python2.7/dist-packages/RBTools-0.7.5-py2.7.egg/rbtools/commands/post.py",
>>>>
>>>> line 902, in main
>>>> p4_reviewers =
>>>> self.tool.get_commit_message(self.revisions)['p4_reviewers']
>>>> File
>>>> "/usr/local/lib/python2.7/dist-packages/RBTools-0.7.5-py2.7.egg/rbtools/clients/__init__.py",
>>>>
>>>> line 267, in get_commit_message
>>>> commit_message = self.get_raw_commit_message(revisions)
>>>> File
>>>> "/usr/local/lib/python2.7/dist-packages/RBTools-0.7.5-py2.7.egg/rbtools/clients/perforce.py",
>>>>
>>>> line 1322, in get_raw_commit_message
>>>> changelist = revisions['tip']
>>>> TypeError: 'NoneType' object has no attribute '__getitem__'
>>>>
>>>>
>>>> Is this the expected behavior?
>>>>
>>>> Need urgent help as to what could be the problem. Also if somebody can
>>>> give me the exact command for posting diff file then that would be of
>>>> great
>>>> help.
>>>>
>>>> RB on OS: Centos 5
>>>> repository type: Perforce
>>>>
>>>> --
>>>> Supercharge your Review Board with Power Pack:
>>>> https://www.reviewboard.org/powerpack/
>>>> Want us to host Review Board for you? Check out RBCommons:
>>>> https://rbcommons.com/
>>>> Happy user? Let us know! https://www.reviewboard.org/users/
>>>> ---
>>>> You received this message because you are subscribed to the Google
>>>> Groups "reviewboard" group.
>>>>
>>> To unsubscribe from this group and stop receiving emails from it, send
>>>> an email to [email protected].
>>>
>>>
>>>> For more options, visit https://groups.google.com/d/optout.
>>>>
>>> --
>>> -David
>>>
>> --
>> Supercharge your Review Board with Power Pack:
>> https://www.reviewboard.org/powerpack/
>> Want us to host Review Board for you? Check out RBCommons:
>> https://rbcommons.com/
>> Happy user? Let us know! https://www.reviewboard.org/users/
>> ---
>> You received this message because you are subscribed to the Google Groups
>> "reviewboard" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to [email protected] <javascript:>.
>> For more options, visit https://groups.google.com/d/optout.
>>
> --
> -David
>
--
Supercharge your Review Board with Power Pack:
https://www.reviewboard.org/powerpack/
Want us to host Review Board for you? Check out RBCommons:
https://rbcommons.com/
Happy user? Let us know! https://www.reviewboard.org/users/
---
You received this message because you are subscribed to the Google Groups
"reviewboard" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
