Re: [PATCH] V2, diff preview without text deltas

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