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

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