JavaHL: Bindings for svn_client_diff_summarize()

[Daniel Rall <dl...@collab.net>]

On Wed, 22 Nov 2006, Mark Phippard wrote:

> Daniel Rall wrote:
> >On Tue, 21 Nov 2006, Mark Phippard wrote:
...
> >>Since you are doing some stuff with JavaHL lately, one to add to your 
> >>to-do list would be some kind of support for the svn diff --summarize 
> >>option.  I think this is something we could use in Subclipse to improve 
> >>performance when comparing two tags of a large project.
...
> ... it would return an array of the items that have been modified,
> or maybe it would send notifications that would have to be trapped.
> Either way, it would be something really different from what diff
> does now.
> 
> Ideally, I'd probably like the API to return an array of some sort of Java 
> objects that describe the item and the type of change etc.

Mark, I've written the code for the diff summarize JavaHL bindings:

  http://svn.collab.net/repos/svn/branches/javahl-diff-summarize

When you have a moment, I'd appreciate review of at least the Java
APIs.  The JUnit test case in BasicTests.java shows how to use it, and
return all diff summaries in a list.

Unfortunately, there's a bug hiding somewhere in my implementation
which I haven't been able to shake out.  The bug is triggering an
exception (in native code, I think), which subsequently causes a JVM
segfault when JavaHL's error handling code calls the
env->DescribeException() JNI routine to output a stack trace for the
exception.  I've spent quite a bit of time on this bug, and could
really use some extra eyes.

Re: JavaHL: Bindings for svn_client_diff_summarize()

[Mark Phippard <ma...@gmail.com>]

On 12/15/06, Daniel Rall <dl...@collab.net> wrote:
>
> On Thu, 14 Dec 2006, Mark Phippard wrote:
> The way the C API handles the "prop changes and possible text changes"
> case is via the svn_client_diff_summarize_t's summarize_kind field,
> which is set to svn_client_diff_summarize_kind_normal (documented to
> describe an "item with no text modifications"), or another value.  The
> JavaHL API currently provides access to this value through the
> DiffSummary.getDiffKind() API.
>
> Do you think we should represent this differently in the JavaHL API,
> or simply document things better?


Now that you have said it, it seems obvious.  I suppose the JavaDoc could
state it more clearly as it is not likely that most consumers of the API
would be aware of the C API.

Thanks

Mark Phippard
http://markphip.blogspot.com/

Re: JavaHL: Bindings for svn_client_diff_summarize()

[Daniel Rall <dl...@collab.net>]

On Thu, 14 Dec 2006, Mark Phippard wrote:
...
> >On Thu, 07 Dec 2006, Daniel Rall wrote:
> >...
> >> Mark, could you take a look at the DiffSummaryReceiver and
> >> DiffSummary APIs, and make sure they're in usable shape?  I'm
> >> especially interested if you think we should pull the
> >> DiffSummary.DiffKind class out and make it more like the other
> >> classes which correspond to C enums which are found in JavaHL.
...
> 1)  Should the diffSummarize method take pegRevision arguments?

Yes, we should also support svn_client.h's svn_client_diff_summarize_peg()
API.

> 2)  Should DiffSummary have a textChanged boolean that corresponds to the
> propsChanged boolean?  Obviously if an item is modified, and propsChanged is
> false, then you know the text has changed.  But if propsChanged is true, you
> have no way to know whether the text was also changed.  It is conceivable
> that someone doing a graphical diff tool would only care about file changes
> and not prop changes.

The way the C API handles the "prop changes and possible text changes"
case is via the svn_client_diff_summarize_t's summarize_kind field,
which is set to svn_client_diff_summarize_kind_normal (documented to
describe an "item with no text modifications"), or another value.  The
JavaHL API currently provides access to this value through the
DiffSummary.getDiffKind() API.

Do you think we should represent this differently in the JavaHL API,
or simply document things better?

- Dan

p.s. Merge Tracking questions to be answered in a separate email.

