Summary

Reimplement directory diffing for ClearCase and VersionVault.

Review Request #12581 — Created Sept. 6, 2022 and submitted Sept. 8, 2022, 2:46 p.m.

Information

Owner

david

Repository

RBTools

Branch

release-3.x

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

The community-driven ClearCase implementation included diffs of
directory elements (basically doing a diff of the ls output).
Directory diffs aren't a thing anywhere else in Review Board, but it's
apparently a requirement from our partners at HCL, so we're going to
reimplement this behavior.

This change adds back directory diffs, but does so in a hopefully
forward-looking way. We're including the correct "directory" type in the
diffx metadata to indicate that a file entry is actually for a
directory, and we're adding a new key to the VersionVault metadata
section to say that the directory diff is a legacy "filenames" diff.
This will allow us to implement directory diffs in a more useful/correct
way later down the line without having painted ourselves into a corner
relying on only this behavior.

I've added unit tests not only for the new directory diffs, but also for
basic file diff generation (in both legacy and diffx mode). While doing
this, I discovered a bug where creating diffs for the legacy mode with
dynamic views would end up replacing filenames twice, resulting in
filenames that looked like vob/vob/file, instead of vob/file. I've
changed it to explicitly replace the old and new filenames only on the
first and second lines of the diff.

Testing Done


Posted changes including directory diffs (along with a corresponding

  Power Pack change) and saw the correct output.
Ran unit tests.

Commits

Summary	ID
Reimplement directory diffing for ClearCase and VersionVault. The community-driven ClearCase implementation included diffs of directory elements (basically doing a diff of the `ls` output). Directory diffs aren't a thing anywhere else in Review Board, but it's apparently a requirement from our partners at HCL, so we're going to reimplement this behavior. This change adds back directory diffs, but does so in a hopefully forward-looking way. We're including the correct "directory" type in the diffx metadata to indicate that a file entry is actually for a directory, and we're adding a new key to the VersionVault metadata section to say that the directory diff is a legacy "filenames" diff. This will allow us to implement directory diffs in a more useful/correct way later down the line without having painted ourselves into a corner relying on only this behavior. I've added unit tests not only for the new directory diffs, but also for basic file diff generation (in both legacy and diffx mode). While doing this, I discovered a bug where creating diffs for the legacy mode with dynamic views would end up replacing filenames twice, resulting in filenames that looked like `vob/vob/file`, instead of `vob/file`. I've changed it to explicitly replace the old and new filenames only on the first and second lines of the diff. Testing Done: - Posted changes including directory diffs (along with a corresponding Power Pack change) and saw the correct output. - Ran unit tests.	cf78b149bee7461d73bd10845ae7cce83fd1d07b

Summary

Reimplement directory diffing for ClearCase and VersionVault.

The community-driven ClearCase implementation included diffs of directory elements (basically doing a diff of the `ls` output). Directory diffs aren't a thing anywhere else in Review Board, but it's apparently a requirement from our partners at HCL, so we're going to reimplement this behavior. This change adds back directory diffs, but does so in a hopefully forward-looking way. We're including the correct "directory" type in the diffx metadata to indicate that a file entry is actually for a directory, and we're adding a new key to the VersionVault metadata section to say that the directory diff is a legacy "filenames" diff. This will allow us to implement directory diffs in a more useful/correct way later down the line without having painted ourselves into a corner relying on only this behavior. I've added unit tests not only for the new directory diffs, but also for basic file diff generation (in both legacy and diffx mode). While doing this, I discovered a bug where creating diffs for the legacy mode with dynamic views would end up replacing filenames twice, resulting in filenames that looked like `vob/vob/file`, instead of `vob/file`. I've changed it to explicitly replace the old and new filenames only on the first and second lines of the diff. Testing Done: - Posted changes including directory diffs (along with a corresponding Power Pack change) and saw the correct output. - Ran unit tests.

cf78b149bee7461d73bd10845ae7cce83fd1d07b

Issues

