[ only a cursory review... detailed upon checkin or maybe next post ]
On Thu, Jan 31, 2002 at 11:53:20PM -0600, Karl Fogel wrote:
>...
> svn_error_t *
> svn_client_checkout (const svn_delta_edit_fns_t *before_editor,
> void *before_edit_baton,
> @@ -191,39 +199,37 @@
> svn_client_auth_baton_t *auth_baton,
> svn_stringbuf_t *URL,
> svn_stringbuf_t *path,
> - svn_revnum_t revision,
> + svn_client_revision_t *revision,
These should be "const svn_client_revision_t *revision" parameters.
>...
> +++ ./subversion/libsvn_client/switch.c Thu Jan 31 22:15:34 2002
>...
> @@ -79,13 +79,6 @@
> assert (switch_url != NULL);
> assert (switch_url->len > 0);
>
> - /* If both REVISION and TM are specified, this is an error.
> - They mostly likely contradict one another. */
> - if ((revision != SVN_INVALID_REVNUM) && tm)
> - return
> - svn_error_create(SVN_ERR_CL_MUTUALLY_EXCLUSIVE_ARGS, 0, NULL, pool,
> - "Cannot specify _both_ revision and time.");
Who is performing this consistency check now?
svn_client__get_revision_number ?
Does that function have enough context to do the right checks?
>...
> +svn_client_diff (const apr_array_header_t *diff_options,
> svn_client_auth_baton_t *auth_baton,
> - svn_revnum_t start_revision,
> - apr_time_t start_date,
> - svn_revnum_t end_revision,
> - apr_time_t end_date,
> + svn_client_revision_t *start,
> + svn_client_revision_t *end,
> + svn_stringbuf_t *path,
> svn_boolean_t recurse,
> apr_pool_t *pool)
> {
> + svn_revnum_t start_revnum, end_revnum;
> svn_string_t path_str;
> svn_boolean_t path_is_url;
> svn_boolean_t use_admin;
> @@ -168,13 +167,14 @@
> void *diff_edit_baton;
> struct diff_cmd_baton diff_cmd_baton;
>
> - /* If both a revision and a date/time are specified, this is an error.
> - They mostly likely contradict one another. */
> - if ((SVN_IS_VALID_REVNUM(start_revision) && start_date)
> - || (SVN_IS_VALID_REVNUM(end_revision) && end_date))
> - return
> - svn_error_create(SVN_ERR_CL_MUTUALLY_EXCLUSIVE_ARGS, 0, NULL, pool,
> - "Cannot specify _both_ revision and time.");
> + /* Sanity check. */
> + if ((start->kind == svn_client_revision_unspecified)
> + || (end->kind == svn_client_revision_unspecified))
> + {
> + return svn_error_create
> + (SVN_ERR_CLIENT_BAD_REVISION, 0, NULL, pool,
> + "svn_client_diff: caller failed to specify any revisions");
> + }
Why does this one do it, yet the others don't?
>...
> + /* ### todo: This whole svn_wc_entry_t thing is a mess, with
> + some kinds of data being "first class", in that they get
> + fields directly in the structure, while other kinds need
> + manual retrieval (augh!) from the hash. There are now
> + several places in Subversion like this, where we grab the
> + value from the entry's attribute hash, and convert it
> + from string to revnum. Wouldn't it be nice to make some
> + sort of accessor layer to svn_wc_entry_t? Or would that
> + just be adding more bureaucracy? Perhaps the committed
> + rev should simply be invited into the first class lounge,
> + where it can enjoy free martinis with current rev? */
Hey! I'll party with those guys... martinis?! Sure... I'm in :-)
>...
> +++ ./subversion/clients/cmdline/cl.h Fri Feb 1 00:12:55 2002
> @@ -57,15 +57,10 @@
> commands. */
> typedef struct svn_cl__opt_state_t
> {
> - /* You probably want start_revision to default SVN_INVALID_REVNUM
> - (which means `head' to the RA layer), and end_revision to default
> - to 0 or 1, which are the two possibilities for oldest revision. */
> - svn_revnum_t start_revision; /* X in "svn blah -r X" or "svn blah -r X:Y" */
> - svn_revnum_t end_revision; /* Y in "svn blah -r X:Y" */
> -
> - /* These should default to 0. */
> - apr_time_t start_date; /* X in "svn blah -D X" or "svn blah -D X:Y" */
> - apr_time_t end_date; /* Y in "svn blah -D X:Y" */
> + /* These get set as a result of revisions or dates being specified.
> + When only one revision is given, it is stored in start_revision,
> + and end_revision remains `svn_client_revision_unspecified'. */
> + svn_client_revision_t *start_revision, *end_revision;
Repeat after me: const. const. const. const....
:-)
However, I would simply recommend making those embedded structures. I'm not
sure that I see a reason to allocate that stuff on the heap. Especially when
the structure can represent "unspecified" explicitly (rather than using a
NULL pointer to say the same).
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 21 14:37:02 2006