Correct minor issues with history scheduled with commit processing for SVN.

Review Request #7653 — Created Sept. 20, 2015 and submitted

Information

RBTools
master

Reviewers

For Subversion there is special processing to determine when a changeset has history scheduled with commit, which in turn requires the --svn-show-copies-as-adds option to be specified. There are two issues with this processing:

  1. A logic fault was introduced in (e20a1ba) related to Subversion changelists. When svn diff is executed with the --changelist option then only files that are members of that changelist are included in the diff. But, the inverse is not true. That is, if a working copy contains modified files that are not members of any changelist as well as other modified files that are members of a changelist, then svn diff (with no extra options) will produce a diff for all modified files, regardless of changelist membership. In (e20a1ba) code was added for the case of not working with a changelist, to return early from parsing svn status output when the changelist "section" of the status output was encountered. This early exist should not occur as even files in changelists must be considered for having history scheduled with commit.
  2. history_scheduled_with_commit() parses the output of svn status to find if any file status has '+' in 4th column. svn status is only pertinent to working copies, however there is no code in place to prevent executing history_scheduled_with_commit() when generating a diff between specific number revisions. This is problematic when executing rbt within a working copy with history scheduled with commit, but attempting to diff/post not against the working copy but instead between two revisions (e.g. rbt post 10:12). In this case the diff generation will terminate indicating that --svn-show-copies-as-adds must be specified, which is both confusing and wrong because it depends on the state of the current working copy and has nothing to do with the specified revisions.

New test cases were added to exercise these issues, and they failed as expected before the fixes were introduced. test_history_scheduled_with_commit_special_case_changelist() was dropped altogether as it simply verified the faulty logic in item 1. Instead an additional test was added to test_history_scheduled_with_commit_nominal() to ensure that files in changelists are not excluded from consideration when evaluating history scheduled with commit. Item 2 is exercised with the new test_history_scheduled_with_commit_special_case_non_local_mods().

Finally, test_history_scheduled_with_commit_special_case_exclude() was extended to consider the joint case of an excluded file in a changelist, and check_show_copies_as_adds() was extended to test against future logic faults related to changelists.

Ran all unit tests under both SVN 1.6.23 and 1.7.14.
Description From Last Updated

Only one space between sentences, please.

daviddavid

Can you surround the whole conditional in parens instead of using ?

daviddavid

Can we format this as a list with one item per line?

daviddavid

One item per line here, too.

daviddavid

Only one space between sentences.

daviddavid
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/clients/svn.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/clients/svn.py
    
    
  2. 
      
david
  1. The code looks great but I have a handful of trivial formatting requests.

  2. rbtools/clients/svn.py (Diff revision 1)
     
     
     
    Show all issues

    Only one space between sentences, please.

  3. rbtools/clients/svn.py (Diff revision 1)
     
     
     
     
    Show all issues

    Can you surround the whole conditional in parens instead of using ?

  4. rbtools/clients/tests.py (Diff revision 1)
     
     
     
     
    Show all issues

    Can we format this as a list with one item per line?

  5. rbtools/clients/tests.py (Diff revision 1)
     
     
     
     
    Show all issues

    One item per line here, too.

  6. rbtools/clients/tests.py (Diff revision 1)
     
     
    Show all issues

    Only one space between sentences.

  7. 
      
gmyers
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/clients/tests.py
        rbtools/clients/svn.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/clients/tests.py
        rbtools/clients/svn.py
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
gmyers
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-0.7.x (9d1e64a)
Loading...