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

Re: [PATCH] Extra options for libsvn_diff

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: 2006-02-17 03:10:16 CET

Peter N. Lundblad wrote:

> ------------------------------------------------------------------------
> r18487 | lundblad | 2006-02-16 22:23:05 +0000 (Thu, 16 Feb 2006) | 44 lines
> Changed paths:
> M /trunk/subversion/include/svn_diff.h
> M /trunk/subversion/include/svn_error_codes.h
> M /trunk/subversion/libsvn_client/diff.c
> M /trunk/subversion/libsvn_diff/diff_file.c
>
> Add options to ignore whitespace (all whitespace and changes in
> sequences of whitespace) and to ignore EOL style to our internal diff
> library. Use this functionality in the client diff functions.
[...]
> Review by: julianfoad

Oh, I did point out two little things when you sent an earlier version of this
to the mailing list, but that wasn't a review, it was just a quick skim through
the email. Sorry I didn't bother to say whether it was or wasn't.

This is a review. :-)

One other thing in the log message: it doesn't mention that you changed
svn_error_codes.h.

I think the "parse command-line options" function should be at a much higher
level, at least in libsvn_client and really in "svn" itself.
Implementation-wise it's trivial, but conceptually a library shouldn't know
about command-line options, as the concept doesn't apply to other clients.

Other than that, there are just a few documentation nits and a suggestion.

> Index: subversion/include/svn_diff.h
> ===================================================================

> +typedef struct svn_diff_file_options_t
> +{
> + /** To what extent whitespace should be ignored when comparing lines.
> + * The default is @c svn_diff_file_ignore_space_none. */

It's a bit odd for "The default" to be documented here; this is only a data
type, and different functions could provide different default values for an
object of this type.

> + svn_diff_file_ignore_space_t ignore_space;
> + /** Whether to treat all end-of-line markers the same when comparing lines.
> + * The default is @c FALSE. */
> + svn_boolean_t ignore_eol_style;

I think you might do well to have a set of boolean flags instead of these
individual fields. It seems likely that we will want to add a few more aspects
of space-handling behaviour.

> +} svn_diff_file_options_t;
> +
> +/** Allocate a @c svn_diff_file_options_t structure in @a pool, initializing
> + * it with default values.
> + *
> + * @since New in 1.4.
> + */
> +svn_diff_file_options_t *
> +svn_diff_file_options_create(apr_pool_t *pool);
> +
> +/**
> + * Parse @a args, an array of <ttconst char *</tt> command line switches

Missing close-angle-bracket.

> + * and adjust @a options accordingly. @a options is assumed to be initialized
> + * with default values. @a pool is used for temporary allocation.

It's not clear here whether "default values" must be the defaults provided by
options_create() or can be the caller's own choice of defaults. Can we say,
"assumed to be initialized with valid values," to explicitly allow a caller to
fill in its own defaults first and then parse options to override some of them.

> Index: subversion/libsvn_diff/diff_file.c
> ===================================================================

> +/* Normalize the characters pointed to by BUF of length *LENGTTHP, starting
> + * in state *STATEP according to the OPTIONS.

Spelling: "LENGTHP" and "OPTS".

> + * Adjust *LENGTHP and *STATEP to be the length of the normalized buffer and
> + * the final state, respectively.
> + * The normalization is done in-place, so the new length will be <= the old. */
> +static void
> +normalize(char *buf, apr_off_t *lengthp, normalize_state_t *statep,
> + const svn_diff_file_options_t *opts)
> +{
[...]

I had a bit of a look through the implementation code, though not in great
detail, and it seems OK, and I think others have studied it and you have tested it.

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Feb 17 03:10:58 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.