On Mon, Jul 26, 2010 at 01:07:14PM +0200, Stefan Sperling wrote: > On Mon, Jul 26, 2010 at 09:59:01AM +0200, Daniel Näslund wrote: > > Index: subversion/libsvn_client/patch.c > > =================================================================== > > --- subversion/libsvn_client/patch.c (revision 979179) > > +++ subversion/libsvn_client/patch.c (arbetskopia) > > @@ -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.
Ok, I agree with you on the badness of using examples, but I do think it would benefit the reader to have a clear definition of when a target should be skipped. > > * 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. Either way is fine by me. > > 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. */ Yeah, that's better. > > + 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; Oups. > 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. Ouch, good catch. I was sure that svn_diff_parse_next_patch() set the patch to NULL if it didn't contain a patch. That wasn't the case so I need to check for (!A && B). > > 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. Thanks for your review, Daniel