"Benjamin Pflugmann" <benjamin-svn-dev@pflugmann.de> schreef in bericht
news:20030727071110.GJ26011@spin.de...
> 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.
Yeah, i'll have to adapt the log message. The "0 0 ?" might be
good to have for machine parsable output.
> > # 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.
I'll have to double check this, but IIRC somewhere before the
code of this patch got executed the svn client allready exits with
the message that the current directory is not under version control
(no working directory). So that takes care of that situation.
> 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.
See above comment.
> > +/* 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.
How stupid of me, good thing i don't have commit rights :-)
> > + * 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.
Will change.
> > + 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)
> {
It's superfluous, and should go. I'll remove it.
> > + }
> > + 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).
That too, i will change.
> HTH,
>
> Benjamin.
Thanks Benjamin, I didn't expect much attention for this patch, but you
really
looked at it! Thanks for your time and thoughts! (And stay tuned for my
resend :-)
Jilles Oldenbeuving
www.jillesoldenbeuving.nl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Jul 27 14:28:47 2003