Re: Updated [patch] new part 1 for log command

On Thu, Feb 28, 2002 at 06:10:02PM -0500, Ben Collins wrote:
>...
> +++ ./subversion/svnadmin/main.c Tue Feb 26 09:27:23 2002
> @@ -275,7 +275,7 @@
> {
> svn_revnum_t youngest_rev;
> svn_fs_root_t *rev_root;
> - apr_array_header_t *revs;
> + apr_array_header_t *revs, *paths;
> int i;
>
> if (argc != 4)
> @@ -284,14 +284,17 @@
> /* NOTREACHED */
> }
>
> + paths = apr_array_make (pool, 1, sizeof (svn_stringbuf_t *));
> + (*(svn_stringbuf_t **)apr_array_push(paths)) = svn_stringbuf_create(argv[3], pool);

See previous email re: stringbuf and the FS API.

>...
> +++ ./subversion/libsvn_fs/tree.c Wed Feb 27 22:05:48 2002
>...
> + /* Now sort the array and make it "go live". */
> + qsort(array->elts, array->nelts, array->elt_size,
> + svn_sort_compare_revisions);

Couldn't we end up with a lot of duplicates in there? Seems we should
probably remove dups.

>...
> @@ -2800,19 +2808,28 @@
> svn_error_t *
> svn_fs_revisions_changed (apr_array_header_t **revs,
> svn_fs_root_t *root,
> - const char *path,
> + const apr_array_header_t *paths,
> apr_pool_t *pool)
> {
> struct revisions_changed_args args;
> svn_fs_t *fs = svn_fs_root_fs (root);
> + int i;
> + svn_fs_id_t *tmp_id;
> + svn_stringbuf_t *this_path;

Generaly, we tend to put declarations into the "smallest scope possible".
Thus, the tmp_id and this_path variables could go into the for-loop block.

>...
> +++ ./subversion/libsvn_client/log.c Tue Feb 26 16:08:10 2002
> @@ -58,8 +58,8 @@
> svn_ra_plugin_t *ra_lib;
> void *ra_baton, *session;
> svn_stringbuf_t *URL;
> - svn_wc_entry_t *entry;
> - svn_stringbuf_t *basename;
> + svn_stringbuf_t *basename = NULL;

Why is this set to NULL? That doesn't seem to be needed. If so, then a
comment to clarify...

> + svn_string_t path_str;
> apr_array_header_t *condensed_targets;
> svn_revnum_t start_revnum, end_revnum;
>
> @@ -71,20 +71,68 @@
> "svn_client_log: caller failed to supply revision");
> }
>
> - SVN_ERR (svn_path_condense_targets (&basename, &condensed_targets,
> - targets, pool));
> + start_revnum = end_revnum = SVN_INVALID_REVNUM;
>
> - /* Get full URL from the common path, carefully. */
> - SVN_ERR (svn_wc_entry (&entry, basename, pool));
> - if (! entry)
> - return svn_error_createf
> - (SVN_ERR_UNVERSIONED_RESOURCE, 0, NULL, pool,
> - "svn_client_log: %s is not under revision control", basename->data);
> - if (! entry->url)
> - return svn_error_createf
> - (SVN_ERR_ENTRY_MISSING_URL, 0, NULL, pool,
> - "svn_client_log: entry '%s' has no URL", basename->data);
> - URL = svn_stringbuf_dup (entry->url, pool);
> + path_str.data = ((svn_stringbuf_t **)targets->elts)[0]->data;
> + path_str.len = ((svn_stringbuf_t **)targets->elts)[0]->len;
> +
> + /* Use the passed URL, if there is one. */
> + if (svn_path_is_url (&path_str))
> + {
> + svn_stringbuf_t *base, *file;
> +
> + /* The logic here is this: If we get passed one argument, we assume
> + * it is the full URL to a file/dir we want log info for. If we get
> + * a URL plus some paths, then we assume that the URL is the base,
> + * and that the paths passed are relative to it. */

I don't see where this logic occurs. There isn't a 'join' anywhere.

> + base = ((svn_stringbuf_t **) (targets->elts))[0];
> + svn_path_split (base, &basename, &file, pool);

This split is only used by the nelts == 1 case (to get the file). It should
move for clarity. Also, note that the nelts==1 case doesn't need basename,
so you can pass NULL for that parameter (not &foo where foo is NULL).

> + URL = svn_path_uri_encode(&path_str, pool);

This is only needed for the nelts > 1 case, so it should be moved.

>...
> +++ ./subversion/libsvn_repos/log.c Tue Feb 26 15:09:08 2002
> @@ -104,6 +104,8 @@
> apr_pool_t *subpool = svn_pool_create (pool);
> apr_hash_t *changed_paths = NULL;
> svn_fs_t *fs = repos->fs;
> + svn_fs_root_t *root;
> + apr_array_header_t *revs = NULL;
>
> if (start == SVN_INVALID_REVNUM)
> SVN_ERR (svn_fs_youngest_rev (&start, fs, pool));
> @@ -111,12 +113,32 @@
> if (end == SVN_INVALID_REVNUM)
> SVN_ERR (svn_fs_youngest_rev (&end, fs, pool));
>
> + if (paths && paths->nelts)
> + {
> + SVN_ERR (svn_fs_revision_root (&root, fs, (start > end) ? start : end, subpool));
> + SVN_ERR (svn_fs_revisions_changed (&revs, root, paths, subpool));
> + if (!revs || !revs->nelts)
> + return SVN_NO_ERROR;

The indentiation here looks weird. Tab characters? Please avoid the use of
hard tabs in the code. In emacs, we ues (setq indent-tabs-mode nil)

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org