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