Re: JavaHL: Bindings for svn_client_diff_summarize()

[Mark Phippard <ma...@gmail.com>]

On 12/13/06, Daniel Rall <dl...@collab.net> wrote:
>
> On Thu, 07 Dec 2006, Daniel Rall wrote:
> ...
> > Mark, could you take a look at the DiffSummaryReceiver and
> > DiffSummary APIs, and make sure they're in usable shape?  I'm
> > especially interested if you think we should pull the
> > DiffSummary.DiffKind class out and make it more like the other
> > classes which correspond to C enums which are found in JavaHL.
>
> Ping.  Not urgent, just don't want to forget about it before the API
> ships and is frozen...


I had forgotten, thanks.

I think the shape of the API's is OK.  These are the comments I would make:

1)  Should the diffSummarize method take pegRevision arguments?

2)  Should DiffSummary have a textChanged boolean that corresponds to the
propsChanged boolean?  Obviously if an item is modified, and propsChanged is
false, then you know the text has changed.  But if propsChanged is true, you
have no way to know whether the text was also changed.  It is conceivable
that someone doing a graphical diff tool would only care about file changes
and not prop changes.

On a related note, when the merge tracking stuff is brought to trunk, will
their be JavaHL related changes to make?  I assume there will be some new
methods and/or new method signatures?  I have not looked into what is
happening on that branch too closely.

Thanks

Mark

Re: JavaHL: Bindings for svn_client_diff_summarize()

[Daniel Rall <dl...@collab.net>]

On Thu, 07 Dec 2006, Daniel Rall wrote:
...
> Mark, could you take a look at the DiffSummaryReceiver and
> DiffSummary APIs, and make sure they're in usable shape?  I'm
> especially interested if you think we should pull the
> DiffSummary.DiffKind class out and make it more like the other
> classes which correspond to C enums which are found in JavaHL.

Ping.  Not urgent, just don't want to forget about it before the API
ships and is frozen...

Re: JavaHL: Bindings for svn_client_diff_summarize()

[Daniel Rall <dl...@collab.net>]

On Thu, 07 Dec 2006, Daniel Rall wrote:

> On Thu, 07 Dec 2006, Mark Phippard wrote:
> 
> > On 12/7/06, Mark Phippard <ma...@gmail.com> wrote:
> > >
> > >On 12/6/06, Daniel Rall <dl...@collab.net> wrote:
> > >
> > >> On Wed, 22 Nov 2006, Mark Phippard wrote:
> > >>
> > >> > Daniel Rall wrote:
> > >> > >On Tue, 21 Nov 2006, Mark Phippard wrote:
> > >> ...
> > >> > >>Since you are doing some stuff with JavaHL lately, one to add to
> > >> your
> > >> > >>to-do list would be some kind of support for the svn diff
> > >> --summarize
> > >> > >>option.  I think this is something we could use in Subclipse to
> > >> improve
> > >> > >>performance when comparing two tags of a large project.
> > >> ...
> > >> > ... it would return an array of the items that have been modified,
> > >> > or maybe it would send notifications that would have to be trapped.
> > >> > Either way, it would be something really different from what diff
> > >> > does now.
> > >> >
> > >> > Ideally, I'd probably like the API to return an array of some sort of
> > >> Java
> > >> > objects that describe the item and the type of change etc.
> > >>
> > >> Mark, I've written the code for the diff summarize JavaHL bindings:
> > >>
> > >>   http://svn.collab.net/repos/svn/branches/javahl-diff-summarize
> > >>
> > >> When you have a moment, I'd appreciate review of at least the Java
> > >> APIs.  The JUnit test case in BasicTests.java shows how to use it, and
> > >> return all diff summaries in a list.
> > >>
> > >> Unfortunately, there's a bug hiding somewhere in my implementation
> > >> which I haven't been able to shake out.  The bug is triggering an
> > >> exception (in native code, I think), which subsequently causes a JVM
> > >> segfault when JavaHL's error handling code calls the
> > >> env->DescribeException() JNI routine to output a stack trace for the
> > >> exception.  I've spent quite a bit of time on this bug, and could
> > >> really use some extra eyes.

