You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Hyrum K. Wright" <hy...@mail.utexas.edu> on 2008/09/10 18:01:32 UTC
Re: [PATCH] 'svn st --ignore-prop' (v3)
Julian Foad wrote:
> 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.
Yes, I do plan on making it work with 'status -u'. I think that lots of the
machinery in this patch can be reused in that case (it may already work!), but I
haven't yet thoroughly tested it.
>> * 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.
Agreed. The new patch includes this case in the test, and it works as expected.
>> +
>> + # 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 :-)
My initial reason for doing this was do that if I start seeing the value
0xdeadbeef somewhere, I'd know from whence it was coming. But I suppose that
isn't really needed here, so I've gone with the somewhat duller 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.
Correct. The updated docstring should now reflect that.
>> +
>> + /* 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.
Nope. The docstring already has the catchall of "All other options are the same
as those passed to svn_client_status()". (Though I've updated that line to
revert to svn_client_status4()).
Thanks for the review! The log message hasn't changed, but it's included here
for completeness.
-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.
]]]
Re: [PATCH] 'svn st --ignore-prop' (v3)
Posted by Peter Wemm <pe...@wemm.org>.
On Thu, Sep 11, 2008 at 11:09 AM, Gunnar Dalsnes <ha...@online.no> wrote:
[..]
> I agree that mergeinfo must be filtered out (until a better solution is
> found), I also agree that generic property ignore is probably not very
> useful, and it makes it even easier to create bad designs and abuse
> properties further. But why set an option to ignore it? I would rather
> default to ignoring mergeinfo and add an option to turn it on!
I think I agree. The whole mergeinfo user interface is kind of...
unfortunate ... anyway. For example, how do you find which rev
trashed the mergeinfo? (annotate/blame for properties?)
Anyway, I posted too soon after waking up with insufficient coffee. I
see mergeinfo spam as the problem, not generic property changes as a
problem. Although at one point I'd have loved a way to ignore
cvs2svn:cvs-rev and problems that caused.
--
Peter Wemm - peter@wemm.org; peter@FreeBSD.org; peter@yahoo-inc.com; KI6FJV
"All of this is for nothing if we don't go to the stars" - JMS/B5
"If Java had true garbage collection, most programs would delete
themselves upon execution." -- Robert Sewell
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] 'svn st --ignore-prop' (v3)
Posted by Gunnar Dalsnes <ha...@online.no>.
<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
<title></title>
</head>
<body bgcolor="#ffffff" text="#000000">
Peter Wemm wrote:
<blockquote
cite="mid:e7db6d980809111053i234a1e7ag5a0c550b012a5bf@mail.gmail.com"
type="cite">
<pre wrap="">On Thu, Sep 11, 2008 at 3:30 AM, Julian Foad <a class="moz-txt-link-rfc2396E" href="mailto:julianfoad@btopenworld.com"><julianfoad@btopenworld.com></a> wrote:
</pre>
<blockquote type="cite">
<pre wrap="">At the UI level, I have no evidence that we need a general
"--ignore-prop" feature, and, although we could commit this for
experimentation, I would not like to see it in a released version
without knowing it's valuable enough to justify its existence. (If it is
valuable, then it seems likely that the converse feature for noticing
ONLY certain properties would be needed too.)
</pre>
</blockquote>
<pre wrap=""><!---->
Please have a look at 'svn log -v
<a class="moz-txt-link-freetext" href="svn://svn.freebsd.org/base/stable/7/sys/nlm">svn://svn.freebsd.org/base/stable/7/sys/nlm</a>' and see if you still
agree by the time you get to the end. Try and find some real content
in that log output other than useless property spam.
</pre>
</blockquote>
I agree that mergeinfo must be filtered out (until a better solution is
found), I also agree that generic property ignore is probably not very
useful, and it makes it even easier to create bad designs and abuse
properties further. But why set an option to ignore it? I would rather
default to ignoring mergeinfo and add an option to turn it on!<br>
<br>
Gunnar.<br>
</body>
</html>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] 'svn st --ignore-prop' (v3)
Posted by Julian Foad <ju...@btopenworld.com>.
On Thu, 2008-09-11 at 10:53 -0700, Peter Wemm wrote:
> On Thu, Sep 11, 2008 at 3:30 AM, Julian Foad <ju...@btopenworld.com> wrote:
>
> > At the UI level, I have no evidence that we need a general
> > "--ignore-prop" feature, and, although we could commit this for
> > experimentation, I would not like to see it in a released version
> > without knowing it's valuable enough to justify its existence. (If it is
> > valuable, then it seems likely that the converse feature for noticing
> > ONLY certain properties would be needed too.)
>
> Please have a look at 'svn log -v
> svn://svn.freebsd.org/base/stable/7/sys/nlm' and see if you still
> agree by the time you get to the end. Try and find some real content
> in that log output other than useless property spam.
Peter, you must have missed the beginning of this thread. I'm not
arguing against the need for a way of ignoring changes to svn:mergeinfo,
only against the need for a generic "ignore properties P1, P2, ..."
feature. I agree that there is an acute problem with mergeinfo spam, and
an "ignore mergeinfo changes" interface could be an effective short-term
way to address it.
Unless you're saying you have a problem with properties other than
svn:mergeinfo?
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] 'svn st --ignore-prop' (v3)
Posted by Peter Wemm <pe...@wemm.org>.
On Thu, Sep 11, 2008 at 3:30 AM, Julian Foad <ju...@btopenworld.com> wrote:
> At the UI level, I have no evidence that we need a general
> "--ignore-prop" feature, and, although we could commit this for
> experimentation, I would not like to see it in a released version
> without knowing it's valuable enough to justify its existence. (If it is
> valuable, then it seems likely that the converse feature for noticing
> ONLY certain properties would be needed too.)
Please have a look at 'svn log -v
svn://svn.freebsd.org/base/stable/7/sys/nlm' and see if you still
agree by the time you get to the end. Try and find some real content
in that log output other than useless property spam.
--
Peter Wemm - peter@wemm.org; peter@FreeBSD.org; peter@yahoo-inc.com; KI6FJV
"All of this is for nothing if we don't go to the stars" - JMS/B5
"If Java had true garbage collection, most programs would delete
themselves upon execution." -- Robert Sewell
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] 'svn st --ignore-prop' (v3)
Posted by Mark Phippard <ma...@gmail.com>.
On Thu, Sep 11, 2008 at 1:33 PM, Julian Foad <ju...@btopenworld.com> wrote:
> On Thu, 2008-09-11 at 11:38 -0500, Hyrum K. Wright wrote:
>> As for the generality, I can imagine, though I don't know of any personally,
>> circumstances where projects use custom properties on files for some purpose,
>> use like we use properties for mergeinfo. Why *not* provide the ability for
>> users to filter arbitrary properties, especially since it doesn't require any
>> additional work in the implementation?
>
> Why not? Only reasons like: If there's no real gain (which isn't yet
> known), then the growth in out API and UI and implementation would be
> just a drain. If this is to be a real feature, it needs to be "complete"
> like, I suggest, having the converse operation of "show only properties
> P1, P2, ...". If this is to be frequently used for ignoring mergeinfo,
> there ought to be an easier-to-type command than "svn st
> --ignore-prop=svn:mergeinfo ...". We need to consider whether to provide
> a corresponding configuration option.
>
> None of these are showstoppers, but they need considering.
I am not 100% sold on this either, but there is one benefit to the
generic approach Hyrum is taking. My argument against this, is that I
feel like we are putting a band-aid on the problem instead of fixing
it. The fix would be to look for a better way to store this
information than properties. If we did that, then it would hopefully
remove the need for this change. So getting back to my first
sentence, if the UI is generic, then at least the general feature
still has some value in terms of being in the UI, even if no one has a
need for it any longer.
Does --ignore-prop=* or ALL or something like that work? Maybe
--ignore-prop could even have VAL be optional and mean ALL if none is
specified?
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] 'svn st --ignore-prop' (v3)
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Hyrum K. Wright wrote:
> Julian Foad wrote:
>> On Thu, 2008-09-11 at 11:38 -0500, Hyrum K. Wright wrote:
>>> Julian Foad wrote:
>>>> Hyrum,
>>>>
>>>> this is going to sound inconsiderate after I've just reviewed the
>>>> implementation, but I've just been thinking back to why we're doing
>>>> this, and I am still not convinced that this is not over-generalising.
>>>>
>>>> At the UI level, I have no evidence that we need a general
>>>> "--ignore-prop" feature, and, although we could commit this for
>>>> experimentation, I would not like to see it in a released version
>>>> without knowing it's valuable enough to justify its existence. (If it is
>>>> valuable, then it seems likely that the converse feature for noticing
>>>> ONLY certain properties would be needed too.)
>>>>
>>>> At the libsvn_client level, exactly the same applies.
>>> To a large degree, this set of features is solving a personal itch. In working
>>> with the 1.5.x branch, the mergeinfo modifications are largely noise in 'svn st'
>>> and 'svn diff', and I can't find the revisions I'm looking for using 'svn log'.
>>> That's what's motivating me to add this. I (currently) don't have any need for
>>> the converse, so I haven't thought much about those use cases.
>>>
>>> As for the generality, I can imagine, though I don't know of any personally,
>>> circumstances where projects use custom properties on files for some purpose,
>>> use like we use properties for mergeinfo. Why *not* provide the ability for
>>> users to filter arbitrary properties, especially since it doesn't require any
>>> additional work in the implementation?
>> Why not? Only reasons like: If there's no real gain (which isn't yet
>> known), then the growth in out API and UI and implementation would be
>> just a drain. If this is to be a real feature, it needs to be "complete"
>> like, I suggest, having the converse operation of "show only properties
>> P1, P2, ...". If this is to be frequently used for ignoring mergeinfo,
>> there ought to be an easier-to-type command than "svn st
>> --ignore-prop=svn:mergeinfo ...". We need to consider whether to provide
>> a corresponding configuration option.
>>
>> None of these are showstoppers, but they need considering.
>>
>>> I feel strongly that the implementation should be generic, since we using the
>>> generic property mechanism for storing mergeinfo.
>> Well, the project has a bit of an uncertain viewpoint on that. We don't
>> give users a choice about what property we store it in, so they don't
>> need a choice about what property to ignore its changes in. We do
>> special things with that property, yet in many ways it is one of the
>> "generic" properties.
>>
>>> Just so I make sure I can
>>> understand correctly, are you concerned about exposing that generality through
>>> the API? (e.g., using 'apr_hash_t *ignored_props', instead of 'svn_boolean_t
>>> ignore_mergeinfo')
>> Yes, in the sense that I'm primarily concerned about exposing it (in the
>> UI and API). I would be much happier if that same functionality were to
>> live privately inside one library, perhaps only being exercised via a
>> boolean API switch that asks it to ignore the mergeinfo property.
>
> I'm starting to come around on this (thanks for the patience!) Any suggestions
> for a switch name? '--ignore-mergeinfo'?
r33046 on the branch implements this.
-Hyrum
Re: [PATCH] 'svn st --ignore-prop' (v3)
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Julian Foad wrote:
> On Thu, 2008-09-11 at 11:38 -0500, Hyrum K. Wright wrote:
>> Julian Foad wrote:
>>> Hyrum,
>>>
>>> this is going to sound inconsiderate after I've just reviewed the
>>> implementation, but I've just been thinking back to why we're doing
>>> this, and I am still not convinced that this is not over-generalising.
>>>
>>> At the UI level, I have no evidence that we need a general
>>> "--ignore-prop" feature, and, although we could commit this for
>>> experimentation, I would not like to see it in a released version
>>> without knowing it's valuable enough to justify its existence. (If it is
>>> valuable, then it seems likely that the converse feature for noticing
>>> ONLY certain properties would be needed too.)
>>>
>>> At the libsvn_client level, exactly the same applies.
>> To a large degree, this set of features is solving a personal itch. In working
>> with the 1.5.x branch, the mergeinfo modifications are largely noise in 'svn st'
>> and 'svn diff', and I can't find the revisions I'm looking for using 'svn log'.
>> That's what's motivating me to add this. I (currently) don't have any need for
>> the converse, so I haven't thought much about those use cases.
>>
>> As for the generality, I can imagine, though I don't know of any personally,
>> circumstances where projects use custom properties on files for some purpose,
>> use like we use properties for mergeinfo. Why *not* provide the ability for
>> users to filter arbitrary properties, especially since it doesn't require any
>> additional work in the implementation?
>
> Why not? Only reasons like: If there's no real gain (which isn't yet
> known), then the growth in out API and UI and implementation would be
> just a drain. If this is to be a real feature, it needs to be "complete"
> like, I suggest, having the converse operation of "show only properties
> P1, P2, ...". If this is to be frequently used for ignoring mergeinfo,
> there ought to be an easier-to-type command than "svn st
> --ignore-prop=svn:mergeinfo ...". We need to consider whether to provide
> a corresponding configuration option.
>
> None of these are showstoppers, but they need considering.
>
>> I feel strongly that the implementation should be generic, since we using the
>> generic property mechanism for storing mergeinfo.
>
> Well, the project has a bit of an uncertain viewpoint on that. We don't
> give users a choice about what property we store it in, so they don't
> need a choice about what property to ignore its changes in. We do
> special things with that property, yet in many ways it is one of the
> "generic" properties.
>
>> Just so I make sure I can
>> understand correctly, are you concerned about exposing that generality through
>> the API? (e.g., using 'apr_hash_t *ignored_props', instead of 'svn_boolean_t
>> ignore_mergeinfo')
>
> Yes, in the sense that I'm primarily concerned about exposing it (in the
> UI and API). I would be much happier if that same functionality were to
> live privately inside one library, perhaps only being exercised via a
> boolean API switch that asks it to ignore the mergeinfo property.
I'm starting to come around on this (thanks for the patience!) Any suggestions
for a switch name? '--ignore-mergeinfo'?
>>>> * subversion/include/svn_wc.h
>>>> (svn_wc_props_modified2): New.
>>> Can you see if you can use the existing svn_wc_get_prop_diffs() instead?
>>> I think there is no need for this new function. (If you do replace
>>> svn_wc_props_modified_p() with svn_wc_props_modified2(), I would have
>>> some comments on that.)
>> svn_wc_prps_modified2() is simply a wrapper around the same function that
>> svn_wc_props_modified_p() is wrapping, but exposing the additional which_props
>> parameter. The functionality is already there, this patch just exposes it to
>> the public API.
>>
>> svn_wc_prop_diffs() works at a slightly lower level of abstraction, require the
>> caller to specify a set of properties to take the diff against. I'd prefer to
>> avoid the extra work of building that set of properties, when we have an API
>> that can give me the information which I want, essentially for free (in terms of
>> programmer work, not execution time).
>
> You misunderstood svn_wc_get_prop_diffs(). Both its "**propchanges" and
> "**original_props" arguments are outputs. If you set original_props to
> NULL it does exactly what you want.
Ah, so I did. r330xx on the branch removes the new API and uses this existing
one, thanks for the suggestion.
-Hyrum
Re: [PATCH] 'svn st --ignore-prop' (v3)
Posted by Julian Foad <ju...@btopenworld.com>.
On Thu, 2008-09-11 at 11:38 -0500, Hyrum K. Wright wrote:
> Julian Foad wrote:
> > Hyrum,
> >
> > this is going to sound inconsiderate after I've just reviewed the
> > implementation, but I've just been thinking back to why we're doing
> > this, and I am still not convinced that this is not over-generalising.
> >
> > At the UI level, I have no evidence that we need a general
> > "--ignore-prop" feature, and, although we could commit this for
> > experimentation, I would not like to see it in a released version
> > without knowing it's valuable enough to justify its existence. (If it is
> > valuable, then it seems likely that the converse feature for noticing
> > ONLY certain properties would be needed too.)
> >
> > At the libsvn_client level, exactly the same applies.
>
> To a large degree, this set of features is solving a personal itch. In working
> with the 1.5.x branch, the mergeinfo modifications are largely noise in 'svn st'
> and 'svn diff', and I can't find the revisions I'm looking for using 'svn log'.
> That's what's motivating me to add this. I (currently) don't have any need for
> the converse, so I haven't thought much about those use cases.
>
> As for the generality, I can imagine, though I don't know of any personally,
> circumstances where projects use custom properties on files for some purpose,
> use like we use properties for mergeinfo. Why *not* provide the ability for
> users to filter arbitrary properties, especially since it doesn't require any
> additional work in the implementation?
Why not? Only reasons like: If there's no real gain (which isn't yet
known), then the growth in out API and UI and implementation would be
just a drain. If this is to be a real feature, it needs to be "complete"
like, I suggest, having the converse operation of "show only properties
P1, P2, ...". If this is to be frequently used for ignoring mergeinfo,
there ought to be an easier-to-type command than "svn st
--ignore-prop=svn:mergeinfo ...". We need to consider whether to provide
a corresponding configuration option.
None of these are showstoppers, but they need considering.
> I feel strongly that the implementation should be generic, since we using the
> generic property mechanism for storing mergeinfo.
Well, the project has a bit of an uncertain viewpoint on that. We don't
give users a choice about what property we store it in, so they don't
need a choice about what property to ignore its changes in. We do
special things with that property, yet in many ways it is one of the
"generic" properties.
> Just so I make sure I can
> understand correctly, are you concerned about exposing that generality through
> the API? (e.g., using 'apr_hash_t *ignored_props', instead of 'svn_boolean_t
> ignore_mergeinfo')
Yes, in the sense that I'm primarily concerned about exposing it (in the
UI and API). I would be much happier if that same functionality were to
live privately inside one library, perhaps only being exercised via a
boolean API switch that asks it to ignore the mergeinfo property.
> > Do you need to commit this in order to continue developing the idea?
> > Might it be better to work on a branch, if this is still in the
> > proof-of-concept stage?
>
> I'd like to commit the patch at some point, if only to take advantage of the
> benefits of version control. I've no objection to working on a branch while
> these issues are addressed.
OK, I see you did this. That's fine.
> >> * subversion/include/svn_wc.h
> >> (svn_wc_props_modified2): New.
> >
> > Can you see if you can use the existing svn_wc_get_prop_diffs() instead?
> > I think there is no need for this new function. (If you do replace
> > svn_wc_props_modified_p() with svn_wc_props_modified2(), I would have
> > some comments on that.)
>
> svn_wc_prps_modified2() is simply a wrapper around the same function that
> svn_wc_props_modified_p() is wrapping, but exposing the additional which_props
> parameter. The functionality is already there, this patch just exposes it to
> the public API.
>
> svn_wc_prop_diffs() works at a slightly lower level of abstraction, require the
> caller to specify a set of properties to take the diff against. I'd prefer to
> avoid the extra work of building that set of properties, when we have an API
> that can give me the information which I want, essentially for free (in terms of
> programmer work, not execution time).
You misunderstood svn_wc_get_prop_diffs(). Both its "**propchanges" and
"**original_props" arguments are outputs. If you set original_props to
NULL it does exactly what you want.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] 'svn st --ignore-prop' (v3)
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Julian Foad wrote:
> Hyrum,
>
> this is going to sound inconsiderate after I've just reviewed the
> implementation, but I've just been thinking back to why we're doing
> this, and I am still not convinced that this is not over-generalising.
>
> At the UI level, I have no evidence that we need a general
> "--ignore-prop" feature, and, although we could commit this for
> experimentation, I would not like to see it in a released version
> without knowing it's valuable enough to justify its existence. (If it is
> valuable, then it seems likely that the converse feature for noticing
> ONLY certain properties would be needed too.)
>
> At the libsvn_client level, exactly the same applies.
To a large degree, this set of features is solving a personal itch. In working
with the 1.5.x branch, the mergeinfo modifications are largely noise in 'svn st'
and 'svn diff', and I can't find the revisions I'm looking for using 'svn log'.
That's what's motivating me to add this. I (currently) don't have any need for
the converse, so I haven't thought much about those use cases.
As for the generality, I can imagine, though I don't know of any personally,
circumstances where projects use custom properties on files for some purpose,
use like we use properties for mergeinfo. Why *not* provide the ability for
users to filter arbitrary properties, especially since it doesn't require any
additional work in the implementation?
I feel strongly that the implementation should be generic, since we using the
generic property mechanism for storing mergeinfo. Just so I make sure I can
understand correctly, are you concerned about exposing that generality through
the API? (e.g., using 'apr_hash_t *ignored_props', instead of 'svn_boolean_t
ignore_mergeinfo')
> Do you need to commit this in order to continue developing the idea?
> Might it be better to work on a branch, if this is still in the
> proof-of-concept stage?
I'd like to commit the patch at some point, if only to take advantage of the
benefits of version control. I've no objection to working on a branch while
these issues are addressed.
>
> A few more comments on the current implementation.
>
> On Wed, 2008-09-10 at 13:01 -0500, Hyrum K. Wright wrote:
> [...]
>> Yes, I do plan on making it work with 'status -u'. I think that lots of the
>> machinery in this patch can be reused in that case (it may already work!), but I
>> haven't yet thoroughly tested it.
>
> That's good.
>
> [...]
>>>> + 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 :-)
>> My initial reason for doing this was do that if I start seeing the
>> value 0xdeadbeef somewhere, I'd know from whence it was coming. But I
>> suppose that isn't really needed here, so I've gone with the somewhat
>> duller NULL. :)
>
> Heh. It doesn't work at all now! Sorry about that. I forgot that null
> values are not allowed; it means "delete this key".
>
> [...]
>> [[[
>> Add support for ignoring arbitrary property modifications in 'svn st'.
>> This currently only works locally, not with 'svn st -u'.
>>
>> * subversion/include/svn_wc.h
>> (svn_wc_props_modified2): New.
>
> Can you see if you can use the existing svn_wc_get_prop_diffs() instead?
> I think there is no need for this new function. (If you do replace
> svn_wc_props_modified_p() with svn_wc_props_modified2(), I would have
> some comments on that.)
svn_wc_prps_modified2() is simply a wrapper around the same function that
svn_wc_props_modified_p() is wrapping, but exposing the additional which_props
parameter. The functionality is already there, this patch just exposes it to
the public API.
svn_wc_prop_diffs() works at a slightly lower level of abstraction, require the
caller to specify a set of properties to take the diff against. I'd prefer to
avoid the extra work of building that set of properties, when we have an API
that can give me the information which I want, essentially for free (in terms of
programmer work, not execution time).
>> Index: subversion/svn/main.c
>> ===================================================================
>> --- subversion/svn/main.c (revision 33015)
>> +++ subversion/svn/main.c (working copy)
>> @@ -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")},
>
> Use lower-case "i", and spelling "ignore".
>
> Thanks for fixing the other bits.
Thanks for the review!
-Hyrum
Re: [PATCH] 'svn st --ignore-prop' (v3)
Posted by Julian Foad <ju...@btopenworld.com>.
Hyrum,
this is going to sound inconsiderate after I've just reviewed the
implementation, but I've just been thinking back to why we're doing
this, and I am still not convinced that this is not over-generalising.
At the UI level, I have no evidence that we need a general
"--ignore-prop" feature, and, although we could commit this for
experimentation, I would not like to see it in a released version
without knowing it's valuable enough to justify its existence. (If it is
valuable, then it seems likely that the converse feature for noticing
ONLY certain properties would be needed too.)
At the libsvn_client level, exactly the same applies.
Do you need to commit this in order to continue developing the idea?
Might it be better to work on a branch, if this is still in the
proof-of-concept stage?
A few more comments on the current implementation.
On Wed, 2008-09-10 at 13:01 -0500, Hyrum K. Wright wrote:
[...]
> Yes, I do plan on making it work with 'status -u'. I think that lots of the
> machinery in this patch can be reused in that case (it may already work!), but I
> haven't yet thoroughly tested it.
That's good.
[...]
> >> + 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 :-)
>
> My initial reason for doing this was do that if I start seeing the
> value 0xdeadbeef somewhere, I'd know from whence it was coming. But I
> suppose that isn't really needed here, so I've gone with the somewhat
> duller NULL. :)
Heh. It doesn't work at all now! Sorry about that. I forgot that null
values are not allowed; it means "delete this key".
[...]
> [[[
> Add support for ignoring arbitrary property modifications in 'svn st'.
> This currently only works locally, not with 'svn st -u'.
>
> * subversion/include/svn_wc.h
> (svn_wc_props_modified2): New.
Can you see if you can use the existing svn_wc_get_prop_diffs() instead?
I think there is no need for this new function. (If you do replace
svn_wc_props_modified_p() with svn_wc_props_modified2(), I would have
some comments on that.)
> Index: subversion/svn/main.c
> ===================================================================
> --- subversion/svn/main.c (revision 33015)
> +++ subversion/svn/main.c (working copy)
> @@ -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")},
Use lower-case "i", and spelling "ignore".
Thanks for fixing the other bits.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org