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

[Julian Foad <ju...@wandisco.com>]

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

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

[Julian Foad <ju...@wandisco.com>]

On Thu, 2010-06-17, Johan Corveleyn wrote:
> On Thu, Jun 17, 2010 at 5:54 PM, C. Michael Pilato <cm...@collab.net> wrote:
> > Julian Foad wrote:
> >> 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.
> >
> > +1
> 
> Ok, thanks for the feedback, all.
> 
> In attachment a new version of the patch, which is compatible with the
> old behavior for sub-1000000 end_revnum's. make check now passes. I've
> also added a comment in the source explaining what's going on with the
> 6 and the 1000000 (feel free to edit/lose that comment if you want).

Hi Johan.

Committed revision 955895.

Thank you very much for this work!

- Julian


> And an updated log message:
> [[[
> 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 increase the column
>   width for the revision number if needed.
> * subversion/libsvn_client/deprecated.c
>  (blame_wrapper_receiver2): Implement the updated
>   svn_client_blame_receiver3_t.
> ]]]
> 
> Cheers,

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

[Johan Corveleyn <jc...@gmail.com>]

On Thu, Jun 17, 2010 at 5:54 PM, C. Michael Pilato <cm...@collab.net> wrote:
> Julian Foad wrote:
>> 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.
>
> +1

Ok, thanks for the feedback, all.

In attachment a new version of the patch, which is compatible with the
old behavior for sub-1000000 end_revnum's. make check now passes. I've
also added a comment in the source explaining what's going on with the
6 and the 1000000 (feel free to edit/lose that comment if you want).

And an updated log message:
[[[
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 increase the column
  width for the revision number if needed.
* subversion/libsvn_client/deprecated.c
 (blame_wrapper_receiver2): Implement the updated
  svn_client_blame_receiver3_t.
]]]

Cheers,
-- 
Johan

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

[Johan Corveleyn <jc...@gmail.com>]

On Thu, Jun 17, 2010 at 5:54 PM, C. Michael Pilato <cm...@collab.net> wrote:
> Julian Foad wrote:
>> 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.
>
> +1

Ok, thanks for the feedback, all.

In attachment a new version of the patch, which is compatible with the
old behavior for sub-1000000 end_revnum's. make check now passes. I've
also added a comment in the source explaining what's going on with the
6 and the 1000000 (feel free to edit/lose that comment if you want).

And an updated log message:
[[[
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 increase the column
  width for the revision number if needed.
* subversion/libsvn_client/deprecated.c
 (blame_wrapper_receiver2): Implement the updated
  svn_client_blame_receiver3_t.
]]]

Cheers,
-- 
Johan

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

["C. Michael Pilato" <cm...@collab.net>]

Julian Foad wrote:
> 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.

+1

-- 
C. Michael Pilato <cm...@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand