On Wednesday 04 May 2005 15:00, kfogel@collab.net wrote:
> 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.)
Hmph... I was following the pattern that I saw in other files (the
internal functions were document a lot less formally than
public/semi-public ones). I can go back and update this though. I
assume that it should be doxygenified? :-)
> Also, typo: "version" not "verison" :-).
I believe the typo was corrected in r14133.
> 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.
svn_opt_revision_working, svn_opt_revision_base, and
svn_opt_revision_comitted are the acceptable values. I'll make sure to
document that as well as put in a few assert() statements.
> > + 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.)
Will fix.
> 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?
I missed some of the peg discussions, so I wasn't certain if it was really
feasible to specify a peg revision on a local path. I actually tried it
on the command line, and the peg revision is something other than
unspecified if you @REVISION syntax (my-local-file@BASE, for example).
From my understanding of the peg revision, it's where we're supposed to
start looking as far as history tracing is concerned. If it's anything
other than unspecified, base, or committed, then I assumed we'd have to
ask the repository for that information (how would I know if the file was
called x at revision y without contacting the repository?). Hence the
check. If I'm wrong, please enlighten me, and I'll either remove the
redundant code or document why I chose to check the peg revision.
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed May 4 22:34:21 2005