On Mon, 2008-09-08 at 16:45 -0500, Hyrum K. Wright wrote:
> Julian Foad wrote:
> > On Thu, 2008-09-04 at 15:06 +0100, Julian Foad wrote:
> >> On Thu, 2008-09-04 at 08:55 -0500, Hyrum K. Wright wrote:
> >>> Hyrum K. Wright wrote:
> >>>> I'm currently investigating a generic '--ignore-prop' switch, initially for 'svn
> >>>> st'. It is being written in such a way that we could easily alias
> >>>> '--ignore-mergeinfo' or '--forget-merges' or
> >>>> '--dont-get-in-my-way-i'm-trying-to-do-real-work' to mean '--ignore-prop
> >>>> svn:mergeinfo'. I hope to have a proof-of-concept patch done in a few days.
> >>> As promised, here's a patch which implements the above. It completely
> >>> implements the functionality which it claims to, but there is still future work
> >>> to be done to flesh out the feature. This does not add a '--ignore-mergeinfo'
> >>> alias as discussed above, but adding such would be trivial.
> >>>
> >>> I'd appreciate any feedback, but if there aren't serious objections, I'll commit
> >>> the patch in the next day or two.
> >>>
> >>> [[[
> >>> Add support for ignoring arbitrary property modifications in 'svn st'.
> >>> This currently only works locally, not with 'svn st -u'.
> >> We should always be cautious about complexifying libsvn_wc. Can I ask
> >> whether and why it is beneficial to add this complexity into libsvn_wc
> >> rather than filtering at the presentation layer?
> >
> > Starting to answer my own question:
> >
> > The client side of a status query doesn't receive a list of the property
> > changes, just the flag saying "props are changed". Therefore no way to
> > filter on the client side, using the present API.
> >
> > However, there's a concept mis-match going on. The WC has the concept of
> > properties; it has the concept of properties that are special to
> > Subversion ("svn:*"), and rules on how to treat them. Now we're wanting
> > a concept of "properties whose changes we don't want to see in the
> > UI"... huh, but obviously the place to implement that is in the UI code.
> >
> > The problem is there isn't a convenient API to tell the UI what all the
> > prop changes in the WC are. Let's design one.
>
> It turns out that the WC already has ways of getting to the information which we
> are looking for. r32970 and r32973 helped expose some of that, and this patch
> capitalizes on that new functionality.
>
> Feedback welcome and encouraged. If I don't hear anything in a day or two, I'll
> commit.
I don't have time to do a thorough review in the next two days. Here's a
cursory one.
> -Hyrum
>
> [[[
> Add support for ignoring arbitrary property modifications in 'svn st'.
> This currently only works locally, not with 'svn st -u'.
Do you plan to make it work with "-u" next? Any particular problem
forseen? I would hope we can do, as non-orthogonal switches are a pain.
>
> * subversion/tests/cmdline/stat_tests.py
> (status_ignored_props): New test.
> (test_list): Run the new test.
>
> * subversion/svn/cl.h
> (svn_cl__opt_state_t): New hash to hold the ignore properties.
>
> * subversion/svn/main.c
> (svn_cl__longopt_t, svn_cl__options, main): Add support for the
> '--ignore-prop' switch.
> (svn_opt_subcommand_desc2_t): Let 'svn st' accept '--ignore-prop'.
>
> * subversion/svn/status-cmd.c
> (svn_cl__status): Pass along the ignored props to the client library.
>
> * subversion/include/svn_wc.h
> (svn_wc_props_modified2): New.
>
> * subversion/include/svn_client.h
> (svn_client_status4): Add ignored_prop parameter and update doc string.
> (svn_client_status3): Update docstring.
>
> * subversion/libsvn_wc/props.c:
> (svn_wc_props_modified2): New.
>
> * subversion/libsvn_client/externals.c,
> subversion/libsvn_client/client.h
> (svn_client__do_external_status): Pass ignored_props through for external
> statii.
>
> * subversion/libsvn_client/status.c
> (svn_client_status4): Add ignored_props param, and use it.
> (svn_client_status3): Use a NULL default ignored_props.
> (status_baton): Add a few additional members.
> (props_hash_diff_func): New.
> (tweak_status): Filter statii returned from the WC by ignored properties,
> if requested.
> ]]]
> plain text document attachment (props.patch)
> Index: subversion/tests/cmdline/stat_tests.py
> ===================================================================
> --- subversion/tests/cmdline/stat_tests.py (revision 32974)
> +++ subversion/tests/cmdline/stat_tests.py (working copy)
> @@ -1510,6 +1510,55 @@
> [],
> "status", "-u")
>
> +
> +#----------------------------------------------------------------------
> +
> +def status_ignored_props(sbox):
> + "'svn st --ignore-prop FOO'"
> +
> + sbox.build()
> + wc_dir = sbox.wc_dir
> +
> + os.chdir(wc_dir)
> +
> + A_path = 'A'
> + C_path = os.path.join(A_path, 'C')
> + D_path = os.path.join(A_path, 'D')
> + H_path = os.path.join(D_path, 'H')
> + beta_path = os.path.join(A_path, 'B', 'E', 'beta')
> + gamma_path = os.path.join(D_path, 'gamma')
> + iota_path = 'iota'
> + chi_path = os.path.join(H_path, 'chi')
> +
> + # Set some properties
> + svntest.main.run_svn(None, 'propset', 'svn:foo', 'bar', beta_path, H_path,
> + C_path, gamma_path)
> + svntest.main.run_svn(None, 'propset', 'svn:bar', 'foo', iota_path, chi_path)
It would be good to have an overlap (a file that has both properties set
on it) to test that suppressing one property doesn't accidentally
suppress all property changes on that file.
> +
> + # Check vanilla status
> + expected = svntest.verify.UnorderedOutput(
> + [' M ' + beta_path + '\n',
> + ' M ' + H_path + '\n',
> + ' M ' + C_path + '\n',
> + ' M ' + gamma_path + '\n',
> + ' M ' + iota_path + '\n',
> + ' M ' + chi_path + '\n'])
> + svntest.actions.run_and_verify_svn(None, expected, [], 'status')
> +
> + # Check '--ignore-prop' status on one property
> + expected = svntest.verify.UnorderedOutput([
> + ' M ' + iota_path + '\n',
> + ' M ' + chi_path + '\n'])
> + svntest.actions.run_and_verify_svn(None, expected, [], 'status',
> + '--ignore-prop', 'svn:foo')
> +
> + # Check '--ignore-prop' status on all the properties
> + expected = svntest.verify.UnorderedOutput([])
> + svntest.actions.run_and_verify_svn(None, expected, [], 'status',
> + '--ignore-prop', 'svn:foo',
> + '--ignore-prop', 'svn:bar')
> +
> +
> ########################################################################
> # Run the tests
>
> @@ -1547,6 +1596,7 @@
> status_depth_local,
> status_depth_update,
> status_dash_u_type_change,
> + status_ignored_props,
> ]
>
> if __name__ == '__main__':
> Index: subversion/svn/cl.h
> ===================================================================
> --- subversion/svn/cl.h (revision 32974)
> +++ subversion/svn/cl.h (working copy)
> @@ -209,6 +209,7 @@
> svn_boolean_t reintegrate; /* use "reintegrate" merge-source heuristic */
> svn_boolean_t trust_server_cert; /* trust server SSL certs that would
> otherwise be rejected as "untrusted" */
> + apr_hash_t *ignored_props; /* list of props to ignore changes to */
Here is the place to document that only the keys of the hash are
relevant and the values are meaningless.
> } svn_cl__opt_state_t;
>
>
> Index: subversion/svn/main.c
> ===================================================================
> --- subversion/svn/main.c (revision 32974)
> +++ subversion/svn/main.c (working copy)
> @@ -102,7 +102,8 @@
> opt_accept,
> opt_show_revs,
> opt_reintegrate,
> - opt_trust_server_cert
> + opt_trust_server_cert,
> + opt_ignore_prop
> } svn_cl__longopt_t;
>
> /* Option codes and descriptions for the command line client.
> @@ -269,6 +270,8 @@
> "('merged', 'eligible')")},
> {"reintegrate", opt_reintegrate, 0,
> N_("lump-merge all of source URL's unmerged changes")},
> + {"ignore-prop", opt_ignore_prop, 1,
> + N_("Ingore changes to property ARG")},
>
> /* Long-opt Aliases
> *
> @@ -873,7 +876,7 @@
> " 965 687 joe wc/zig.c\n"
> " Status against revision: 981\n"),
> { 'u', 'v', 'N', opt_depth, 'q', opt_no_ignore, opt_incremental, opt_xml,
> - opt_ignore_externals, opt_changelist} },
> + opt_ignore_externals, opt_changelist, opt_ignore_prop} },
>
> { "switch", svn_cl__switch, {"sw"}, N_
> ("Update the working copy to a different URL.\n"
> @@ -1487,6 +1490,13 @@
> case opt_reintegrate:
> opt_state.reintegrate = TRUE;
> break;
> + case opt_ignore_prop:
> + if (opt_state.ignored_props == NULL)
> + opt_state.ignored_props = apr_hash_make(pool);
> +
> + apr_hash_set(opt_state.ignored_props, apr_pstrdup(pool, opt_arg),
> + APR_HASH_KEY_STRING, (void *) 0xdeadbeef);
Cute, but the names of all cows must be declared in a header file. Just
use NULL :-)
> + break;
> default:
> /* Hmmm. Perhaps this would be a good place to squirrel away
> opts that commands like svn diff might need. Hmmm indeed. */
> Index: subversion/svn/status-cmd.c
> ===================================================================
> --- subversion/svn/status-cmd.c (revision 32974)
> +++ subversion/svn/status-cmd.c (working copy)
> @@ -241,6 +241,7 @@
> opt_state->no_ignore,
> opt_state->ignore_externals,
> opt_state->changelists,
> + opt_state->ignored_props,
> ctx, subpool),
> NULL, opt_state->quiet,
> /* not versioned: */
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 32974)
> +++ subversion/include/svn_wc.h (working copy)
> @@ -1703,8 +1703,24 @@
> /** Set @a *modified_p to non-zero if @a path's properties are modified
> * with regard to the base revision, else set @a modified_p to zero.
> * @a adm_access must be an access baton for @a path.
Need to describe @a which_props.
> + *
> + * @since New in 1.6.
> */
> svn_error_t *
> +svn_wc_props_modified2(svn_boolean_t *modified_p,
> + const char *path,
> + apr_hash_t **which_props,
> + svn_wc_adm_access_t *adm_access,
> + apr_pool_t *pool);
> +
> +
> +/**
> + * Same as svn_wc_props_modified2(), but with @c NULL for @a which_props.
> + *
> + * @deprecated Provided for backward compatibility with the 1.5 API.
> + */
> +SVN_DEPRECATED
> +svn_error_t *
> svn_wc_props_modified_p(svn_boolean_t *modified_p,
> const char *path,
> svn_wc_adm_access_t *adm_access,
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 32974)
> +++ subversion/include/svn_client.h (working copy)
> @@ -1805,6 +1805,9 @@
> * it's a member of one of those changelists. If @a changelists is
> * empty (or altogether @c NULL), no changelist filtering occurs.
> *
> + * @a ignored_props is a hash of property names for which modifications
Again, say something like "a hash of (const char *) names and ignored
values". May it be empty? May it be null?
> + * will be ignored.
> + *
> * @since New in 1.6.
> */
> svn_error_t *
> @@ -1819,12 +1822,14 @@
> svn_boolean_t no_ignore,
> svn_boolean_t ignore_externals,
> const apr_array_header_t *changelists,
> + apr_hash_t *ignored_props,
> svn_client_ctx_t *ctx,
> apr_pool_t *pool);
>
> /**
> * Same as svn_client_status4(), but using an @c svn_wc_status_func2_t
> - * instead of an @c svn_wc_status_func3_t.
> + * instead of an @c svn_wc_status_func3_t, and with @a ignored_props passed
> + * as @c NULL.
Just a small matter of style: it's nicer to describe the behaviour in
the user's terms rather than with reference to how this implementation
achieves it; thus, something like "and without being able to ignore
properties."
> *
> * @since New in 1.5.
> * @deprecated Provided for backward compatibility with the 1.5 API.
> Index: subversion/libsvn_wc/props.c
> ===================================================================
> --- subversion/libsvn_wc/props.c (revision 32974)
> +++ subversion/libsvn_wc/props.c (working copy)
> @@ -3031,7 +3031,17 @@
> return SVN_NO_ERROR;
> }
>
> +svn_error_t *
> +svn_wc_props_modified2(svn_boolean_t *modified_p,
> + const char *path,
> + apr_hash_t **which_props,
> + svn_wc_adm_access_t *adm_access,
> + apr_pool_t *pool)
> +{
> + return modified_props(modified_p, path, which_props, adm_access, pool);
> +}
>
> +
> svn_error_t *
> svn_wc_props_modified_p(svn_boolean_t *modified_p,
> const char *path,
> Index: subversion/libsvn_client/externals.c
> ===================================================================
> --- subversion/libsvn_client/externals.c (revision 32974)
> +++ subversion/libsvn_client/externals.c (working copy)
> @@ -869,6 +869,7 @@
> svn_boolean_t get_all,
> svn_boolean_t update,
> svn_boolean_t no_ignore,
> + apr_hash_t *ignored_props,
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
> {
> @@ -939,7 +940,8 @@
> &(external->revision),
> status_func, status_baton,
> depth, get_all, update,
> - no_ignore, FALSE, NULL, ctx, iterpool));
> + no_ignore, FALSE, NULL, ignored_props,
> + ctx, iterpool));
> }
> }
>
> Index: subversion/libsvn_client/status.c
> ===================================================================
> --- subversion/libsvn_client/status.c (revision 32974)
> +++ subversion/libsvn_client/status.c (working copy)
> @@ -49,8 +49,25 @@
> apr_hash_t *changelist_hash; /* keys are changelist names */
> svn_wc_status_func3_t real_status_func; /* real status function */
> void *real_status_baton; /* real status baton */
> + apr_hash_t *ignored_props; /* props to ignore mods to */
> + svn_wc_adm_access_t *adm_access; /* access to the wc */
> + svn_boolean_t no_ignore;
> + svn_boolean_t get_all;
Keep the doc strings going!
> };
>
> +
> +static svn_error_t *
Even static functions need a doc string!
> +props_hash_diff_func(const void *key,
> + apr_ssize_t klen,
> + enum svn_hash_diff_key_status status,
> + void *baton)
> +{
> + if (status == svn_hash_diff_key_b)
> + *((svn_boolean_t *) baton) = FALSE;
> +
> + return SVN_NO_ERROR;
> +}
> +
> /* A status callback function which wraps the *real* status
> function/baton. This sucker takes care of any status tweaks we
> need to make (such as noting that the target of the status is
> @@ -77,6 +94,28 @@
> if (! SVN_WC__CL_MATCH(sb->changelist_hash, status->entry))
> return SVN_NO_ERROR;
>
> + /* If we have ignored props, the information returned by libsvn_wc isn't
> + sufficient, so we'll need to do a bit more digging. */
> + if (sb->ignored_props && status->prop_status == svn_wc_status_modified)
> + {
> + svn_boolean_t ignore = TRUE;
> + svn_boolean_t mod_props;
> + apr_hash_t *which_props;
> +
> + SVN_ERR(svn_wc_props_modified2(&mod_props, path, &which_props,
> + sb->adm_access, pool));
> + SVN_ERR(svn_hash_diff(sb->ignored_props, which_props,
> + props_hash_diff_func, &ignore, pool));
> +
> + if (ignore)
> + {
> + status->prop_status = svn_wc_status_normal;
> +
> + if (!svn_wc__is_sendable_status(status, sb->no_ignore, sb->get_all))
> + return SVN_NO_ERROR;
> + }
> + }
> +
> /* Call the real status function/baton. */
> return sb->real_status_func(sb->real_status_baton, path, status, pool);
> }
> @@ -218,6 +257,7 @@
> svn_boolean_t no_ignore,
> svn_boolean_t ignore_externals,
> const apr_array_header_t *changelists,
> + apr_hash_t *ignored_props,
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
> {
> @@ -236,10 +276,21 @@
> if (changelists && changelists->nelts)
> SVN_ERR(svn_hash_from_cstring_keys(&changelist_hash, changelists, pool));
>
> + if (ignored_props && apr_hash_count(ignored_props) == 0)
> + ignored_props = NULL;
This implies it may be empty or null and both mean the same thing.
> +
> + /* We don't yet support ignored properties with '-u', so error. */
> + if (ignored_props && update)
> + return svn_error_create(SVN_ERR_UNSUPPORTED_FEATURE, NULL,
> + _("Ignoring properties with a remote status not yet supported."));
> +
> sb.real_status_func = status_func;
> sb.real_status_baton = status_baton;
> sb.deleted_in_repos = FALSE;
> sb.changelist_hash = changelist_hash;
> + sb.ignored_props = ignored_props;
> + sb.no_ignore = no_ignore;
> + sb.get_all = get_all;
>
> /* Try to open the target directory. If the target is a file or an
> unversioned directory, open the parent directory instead */
> @@ -265,6 +316,7 @@
> return err;
>
> anchor = svn_wc_adm_access_path(anchor_access);
> + sb.adm_access = anchor_access;
>
> /* Get the status edit, and use our wrapping status function/baton
> as the callback pair. */
> @@ -402,7 +454,8 @@
> if (SVN_DEPTH_IS_RECURSIVE(depth) && (! ignore_externals))
> SVN_ERR(svn_client__do_external_status(traversal_info, status_func,
> status_baton, depth, get_all,
> - update, no_ignore, ctx, pool));
> + update, no_ignore, ignored_props,
> + ctx, pool));
>
> return SVN_NO_ERROR;
> }
> @@ -444,7 +497,7 @@
>
> return svn_client_status4(result_rev, path, revision, status3_wrapper_func,
> &swb, depth, get_all, update, no_ignore,
> - ignore_externals, changelists, ctx, pool);
> + ignore_externals, changelists, NULL, ctx, pool);
>
> }
>
> Index: subversion/libsvn_client/client.h
> ===================================================================
> --- subversion/libsvn_client/client.h (revision 32974)
> +++ subversion/libsvn_client/client.h (working copy)
> @@ -1007,6 +1007,7 @@
> svn_boolean_t get_all,
> svn_boolean_t update,
> svn_boolean_t no_ignore,
> + apr_hash_t *ignored_props,
I'm guessing there's a doc string above here that needs updating. If
there isn't, it would be very nice if you would add one.
> svn_client_ctx_t *ctx,
> apr_pool_t *pool);
>
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-09-09 20:59:28 CEST