Comment 1 by William Martin, Mar 11, 2011
Thanks for the report. Can you attach the patch file, please. See http://indefero.stedmeister.co.uk/index.php/p/test-repos/review/
Status:
Accepted
Comment 2 by William Martin, Mar 11, 2011
In first time, i think this issue is linked to the diff Parser but not ;) Fixed in commit e6756a207296d5df20ad3e569f3e107abb0de059
Labels:
Milestone:Release1.1
Status: Fixed
Owner: wysman
Status: Fixed
Owner: wysman
Comment 3 by Steddy Smit, Mar 11, 2011
Hi, that was quick :) From looking at the commit, I think that you may have fixed something else that I noticed after raising this issue. When I tried to access a review http://indefero.stedmeister.co.uk/index.php/p/test-repos/review/4 when I was not signed into my indefero server, I received the error about show_full not being set. This error did not appear when I was signed in. I don't think that this will have fixed the original issue. I have patched my server with your fix in the commit above, so that you can see the original error at http://indefero.stedmeister.co.uk/index.php/p/test-repos/review/4/ Thanks Steddy
Comment 4 by William Martin, Mar 11, 2011
Hi, can you attach your patch file please. @patrickg: Something is wrong in IDF_Diff. Thanks
Status:
Accepted
Comment 5 by Steddy Smit, Mar 11, 2011
Patch file attached:
- 2aa16dac407a1bec4a3d1c334a1794103ec53b67.diff - 151 bytes - view
Comment 6 by Thomas Keller, Mar 17, 2011
The new diff parser shows the error as well. I added a test case for this problem in revision 51dde57. The output is: 1) IDF_DiffTest::testParse Undefined offset: 7 /Users/tommyd/Entwicklung/indefero/src/IDF/Diff.php:102 /Users/tommyd/Entwicklung/indefero/test/IDF/DiffTest.php:25 If the parse() command is silenced with @, the test succeeds, so we should probably just avoid accessing the above mentioned offset.
Status:
Started
Owner: pgeorgi
Owner: pgeorgi
Comment 7 by Thomas Keller, Mar 17, 2011
The test is very similar to test-06-mtn-single-line.diff, only that the git test-15-git-change-single-line.diff (which is the above file) does not contain a trailing new line. Usually, a '\No newline at the end of the file' stanza should be printed by the diff generator, I wonder why git is missing this...
Comment 8 by Patrick Georgi, Mar 17, 2011
I added bug-for-bug compatibility with the old diff parser, but adding the offset back (which is more sensible imho) is easy, full-chunk-header.diff should do.
- full-chunk-header.diff - 1.11 kB - view
Comment 9 by Thomas Keller, Mar 17, 2011
Unfortunately adding back the offset doesn't fix the error above. I switched all the expected files to include the offset also for single line changes and attached the complete patch.
- issue-633.diff - 6.32 kB - view
Comment 10 by Thomas Keller, Mar 18, 2011
I just pushed commit a3dd1c4 which resolves the above issues, minus what I noted in issue 636 - I think we shouldn't do these fixes there in 1.1 given the short timeframe and rather handle this as a "known issue" for 1.1 (and 1.0 as well). The problem was that the patch supplied by Steddy Smit was incomplete, because it lacked the final newline or the marker "\No newline at the end of file". The inner loop that iterates the individual lines for each hunk therefor gained an additional check to ensure that $i is smaller than $diffsize. While dealing with newline handling, I figured that the code would still horribly break in case "\No newline at the end of file" entries would not be handled somehow, so I added a case for lines beginning with "\", skip them and note that we deal to fix this properly in the next release (tracking issue 636).
Status:
Fixed
Comment 11 by Thomas Keller, Mar 18, 2011
Don't mind commit a3dd1c4, commit 5a7bf49 is the interesting one. And don't trust the changed hunks for the *.expected files in the commit details, they miss the line adds because of the newline problem.
Sign in to reply to this comment.
Reported by Steddy Smit, Mar 11, 2011
After creating a code review, I was unable to view it and received the following error: PlufErrorHandlerException making GET request to /index.php/p/test-repos/review/4/ 8 : Undefined offset: 1 It looks as though on line 234 of IDF/Diff.php, $chunk is being dereferenced incorrectly: It is trying to deference $chunk[0][1] which doesn't appear to exist in the arg list. And changing the code to read $chunk[1][0] seemed to allow me to look at the code review. See below for the extracted args and src list. I am running indefero-1.0 from the downloads link on this website. Args: [chunks_def] => Array ( [0] => Array ( [0] => Array ( [0] => 1 ) [1] => Array ( [0] => 1 ) ) ) Src: 229. $min_line = 0; 230. $max_line = 0; 231. //if (count($chunks['chunks_def']) == 0) return ''; 232. foreach ($chunks['chunks_def'] as $chunk) { 233. $start = ($chunk[0][0] > $context) ? $chunk[0][0]-$context : 0; 234. $end = (($chunk[0][0]+$chunk[0][1]+$context-1) < count($orig_lines)) ? $chunk[0][0]+$chunk[0][1]+$context-1 : count($orig_lines); 235. $spans[] = array($start, $end); 236. } 237. // merge chunks/get the chunk lines 238. // these are reference lines 239. $chunk_lines = array();