[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Fix alignment of blame output containing revision numbers >= 1000000

From: Julian Foad <julian.foad_at_wandisco.com>
Date: Thu, 17 Jun 2010 14:47:08 +0100

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
Received on 2010-06-17 15:47:53 CEST

This is an archived mail posted to the Subversion Dev mailing list.