You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Johan Corveleyn <jc...@gmail.com> on 2010/06/15 14:09:11 UTC
Fixing blame misalignment for >1m revs - some questions
Hi devs,
Recall a discussion on this list about a small bug in command line
"svn blame": if blame output contains a revision number larger than 6
characters (i.e. >= 1000000) the columns would not be correctly
aligned (see [1]). I promised in that thread to fix this, as my first
svn hacking exercise. However, finding some free time to do this took
me much longer than I expected/wanted ("a couple of days" has now
become "a couple of months). Sorry about that.
Anyway, moving on, I want to pick up the ball again. I have some
(newbie) questions about the proposed solution (using "end_revnum" to
determine the maximum column width for the revnum).
1) To do this, the blame_receiver callback needs to know the
end_revnum. So I need to extend svn_client_blame_receiver3_t to
include end_revnum as a parameter, right (so it can be passed to the
receiver inside svn_client_blame5 (blame.c))?
2) Since svn_client_blame_receiver3_t was introduced in 1.7 (according
to the comment), can I just change it, or do I still have to introduce
a svn_client_blame_receiver4_t for backward compat?
3) Do I add the end_revnum parameter at the end of the parameter list,
just before the *pool?
Thanks.
[1] http://svn.haxx.se/dev/archive-2010-04/0463.shtml
Cheers,
--
Johan
Re: Fixing blame misalignment for >1m revs - some questions
Posted by Johan Corveleyn <jc...@gmail.com>.
On Wed, Jun 16, 2010 at 3:58 PM, Julian Foad <ju...@wandisco.com> wrote:
> Johan Corveleyn wrote:
>> On Wed, Jun 16, 2010 at 1:11 PM, Julian Foad <ju...@wandisco.com> wrote:
>> > Note that the caller can request a blame where the "end" revision is the
>> > "WORKING" pseudo-revision, so be careful not to confuse the reader when
>> > documenting this new "end revision number" parameter which is a revision
>> > number as determined from the repository and which will not include the
>> > "WORKING" pseudo-revision. (Maybe we should also pass a parameter that
>> > provides this information: "svn_boolean_t
>> > range_includes_working_version"?)
[...]
> On the other hand, this is information that the client already knows and
> can pass via the "baton", so it doesn't need to be passed through a
> separate argument to this callback.
Ah, yes indeed. In fact it's already passed via the baton (at least in
blame-cmd's case). Inside svn_cl__blame, the blame_baton_t gets a
reference to the opt_state. And then there is the following piece of
code, right before svn_client_blame5 is called:
[[[
if (end_revision_unspecified)
{
if (peg_revision.kind != svn_opt_revision_unspecified)
opt_state->end_revision = peg_revision;
else if (svn_path_is_url(target))
opt_state->end_revision.kind = svn_opt_revision_head;
else
opt_state->end_revision.kind = svn_opt_revision_working;
}
]]]
So I guess that should be ok.
Cheers,
--
Johan
Re: Fixing blame misalignment for >1m revs - some questions
Posted by Julian Foad <ju...@wandisco.com>.
Johan Corveleyn wrote:
> On Wed, Jun 16, 2010 at 1:11 PM, Julian Foad <ju...@wandisco.com> wrote:
> > It seems sensible to pass the start and end revision numbers of the
> > requested blame range to this callback.
>
> Heh, I was just about to send a patch (was fiddling with getting gmail
> to set a good mime-type for the patch attachment; I think I'll just
> give it a .txt extension, as others have done before me). But it only
> added end_revnum to the callback, because that's the only thing that
> will be used currently. But I can just as well add the start revision
> number also, for consistency/symmetry.
>
> I'll update the patch and send it to the list this evening.
>
> However, I saw that my changes to svn_client_blame_receiver3_t cause
> the bindings to break (at least something in javahl didn't compile
> anymore). Am I supposed to address those as well (I don't know much
> about JNI etc), or will other people take care of those?
It's nice if you can fix that too, but in my opinion it's optional.
> > Note that the caller can request a blame where the "end" revision is the
> > "WORKING" pseudo-revision, so be careful not to confuse the reader when
> > documenting this new "end revision number" parameter which is a revision
> > number as determined from the repository and which will not include the
> > "WORKING" pseudo-revision. (Maybe we should also pass a parameter that
> > provides this information: "svn_boolean_t
> > range_includes_working_version"?)
>
> Good point. But I think it should suffice if it's clearly documented
> that the end_revnum is the end revision of the blame range, as
> determined from the repository (highest revision in the repository
> that's relevant for this blame operation). For lines that have been
> locally changed there is already the boolean local_change. Is it
> important for the receiver to know whether the caller asked for a
> range including the WORKING revision? Right now I can't see a use for
> this (in the current fix of making sure the (plain-text) blame output
> is correctly aligned, I just use the end_revnum as an indication of
> the maximum width of that column, regardless of what range the user
> asked for or whether it includes local changes etc).
I was thinking, for consistency/symmetry, if we're going to be
communicating the requested range, then I think it is important to
indicate the full range. Determining the field width is not the only
potential use for this information, and even if it is, the receiver
might want to display some string such as "working" in the
revision-number column, and wants to know whether it needs to allow
space for that string in its revision-number column.
On the other hand, this is information that the client already knows and
can pass via the "baton", so it doesn't need to be passed through a
separate argument to this callback.
- Julian
Re: Fixing blame misalignment for >1m revs - some questions
Posted by Johan Corveleyn <jc...@gmail.com>.
On Wed, Jun 16, 2010 at 1:11 PM, Julian Foad <ju...@wandisco.com> wrote:
> On Tue, 2010-06-15, Johan Corveleyn wrote:
>> On Tue, Jun 15, 2010 at 4:36 PM, Philip Martin
>> <ph...@wandisco.com> wrote:
>> > Johan Corveleyn <jc...@gmail.com> writes:
>> >
>> >> 1) To do this, the blame_receiver callback needs to know the
>> >> end_revnum. So I need to extend svn_client_blame_receiver3_t to
>> >> include end_revnum as a parameter, right (so it can be passed to the
>> >> receiver inside svn_client_blame5 (blame.c))?
>> >
>> > Not sure. Does end_revnum correspond to the end parameter passed to
>> > svn_client_blame5?
>>
>> Only if the user specifies an end revnum explicitly. Otherwise the end
>> parameter refers to HEAD or WORKING (depending on whether the target
>> is a url or a path). So in general the caller doesn't know the exact
>> end_revnum, and it's only retrieved in svn_client_blame5 (when it
>> calls "svn_client__ra_session_from_path(&ra_session, &end_revnum,
>> ...").
>>
>> > If so then it is probably the callers
>> > responsibility to put it into receiver_baton, and it doesn't have to
>> > appear in svn_client_blame_receiver3_t.
>> >
>> >> 2) Since svn_client_blame_receiver3_t was introduced in 1.7 (according
>> >> to the comment), can I just change it, or do I still have to introduce
>> >> a svn_client_blame_receiver4_t for backward compat?
>> >
>> > svn_client_blame_receiver3_t can be changed.
>>
>> Ok, I guess I'll have to do that then.
>
> It seems sensible to pass the start and end revision numbers of the
> requested blame range to this callback.
Heh, I was just about to send a patch (was fiddling with getting gmail
to set a good mime-type for the patch attachment; I think I'll just
give it a .txt extension, as others have done before me). But it only
added end_revnum to the callback, because that's the only thing that
will be used currently. But I can just as well add the start revision
number also, for consistency/symmetry.
I'll update the patch and send it to the list this evening.
However, I saw that my changes to svn_client_blame_receiver3_t cause
the bindings to break (at least something in javahl didn't compile
anymore). Am I supposed to address those as well (I don't know much
about JNI etc), or will other people take care of those?
> Note that the caller can request a blame where the "end" revision is the
> "WORKING" pseudo-revision, so be careful not to confuse the reader when
> documenting this new "end revision number" parameter which is a revision
> number as determined from the repository and which will not include the
> "WORKING" pseudo-revision. (Maybe we should also pass a parameter that
> provides this information: "svn_boolean_t
> range_includes_working_version"?)
Good point. But I think it should suffice if it's clearly documented
that the end_revnum is the end revision of the blame range, as
determined from the repository (highest revision in the repository
that's relevant for this blame operation). For lines that have been
locally changed there is already the boolean local_change. Is it
important for the receiver to know whether the caller asked for a
range including the WORKING revision? Right now I can't see a use for
this (in the current fix of making sure the (plain-text) blame output
is correctly aligned, I just use the end_revnum as an indication of
the maximum width of that column, regardless of what range the user
asked for or whether it includes local changes etc).
Cheers,
--
Johan
Re: Fixing blame misalignment for >1m revs - some questions
Posted by Julian Foad <ju...@wandisco.com>.
On Tue, 2010-06-15, Johan Corveleyn wrote:
> On Tue, Jun 15, 2010 at 4:36 PM, Philip Martin
> <ph...@wandisco.com> wrote:
> > Johan Corveleyn <jc...@gmail.com> writes:
> >
> >> 1) To do this, the blame_receiver callback needs to know the
> >> end_revnum. So I need to extend svn_client_blame_receiver3_t to
> >> include end_revnum as a parameter, right (so it can be passed to the
> >> receiver inside svn_client_blame5 (blame.c))?
> >
> > Not sure. Does end_revnum correspond to the end parameter passed to
> > svn_client_blame5?
>
> Only if the user specifies an end revnum explicitly. Otherwise the end
> parameter refers to HEAD or WORKING (depending on whether the target
> is a url or a path). So in general the caller doesn't know the exact
> end_revnum, and it's only retrieved in svn_client_blame5 (when it
> calls "svn_client__ra_session_from_path(&ra_session, &end_revnum,
> ...").
>
> > If so then it is probably the callers
> > responsibility to put it into receiver_baton, and it doesn't have to
> > appear in svn_client_blame_receiver3_t.
> >
> >> 2) Since svn_client_blame_receiver3_t was introduced in 1.7 (according
> >> to the comment), can I just change it, or do I still have to introduce
> >> a svn_client_blame_receiver4_t for backward compat?
> >
> > svn_client_blame_receiver3_t can be changed.
>
> Ok, I guess I'll have to do that then.
It seems sensible to pass the start and end revision numbers of the
requested blame range to this callback.
Note that the caller can request a blame where the "end" revision is the
"WORKING" pseudo-revision, so be careful not to confuse the reader when
documenting this new "end revision number" parameter which is a revision
number as determined from the repository and which will not include the
"WORKING" pseudo-revision. (Maybe we should also pass a parameter that
provides this information: "svn_boolean_t
range_includes_working_version"?)
- Julian
Re: Fixing blame misalignment for >1m revs - some questions
Posted by Johan Corveleyn <jc...@gmail.com>.
On Tue, Jun 15, 2010 at 4:36 PM, Philip Martin
<ph...@wandisco.com> wrote:
> Johan Corveleyn <jc...@gmail.com> writes:
>
>> 1) To do this, the blame_receiver callback needs to know the
>> end_revnum. So I need to extend svn_client_blame_receiver3_t to
>> include end_revnum as a parameter, right (so it can be passed to the
>> receiver inside svn_client_blame5 (blame.c))?
>
> Not sure. Does end_revnum correspond to the end parameter passed to
> svn_client_blame5?
Only if the user specifies an end revnum explicitly. Otherwise the end
parameter refers to HEAD or WORKING (depending on whether the target
is a url or a path). So in general the caller doesn't know the exact
end_revnum, and it's only retrieved in svn_client_blame5 (when it
calls "svn_client__ra_session_from_path(&ra_session, &end_revnum,
...").
> If so then it is probably the callers
> responsibility to put it into receiver_baton, and it doesn't have to
> appear in svn_client_blame_receiver3_t.
>
>> 2) Since svn_client_blame_receiver3_t was introduced in 1.7 (according
>> to the comment), can I just change it, or do I still have to introduce
>> a svn_client_blame_receiver4_t for backward compat?
>
> svn_client_blame_receiver3_t can be changed.
Ok, I guess I'll have to do that then.
Thanks.
Cheers,
Johan
>
>> 3) Do I add the end_revnum parameter at the end of the parameter list,
>> just before the *pool?
>
> We generally put outputs before inputs with pools last; there is no
> requirement that new function parameters go at the end.
>
> --
> Philip
>
Re: Fixing blame misalignment for >1m revs - some questions
Posted by Philip Martin <ph...@wandisco.com>.
Johan Corveleyn <jc...@gmail.com> writes:
> 1) To do this, the blame_receiver callback needs to know the
> end_revnum. So I need to extend svn_client_blame_receiver3_t to
> include end_revnum as a parameter, right (so it can be passed to the
> receiver inside svn_client_blame5 (blame.c))?
Not sure. Does end_revnum correspond to the end parameter passed to
svn_client_blame5? If so then it is probably the callers
responsibility to put it into receiver_baton, and it doesn't have to
appear in svn_client_blame_receiver3_t.
> 2) Since svn_client_blame_receiver3_t was introduced in 1.7 (according
> to the comment), can I just change it, or do I still have to introduce
> a svn_client_blame_receiver4_t for backward compat?
svn_client_blame_receiver3_t can be changed.
> 3) Do I add the end_revnum parameter at the end of the parameter list,
> just before the *pool?
We generally put outputs before inputs with pools last; there is no
requirement that new function parameters go at the end.
--
Philip