Summary

Fix diff fragment header expansion issues.

Review Request #6889 — Created Feb. 1, 2015 and submitted Feb. 9, 2015, 2:08 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

master

Bugs

Depends On

Commit

6a71060...

Reviewers

Groups

reviewboard

People

Description

Previously diff fragments would sometimes expand to the incorrect

place. This is beacuse the generated header information was using

original/patched line numbers instead of the virtual (unified original

and patched) line numbers. Header generation has been modified to use

these line numbers and the issue has been fixed.
Headers also used to sometimes select headers to expand to. The code

that searched for headers wasn't taking into account the difference

between the original/patched line numbers, which are what the headers

use, and the virtual line numbers. This would happen if a deletion or

insertion chunk was above the flagged comment lines.
Update the diff fragment template to allow the left header (which

corresponds to the original file) to be expanded to.
Remove dead code for generation headers from the

get_file_chunks_in_range function. The logic for header generation

wasn't quite right and the only caller of the function is using better

logic to find header information.
Add a unit test to test that the line numbers generated by the new

get_last_header_before_line function are both before the requested

line and have the correct unified line number.

Testing Done

Ran unit tests.

Manually tested the diff fragments with lines flagged that had deletes
and inserts above them. They expanded to the proper locations.

Tested expansion of the left-side of the fragment. It was expandable
and expanded to the correct position.

Issues

Description	From	Last Updated
One entry per line, like: header = { 'left': None, 'right': None, }	chipx86	Feb. 5, 2015, noon
We use lines[0][0] several times, so let's pull it out into a variable.	chipx86	Feb. 5, 2015, 11:59 a.m.
The second line's indentation should match the 'right_headers' on the previous line.	chipx86	Feb. 5, 2015, noon
Can you add a blank line here?	david	Feb. 9, 2015, 2:06 p.m.
Can you add a blank line here?	david	Feb. 9, 2015, 2:06 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/diffviewer/diffutils.py

Ignored Files:
    reviewboard/templates/reviews/diff_comment_fragment.html



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/diffviewer/diffutils.py

Ignored Files:
    reviewboard/templates/reviews/diff_comment_fragment.html

Change Summary:

Remove debug stuff

Commit:

-	fe9795f5db8de5cb729185b760c85f1879f01447
+	6b54a1b2758b1e9ccc6d8deb04566eef658a718d

Diff:

Revision 2 (+91 -48)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/templates/reviews/diff_comment_fragment.html

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/diffviewer/diffutils.py

Ignored Files:
    reviewboard/templates/reviews/diff_comment_fragment.html



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/diffviewer/diffutils.py

Ignored Files:
    reviewboard/templates/reviews/diff_comment_fragment.html

Looks good, pending your investigation of the bug in your testing done.

Change Summary:

Investigated and found that the behaviour is reproducible on master.

Testing Done:

		Ran unit tests.

		Manually tested the diff fragments with lines flagged that had deletes
		and inserts above them. They expanded to the proper locations.

		Tested expansion of the left-side of the fragment. It was expandable
		and expanded to the correct position.
-
-		TODO:
-		Sometimes an interesting header isn't shown as expandable to but I
-		haven't determined if it is due to my code or the interesting lines
-		code.

Can you craft a test case reproducing your original problem?

Does meta no longer have header information?

brennie Feb. 5, 2015, 12:07 p.m.

The chunk 'meta' field still has 'left_header' and 'right_header entries' (if they were generated).
The header information generation was removed from get_file_chunks_in_range because the information wasn't useful there. The header information was only being generated for chunks that were in the range specified. It also generated information for all those chunks, whereas we really only need the information for chunks we are going to expand into (i.e. the previous chunks). The function also did not take into account the difference between original/modified line numbers (which are what the 'meta' field uses) and unified line numbers (what the diffviewer and diff fragment viewer use).
This behaviour has been corrected and put into get_last_header_before_line.

brennie Feb. 5, 2015, 2 p.m.
```
A unit test has been added.
```

reviewboard/diffviewer/diffutils.py (Diff revision 2)

The issue has been resolved. Show all issues

One entry per line, like:

header = {
    'left': None,
    'right': None,
}

reviewboard/diffviewer/diffutils.py (Diff revision 2)
The issue has been resolved. Show all issues
```
We use lines[0][0] several times, so let's pull it out into a variable.
```

reviewboard/diffviewer/diffutils.py (Diff revision 2)

The issue has been resolved. Show all issues

The second line's indentation should match the 'right_headers' on the previous line.

Change Summary:

Address Christian's issues.

Description:

		Previously diff fragments would sometimes expand to the incorrect
		place. This is beacuse the generated header information was using
		original/patched line numbers instead of the virtual (unified original
		and patched) line numbers. Header generation has been modified to use
		these line numbers and the issue has been fixed.

		Headers also used to sometimes select headers to expand to. The code
		that searched for headers wasn't taking into account the difference
		between the original/patched line numbers, which are what the headers
		use, and the virtual line numbers. This would happen if a deletion or
		insertion chunk was above the flagged comment lines.

		Update the diff fragment template to allow the left header (which
		corresponds to the original file) to be expanded to.

		Remove dead code for generation headers from the
		`get_file_chunks_in_range` function. The logic for header generation
		wasn't quite right and the only caller of the function is using better
		logic to find header information.
	+
	+	Add a unit test to test that the line numbers generated by the new
	+	`get_last_header_before_line` function are both before the line and
	+	have the correct unified line number.

Commit:

-	6b54a1b2758b1e9ccc6d8deb04566eef658a718d
+	6a7106090c492874d57e5a8794e75583cd20f82c

Diff:

Revision 3 (+209 -48)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/tests.py
	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/templates/reviews/diff_comment_fragment.html

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py

Ignored Files:
    reviewboard/templates/reviews/diff_comment_fragment.html



Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/diffutils.py

Ignored Files:
    reviewboard/templates/reviews/diff_comment_fragment.html

Change Summary:

clarity

Description:

		Previously diff fragments would sometimes expand to the incorrect
		place. This is beacuse the generated header information was using
		original/patched line numbers instead of the virtual (unified original
		and patched) line numbers. Header generation has been modified to use
		these line numbers and the issue has been fixed.

		Headers also used to sometimes select headers to expand to. The code
		that searched for headers wasn't taking into account the difference
		between the original/patched line numbers, which are what the headers
		use, and the virtual line numbers. This would happen if a deletion or
		insertion chunk was above the flagged comment lines.

		Update the diff fragment template to allow the left header (which
		corresponds to the original file) to be expanded to.

		Remove dead code for generation headers from the
		`get_file_chunks_in_range` function. The logic for header generation
		wasn't quite right and the only caller of the function is using better
		logic to find header information.

		Add a unit test to test that the line numbers generated by the new
~		`get_last_header_before_line` function are both before the line and
~		have the correct unified line number.
	~	`get_last_header_before_line` function are both before the requested
	~	line and have the correct unified line number.

Ship it!

reviewboard/diffviewer/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can you add a blank line here?
```
reviewboard/diffviewer/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can you add a blank line here?
```

Status: Closed (submitted)

Change Summary:

Pushed to master (567f659)