Hi!
On Sat 2003-07-26 at 23:49:06 +0000, Jilles Oldenbeuving wrote:
> /* This is my first (oh maybe my second) patch submission. I've
> * read all in HACKING, etc. So please, if I skip on some
> * rules (written or not) please tell me, i'll adapt my patch */
Some (minor) issues I noticed when looking over it, not that I would
have any more submitting experience than you. :-)
[...]
> So, thanks to Shlomi Fish, i've rewritten this patch so that it now
> correctly determines wether or not the repository is empty. This is done by
> first determining the number of files/directories under version control in
> the current directory. If this is > 0, bail out. Otherwise do the same for the
> parent directory. If this returns either a count of 0 or an error, the
> repository is empty and a helpfull message is displayed to the user.
>
> Anyway, typical output with this patch looks like:
>
> # on a really empty repository:
> jilles@lorien:~/empty_reptest$ svn st -v
> 0 ? ? .
> The repository is empty.
But the log message below claims
[...] If the repository is empty do not show the "0 0 ?"
but print a message for the user pointing out that the repository is
empty.
I.e. it claims that the "0 0 ?" would be gone, but it isn't.
Personally, I think, it's fine that the "0 0 ?" stays, so maybe you
just want to adjust the log message.
[...]
> # on a repository that had some files in it,
> # but not anymore:
> jilles@lorien:~/is_empty_again$ svn st -v
> 2 2 jilles .
> The repository is empty.
Hm. What happens, if I do this in an completely unversioned directory
by mistake, like /tmp or so? From looking at the patch, I guess, I get
also the "The repository is empty." message. If so, I find the new
message misleading.
In the ideal case, it would only displayed, if you really are
somewhere inside of an working copy.
Hm. That could be done by, instead of directly checking the current
and the parent, by looping "upwards" until you either find an
versioned directory (svn_client_ls not returning an error?) or there
is no "upwards" (don't know how to determine this in a cross-platform
way). And then, in case you got an versioned directory, look if it is
empty and if so, ony then print the message.
But even without that, I think your patch is fine and does the right
thing in most cases.
[...]
> +/* This function will determine if the repository is empty.
> + * Empty, here, means no files or directories are in the
> + * root of the repository (So, it may have had 1000 revisions,
> + * if the last revision all files were deleted, the repos is
> + * empty. If it finds this isn't the case, it does next to nothing.
> + * If it finds that the repos is not empty it will
There's a "not" too much.
> + * print a message to stout mentioning the emptyness of
> + * the repository (and maybe taking away some confusion
> + * from new users). */
[...]
> + int item_count_current_dir = 0;
> + int item_count_parent_dir = 0;
If I understood correctly, the style in this project is to *not*
initialize variables with default values (i.e. drop "= 0") because
that could unintenionally hide logical errors futher down.
> + svn_error_t *err;
> + apr_pool_t *subpool;
> +
> + ls_targets = targets;
> + subpool = svn_pool_create (pool);
> +
> + /* Count the number of files in the current directory,
> + * if thats > 0, we know the repos is not empty. Otherwise,
> + * count the number of files in the parent directory
> + * (if there is a parent directory). For an empty repos
> + * that should generate an error. */
> + svn_opt_push_implicit_dot_target(ls_targets, subpool);
> + ls_target = ((char **) (ls_targets->elts))[0];
> +
> + err = ( svn_client_ls (&ls_dirents,
> + ls_target,
> + &(opt_state->start_revision),
> + 0,
> + ctx,
> + subpool));
> +
> + if (err)
> + {
> + /* Doing an 'ls' on the current dir did generate
> + * an error. But because this functionality is not
> + * *that* important, just skip the error and
> + * return */
> + svn_error_clear(err);
> + item_count_parent_dir = 0;
First, it should probably be item_count_current_dir - we did not get
to the parent yet.
Second, the assigned value is never used anywhere below (after the
if-else-clause). If you meant that an error should simply prevent
further evaluation, this assignment is superfluous. If you meant an
error (do you get an error, if the directory is unversioned?) to
imply that the directory is empty, most of the else-clause doesn't
belong there, i.e. maybe you wanted something like this:
if (err)
{
svn_error_clear(err);
item_count_current_dir = 0;
}
else
{
item_count_current_dir = apr_hash_count(ls_dirents);
}
if (item_count_current_dir == 0)
{
[...]
> + }
> + else
> + {
> + item_count_current_dir = apr_hash_count(ls_dirents);
> +
> + if (item_count_current_dir == 0)
> + {
> + /* 2nd pass */
> + ls_target = "..";
> + err = ( svn_client_ls (&ls_dirents,
> + ls_target,
> + &(opt_state->start_revision),
> + 0,
> + ctx,
> + subpool));
> + if (err)
> + {
> + /* Fetching an 'ls' about the parent directory
> + * did not work out (maybe we're allready in the
> + * root directory). Clear the error and report
> + * the item_count as if it's 0 */
> + svn_error_clear(err);
> + item_count_parent_dir = 0;
> + }
> + else
> + {
> + item_count_parent_dir = apr_hash_count(ls_dirents);
> + }
> + if ((item_count_current_dir == 0)
> + && (item_count_parent_dir == 0))
You can only get here if item_count_current_dir is 0. So it is
redundant in the if-clause. Only test item_count_parent_dir.
> + {
> + printf("The repository is empty.\n");
> + }
> + }
> + }
> +
> + svn_pool_clear (subpool);
> + svn_pool_destroy (subpool);
> + return SVN_NO_ERROR;
> +}
> +
> /* This implements the `svn_opt_subcommand_t' interface. */
> svn_error_t *
> svn_cl__status (apr_getopt_t *os,
> @@ -90,6 +180,15 @@
> opt_state->quiet,
> subpool);
>
> + if (opt_state->verbose)
> + { /* Only show helpfull messages if asked to be
> + verbose. */
> + SVN_ERR (svn_cl__repos_empty (opt_state,
> + ctx,
> + targets,
> + subpool));
If you don't wrap the arguments, this still fits easily into 80 lines
(75, if I am not mistaken). Please avoid such wrapping, if it isn't
necessary (I don't know what the project policy is... this is my
personal opinion).
HTH,
Benjamin.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Jul 27 09:12:04 2003