I found and fixed this bug (thanks to glasser for helping me narrow
things down), and merged the branch into trunk in r22601.

Mark, could you take a look at the DiffSummaryReceiver and DiffSummary
APIs, and make sure they're in usable shape?  I'm especially
interested if you think we should pull the DiffSummary.DiffKind class
out and make it more like the other classes which correspond to C
enums which are found in JavaHL.

Re: JavaHL: Bindings for svn_client_diff_summarize()

[Daniel Rall <dl...@collab.net>]

On Thu, 07 Dec 2006, Mark Phippard wrote:

> On 12/7/06, Mark Phippard <ma...@gmail.com> wrote:
> >
> >On 12/6/06, Daniel Rall <dl...@collab.net> wrote:
> >
> >> On Wed, 22 Nov 2006, Mark Phippard wrote:
> >>
> >> > Daniel Rall wrote:
> >> > >On Tue, 21 Nov 2006, Mark Phippard wrote:
> >> ...
> >> > >>Since you are doing some stuff with JavaHL lately, one to add to
> >> your
> >> > >>to-do list would be some kind of support for the svn diff
> >> --summarize
> >> > >>option.  I think this is something we could use in Subclipse to
> >> improve
> >> > >>performance when comparing two tags of a large project.
> >> ...
> >> > ... it would return an array of the items that have been modified,
> >> > or maybe it would send notifications that would have to be trapped.
> >> > Either way, it would be something really different from what diff
> >> > does now.
> >> >
> >> > Ideally, I'd probably like the API to return an array of some sort of
> >> Java
> >> > objects that describe the item and the type of change etc.
> >>
> >> Mark, I've written the code for the diff summarize JavaHL bindings:
> >>
> >>   http://svn.collab.net/repos/svn/branches/javahl-diff-summarize
> >>
> >> When you have a moment, I'd appreciate review of at least the Java
> >> APIs.  The JUnit test case in BasicTests.java shows how to use it, and
> >> return all diff summaries in a list.
> >>
> >> Unfortunately, there's a bug hiding somewhere in my implementation
> >> which I haven't been able to shake out.  The bug is triggering an
> >> exception (in native code, I think), which subsequently causes a JVM
> >> segfault when JavaHL's error handling code calls the
> >> env->DescribeException() JNI routine to output a stack trace for the
> >> exception.  I've spent quite a bit of time on this bug, and could
> >> really use some extra eyes.
> >
> >I had a little trouble getting a diff for this since you made the branch
> >and commit in one step.  Just an fyi..  I had to resort to the commit email
> >and eventually just checked it out.

Sorry, didn't mean to make things more difficult than they have to be.
FYI, you can get the delta for a branch which was branched with
changes by diffing its HEAD against the branch it was created from at
the revision it was created from (determined via 'svn log -v').

# Find trunk revnum
svn log -v --stop-on-copy \
    http://svn.collab.net/repos/svn/branches/javahl-diff-summarize

# Get delta
svn di http://svn.collab.net/repos/svn/trunk@22588 \
       http://svn.collab.net/repos/svn/branches/javahl-diff-summarize@HEAD

> >Is there a reason that DiffSummary extends EventObject?  I do not know
> >what that class is and cannot see any precedent for extending it in JavaHL.
> >
> >I'll keep digging though.
> 
> I see that the Notify stuff uses the same technique, so just ignore
> this comment.  I'll keep looking.

It's just conventional (like using the EventListener marker
interface).  EventObject is a common marker interface used for events
passed to Java listeners.  It's not strictly necessary.

Re: JavaHL: Bindings for svn_client_diff_summarize()

[Mark Phippard <ma...@gmail.com>]

