You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@subversion.apache.org by kf...@collab.net on 2005/05/04 19:00:45 UTC

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

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