Re: [PATCH] Issue #2064

"Peter N. Lundblad" <peter@famlundblad.se> writes:

> Hi,
>
> Here comes a little patch for #2064, i.e. merge doesn't handle symlinks.
> It passes tests and so on and it solves the issue. The problem is that it
> isn't really complete. Keyword substitution only takes effect if the file
> contents are changed (or when the file is added). We have this bug in
> other places as well. What do you think? Should I commit this anyway?

I think it's reasonable to commit incremental improvements if they
don't cause known regressions.

> +static svn_error_t *
> +diff_props_changed (svn_wc_adm_access_t *adm_access,
> + svn_wc_notify_state_t *state,
> + const char *path,
> + const apr_array_header_t *propchanges,
> + apr_hash_t *original_props,
> + void *diff_baton)
> +{
> + struct diff_cmd_baton *diff_cmd_baton = diff_baton;
> + apr_array_header_t *props;
> + apr_pool_t *subpool = svn_pool_create (diff_cmd_baton->pool);

Normally subpools are used where there is iteration.

> +
> + SVN_ERR (svn_categorize_props (propchanges, NULL, NULL, &props, subpool));
> +
> + if (props->nelts > 0)
> + SVN_ERR (display_prop_diffs (props, original_props, path,
> + diff_cmd_baton->outfile, subpool));
> +
> + if (state)
> + *state = svn_wc_notify_state_unknown;
> +
> + svn_pool_destroy (subpool);
> + return SVN_NO_ERROR;
> +}
> +
> return SVN_NO_ERROR;
> }
  
> +static svn_error_t *
> +diff_file_changed (svn_wc_adm_access_t *adm_access,
> + svn_wc_notify_state_t *content_state,
> + svn_wc_notify_state_t *prop_state,
> + const char *path,
> + const char *tmpfile1,
> + const char *tmpfile2,
> + svn_revnum_t rev1,
> + svn_revnum_t rev2,
> + const char *mimetype1,
> + const char *mimetype2,
> + const apr_array_header_t *prop_changes,
> + apr_hash_t *original_props,
> + void *diff_baton)
> +{
> + if (tmpfile1)
> + SVN_ERR (diff_content_changed (path,
> + tmpfile1, tmpfile2, rev1, rev2,

Strange indentation.

> + mimetype1, mimetype2, diff_baton));
> + if (prop_changes->nelts > 0)
> + SVN_ERR (diff_props_changed (adm_access, prop_state, path, prop_changes,
> + original_props, diff_baton));
> + if (content_state)
> + *content_state = svn_wc_notify_state_unknown;
> + if (prop_state)
> + *prop_state = svn_wc_notify_state_unknown;
> + return SVN_NO_ERROR;
> +}

> -static svn_error_t *
> -diff_props_changed (svn_wc_adm_access_t *adm_access,
> - svn_wc_notify_state_t *state,
> - const char *path,
> - const apr_array_header_t *propchanges,
> - apr_hash_t *original_props,
> - void *diff_baton)
> -{
> - struct diff_cmd_baton *diff_cmd_baton = diff_baton;
> - apr_array_header_t *props;
> - apr_pool_t *subpool = svn_pool_create (diff_cmd_baton->pool);

Ah, I see you copied the subpool stuff. It's still odd.

>
> - SVN_ERR (svn_categorize_props (propchanges, NULL, NULL, &props, subpool));
> -
> - if (props->nelts > 0)
> - SVN_ERR (display_prop_diffs (props, original_props, path,
> - diff_cmd_baton->outfile, subpool));
> -
> - if (state)
> - *state = svn_wc_notify_state_unknown;
> -
> - svn_pool_destroy (subpool);
> - return SVN_NO_ERROR;
> -}
> -
> -

> +static svn_error_t *
> +merge_props_changed (svn_wc_adm_access_t *adm_access,
> + svn_wc_notify_state_t *state,
> + const char *path,
> + const apr_array_header_t *propchanges,
> + apr_hash_t *original_props,
> + void *baton)
> +{
> + apr_array_header_t *props;
> + struct merge_cmd_baton *merge_b = baton;
> + apr_pool_t *subpool = svn_pool_create (merge_b->pool);
> + svn_error_t *err;

Another odd subpool.

>
> + SVN_ERR (svn_categorize_props (propchanges, NULL, NULL, &props, subpool));
> +
> + /* We only want to merge "regular" version properties: by
> + definition, 'svn merge' shouldn't touch any data within .svn/ */
> + if (props->nelts)
> + {
> + err = svn_wc_merge_prop_diffs (state, path, adm_access, props,
> + FALSE, merge_b->dry_run, subpool);
> + if (err && (err->apr_err == SVN_ERR_ENTRY_NOT_FOUND
> + || err->apr_err == SVN_ERR_UNVERSIONED_RESOURCE))
> + {
> + /* if the entry doesn't exist in the wc, just 'skip' over
> + this part of the tree-delta. */
> + if (state)
> + *state = svn_wc_notify_state_missing;
> + svn_error_clear (err);
> + return SVN_NO_ERROR;
> + }
> + else if (err)
> + return err;
> + }
> +
> + svn_pool_destroy (subpool);
> + return SVN_NO_ERROR;
> +}
> +

> +def merge_keyword_expansions(sbox):
> + "merge changes to keyword expansion property"
> +
> + sbox.build()
> +
> + wcpath = sbox.wc_dir
> + tpath = os.path.join(wcpath, "t")
> + bpath = os.path.join(wcpath, "b")
> + t_fpath = os.path.join(tpath, 'f')
> + b_fpath = os.path.join(bpath, 'f')
> +
> + os.mkdir(tpath)
> + svntest.main.run_svn(None, "add", tpath)
> + # Commit r2.
> + svntest.actions.run_and_verify_svn(None, None, [],
> + "ci", "-m", "r2", wcpath)
> +
> + # Copy t to b.
> + svntest.main.run_svn(None, "cp", tpath, bpath)
> + # Commit r3
> + svntest.actions.run_and_verify_svn(None, None, [],
> + "ci", "-m", "r3", wcpath)
> +
> + # Add a file to t.
> + svntest.main.file_append(t_fpath, "$Revision$")
> + svntest.actions.run_and_verify_svn(None, None, [],
> + 'add', t_fpath)
> + # Ask for keyword expansion in the file.
> + svntest.actions.run_and_verify_svn(None, None, [],
> + 'propset', 'svn:keywords', 'Revision',
> + t_fpath)
> + # Commit r4
> + svntest.actions.run_and_verify_svn(None, None, [],
> + 'ci', '-m', 'r4', wcpath)
> +
> + # Update the wc before the merge.
> + svntest.actions.run_and_verify_svn(None, None, [],
> + 'update', wcpath)
> +

If you are not going to use run_and_verify_commit/update then I
suggest at least a run_and_verify_status before the merge.

> + # Do the merge.
> + expected_output = wc.State(bpath, {
> + 'f' : Item(status='A '),
> + })
> + expected_disk = wc.State('', {
> + 'f' : Item("$Revision: 4 $"),
> + })
> + expected_status = wc.State(bpath, {
> + '' : Item(status=' ', wc_rev=4, repos_rev=4),
> + 'f' : Item(status='A ', wc_rev='-', copied='+', repos_rev=4),
> + })
> + expected_skip = wc.State(bpath, { })
> + svntest.actions.run_and_verify_merge(bpath, '2', 'HEAD',
> + svntest.main.current_repo_url + '/t',
> + expected_output,
> + expected_disk,
> + expected_status,
> + expected_skip)
> +

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org