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_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-09-11 19:34:30 CEST