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

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