On 12/7/06, Mark Phippard <ma...@gmail.com> wrote:
>
> On 12/6/06, Daniel Rall <dl...@collab.net> wrote:
>
> > On Wed, 22 Nov 2006, Mark Phippard wrote:
> >
> > > Daniel Rall wrote:
> > > >On Tue, 21 Nov 2006, Mark Phippard wrote:
> > ...
> > > >>Since you are doing some stuff with JavaHL lately, one to add to
> > your
> > > >>to-do list would be some kind of support for the svn diff
> > --summarize
> > > >>option.  I think this is something we could use in Subclipse to
> > improve
> > > >>performance when comparing two tags of a large project.
> > ...
> > > ... it would return an array of the items that have been modified,
> > > or maybe it would send notifications that would have to be trapped.
> > > Either way, it would be something really different from what diff
> > > does now.
> > >
> > > Ideally, I'd probably like the API to return an array of some sort of
> > Java
> > > objects that describe the item and the type of change etc.
> >
> > Mark, I've written the code for the diff summarize JavaHL bindings:
> >
> >   http://svn.collab.net/repos/svn/branches/javahl-diff-summarize
> >
> > When you have a moment, I'd appreciate review of at least the Java
> > APIs.  The JUnit test case in BasicTests.java shows how to use it, and
> > return all diff summaries in a list.
> >
> > Unfortunately, there's a bug hiding somewhere in my implementation
> > which I haven't been able to shake out.  The bug is triggering an
> > exception (in native code, I think), which subsequently causes a JVM
> > segfault when JavaHL's error handling code calls the
> > env->DescribeException() JNI routine to output a stack trace for the
> > exception.  I've spent quite a bit of time on this bug, and could
> > really use some extra eyes.
>
>
> I had a little trouble getting a diff for this since you made the branch
> and commit in one step.  Just an fyi..  I had to resort to the commit email
> and eventually just checked it out.
>
> Is there a reason that DiffSummary extends EventObject?  I do not know
> what that class is and cannot see any precedent for extending it in JavaHL.
>
> I'll keep digging though.
>

I see that the Notify stuff uses the same technique, so just ignore this
comment.  I'll keep looking.

Mark

Re: JavaHL: Bindings for svn_client_diff_summarize()

[Mark Phippard <ma...@gmail.com>]

On 12/6/06, Daniel Rall <dl...@collab.net> wrote:
>
> On Wed, 22 Nov 2006, Mark Phippard wrote:
>
> > Daniel Rall wrote:
> > >On Tue, 21 Nov 2006, Mark Phippard wrote:
> ...
> > >>Since you are doing some stuff with JavaHL lately, one to add to your
> > >>to-do list would be some kind of support for the svn diff --summarize
> > >>option.  I think this is something we could use in Subclipse to
> improve
> > >>performance when comparing two tags of a large project.
> ...
> > ... it would return an array of the items that have been modified,
> > or maybe it would send notifications that would have to be trapped.
> > Either way, it would be something really different from what diff
> > does now.
> >
> > Ideally, I'd probably like the API to return an array of some sort of
> Java
> > objects that describe the item and the type of change etc.
>
> Mark, I've written the code for the diff summarize JavaHL bindings:
>
>   http://svn.collab.net/repos/svn/branches/javahl-diff-summarize
>
> When you have a moment, I'd appreciate review of at least the Java
> APIs.  The JUnit test case in BasicTests.java shows how to use it, and
> return all diff summaries in a list.
>
> Unfortunately, there's a bug hiding somewhere in my implementation
> which I haven't been able to shake out.  The bug is triggering an
> exception (in native code, I think), which subsequently causes a JVM
> segfault when JavaHL's error handling code calls the
> env->DescribeException() JNI routine to output a stack trace for the
> exception.  I've spent quite a bit of time on this bug, and could
> really use some extra eyes.


I had a little trouble getting a diff for this since you made the branch and
commit in one step.  Just an fyi..  I had to resort to the commit email and
eventually just checked it out.

Is there a reason that DiffSummary extends EventObject?  I do not know what
that class is and cannot see any precedent for extending it in JavaHL.

I'll keep digging though.

Mark