David Summers <david@summersoft.fay.ar.us> writes:
> I made the changes that people suggested, here is the revised edition:
In general, your patch is heading the right direction, but there are
some things I'd like to see cleared up before it gets committed.
First of all, you made no edits in the command-line client to show
this new usage. My suspicion is that we haven't really thought far
enough along to actually decide what the usage *should* be. Your code
only adds some attention to the first target passed to the function.
That works great if I do:
svn log http://svn.collab.net/repos/svn
But if I, while in a working copy not related to the above url, type:
svn log . http://svn.collab.net/repos/svn
I only see the log for the working copy I'm in, the second argument
seems to be completely ignored, without any feedback.
In other words, we need to either code for the complexities that can
occur when human are handling the input process, *or* enhance our
input validation (back in clients/cmdline/log-cmd.c:svn_cl__log) to
simply rule out all the cases we don't support.
My proposal:
usage: svn log WC_PATH1 [ WC_PATH2 [ WC_PATH2 ... ]]
svn log REPOS_URL
And have svn_cl__log verify that the input matches one of these.
> * ./subversion/libsvn_client/log.c (svn_client_log): Fixed svn log command
> to check to see if the argument passed is a URL, and if so to skip the
> check to see if it is a working copy.
>
> Index: ./subversion/libsvn_client/log.c
> ===================================================================
> --- ./subversion/libsvn_client/log.c
> +++ ./subversion/libsvn_client/log.c Sat Feb 9 13:21:17 2002
> @@ -62,6 +62,8 @@
> svn_stringbuf_t *basename;
> apr_array_header_t *condensed_targets;
> svn_revnum_t start_revnum, end_revnum;
> + svn_string_t path_str;
> + svn_stringbuf_t *path_strbuf;
>
> if ((start->kind == svn_client_revision_unspecified)
> || (end->kind == svn_client_revision_unspecified))
> @@ -74,17 +76,28 @@
> SVN_ERR (svn_path_condense_targets (&basename, &condensed_targets,
> targets, pool));
Here we are condensing targets, with no regard to whether those
targets are all URLs, all working copy paths, or some mix of those.
I think we only want to do this when we are looking at working copy
paths and ensure that condensed_targets is NULL in the URL case.
Again, in general, I like the patch. I'd just like to see it go the
whole distance.
[rest of original mail omitted]
---------------------------------------------------------------------
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:07 2006