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

Re: svn commit: r14098 - in trunk: . subversion/libsvn_client subversion/tests/clients/cmdline

From: <kfogel_at_collab.net>
Date: 2005-05-04 21:00:45 CEST

jszakmeister@tigris.org writes:
> --- trunk/subversion/libsvn_client/cat.c (original)
> +++ trunk/subversion/libsvn_client/cat.c Mon Apr 11 05:08:14 2005
> @@ -37,6 +37,8 @@
>
> /*** Code. ***/
>
> +/* Helper function to handle copying a potentially translated verison of BASE
> + or WORKING revision of a file to an output stream. */
> static svn_error_t *
> cat_local_file (const char *path,
> svn_stream_t *output,

Go all the way and document the function according to the full
standards: document every parameter, and the return value, etc.
It's important for internal functions too, not just public APIs.

For example, from the current doc string, I can't tell whether any
value is allowable for REVISION. Is it limited to just
svn_opt_revision_working and svn_opt_revision_base? (See below for
more on that.)

Also, typo: "version" not "verison" :-).

The rest of this is from r14081, not r14098:

> + if (revision->kind != svn_opt_revision_working)
> + {
> + SVN_ERR (svn_wc_get_pristine_copy_path (path, &base, pool));
> + SVN_ERR (svn_wc_get_prop_diffs (NULL, &props, path, adm_access, pool));
> + }
> + else
> + {
> + svn_wc_status2_t *status;
> +
> + base = path;
> + SVN_ERR (svn_wc_prop_list (&props, path, adm_access, pool));
> + SVN_ERR (svn_wc_status2 (&status, path, adm_access, pool));
> + if (status->text_status != svn_wc_status_normal)
> + local_mod = TRUE;
> + }

This just assumes that if it's not svn_opt_revision_working, it must
be svn_opt_revision_base. If those are the only acceptable two, then
we should check strictly for that, and error if passed something else.

> + if (local_mod && (! special))
> + {
> + /* Use the modified time from the working copy if
> + the file */
> + SVN_ERR (svn_io_file_affected_time (&tm, path, pool));
> + }
> + else
> + {
> + tm = entry->cmt_date;
> + }

"of the file" not "if the file". (Being picky about this typo because
it can lead to genuine misunderstanding -- especially without the
period at the end, someone might wonder if it was just a sentence left
unfinished. Of course, reading the code clarifies it immediately.)

In svn_client_cat2():

> + if (! svn_path_is_url (path_or_url)
> + && (peg_revision->kind == svn_opt_revision_base
> + || peg_revision->kind == svn_opt_revision_committed
> + || peg_revision->kind == svn_opt_revision_unspecified)
> + && (revision->kind == svn_opt_revision_base
> + || revision->kind == svn_opt_revision_committed
> + || revision->kind == svn_opt_revision_unspecified))
> + {
> + svn_wc_adm_access_t *adm_access;
> +
> + SVN_ERR (svn_wc_adm_open3 (&adm_access, NULL,
> + svn_path_dirname (path_or_url, pool), FALSE,
> + 0, ctx->cancel_func, ctx->cancel_baton,
> + pool));
> +
> + SVN_ERR (cat_local_file (path_or_url, out, adm_access, revision, pool));
> +
> + SVN_ERR (svn_wc_adm_close (adm_access));
> +
> + return SVN_NO_ERROR;
> + }

This is tricky.

Formally, I'm not sure that peg_revision matters here. I mean, you've
already determined that path_or_url is a local path. If you also know
that revision is BASE or COMMITTED or something else that can be found
locally, then from a pure correctness standpoint it really doesn't
matter what peg_revision is -- the only question is, have you got the
appropriate revision of that node at hand locally.

However, I can see why (as a code simplification) you might want to
check that peg_revision is also locally determinable here, so that you
don't need special cases to handle identity switcheroos in the node's
history.

If this is what's going on, maybe a comment explaining this would be a
good idea?

-Karl

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed May 4 21:33:44 2005

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