• 
      

    post-review uses --no-prefix option to git which diff disagrees with git.py GitDiffParser::__parse_diff()

    Review Request #740 — Created Feb. 10, 2009 and submitted

    Information

    Review Board SVN (deprecated)

    Reviewers

    djs
    The post-review script uses the --no-prefix option to git diff.  
    This option specifically omits the artificial a/ and b/ prefixes of the pathnames of the files being diffed.
    git.py GitDiffParser::__parse_diff() is hard-coded to remove the first 2 chars from the path names, assuming that the a/ and b/ are present.
    This disagreement results in reviewboard dropping the first two characters of the filenames produced by post-review when using git as the scm.
    
    I chose to change post-review rather than git.py since the test in test.py::GitTests specifically test for the _presence_ of the a/ and b/ prefixes.  That makes it fairly clear that post-review is in error.
    Posted a review to my own reviewboard (1.0 alpha 2) server using git as the scm and my modified post-review.  Note that the filenames in the diff now include the first two characters which had been dropped using the stock version of post-review.
    
    Note that this patch changes the behaviour for git-svn as well as native git.  I only tested native git.
    UG
    DJ
    1. This change for git proper looks good to me. I didn't notice the paths being cut off originally...
      
      But I suspect the change should not be made for git-svn.
    2. I think this should keep the --no-prefix since git-svn uses the svn diff parser. However, I don't use git-svn so I can't say for sure.
    3. 
        
    UG
    Review request changed
    Change Summary:
    I put back the "--no-prefix" option in the git-svn case.  I confirmed that Dan was right about this being required for git-svn.  I set up an SVN repo and cloned from it using git-svn.  With my r1 patch, I got extra 'a/' and 'b/' prefixes on the filenames.  With my r2 patch, everything works fine.  So, only the native git case should have the "--no-prefix" option.  New patch is attached.
    
    People:
    DJ
    1. Looks good to me.
    2. 
        
    chipx86
    1. Looks good to me too. Committed as r1813.
    2.