On Mon, Jul 26, 2010 at 09:59:01AM +0200, Daniel Näslund wrote:
> [[[
> Enable the patch code to use dirs as targets if we only have property
> changes.
>
> * subversion/libsvn_client/patch.c
> (patch_target_t): Add fields 'has_text_changes' and 'has_prop_changes'
> to be able to decide if we should install tmp files for text and/or
> props. It's needed since we may have a dir as a target for a
> property. If we'd try to copy the tmp file for the text changes onto
> a dir we get an error.
> (resolve_target_path): Add new parameter 'only_prop_changes' to help
> us determine the case when a dir is a valid target.
> (init_patch_target): Update caller of resolve_target_path().
> (apply_hunk): Record if we've done any changes to props or text
> content.
> (apply_patches): Only call install_patched_target() if we have changed
> the text contents.
> ]]]
>
> Thanks,
> Daniel
> Index: subversion/libsvn_client/patch.c
> ===================================================================
> --- subversion/libsvn_client/patch.c (revision 979179)
> +++ subversion/libsvn_client/patch.c (arbetskopia)
> @@ -186,6 +186,12 @@ typedef struct patch_target_t {
> /* True if the target has the executable bit set. */
> svn_boolean_t executable;
>
> + /* True if the patch changes the text of the target */
> + svn_boolean_t has_text_changes;
> +
> + /* True if the patch changes any of the properties of the target */
> + svn_boolean_t has_prop_changes;
> +
> /* All the information that is specifict to the content of the target. */
> target_content_info_t *content_info;
>
> @@ -303,6 +309,9 @@ obtain_eol_and_keywords_for_file(apr_hash_t **keyw
> * If possible, determine TARGET->WC_PATH, TARGET->ABS_PATH, TARGET->KIND,
> * TARGET->ADDED, and TARGET->PARENT_DIR_EXISTS.
> * Indicate in TARGET->SKIPPED whether the target should be skipped.
> + * The target should be skipped if the versioned path does not exist or does
> + * not match our expected type, e.g. it should be a file unless we're
> + * dealing with a patch that has ONLY_PROP_CHANGES.
I'd say don't put the "e.g. it should ..." part in the docstring of the
function. It has nothing to do with the interface the caller uses.
The caller does not care how this is done internally.
In general, providing an example in this way doesn't really help,
because it makes the reader ask "ok, fine, so you've told me one
of the rules... what are the other rules?" So it's better to just
put explicit explanations of internal workings of the function into
comments inside the function's body.
> * STRIP_COUNT specifies the number of leading path components
> * which should be stripped from target paths in the patch.
> * Use RESULT_POOL for allocations of fields in TARGET.
> @@ -312,6 +321,7 @@ resolve_target_path(patch_target_t *target,
> const char *path_from_patchfile,
> const char *local_abspath,
> int strip_count,
> + svn_boolean_t only_prop_changes,
I'd prefer call this prop_changes_only everywhere,
but that's a matter of taste.
> svn_wc_context_t *wc_ctx,
> apr_pool_t *result_pool,
> apr_pool_t *scratch_pool)
> @@ -341,6 +351,8 @@ resolve_target_path(patch_target_t *target,
> {
> target->local_relpath = svn_dirent_is_child(local_abspath,
> stripped_path,
> result_pool);
> +
> + /* ### We need to allow setting props on the wc root dir */
> if (! target->local_relpath)
> {
> /* The target path is either outside of the working copy
> @@ -403,7 +415,9 @@ resolve_target_path(patch_target_t *target,
> }
> SVN_ERR(svn_wc_read_kind(&target->db_kind, wc_ctx, target->local_abspath,
> FALSE, scratch_pool));
> - if (target->db_kind == svn_node_dir)
> +
> + /* We allow properties to have dirs as targets */
I'd suggest the following comment instead:
/* If the target is a version directory not missing from disk,
* and there are only property changes in the patch, we accept
* a directory target. Else, we skip directories. */
> + if (target->db_kind == svn_node_dir && ! only_prop_changes)
> {
> /* ### We cannot yet replace a locally deleted dir with a file,
> * ### but some day we might want to allow it. */
> @@ -502,6 +516,7 @@ init_patch_target(patch_target_t **patch_target,
> {
> patch_target_t *target;
> target_content_info_t *content_info;
> + svn_boolean_t only_prop_changes = patch->hunks->nelts == 0 ? TRUE : FALSE;
Don't use ? TRUE : FALSE. It's redundant, and AFAIK e.g. Julian has
been trying to get rid of these everywhere. This is enough:
svn_boolean_t only_prop_changes = patch->hunks->nelts == 0;
Is it a given that if text changes are empty, we only have property changes?
Might be better to also make it check for the existence of property hunks.
In case we add something else later. Dunno. It just seems more robust to
write if (!A && B) instead of just (!A) if you also rely on B to be TRUE.
>
> content_info = apr_pcalloc(result_pool, sizeof(*content_info));
>
> @@ -525,8 +540,8 @@ init_patch_target(patch_target_t **patch_target,
> target->pool = result_pool;
>
> SVN_ERR(resolve_target_path(target, patch->new_filename,
> - base_dir, strip_count, wc_ctx,
> - result_pool, scratch_pool));
> + base_dir, strip_count, only_prop_changes,
> + wc_ctx, result_pool, scratch_pool));
> if (! target->skipped)
> {
> const char *diff_header;
> @@ -1350,6 +1365,11 @@ apply_hunk(patch_target_t *target, target_content_
> while (! eof);
> svn_pool_destroy(iterpool);
>
> + if (is_prop_hunk)
> + target->has_prop_changes = TRUE;
> + else
> + target->has_text_changes = TRUE;
> +
> return SVN_NO_ERROR;
> }
>
> @@ -2328,9 +2348,14 @@ apply_patches(void *baton,
> patch_target_info_t *) = target_info;
>
> if (! target->skipped)
> - SVN_ERR(install_patched_target(target, btn->abs_wc_path,
> - btn->ctx, btn->dry_run,
> - iterpool));
> + {
> + if (target->has_text_changes)
> + SVN_ERR(install_patched_target(target, btn->abs_wc_path,
> + btn->ctx, btn->dry_run,
> + iterpool));
> + if (target->has_prop_changes)
> + ; /* ### Here we'll going to call
> install_patched_prop_target() */
> + }
> SVN_ERR(send_patch_notification(target, btn->ctx, iterpool));
> }
Looks fine to me otherwise.
Stefan
