Summary

Fix computation of new-file state with commit ranges.

Review Request #13674 — Created March 28, 2024 and submitted April 10, 2024, 8:20 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-7.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

When selecting a range of commits where the first commit adds a file and
the later commit(s) change it, the is_new_file state was being
incorrectly computed as True. In the case where we have a base
filediff, we instead should be treating it as an existing file rather
than a new one.

Testing Done

Loaded a commit range that was previously triggering the bug (especially
noticeable with the way binary files were loaded). Saw that I was now
seeing it treated as an existing file rather than new.

Commits

Summary	ID
Fix computation of new-file state with commit ranges. When selecting a range of commits where the first commit adds a file and the later commit(s) change it, the `is_new_file` state was being incorrectly computed as `True`. In the case where we have a base filediff, we instead should be treating it as an existing file rather than a new one. Testing Done: Loaded a commit range that was previously triggering the bug (especially noticeable with the way binary files were loaded). Saw that I was now seeing it treated as an existing file rather than new.	2410a01f205ffbe9a31848d4b08968dcd24f9d05

Summary

Fix computation of new-file state with commit ranges.

When selecting a range of commits where the first commit adds a file and the later commit(s) change it, the `is_new_file` state was being incorrectly computed as `True`. In the case where we have a base filediff, we instead should be treating it as an existing file rather than a new one. Testing Done: Loaded a commit range that was previously triggering the bug (especially noticeable with the way binary files were loaded). Saw that I was now seeing it treated as an existing file rather than new.

2410a01f205ffbe9a31848d4b08968dcd24f9d05

Issues

Description	From	Last Updated
Given the nature of the fix, we should have comprehensive unit testing for the various states. I wish we had …	chipx86	April 3, 2024, 3:47 a.m.

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Given the nature of the fix, we should have comprehensive unit testing for the various states.

I wish we had a better idea of what the more complex logic was working to do.

david April 3, 2024, 12:34 a.m.

Looking at the history of the code, it looks like that was added during an intermediate version of the multi-commit implementation, where we were computing the cumulative diff ourselves based on all the individual commits (whereas now we have the client upload the cumulative diff separately).

Change Summary:

Add a unit test to cover the case of a new file that's not new once viewing a different commit range.

Commits:

	Summary	ID
	Fix computation of new-file state with commit ranges. When selecting a range of commits where the first commit adds a file and the later commit(s) change it, the `is_new_file` state was being incorrectly computed as `True`. In the case where we have a base filediff, we instead should be treating it as an existing file rather than a new one. Testing Done: Loaded a commit range that was previously triggering the bug (especially noticeable with the way binary files were loaded). Saw that I was now seeing it treated as an existing file rather than new.	6801fb07f535551842055dbcefa164f49c80f47b
	Fix computation of new-file state with commit ranges. When selecting a range of commits where the first commit adds a file and the later commit(s) change it, the `is_new_file` state was being incorrectly computed as `True`. In the case where we have a base filediff, we instead should be treating it as an existing file rather than a new one. Testing Done: Loaded a commit range that was previously triggering the bug (especially noticeable with the way binary files were loaded). Saw that I was now seeing it treated as an existing file rather than new.	2410a01f205ffbe9a31848d4b08968dcd24f9d05

Diff:

Revision 2 (+72 -8)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/tests/test_diffutils.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-7.x (442e57f)