You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Martin Hauner <ma...@gmx.net> on 2005/08/12 17:07:05 UTC
[PATCH] V2, diff preview without text deltas
Hi,
attached is a second version of a patch for the diff preview api call
(which the gui clients like to have) that doesn't produce text deltas.
I have updated it based on Julians notes and changed and cleaned up
a few other things.
Since the first version is a couple of weeks old i repeat a few notes:
* this is an api only patch. For easier review i have created a small
patch to run the preview from the command line.
* there is a lot of code duplication between the diff and the diff
preview code that i want to address in another patch if the preview
stuff makes it into the repository.
* the preview handles only the repository <-> repoditory case for now.
the other combinations are another thing i would like to address in
different patches.
Unsolved problem
----------------
The open_file slot in the diff editor is called for files that are
not changed (a diff shows no text differences). These files don't
show up in a normal diff.
It looks like this has something todo with merging.
After creating a branch from trunk and merging several files from
trunk to that branch (making the files on the branch and trunk equal
again (text content)) they appear in a diff preview between the
branch and trunk.
Is this supposed to happen (ie. open_file is called for such files)?
And if so how do i recognize this situation so i can filter those
files?
minor version compatibility:
----------------------------
The preview patch adds an optional boolean to do_diff.
To handle minor version compatibility i modified vparse_tuple to
handle optional bool values simply by not returning an error. This
requires to initialize the optional bool before calling vparse_tuple.
preview or summarize:
---------------------
How should it be named? preview or summarize?
I named it preview after the inital confusion with #issue 2015
(summarize) because my patch doesn't really solve that issue.
But i have to problem with changing it again to summarize.
--
Martin
Subcommander, http://subcommander.tigris.org
a cross platform Win32/Unix/MacOSX subversion gui client & diff/merge tool.
Re: [PATCH] V2, diff preview without text deltas
Posted by Martin Hauner <Ma...@gmx.net>.
> On Sun, 14 Aug 2005, [UTF-8] Branko Ä^Libej wrote:
>
> > Martin Hauner wrote:
> >
> > > svn diff -p file:///d:/dev/src/subcommander/preview/branches/0.1.x
> > > file:///d:/dev/src/subcommander/preview/trunk
> > > > M CHANGES
> > >
> > > # ups.. that's unexpected here. Looks like the open_file method of the
> > > # diff preview editor was called.
> > >
> -----snip---------------------------------------------------------------
> > >
> > >
> > > Is this to be expected?
> >
> > Yes.
> >
> But is apply_text_delta() called. That's what indicates content changes.
> open_file may be called for no changes at all.
Ah, looks like that is the missing piece of information. :)
apply_text_delta() is currently just a "do nothing" method in the patch.
> (Martin, I'll take a look at your patch this week.)
Thanks.
--
Martin
Subcommander, http://subcommander.tigris.org
a cross platform Win32/Unix/MacOSX subversion gui client & diff/merge tool.
Lust, ein paar Euro nebenbei zu verdienen? Ohne Kosten, ohne Risiko!
Satte Provisionen für GMX Partner: http://www.gmx.net/de/go/partner
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] V2, diff preview without text deltas
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Tue, 16 Aug 2005, [UTF-8] Branko �^Libej wrote:
> Peter N. Lundblad wrote:
>
> > And what does 2.0 have to do with this discussion?
> >We're not talking about moving the line-based diff to the server, as I
> >understand it.
> >
> >
> No. And if I'd thought about it a bit, it would be obvious that if the
> server is to compute deltas, it must compare file contents (at least in
> principle).
>
> But won't the client have to actually download the delta in order to
> figure out if the file has actually been changed?
>
Not in the ignoer_ancestry case according to the current code. Then
svn_repos__compare_files, which will do an actual byte-by-byte comparison
after some shortcuts have been tried.
In the --notice-ancestry case, you'll get false positives. Then, the
apply_text_delta call tells you something like "the contents have changed
between these versions, but they might be the same anyway" or something.
For the summarized diff we're discussing here, it seems to make sense to
use ignore_ancestry, since that's the default for a diff with deltas. If
it seems necessary, it would not be hard to add another flag, like
no_empty_text_deltas or something. I'm not sure how this feature wil be
used most.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] V2, diff preview without text deltas
Posted by Branko Čibej <br...@xbc.nu>.
Peter N. Lundblad wrote:
>On Sun, 14 Aug 2005, [UTF-8] Branko �^Libej wrote:
>
>
>> The same thing "svn diff" does: compare the files. Yes, I know that
>>
>>means you have to actually download them.
>>
>>The only other option would be to teach the server to compare files on
>>the server side. Ouch. Not before 2.0.
>>
>h, we might have to congratulate our selves; Subversion 2.0 seem to
>already be released:-) See delta_files in libsvn_repos/reporter.c. If
>ignore_ancestry is true, it will actually compare the file contents before
>sending a text delta.
>
Heh, Serves me right for not checking the code.
> And what does 2.0 have to do with this discussion?
>We're not talking about moving the line-based diff to the server, as I
>understand it.
>
>
No. And if I'd thought about it a bit, it would be obvious that if the
server is to compute deltas, it must compare file contents (at least in
principle).
But won't the client have to actually download the delta in order to
figure out if the file has actually been changed?
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] V2, diff preview without text deltas
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Sun, 14 Aug 2005, [UTF-8] Branko �^Libej wrote:
> Martin Hauner wrote:
>
> > svn diff -p file:///d:/dev/src/subcommander/preview/branches/0.1.x
> > file:///d:/dev/src/subcommander/preview/trunk
> > > M CHANGES
> >
> > # ups.. that's unexpected here. Looks like the open_file method of the
> > # diff preview editor was called.
> > -----snip---------------------------------------------------------------
> >
> >
> > Is this to be expected?
>
> Yes.
>
But is apply_text_delta() called. That's what indicates content changes.
open_file may be called for no changes at all.
> > And if so how do i recognize it so i can filter those files?
>
> The same thing "svn diff" does: compare the files. Yes, I know that
> means you have to actually download them.
>
> The only other option would be to teach the server to compare files on
> the server side. Ouch. Not before 2.0.
>
Oh, we might have to congratulate our selves; Subversion 2.0 seem to
already be released:-) See delta_files in libsvn_repos/reporter.c. If
ignore_ancestry is true, it will actually compare the file contents before
sending a text delta. And what does 2.0 have to do with this discussion?
We're not talking about moving the line-based diff to the server, as I
understand it.
(Martin, I'll take a look at your patch this week.)
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] V2, diff preview without text deltas
Posted by Martin Hauner <ma...@gmx.net>.
Branko Čibej wrote:
> Martin Hauner wrote:
>
>> svn diff -p file:///d:/dev/src/subcommander/preview/branches/0.1.x
>> file:///d:/dev/src/subcommander/preview/trunk
>> > M CHANGES
>>
>> # ups.. that's unexpected here. Looks like the open_file method of the
>> # diff preview editor was called.
>> -----snip---------------------------------------------------------------
>>
>>
>> Is this to be expected?
>
>
> Yes.
>
>> And if so how do i recognize it so i can filter those files?
>
>
> The same thing "svn diff" does: compare the files. Yes, I know that
> means you have to actually download them.
>
> The only other option would be to teach the server to compare files on
> the server side. Ouch. Not before 2.0.
Uh, that is not good, the whole idea of the patch was to avoid
downloading the files...
--
Martin
Subcommander, http://subcommander.tigris.org
a cross platform Win32/Unix/MacOSX subversion gui client & diff/merge tool.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] V2, diff preview without text deltas
Posted by Branko Čibej <br...@xbc.nu>.
Martin Hauner wrote:
> svn diff -p file:///d:/dev/src/subcommander/preview/branches/0.1.x
> file:///d:/dev/src/subcommander/preview/trunk
> > M CHANGES
>
> # ups.. that's unexpected here. Looks like the open_file method of the
> # diff preview editor was called.
> -----snip---------------------------------------------------------------
>
>
> Is this to be expected?
Yes.
> And if so how do i recognize it so i can filter those files?
The same thing "svn diff" does: compare the files. Yes, I know that
means you have to actually download them.
The only other option would be to teach the server to compare files on
the server side. Ouch. Not before 2.0.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] V2, diff preview without text deltas
Posted by Martin Hauner <ma...@gmx.net>.
Ben Collins-Sussman wrote:
>
> On Aug 12, 2005, at 12:07 PM, Martin Hauner wrote:
>
>>
>> Unsolved problem
>> ----------------
>>
>> The open_file slot in the diff editor is called for files that are
>> not changed (a diff shows no text differences). These files don't
>> show up in a normal diff.
>>
>> It looks like this has something todo with merging.
>>
>> After creating a branch from trunk and merging several files from
>> trunk to that branch (making the files on the branch and trunk equal
>> again (text content)) they appear in a diff preview between the
>> branch and trunk.
>
>
>
> Can you show us a transcript which demonstrates this? So we can all
> reproduce and examine what's going on?
Here we go:
the output of svn where interesting is prefixed with "> ", comments start
with #. I hope it is not too confusing :)
-----snip---------------------------------------------------------------
svn admin create preview
svn co file:///d:/dev/src/subcommander/preview pwc
cd pwc
svn mkdir trunk
svn mkdir branches
svn ci -m "created layout"
svn up
cd trunk
touch CHANGES
svn add CHANGES
svn ci -m "added CHANGES"
svn up
svn cp file:///d:/dev/src/subcommander/preview/trunk
file:///d:/dev/src/subcommander/preview/branches/0.1.x -m "created branch"
> <no output>
svn diff -p file:///d:/dev/src/subcommander/preview/branches/0.1.x
file:///d:/dev/src/subcommander/preview/trunk
> <no output>
cd trunk
echo 'strange!' >> CHANGES
svn ci -m "modified CHANGES"
svn up
# now lets take a look at the diff and the diff preview:
svn diff file:///d:/dev/src/subcommander/preview/branches/0.1.x
file:///d:/dev/src/subcommander/preview/trunk
> <a diff with the modification to the CHANGES file>
svn diff -p file:///d:/dev/src/subcommander/preview/branches/0.1.x
file:///d:/dev/src/subcommander/preview/trunk
> M CHANGES
# ok it looks good so far, now lets merge the modification into the branch
cd ../branches/0.1.x
svn up
svn log file:///d:/dev/src/subcommander/preview/trunk
> ------------------------------------------------------------------------
> r4 | martin | 2005-08-14 15:06:36 +0200 (So, 14 Aug 2005) | 1 line
>
> modified CHANGES
> ...
svn merge -r3:4 file:///d:/dev/src/subcommander/preview/trunk .
> U CHANGES
svn ci -m "merged r4"
svn up
# lets take another look at the diff and the diff preview:
svn diff file:///d:/dev/src/subcommander/preview/branches/0.1.x
file:///d:/dev/src/subcommander/preview/trunk
> <no output>
# looks good again
svn diff -p file:///d:/dev/src/subcommander/preview/branches/0.1.x
file:///d:/dev/src/subcommander/preview/trunk
> M CHANGES
# ups.. that's unexpected here. Looks like the open_file method of the
# diff preview editor was called.
-----snip---------------------------------------------------------------
Is this to be expected? And if so how do i recognize it so i can filter
those files?
--
Martin
Subcommander, http://subcommander.tigris.org
a cross platform Win32/Unix/MacOSX subversion gui client & diff/merge tool.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] V2, diff preview without text deltas
Posted by Ben Collins-Sussman <su...@collab.net>.
On Aug 12, 2005, at 12:07 PM, Martin Hauner wrote:
>
> Unsolved problem
> ----------------
>
> The open_file slot in the diff editor is called for files that are
> not changed (a diff shows no text differences). These files don't
> show up in a normal diff.
>
> It looks like this has something todo with merging.
>
> After creating a branch from trunk and merging several files from
> trunk to that branch (making the files on the branch and trunk equal
> again (text content)) they appear in a diff preview between the
> branch and trunk.
Can you show us a transcript which demonstrates this? So we can all
reproduce and examine what's going on?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] V2, diff preview without text deltas
Posted by Martin Hauner <ma...@gmx.net>.
Peter N. Lundblad wrote:
> On Fri, 12 Aug 2005, Martin Hauner wrote:
>[..]
>>* there is a lot of code duplication between the diff and the diff
>> preview code that i want to address in another patch if the preview
>> stuff makes it into the repository.
>>
[..]
> OK, let's make a deal. I promise to do what I can together with you to
> get this into the repository (if no other full committer objects, of
> course). Would you then like to do the refactoring work first, which
> would probably make this patch significantly smaller?
Ok, i will remove it for the next patch version. But i'm not so sure
it will make the patch smaller.
>>minor version compatibility:
>>----------------------------
>>
>>The preview patch adds an optional boolean to do_diff.
>>
>>To handle minor version compatibility i modified vparse_tuple to
>>handle optional bool values simply by not returning an error. This
>>requires to initialize the optional bool before calling vparse_tuple.
>>
>
> For revving get_commit_editor in 1.2, I solved this problem by checking
> the number of parameters. I don't know if that's cleaner than the
> inconsistency that your approach introduces. >
I will take a look at it. I'm not really happy with having to initialize
the bool but it was the simplest solution.
>>preview or summarize:
>>---------------------
>>
>>How should it be named? preview or summarize?
>>I named it preview after the inital confusion with #issue 2015
>>(summarize) because my patch doesn't really solve that issue.
>>But i have to problem with changing it again to summarize.
>
> Doesn't it solve #2015 without the -v option, or do I miss something? I
> like summarize more. (I mean make it possible to solve, since this patch
> doesn't touch the cmdline client, of course.)
Looks like you are right. Then back to summarize :)
> In the review below, I'm not picking on style, but am instead
> concentrating on substantial things. But don't worry, we'll come to style
> later:-)
That really seems to be your (all reviewers) favorite issue ;-)
>>+{
>>+ /** an unchagned item */
>>+ svn_diff_preview_kind_unchanged,
>>+
>
> This means no content changes, right?
Yes.
>>+ * @since New in 1.3.
>>+ */
>>+typedef struct svn_diff_preview_t
>>+{
>>+ /** file or dir */
>>+ svn_node_kind_t node_kind;
>>+
>>+ /** item paths, relative to the old diff target and new diff target */
>>+ const char* path_old;
>>+ const char* path_new;
>>+
>
>
> Uh, are you not talking about a single path (I mean the part reaative
> to the two targets)?
To run a "real" diff on the file i need the second path if the file was
renamed.
>>+/* Create an editor for a repository diff preview, i.e. comparing one
>>+ * repository version against the other and only providing information
>>+ * about the changed items without the text delta.
>>+ *
>>+ * @preview_func is called with @a preview_baton as parameter by the
>>+ * created svn_delta_editor_t for each changed item.
>>+ *
>>+ * See svn_client__get_diff_editor for a description of the other
>>+ * parameters.
>>+ */
>>+svn_error_t *
>>+svn_client__get_diff_preview_editor (const char *target,
>>+ svn_wc_adm_access_t *adm_access,
>
>
> Is this...
>
>
>>+ svn_diff_preview_func_t preview_func,
>>+ void *preview_baton,
>>+ svn_boolean_t recurse,
>>+ svn_boolean_t dry_run,
>
>
> and this argument used? (Maybe I get an answer below, but at least
> dry_run only applies to merge AFAIK.)
I basically copied it from the declaration of the diff editor..
I don't remember using them, so i think your are right.
>>+/* An editor function. The root of the comparison hierarchy */
>>+static svn_error_t *
>>+open_root (void *edit_baton,
>>+ svn_revnum_t base_revision,
>>+ apr_pool_t *pool,
>>+ void **root_baton)
>>+{
>>+ struct preview_baton *nb = apr_palloc (pool, sizeof (*nb));
>>+ svn_diff_preview_t *diff = apr_palloc (pool, sizeof (*diff));
>>+
>>+ diff->node_kind = svn_node_dir;
>>+ diff->preview_kind = svn_diff_preview_kind_unchanged;
>>+ diff->path_old = NULL;
>>+ diff->path_new = NULL;
>
>
> Shouldn't this be the empty string?
>
>
>>+ diff->props_changed = FALSE;
>>+
>>+ nb->edit_baton = edit_baton;
>>+ nb->preview = diff;
>>+ nb->path = NULL;
>
>
> And this?
>
>[..]
>>+/* An editor function. */
>>+static svn_error_t *
>>+close_directory (void *dir_baton,
>>+ apr_pool_t *pool)
>>+{
>>+ struct preview_baton *pb = dir_baton;
>>+ svn_diff_preview_t *diff = pb->preview;
>>+ struct preview_edit_baton *eb = pb->edit_baton;
>>+
>>+ if (!diff->path_old) /* ignore empty root */
>>+ return SVN_NO_ERROR;
>>+
>
>
> Didn't open_directory set path_old to NULL? And why ignore the root?
> What happens if the root of th edit has property mods? (I haven't
> tested, so I may be missing something.)
Correct, it would miss property changes on the root.
Using the empty string for root will remove the wrong cehck and the
lost property info for root.
My code in subcommander doesn't handle the NULL path that is set for
the root. So I added the check, ups.. ;)
> Thanks,
> //Peter
Thanks for your nice review, it has a lot of info for the next version
of the patch :) And thanks for keeping the focus on the logic of the
code and not on the style issues (for now).
--
Martin
Subcommander, http://subcommander.tigris.org
a cross platform Win32/Unix/MacOSX subversion gui client & diff/merge tool.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] V2, diff preview without text deltas
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Fri, 12 Aug 2005, Martin Hauner wrote:
> attached is a second version of a patch for the diff preview api call
> (which the gui clients like to have) that doesn't produce text deltas.
>
> Since the first version is a couple of weeks old i repeat a few notes:
>
> * this is an api only patch. For easier review i have created a small
> patch to run the preview from the command line.
>
This is good for the moment, but later I guess we want to be able to test
this API in our test suite.
> * there is a lot of code duplication between the diff and the diff
> preview code that i want to address in another patch if the preview
> stuff makes it into the repository.
>
OK. Ass you will discover, I didn't touch that part at all. We could do
this in the order you propose, but I'm not very fond of it. but I also
understand that you don't want to waste your time on lots of refactoring
that might be useless.
OK, let's make a deal. I promise to do what I can together with you to
get this into the repository (if no other full committer objects, of
course). Would you then like to do the refactoring work first, which
would probably make this patch significantly smaller?
> * the preview handles only the repository <-> repoditory case for now.
> the other combinations are another thing i would like to address in
> different patches.
>
>
So, currently BASE and WORKING revisions are not supported. OK.
> Unsolved problem
> ----------------
>
Already discussed.
> minor version compatibility:
> ----------------------------
>
> The preview patch adds an optional boolean to do_diff.
>
> To handle minor version compatibility i modified vparse_tuple to
> handle optional bool values simply by not returning an error. This
> requires to initialize the optional bool before calling vparse_tuple.
>
For revving get_commit_editor in 1.2, I solved this problem by checking
the number of parameters. I don't know if that's cleaner than the
inconsistency that your approach introduces. >
>
> preview or summarize:
> ---------------------
>
> How should it be named? preview or summarize?
> I named it preview after the inital confusion with #issue 2015
> (summarize) because my patch doesn't really solve that issue.
> But i have to problem with changing it again to summarize.
>
>
>
Doesn't it solve #2015 without the -v option, or do I miss something? I
like summarize more. (I mean make it possible to solve, since this patch
doesn't touch the cmdline client, of course.)
In the review below, I'm not picking on style, but am instead
concentrating on substantial things. But don't worry, we'll come to style
later:-)
> Index: subversion/libsvn_ra/ra_loader.c
> ===================================================================
> svn_error_t *svn_ra_do_diff (svn_ra_session_t *session,
> const svn_ra_reporter2_t **reporter,
> void **report_baton,
> @@ -423,7 +442,8 @@
> {
> return session->vtable->do_diff (session, reporter, report_baton, revision,
> diff_target, recurse, ignore_ancestry,
> - versus_url, diff_editor, diff_baton, pool);
> + TRUE, versus_url, diff_editor, diff_baton,
> + pool);
> }
>
When revving a function, implement the old API in terms of the new
one, literally even in a trivial case like this. Then we don't
have to touch it in the next revving round.
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 15693)
> +++ subversion/include/svn_client.h (working copy)
> @@ -500,6 +500,64 @@
> svn_client_commit_info2_t *
> svn_client_create_commit_info (apr_pool_t *pool);
>
> +
> +/** the difference type in an svn_diff_preview_t structure.
> + *
> + * @since New in 1.3.
> + */
> +typedef enum svn_diff_preview_kind
We use the _t suffix even on strct tags.
> +{
> + /** an unchagned item */
> + svn_diff_preview_kind_unchanged,
> +
This means no content changes, right?
> + /** an added item */
> + svn_diff_preview_kind_added,
> +
> + /** a modified item */
> + svn_diff_preview_kind_modified,
> +
> + /** a deleted item */
> + svn_diff_preview_kind_deleted
> +
> +} svn_diff_preview_kind_t;
> +
> +
> +/** a struct that describes the diff of an item. Passed to
> + * svn_diff_preview_func_t.
> + *
Please add a @note that this struct might be extended in future
versions. (See svn_wc_status2_t for an example.) I'm saying this now,
since we tend to forget about that and have to rev the APIs later for
backwards compatibility.
> + * @since New in 1.3.
> + */
> +typedef struct svn_diff_preview_t
> +{
> + /** file or dir */
> + svn_node_kind_t node_kind;
> +
> + /** item paths, relative to the old diff target and new diff target */
> + const char* path_old;
> + const char* path_new;
> +
Uh, are you not talking about a single path (I mean the part reaative
to the two targets)?
> +typedef svn_error_t *
> +(*svn_diff_preview_func_t) (void *preview_baton,
> + svn_diff_preview_t *diff);
> +
Please add a pool argument to make issue #1881 happier...
> +/**
> + * Produce a diff preview which lists the changed items between
> + * @a path1/@a revision1 and @a path2/@a revision2 without creating the
> + * delta. @a path1 and @a path2 can be either working-copy paths or URLs.
> + *
Maybe mention the limitation that the revisions might not be WORKING.
> + * Calls @a preview_func with @a preview_baton for each difference with
> + * a @c svn_diff_preview_t * structure describing the difference.
> + *
> + * See svn_client_diff3 for a description of the other parameters.
> + *
> + * @since New in 1.3.
> + */
> +svn_error_t *
> +svn_client_diff_preview (const char *path1,
> + const svn_opt_revision_t *revision1,
> + const char *path2,
> + const svn_opt_revision_t *revision2,
> + svn_boolean_t recurse,
> + svn_boolean_t ignore_ancestry,
> + svn_boolean_t no_diff_deleted,
no_diff_deleted is used to not show the contents of deleted
files. Seems not applicable here. Same for the _peg version below.
> Index: subversion/include/svn_ra.h
> ===================================================================
> --- subversion/include/svn_ra.h (revision 15693)
> +++ subversion/include/svn_ra.h (working copy)
> @@ -761,10 +761,31 @@
> * finishing the report, and may not perform any RA operations using
> * @a session from within the editing operations of @a diff_editor.
> *
> + * @a text_deltas instructs the driver of the @a diff_editor to enable
> + * the generation of text deltas.
> + *
Might be good to mention that if text_delts is FALSE, then
apply_text_delta wil be called with an empty delta to the provided
window handler.
> +/* Create an editor for a repository diff preview, i.e. comparing one
> + * repository version against the other and only providing information
> + * about the changed items without the text delta.
> + *
> + * @preview_func is called with @a preview_baton as parameter by the
> + * created svn_delta_editor_t for each changed item.
> + *
> + * See svn_client__get_diff_editor for a description of the other
> + * parameters.
> + */
> +svn_error_t *
> +svn_client__get_diff_preview_editor (const char *target,
> + svn_wc_adm_access_t *adm_access,
Is this...
> + svn_diff_preview_func_t preview_func,
> + void *preview_baton,
> + svn_boolean_t recurse,
> + svn_boolean_t dry_run,
and this argument used? (Maybe I get an answer below, but at least
dry_run only applies to merge AFAIK.)
> Index: subversion/libsvn_client/diff.c
> ===================================================================
> --- subversion/libsvn_client/diff.c (revision 15693)
> +++ subversion/libsvn_client/diff.c (working copy)
As you say, here we have a lot of duplicate code. I'm not looking into
it in detail (yet).
> Index: subversion/libsvn_client/repos_diff_preview.c
> ===================================================================
> --- subversion/libsvn_client/repos_diff_preview.c (revision 0)
> +++ subversion/libsvn_client/repos_diff_preview.c (revision 0)
> +/* An editor function. The root of the comparison hierarchy */
> +static svn_error_t *
> +set_target_revision (void *edit_baton,
> + svn_revnum_t target_revision,
> + apr_pool_t *pool)
> +{
> + return SVN_NO_ERROR;
> +}
> +
You don't need this, since that's what thedefault implementation does.
> +/* An editor function. The root of the comparison hierarchy */
> +static svn_error_t *
> +open_root (void *edit_baton,
> + svn_revnum_t base_revision,
> + apr_pool_t *pool,
> + void **root_baton)
> +{
> + struct preview_baton *nb = apr_palloc (pool, sizeof (*nb));
> + svn_diff_preview_t *diff = apr_palloc (pool, sizeof (*diff));
> +
> + diff->node_kind = svn_node_dir;
> + diff->preview_kind = svn_diff_preview_kind_unchanged;
> + diff->path_old = NULL;
> + diff->path_new = NULL;
Shouldn't this be the empty string?
> + diff->props_changed = FALSE;
> +
> + nb->edit_baton = edit_baton;
> + nb->preview = diff;
> + nb->path = NULL;
And this?
> +/* An editor function. */
> +static svn_error_t *
> +delete_entry (const char *path,
> + svn_revnum_t base_revision,
> + void *parent_baton,
> + apr_pool_t *pool)
> +{
> + struct preview_baton *pb = parent_baton;
> + svn_diff_preview_t *diff = apr_palloc (pool, sizeof (*diff));
> + struct preview_edit_baton *eb = pb->edit_baton;
> +
> + diff->node_kind = svn_node_file;
It could also be a directory. I guess you need to use *another* RA
session to figure out the kind of this path. Good that it is only
delete that require this.
> + diff->preview_kind = svn_diff_preview_kind_deleted;
> + diff->path_old = apr_pstrdup (pool, path);
> + diff->path_new = NULL;
> + diff->props_changed = FALSE;
> +
> + SVN_ERR (eb->preview_func (eb->preview_func_baton,diff));
> +
> + return SVN_NO_ERROR;
> +}
> +
> +/* An editor function. */
> +static svn_error_t *
> +add_directory (const char *path,
> + void *parent_baton,
> + const char *copyfrom_path,
> + svn_revnum_t copyfrom_revision,
> + apr_pool_t *pool,
> + void **child_baton)
> +{
> + struct preview_baton *pb = parent_baton;
> + struct preview_baton *nb = apr_palloc (pool, sizeof (*nb));
> + svn_diff_preview_t *diff = apr_palloc (pool, sizeof (*diff));
> +
> + nb->edit_baton = pb->edit_baton;
> + nb->preview = diff;
> + nb->path = apr_pstrdup (pool, path);
> +
> + diff->node_kind = svn_node_dir;
> + diff->preview_kind = svn_diff_preview_kind_added;
> + diff->path_old = apr_pstrdup (pool, copyfrom_path);
> + diff->path_new = apr_pstrdup (pool, path);
I think the use of path_old and path_new is confusing. OTOH, adding
copyfrom_path and copyfrom_rev to the preview struct might be a good
idea.
> + diff->props_changed = FALSE;
> +
> + *child_baton = nb;
> + return SVN_NO_ERROR;
> +}
> +
> +/* An editor function. */
> +static svn_error_t *
> +open_directory (const char *path,
> + void *parent_baton,
> + svn_revnum_t base_revision,
> + apr_pool_t *pool,
> + void **child_baton)
> +{
> + struct preview_baton *pb = parent_baton;
> + struct preview_baton *nb = apr_palloc (pool, sizeof (*nb));
> + svn_diff_preview_t *diff = apr_palloc (pool, sizeof (*diff));
> +
> + nb->edit_baton = pb->edit_baton;
> + nb->preview = diff;
> + nb->path = apr_pstrdup (pool, path);
> +
> + diff->node_kind = svn_node_dir;
> + diff->preview_kind = svn_diff_preview_kind_modified;
> + diff->path_old = NULL;
> + diff->path_new = apr_pstrdup (pool, path);
> + diff->props_changed = FALSE;
> +
Could the baton creation and preview struct dito be factored out into
a helper function. It seems quite similar in all these editor
functions.
> + *child_baton = nb;
> + return SVN_NO_ERROR;
> +}
> +
> +/* An editor function. */
> +static svn_error_t *
> +change_dir_prop (void *dir_baton,
> + const char *name,
> + const svn_string_t *value,
> + apr_pool_t *pool)
> +{
> + struct preview_baton *pb = dir_baton;
> +
> + if ( svn_property_kind (NULL,name) == svn_prop_regular_kind )
> + {
> + pb->preview->props_changed = TRUE;
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> +/* An editor function. */
> +static svn_error_t *
> +close_directory (void *dir_baton,
> + apr_pool_t *pool)
> +{
> + struct preview_baton *pb = dir_baton;
> + svn_diff_preview_t *diff = pb->preview;
> + struct preview_edit_baton *eb = pb->edit_baton;
> +
> + if (!diff->path_old) /* ignore empty root */
> + return SVN_NO_ERROR;
> +
Didn't open_directory set path_old to NULL? And why ignore the root?
What happens if the root of th edit has property mods? (I haven't
tested, so I may be missing something.)
> + if (diff->preview_kind != svn_diff_preview_kind_unchanged
> + || diff->props_changed)
> + {
> + SVN_ERR (eb->preview_func (eb->preview_func_baton,diff));
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> +/* An editor function. */
> +static svn_error_t *
> +absent_directory (const char *path,
> + void *parent_baton,
> + apr_pool_t *pool)
> +{
> + return SVN_NO_ERROR;
> +}
> +
> +/* An editor function. */
> +static svn_error_t *
> +add_file (const char *path,
> + void *parent_baton,
> + const char *copyfrom_path,
> + svn_revnum_t copyfrom_revision,
> + apr_pool_t *pool,
> + void **file_baton)
> +{
> + struct preview_baton *pb = parent_baton;
> + struct preview_baton *nb = apr_pcalloc (pool, sizeof (*nb));
> + svn_diff_preview_t *diff = apr_pcalloc (pool, sizeof (*diff));
> +
> + nb->edit_baton = pb->edit_baton;
> + nb->preview = diff;
> + nb->path = apr_pstrdup (pool, path);
> +
> + diff->node_kind = svn_node_file;
> + diff->preview_kind = svn_diff_preview_kind_added;
> + diff->path_old = apr_pstrdup (pool, copyfrom_path);
Same about path_old/path_new and copyfrom information here.
> + diff->path_new = apr_pstrdup (pool, path);
> + diff->props_changed = FALSE;
> +
> + *file_baton = nb;
> + return SVN_NO_ERROR;
> +}
> +
> +/* An editor function. */
> +static svn_error_t *
> +open_file (const char *path,
> + void *parent_baton,
> + svn_revnum_t base_revision,
> + apr_pool_t *pool,
> + void **file_baton)
> +{
> + /*
> + This code is also called for files that are not changed.
> +
> + It looks like this has something todo with merging.
> +
> + After creating a branch from trunk and merging several files
> + from trunk to that branch (making the files on the branch and
> + trunk equal again) they appear in a diff preview between the
> + branch and trunk.
> +
> + Is this supposed to happen? And if so how do i recognize
> + this situation so i can filter those files?
> + */
Just a tip for the future: When we add a comment regarding some issue
or incompleteness with the code like the above, we use ### markers, so
that such comments are easier to find in the future.
> + struct preview_baton *pb = parent_baton;
> + struct preview_baton *nb = apr_palloc (pool, sizeof (*nb));
> + svn_diff_preview_t *diff = apr_palloc (pool, sizeof (*diff));
> +
> + nb->edit_baton = pb->edit_baton;
> + nb->preview = diff;
> + nb->path = apr_pstrdup (pool, path);
> +
> + diff->node_kind = svn_node_file;
> + diff->preview_kind = svn_diff_preview_kind_modified;
As we discussed earlier in this thread, you should assume that the
contents are not modified here.
> +/* An editor function. Do the work of applying the text delta. */
> +static svn_error_t *
> +window_handler (svn_txdelta_window_t *window,
> + void *window_baton)
This function is not used.
> +/* An editor function. */
> +static svn_error_t *
> +apply_textdelta (void *file_baton,
> + const char *base_checksum,
> + apr_pool_t *pool,
> + svn_txdelta_window_handler_t *handler,
> + void **handler_baton)
> +{
> + *handler = svn_delta_noop_window_handler;
> + *handler_baton = NULL;
> +
Here you note that the contents were modified.
> + return SVN_NO_ERROR;
> +}
> +
> +/* An editor function. */
> +static svn_error_t *
> +change_file_prop (void *file_baton,
> + const char *name,
> + const svn_string_t *value,
> + apr_pool_t *pool)
> +{
> + struct preview_baton *pb = file_baton;
> +
> + if ( svn_property_kind (NULL,name) == svn_prop_regular_kind )
> + {
> + pb->preview->props_changed = TRUE;
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
This function feels very similar to change_dir_prop. Since you use
the same kind of baton for files and dirs, you could have just one
function.
> +/* An editor function. */
> +static svn_error_t *
> +absent_file (const char *path,
> + void *parent_baton,
> + apr_pool_t *pool)
> +{
> + struct preview_dir_baton *pb = parent_baton;
> +
> + return SVN_NO_ERROR;
> +}
> +
This is similar to absent_directory and you shouldn't eed either of
them since the default editor already provide dummy implementations.
> +/* An editor function. */
> +static svn_error_t *
> +close_edit (void *edit_baton,
> + apr_pool_t *pool)
> +{
> + return SVN_NO_ERROR;
> +}
> +
You don't need to provide this.
> +
> +
> +/* Create a repository diff preview editor and baton. */
> +svn_error_t *
> +svn_client__get_diff_preview_editor (const char *target,
> + svn_wc_adm_access_t *adm_access,
> + svn_diff_preview_func_t preview_func,
> + void *preview_baton,
> + svn_boolean_t recurse,
> + svn_boolean_t dry_run,
> + svn_ra_session_t *ra_session,
> + svn_revnum_t revision,
> + svn_wc_notify_func2_t notify_func,
> + void *notify_baton,
> + svn_cancel_func_t cancel_func,
> + void *cancel_baton,
> + const svn_delta_editor_t **editor,
> + void **edit_baton,
> + apr_pool_t *pool)
> +{
> + apr_pool_t *subpool = svn_pool_create (pool);
> +
You don't need a subpool in this function. See about pool usage in
the hacking document.
> + svn_delta_editor_t *tree_editor = svn_delta_default_editor (subpool);
> + struct preview_edit_baton *eb = apr_palloc (subpool, sizeof (*eb));
> +
> + eb->preview_func = preview_func;
> + eb->preview_func_baton = preview_baton;
> +
> + tree_editor->set_target_revision = set_target_revision;
> + tree_editor->open_root = open_root;
> +
> + tree_editor->delete_entry = delete_entry;
> + tree_editor->add_directory = add_directory;
> + tree_editor->open_directory = open_directory;
> + tree_editor->change_dir_prop = change_dir_prop;
> + tree_editor->close_directory = close_directory;
> + tree_editor->absent_directory = absent_directory;
> +
> + tree_editor->add_file = add_file;
> + tree_editor->open_file = open_file;
> + tree_editor->apply_textdelta = apply_textdelta;
> + tree_editor->change_file_prop = change_file_prop;
> + tree_editor->close_file = close_file;
> + tree_editor->absent_file = absent_file;
> +
> + tree_editor->close_edit = close_edit;
> +
> + SVN_ERR (svn_delta_get_cancellation_editor
> + (cancel_func, cancel_baton, tree_editor, eb, editor, edit_baton,
> + pool));
> +
> + return SVN_NO_ERROR;
> +}
There are some unused arguments that you could get rid of here.
> Index: subversion/libsvn_ra_svn/client.c
> ===================================================================
> --- subversion/libsvn_ra_svn/client.c (revision 15693)
> +++ subversion/libsvn_ra_svn/client.c (working copy)
> @@ -1050,6 +1050,7 @@
> svn_revnum_t rev, const char *target,
> svn_boolean_t recurse,
> svn_boolean_t ignore_ancestry,
> + svn_boolean_t text_deltas,
> const char *versus_url,
> const svn_delta_editor_t *diff_editor,
> void *diff_baton, apr_pool_t *pool)
> @@ -1058,8 +1059,9 @@
> svn_ra_svn_conn_t *conn = sess_baton->conn;
>
> /* Tell the server we want to start a diff. */
> - SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "diff", "(?r)cbbc", rev, target,
> - recurse, ignore_ancestry, versus_url));
> + SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "diff", "(?r)cbbcb", rev,
> + target, recurse, ignore_ancestry,
> + versus_url, text_deltas));
> SVN_ERR(handle_auth_request(sess_baton, pool));
>
This means that old servers will send us full text deltas anyway. I
think that's fine, but we should document in the svn_ra_diff2
docstring that this might happen.
> /* Fetch a reporter for the caller to drive. The reporter will drive
> Index: subversion/libsvn_ra_svn/protocol
> ===================================================================
> --- subversion/libsvn_ra_svn/protocol (revision 15693)
> +++ subversion/libsvn_ra_svn/protocol (working copy)
> @@ -292,7 +292,8 @@
> response: ( )
>
> diff
> - params: ( [ rev:number ] target:string recurse:bool url:string )
> + params: ( [ rev:number ] target:string recurse:bool ignore-ancestry:bool
> + url:string ? text-deltas:bool )
Thanks for caring about the protocol document. You did an unrelated
fix to the prototype, which I committed in r15729. Thanks for that.
> Client switches to report command set.
> Upon finish-report, server sends auth-request.
> After auth exchange completes, server switches to editor command set.
> Index: subversion/libsvn_ra_svn/marshal.c
> ===================================================================
> --- subversion/libsvn_ra_svn/marshal.c (revision 15693)
> +++ subversion/libsvn_ra_svn/marshal.c (working copy)
> @@ -691,6 +691,10 @@
> case 'n':
> *va_arg(*ap, apr_uint64_t *) = SVN_RA_SVN_UNSPECIFIED_NUMBER;
> break;
> + case 'b':
> + /* an optional bool value is supposed to have a default value
> + already set, so simply skip over it. */
> + break;
> case '(':
> list_level++;
> break;
As I said before, I'm not sure about this. But if you do this (and we
accept it), you should update the docstring for svn_ra_svn_parse_tuple
in svn_ra_svn.h.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] V2, diff preview without text deltas
Posted by Martin Hauner <ma...@gmx.net>.
steveking wrote:
> Martin Hauner wrote:
>
>> +/** a struct that describes the diff of an item. Passed to
>> + * svn_diff_preview_func_t.
>> + *
>> + * @since New in 1.3.
>> + */
>> +typedef struct svn_diff_preview_t
>> +{
>> + /** file or dir */
>> + svn_node_kind_t node_kind;
>> +
>> + /** item paths, relative to the old diff target and new diff target */
>> + const char* path_old;
>> + const char* path_new;
>> +
>> + /** change kind: unchanged, added, modified or deleted */
>> + svn_diff_preview_kind_t preview_kind;
>> +
>> + /** properties changed? */
>> + svn_boolean_t props_changed;
>> +
>> +} svn_diff_preview_t;
>
>
> Question: how would I find out if *only* the properties have changed and
> not the file contents too?
> Wouldn't it be better to have two booleans there, one indicating a
> change in properties and one indicating a change in text content?
preview_kind set to unchanged
props_changed set to true
But i like the idea with the second bool. It would be more consistent
with the way it is handle by svn status and i guess it is easier to
understand too.
I will change it in the next version.
--
Martin
Subcommander, http://subcommander.tigris.org
a cross platform Win32/Unix/MacOSX subversion gui client & diff/merge tool.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] V2, diff preview without text deltas
Posted by steveking <st...@gmx.ch>.
Martin Hauner wrote:
> +/** a struct that describes the diff of an item. Passed to
> + * svn_diff_preview_func_t.
> + *
> + * @since New in 1.3.
> + */
> +typedef struct svn_diff_preview_t
> +{
> + /** file or dir */
> + svn_node_kind_t node_kind;
> +
> + /** item paths, relative to the old diff target and new diff target */
> + const char* path_old;
> + const char* path_new;
> +
> + /** change kind: unchanged, added, modified or deleted */
> + svn_diff_preview_kind_t preview_kind;
> +
> + /** properties changed? */
> + svn_boolean_t props_changed;
> +
> +} svn_diff_preview_t;
Question: how would I find out if *only* the properties have changed and
not the file contents too?
Wouldn't it be better to have two booleans there, one indicating a
change in properties and one indicating a change in text content?
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org