On Thu, 2010-06-17, Johan Corveleyn wrote: > Please find in attachment my (very first) patch addressing the issue > discussed in [1] and [2]. > > Log message (maybe a bit too verbose ...):
This amount of detail is fine. > [[[ > Fix alignment of blame output containing revision numbers >= 1000000 Rather than saying "fix", let's state what the behaviour change is. Make "svn blame" use a consistent column width even when revision numbers are >= 1000000. > * subversion/include/svn_client.h > (svn_client_blame_receiver3_t): Add parameters start_revnum and end_revnum, > useful to the blame receiver in formatting its output. > * subversion/libsvn_client/blame.c > (svn_client_blame5): Pass start_revnum and end_revnum to the blame receiver. > * subversion/svn/blame-cmd.c > (blame_receiver_xml): Implement the updated svn_client_blame_receiver3_t. > (blame_receiver): Implement the updated svn_client_blame_receiver3_t, and > pass end_revnum to print_line_info. > (print_line_info): Add parameter end_revnum, use it to determine the column > width for the revision number. > * subversion/libsvn_client/deprecated.c > (blame_wrapper_receiver2): Implement the updated > svn_client_blame_receiver3_t. > ]]] > > There are some caveats: > 1) This patch changes the svn_client_blame_receiver3_t interface > (already new in 1.7). I think this breaks the bindings. > > 2) This patch causes the following test failures: > FAIL: basic_tests.py 42: basic relative url target using current dir > FAIL: basic_tests.py 43: basic relative url target using other targets > FAIL: blame_tests.py 6: blame targets with peg-revisions > FAIL: blame_tests.py 8: ignore whitespace when blaming > FAIL: blame_tests.py 9: ignore eol styles when blaming > FAIL: blame_tests.py 12: blame target not in HEAD with peg-revisions > FAIL: blame_tests.py 14: blame -g output with inserted lines > > That's because the revision number column in the blame output now only > has the minimum width necessary (previously this was always fixed to a > width of 6). So for low revision numbers this now comes out as: > > [[[ > 1 jrandom This is the file 'iota'. > ]]] > > instead of (previous behavior): > > [[[ > 1 jrandom This is the file 'iota'. > ]]] > > I see two ways to fix this: > - Change the tests > - Change the patch, so that it is backwards compatible with the old > blame output for revision numbers <1000000 (i.e. always use minimum > width of 6, and larger if needed). My recommendation: Let's strive for compatibility where we can, and where the old behaviour is reasonable. I think a 6-character minimum column width is reasonable for most purposes, even though it's not the purest design. Other than that, the patch looks perfect. - Julian
