[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r21684 - in trunk/tools/hook-scripts/mailer: . tests

From: C. Michael Pilato <cmpilato_at_collab.net>
Date: 2006-09-28 03:35:27 CEST

wein@tigris.org wrote:
> Author: wein
> Date: Wed Sep 27 12:56:57 2006
> New Revision: 21684
>
> Log:
> Add property handling to mailer.py's commit messages.
>
> It is now possible to also report changes of properties (including
> diffs) with a commit message. Therefor several new classes were
                                       ^^^ "Therefore"
> added to mailer.py. To configure the reporting of property changes
> four new parameters are added to mailer.conf. Without adding these
> parameters to an existing configuration file no property changes
> will be reported.
>
> New configuration parameters:
> show_props configure which property changes to report
> (valid options are any combination of
> 'add_path add copy_path modify delete_path
> delete')
> ignore_props configure which property names shall be ignored
> in the list of changed properties
> generate_propdiffs configure for which property changes diffs
> shall be created
> (valid options are any combination of
> 'add_path add copy_path modify delete_path
> delete')
> ignore_propdiffs configure which property names shall be ignored
> when creating diffs for changed properties
>
> * tools/hook-scripts/mailer/mailer.py
> New classes:
> PropSelections
> PropDiffSelections
> PropDiffGenerator.
> New functions:
> TextCommitRenderer._get_prop_list
> TextCommitRenderer._render_props
> TextCommitRenderer.__render_props
> TextCommitRenderer._render_propdiffs
> TextCommitRenderer.__render_propdiffs.
> Several appropriate changes to other classes and functions.
>
> * tools/hook-scripts/mailer/mailer.conf.example
> New parameters show_props, ignore_props, generate_propdiffs and
> ignore_propdiffs.
>
> * tools/hook-scripts/mailer/tests/mailer.conf
> New parameters show_props, ignore_props, generate_propdiffs and
> ignore_propdiffs.
>
> * tools/hook-scripts/mailer/tests/mailer-t1.output
> Now containing property changes.
>
> Modified:
> trunk/tools/hook-scripts/mailer/mailer.conf.example
> trunk/tools/hook-scripts/mailer/mailer.py
> trunk/tools/hook-scripts/mailer/tests/mailer-t1.output
> trunk/tools/hook-scripts/mailer/tests/mailer.conf
>
> Modified: trunk/tools/hook-scripts/mailer/mailer.conf.example
> URL: http://svn.collab.net/viewvc/svn/trunk/tools/hook-scripts/mailer/mailer.conf.example?pathrev=21684&r1=21683&r2=21684
> ==============================================================================
> --- trunk/tools/hook-scripts/mailer/mailer.conf.example (original)
> +++ trunk/tools/hook-scripts/mailer/mailer.conf.example Wed Sep 27 12:56:57 2006
> @@ -193,6 +193,52 @@
> # delete: generates diffs for all removed paths
> generate_diffs = add copy modify
>
> +# Specify which types of property changes mailer.py will report as a
> +# summary at the top of the message, after the summary of changed paths.
> +# Valid options are any combination of 'add_path add copy_path modify
> +# delete_path delete'.
> +# If the show_props option is empty, no list of changed properties will
> +# be created.
> +# Meaning of the possible values:
> +# add_path: lists all properties added with a newly added paths
> +# add: lists all added properties not including the ones added
> +# for newly added paths
> +# copy_path: lists all properties which were implicitly added by
> +# making a copy of a path and not changed or removed after
> +# copying
> +# modify: lists all modified properties, including the ones that
> +# were added by making a copy of a path and modified
> +# afterwards (within the same commit)
> +# delete_path: lists all properties removed with a path
> +# delete: lists all removed properties not including the ones for
> +# removed paths
> +show_props = add modify delete

That's, uh, purty complex stuff you've got there. You sure all that is
necessary?

> +
> +# When set to a regular expression all properties matching this regular
> +# expression will not be included in the summary of changed properties.
> +# Example (ignore all internal properties, i. e. all properties
> +# starting with 'svn:'):
> +# ignore_props = ^svn:
> +ignore_props = ^svn:

I'm not sure if the example file should actually ignore the svn:
properties -- ideally, a person should get the typically desired
behavior with the minimal number of edits to a copy of those sample
file, and I suspect most folks want to see that svn: properties were
changed.

> +
> +# Specify which types of property changes mailer.py will create
> +# diffs for. Valid options are any combination of
> +# 'add_path add copy_path modify delete_path delete'. If the
> +# generate_propdiffs option is empty, no diffs are created.
> +# Note that this only affects the display of property diffs - the mentioning
> +# of property changes in the summary at the top of the message is regulated
> +# by show_props and listed regardless of this option's value.
> +# See show_props for an explanation of the possible values.
> +generate_propdiffs = add modify delete
> +
> +# When set to a regular expression all properties matching this regular
> +# expression will be ignored by the selection made by
> +# generate_propdiffs.
> +# Example (ignore all internal properties, i. e. all properties
> +# starting with 'svn:'):
> +# ignore_propdiffs = ^svn:
> +ignore_propdiffs = ^svn:

See comment above regarding "ignore_props".

> # Commit URL construction. This adds a URL to the top of the message
> # that can lead the reader to a Trac, ViewVC or other view of the
> # commit as a whole.
> @@ -232,8 +278,9 @@
> # match, this variable controls the behaviour for the non-matching
> # paths. Possible values are:
> #
> -# yes: (Default) Show in both summary and diffs.
> -# summary: Show the changed paths in the summary, but omit the diffs.
> +# yes: (Default) Show in both summary and diffs (including properties).
> +# summary: Show the changed paths in the summary, but omit the diffs
> +# (and the property diffs).
> # no: Show nothing more than a note saying "and changes in other areas"
> #
> show_nonmatching_paths = yes

[...]

> Modified: trunk/tools/hook-scripts/mailer/tests/mailer-t1.output
> URL: http://svn.collab.net/viewvc/svn/trunk/tools/hook-scripts/mailer/tests/mailer-t1.output?pathrev=21684&r1=21683&r2=21684
> ==============================================================================
> --- trunk/tools/hook-scripts/mailer/tests/mailer-t1.output (original)
> +++ trunk/tools/hook-scripts/mailer/tests/mailer-t1.output Wed Sep 27 12:56:57 2006

[...]

> @@ -204,6 +238,19 @@
> file1 (props changed)
> file2 (contents, props changed)
>
> +Property Changes:
> + dir1/
> + Added:
> + prop3
> + file1
> + Added:
> + prop1
> + file2
> + Added:
> + prop1
> + svn:keywords
> + svn:new_svn_prop

Wow. I *really* don't see the utility of this whole "Property Changes"
section. Besides being positively atrocious in term of the physical
layout (level four nesting?!), I just really fail to see the utility of
having both this and the property diff section.

I would strongly favor ditching this section altogether (and the
configury associated with it).

> +Added property for: dir1/ (from r1, dir1)
> +==============================================================================
> +--- /dev/null 00:00:00 1970 (empty, because property is newly added)
> ++++ dir1|prop3 Sun Sep 9 04:33:20 2001 (r2)
> +@@ -0,0 +1 @@
> ++propval3
> +

Yikes?! Does that actually say "/dev/null"? And contain a malformatted
date string? And bother to explain "empty, because property is newly
added"?

Also, what kind of problems can we expect to be caused by the fact that
this diff output is something that appears (as far as I can tell) to be
something 'patch' would actually try to apply? That's bad news.

~ ~ ~ ~ ~

In summary, I'm all for mailer.py showing diffs of properties, but those
diffs *must not* appear as something 'patch' will try to understand and
apply. Further, I'm very much opposed to the propchange summary
section, which I believe to be visually unwieldy and largely redundant
with true property diffs. Finally, I think the sample file should
reflect recommended default settings, even if more interesting things
are explained yet commented-out.

-- 
C. Michael Pilato <cmpilato@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

Received on Thu Sep 28 03:35:43 2006

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.