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