malcolm@tigris.org writes:
> Author: malcolm
> Date: Mon Jan 16 15:34:44 2006
> New Revision: 18113
>
> Modified:
> trunk/subversion/libsvn_wc/diff.c
>
> Log:
Your log messages are written at the wrong level of detail, they
contain far too much unnecessary or duplicate information. Please
compare your log messages with those written by the other developers
and it should be obvious that yours are "different".
> Continue work on fixing diff_tests 32 (repos-wc diff showing added
> entries with props).
That bit is OK.
> When performing a repos->wc diff, if the BASE->repos diff reported by the
> server indicates that a directory has been deleted (and so therefore we
> must report the addition of a directory currently in the wc), reconstruct
> the propchanges necessary to create the current WORKING properties,
> and pass them to the dir_props_changed() callback.
That bit is most likely redundant, since you repeat most of it lower
down.
> Like r18098, this is incorrect for repos->BASE diffs, but the whole
> handling of directories reported as deleted during repos->BASE diffs is
> currently disabled due to an earlier check in directory_elements_diff()
> (the function responsible for reporting both local BASE->WORKING diffs
> and for faking added directories for repos->WORKING diffs).
That bit looks like it should be a comment in the code. Bear in mind
that developers reading the code (i.e. reading HEAD) should be able to
understand the code without referring to the log messages.
> [The diff is easier to follow with 'diff -w'; we're just adding a new
> case before the 'if (modified)' condition, not changing it.]
You have put that bit in brackets but you should probably just drop it
altogether.
> * subversion/libsvn_wc/diff.c
Now we come to the meat of the change. It's OK, but if it were my log
message it would be briefer.
> (directory_elements_diff): Before checking whether a directory has local
> property modifications,
I can see from the code that it happens "before".
> first check whether we are faking the addition
> of the directory (true only if we're doing a repos->WORKING diff,
> and the directory doesn't exist in the repos).
Do you really need the bit in the brackets?
> If so, fetch the
> current WORKING properties and determine the changes necessary to
> create them from scratch. Report the changes (together with an empty
> property hash) during the svn_wc_diff_callbacks2_t dir_props_changed()
> callback.
I'd probably just write something like "For added directories
determine the prop changes to create WORKING props from scratch".
> Modified: trunk/subversion/libsvn_wc/diff.c
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/libsvn_wc/diff.c?rev=18113&p1=trunk/subversion/libsvn_wc/diff.c&p2=trunk/subversion/libsvn_wc/diff.c&r1=18112&r2=18113
> ==============================================================================
> --- trunk/subversion/libsvn_wc/diff.c (original)
> +++ trunk/subversion/libsvn_wc/diff.c Mon Jan 16 15:34:44 2006
> @@ -637,24 +637,49 @@
> /* Check for property mods on this directory. */
> if (!in_anchor_not_target)
> {
> - svn_boolean_t modified;
> -
> - SVN_ERR (svn_wc_props_modified_p (&modified, dir_baton->path, adm_access,
> - dir_baton->pool));
> - if (modified)
> + if (added)
> {
> + /* We need to simulate an addition of the WORKING properties. */
> + apr_hash_t *emptyprops = apr_hash_make (dir_baton->pool),
> + *workingprops = NULL;
> apr_array_header_t *propchanges;
> - apr_hash_t *baseprops;
>
> - SVN_ERR (svn_wc_get_prop_diffs (&propchanges, &baseprops,
> - dir_baton->path, adm_access,
> - dir_baton->pool));
> + SVN_ERR (svn_wc_prop_list (&workingprops,
> + dir_baton->path, adm_access,
> + dir_baton->pool));
> +
> + /* Calculate the propchanges necessary to create workingprops. */
That comment is really just a description of svn_prop_diffs -- it's OK
but it's not really adding very much information.
> + SVN_ERR (svn_prop_diffs (&propchanges,
> + workingprops, emptyprops, dir_baton->pool));
>
> SVN_ERR (dir_baton->edit_baton->callbacks->dir_props_changed
> (adm_access, NULL,
> dir_baton->path,
> - propchanges, baseprops,
> + propchanges, emptyprops,
> dir_baton->edit_baton->callback_baton));
> + }
> + else
> + {
> + svn_boolean_t modified;
> +
> + SVN_ERR (svn_wc_props_modified_p (&modified,
> + dir_baton->path, adm_access,
> + dir_baton->pool));
> + if (modified)
> + {
> + apr_array_header_t *propchanges;
> + apr_hash_t *baseprops;
> +
> + SVN_ERR (svn_wc_get_prop_diffs (&propchanges, &baseprops,
> + dir_baton->path, adm_access,
> + dir_baton->pool));
> +
> + SVN_ERR (dir_baton->edit_baton->callbacks->dir_props_changed
> + (adm_access, NULL,
> + dir_baton->path,
> + propchanges, baseprops,
> + dir_baton->edit_baton->callback_baton));
> + }
> }
> }
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Jan 17 00:37:23 2006