Re: [PATCH] 'svn st --ignore-prop' (v2)

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.

-Hyrum

[[[
Add support for ignoring arbitrary property modifications in 'svn st'.
This currently only works locally, not with 'svn st -u'.

* 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.
]]]

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)
+
+ # 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 */
 } 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);
+ 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.
+ *
+ * @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
+ * 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.
  *
  * @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;
 };
 
+
+static svn_error_t *
+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;
+
+ /* 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,
                                svn_client_ctx_t *ctx,
                                apr_pool_t *pool);