Description	From	Last Updated
too many blank lines (2) Column: 9 Error code: E303	reviewbot	Sept. 6, 2022, 9:34 a.m.
line too long (80 > 79 characters) Column: 80 Error code: E501	reviewbot	Sept. 6, 2022, 9:34 a.m.
line too long (80 > 79 characters) Column: 80 Error code: E501	reviewbot	Sept. 6, 2022, 9:35 a.m.
line too long (85 > 79 characters) Column: 80 Error code: E501	reviewbot	Sept. 6, 2022, 9:35 a.m.
line too long (85 > 79 characters) Column: 80 Error code: E501	reviewbot	Sept. 6, 2022, 9:36 a.m.
Should be diffx_change.	chipx86	Sept. 7, 2022, 10:32 p.m.
Something that's come up a lot in my typing work is that the pattern of repurposing a variable to a …	chipx86	Sept. 7, 2022, 10:33 p.m.
Can we alphabetize the keys? Same above.	chipx86	Sept. 7, 2022, 10:33 p.m.
Same general issue as above. We're reusing dl for two different types. I'm not sure what we should be calling …	chipx86	Sept. 7, 2022, 10:34 p.m.
Too many blank lines.	chipx86	Sept. 7, 2022, 10:35 p.m.
Should return a list of bytes.	chipx86	Sept. 8, 2022, 2:45 p.m.
I've really liked the approach I'm using for the SOS unit tests, where we have helper functions that let us …	chipx86	Sept. 7, 2022, 10:36 p.m.
Should be "in diffx mode" instead.	maubin	Sept. 7, 2022, 10:36 p.m.

flake8 failed.

JSHint passed.

flake8

rbtools/clients/clearcase.py (Diff revision 1)
The issue has been resolved. Show all issues
```
too many blank lines (2)

Column: 9
Error code: E303
```
rbtools/clients/clearcase.py (Diff revision 1)
The issue has been resolved. Show all issues
```
line too long (80 > 79 characters)

Column: 80
Error code: E501
```
rbtools/clients/clearcase.py (Diff revision 1)
The issue has been resolved. Show all issues
```
line too long (80 > 79 characters)

Column: 80
Error code: E501
```
rbtools/clients/tests/test_clearcase.py (Diff revision 1)
The issue has been resolved. Show all issues
```
line too long (85 > 79 characters)

Column: 80
Error code: E501
```
rbtools/clients/tests/test_clearcase.py (Diff revision 1)
The issue has been resolved. Show all issues
```
line too long (85 > 79 characters)

Column: 80
Error code: E501
```

Commits:

	Summary	ID
	Reimplement directory diffing for ClearCase and VersionVault. The community-driven ClearCase implementation included diffs of directory elements (basically doing a diff of the `ls` output). Directory diffs aren't a thing anywhere else in Review Board, but it's apparently a requirement from our partners at HCL, so we're going to reimplement this behavior. This change adds back directory diffs, but does so in a hopefully forward-looking way. We're including the correct "directory" type in the diffx metadata to indicate that a file entry is actually for a directory, and we're adding a new key to the VersionVault metadata section to say that the directory diff is a legacy "filenames" diff. This will allow us to implement directory diffs in a more useful/correct way later down the line without having painted ourselves into a corner relying on only this behavior. I've added unit tests not only for the new directory diffs, but also for basic file diff generation (in both legacy and diffx mode). While doing this, I discovered a bug where creating diffs for the legacy mode with dynamic views would end up replacing filenames twice, resulting in filenames that looked like `vob/vob/file`, instead of `vob/file`. I've changed it to explicitly replace the old and new filenames only on the first and second lines of the diff. Testing Done: - Posted changes including directory diffs (along with a corresponding Power Pack change) and saw the correct output. - Ran unit tests.	14088237e73a4f391f2e43f0c39497b5bcabcb08
	Reimplement directory diffing for ClearCase and VersionVault. The community-driven ClearCase implementation included diffs of directory elements (basically doing a diff of the `ls` output). Directory diffs aren't a thing anywhere else in Review Board, but it's apparently a requirement from our partners at HCL, so we're going to reimplement this behavior. This change adds back directory diffs, but does so in a hopefully forward-looking way. We're including the correct "directory" type in the diffx metadata to indicate that a file entry is actually for a directory, and we're adding a new key to the VersionVault metadata section to say that the directory diff is a legacy "filenames" diff. This will allow us to implement directory diffs in a more useful/correct way later down the line without having painted ourselves into a corner relying on only this behavior. I've added unit tests not only for the new directory diffs, but also for basic file diff generation (in both legacy and diffx mode). While doing this, I discovered a bug where creating diffs for the legacy mode with dynamic views would end up replacing filenames twice, resulting in filenames that looked like `vob/vob/file`, instead of `vob/file`. I've changed it to explicitly replace the old and new filenames only on the first and second lines of the diff. Testing Done: - Posted changes including directory diffs (along with a corresponding Power Pack change) and saw the correct output. - Ran unit tests.	72d75e566d2c8bd7df0e39454f2eaf05c60daa06

Diff:

Revision 2 (+1910 -56)

