Reimplement directory diffing for ClearCase and VersionVault.

Review Request #12581 — Created Sept. 6, 2022 and submitted

Information

RBTools
release-3.x

Reviewers

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.

  • Posted changes including directory diffs (along with a corresponding
    Power Pack change) and saw the correct output.
  • Ran unit tests.
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
Description From Last Updated

too many blank lines (2) Column: 9 Error code: E303

reviewbotreviewbot

line too long (80 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (80 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (85 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (85 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

Should be diffx_change.

chipx86chipx86

Something that's come up a lot in my typing work is that the pattern of repurposing a variable to a …

chipx86chipx86

Can we alphabetize the keys? Same above.

chipx86chipx86

Same general issue as above. We're reusing dl for two different types. I'm not sure what we should be calling …

chipx86chipx86

Too many blank lines.

chipx86chipx86

Should return a list of bytes.

chipx86chipx86

I've really liked the approach I'm using for the SOS unit tests, where we have helper functions that let us …

chipx86chipx86

Should be "in diffx mode" instead.

maubinmaubin
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. 
      
  2. rbtools/clients/clearcase.py (Diff revision 2)
     
     
    Show all issues

    Should be diffx_change.

  3. rbtools/clients/clearcase.py (Diff revision 2)
     
     
     
     
     
     
     
    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.

  4. rbtools/clients/clearcase.py (Diff revision 2)
     
     
     
     
     
     
    Show all issues

    Can we alphabetize the keys?

    Same above.

  5. rbtools/clients/clearcase.py (Diff revision 2)
     
     
     
     
    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?

  6. rbtools/clients/clearcase.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
    Show all issues

    Too many blank lines.

  7. rbtools/clients/tests/test_clearcase.py (Diff revision 2)
     
     
     
     
     
    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.

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

    2. I'm fine with that.

  8. 
      
maubin
  1. 
      
  2. rbtools/clients/tests/test_clearcase.py (Diff revision 2)
     
     
     
    Show all issues

    Should be "in diffx mode" instead.

  3. 
      
david
chipx86
  1. 
      
  2. rbtools/clients/clearcase.py (Diff revisions 2 - 3)
     
     
    Show all issues

    Should return a list of bytes.

  3. 
      
maubin
  1. Ship It!
  2. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-3.x (ded9ffe)