Johan Corveleyn wrote:
> On Wed, Jun 16, 2010 at 1:11 PM, Julian Foad <julian.foad_at_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
Received on 2010-06-16 15:59:15 CEST