Show changes

	rbtools/clients/clearcase.py
	rbtools/clients/tests/test_clearcase.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/clients/clearcase.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Should be diffx_change.
```

rbtools/clients/clearcase.py (Diff revision 2)

The issue has been resolved. Show all issues

Something that's come up a lot in my typing work is that the pattern of repurposing a variable to a new type will no longer be doable.

In this case, the first revision can be removed, with the entry.new_version put inline in revision = { ... }.

If we need to use a variable elsewhere, we'll need to ensure it has its own name that doesn't get reused under a new type.

rbtools/clients/clearcase.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can we alphabetize the keys?
Same above.
```

rbtools/clients/clearcase.py (Diff revision 2)

The issue has been resolved. Show all issues

Same general issue as above. We're reusing dl for two different types.

I'm not sure what we should be calling this, but maybe just diff or orig_diff?

rbtools/clients/clearcase.py (Diff revision 2)

The issue has been resolved. Show all issues

Too many blank lines.

rbtools/clients/tests/test_clearcase.py (Diff revision 2)

The issue has been dropped. Show all issues

I've really liked the approach I'm using for the SOS unit tests, where we have helper functions that let us output these rules. That keeps the tests readable, and slims down the code. Also has the benefit of letting us do more extensive matching of arguments (kwargs) and being easier to update for, say, execute -> run_process.
I'll leave it up to you as to whether to do that for this change. If not, every one of these will have to be redone for the run_process migration anyway.

david Sept. 7, 2022, 10:36 p.m.

Given our time constraints, I'd rather leave this as-is and update it when we switch to run_process

chipx86 Sept. 7, 2022, 10:37 p.m.
```
I'm fine with that.
```

rbtools/clients/tests/test_clearcase.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Should be "in diffx mode" instead.
```

Commits:

	Summary	ID
	Reimplement directory diffing for ClearCase and VersionVault. The community-driven ClearCase implementation included diffs of directory elements (basically doing a diff of the `ls` output). Directory diffs aren't a thing anywhere else in Review Board, but it's apparently a requirement from our partners at HCL, so we're going to reimplement this behavior. This change adds back directory diffs, but does so in a hopefully forward-looking way. We're including the correct "directory" type in the diffx metadata to indicate that a file entry is actually for a directory, and we're adding a new key to the VersionVault metadata section to say that the directory diff is a legacy "filenames" diff. This will allow us to implement directory diffs in a more useful/correct way later down the line without having painted ourselves into a corner relying on only this behavior. I've added unit tests not only for the new directory diffs, but also for basic file diff generation (in both legacy and diffx mode). While doing this, I discovered a bug where creating diffs for the legacy mode with dynamic views would end up replacing filenames twice, resulting in filenames that looked like `vob/vob/file`, instead of `vob/file`. I've changed it to explicitly replace the old and new filenames only on the first and second lines of the diff. Testing Done: - Posted changes including directory diffs (along with a corresponding Power Pack change) and saw the correct output. - Ran unit tests.	72d75e566d2c8bd7df0e39454f2eaf05c60daa06
	Reimplement directory diffing for ClearCase and VersionVault. The community-driven ClearCase implementation included diffs of directory elements (basically doing a diff of the `ls` output). Directory diffs aren't a thing anywhere else in Review Board, but it's apparently a requirement from our partners at HCL, so we're going to reimplement this behavior. This change adds back directory diffs, but does so in a hopefully forward-looking way. We're including the correct "directory" type in the diffx metadata to indicate that a file entry is actually for a directory, and we're adding a new key to the VersionVault metadata section to say that the directory diff is a legacy "filenames" diff. This will allow us to implement directory diffs in a more useful/correct way later down the line without having painted ourselves into a corner relying on only this behavior. I've added unit tests not only for the new directory diffs, but also for basic file diff generation (in both legacy and diffx mode). While doing this, I discovered a bug where creating diffs for the legacy mode with dynamic views would end up replacing filenames twice, resulting in filenames that looked like `vob/vob/file`, instead of `vob/file`. I've changed it to explicitly replace the old and new filenames only on the first and second lines of the diff. Testing Done: - Posted changes including directory diffs (along with a corresponding Power Pack change) and saw the correct output. - Ran unit tests.	cf78b149bee7461d73bd10845ae7cce83fd1d07b

Diff:

Revision 3 (+1916 -66)

Show changes

	rbtools/clients/clearcase.py
	rbtools/clients/tests/test_clearcase.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

rbtools/clients/clearcase.py (Diff revisions 2 - 3)
The issue has been resolved. Show all issues
```
Should return a list of bytes.
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-3.x (ded9ffe)