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
Received on 2010-07-26 13:28:54 CEST