You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Jilles Oldenbeuving <oj...@hotmail.com> on 2003/07/26 23:49:06 UTC
[PATCH] 'svn st -v' looks weird on an empty repository (Issue:708) (resend)
/* 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 */
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.
jilles@lorien:~/empty_reptest$ touch unversioned_file
jilles@lorien:~/empty_reptest$ svn st -v
0 ? ? .
? unversioned_file
The repository is empty.
# 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.
# on a repository that does have files, and
# also on an empty subdirectory:
jilles@lorien:~/filled_rep$ svn st -v
1 1 jilles .
1 1 jilles hoi
2 2 jilles versioneddir
jilles@lorien:~/filled_rep$ cd versioneddir/
jilles@lorien:~/filled_rep/versioneddir$ svn st -v
2 2 jilles .
jilles@lorien:~/filled_rep/versioneddir$
So this is the intended behavior. I've attached the patch
because it contained a ^L that looked funny in my news
client, so I wanted to be on the save side.
Thanks for all the help,
Jilles Oldenbeuving
----------------------------------------
LOG:
Fix issue #708: 'svn st -v' looks weird on an empty repository
* status.c
(svn_cl__print_status_list): Fixed issue #708. 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. (This only
works when the verbose option (-v) is passed to the client.)
(svn_cl__repos_empty): New Function.
_________________________________________________________________
Protect your PC - get McAfee.com VirusScan Online
http://clinic.mcafee.com/clinic/ibuy/campaign.asp?cid=3963
Re: [PATCH] 'svn st -v' looks weird on an empty repository (Issue:708) (resend)
Posted by Jilles Oldenbeuving <oj...@hotmail.com>.
> > I think not. From looking at the patch (mind you, I don't know much
> > about svn internals, so I could be dead wrong ;), it just determines
> > how many entries (dirs/files) the local .svn-dir knows about.
>
> I think Garrett is right, calling svn_client_ls is going to access the
> repository which is not something 'svn status -v' is supposed to do.
I'm going to test this as soon as I have time. I think the solution to this
problem should not add the extra depency of having to work online.
> I don't really understand the problem in issue 708, it claims the
> output looks "weird", but doesn't explain why or what should change.
>
> $ svn st -v
> 0 ? ? .
> $ svn st -vu
> 0 ? ? .
> Head revision: 0
>
> The current behaviour looks reasonable to me.
The issue was raised because people new to subversion tend to start out with
an empty repository. Doning 'st -v' then for those people "0 ? ?" is not
very descriptive.
--
Jilles Oldenbeuving
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] 'svn st -v' looks weird on an empty repository
(Issue:708) (resend)
Posted by Philip Martin <ph...@codematters.co.uk>.
Benjamin Pflugmann <be...@pflugmann.de> writes:
> On Sat 2003-07-26 at 21:32:50 -0400, Garrett Rooney wrote:
>>
>> Umh, doesn't this make 'svn status -v' contact the repository in cases
>> where it previously would not?
>
> I think not. From looking at the patch (mind you, I don't know much
> about svn internals, so I could be dead wrong ;), it just determines
> how many entries (dirs/files) the local .svn-dir knows about.
I think Garrett is right, calling svn_client_ls is going to access the
repository which is not something 'svn status -v' is supposed to do.
I don't really understand the problem in issue 708, it claims the
output looks "weird", but doesn't explain why or what should change.
$ svn st -v
0 ? ? .
$ svn st -vu
0 ? ? .
Head revision: 0
The current behaviour looks reasonable to me.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] 'svn st -v' looks weird on an empty repository (Issue:708) (resend)
Posted by Benjamin Pflugmann <be...@pflugmann.de>.
Hi!
On Sat 2003-07-26 at 21:32:50 -0400, Garrett Rooney wrote:
> Jilles Oldenbeuving wrote:
[...]
> > 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.
>
> Umh, doesn't this make 'svn status -v' contact the repository in cases
> where it previously would not?
I think not. From looking at the patch (mind you, I don't know much
about svn internals, so I could be dead wrong ;), it just determines
how many entries (dirs/files) the local .svn-dir knows about.
IIRC, this was even one of the issues mentioned with the first
incarnation of this patch...
Bye,
Benjamin.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] 'svn st -v' looks weird on an empty repository (Issue:708)
(resend)
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
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 */
>
> 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.
Umh, doesn't this make 'svn status -v' contact the repository in cases
where it previously would not? I'm not sure I like the idea of causing
a potential network access whenever you happen to do an 'svn status -v'
in an empty directory just because it makes the output look a little
nicer...
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] 'svn st -v' looks weird on an empty repository (Issue:708) (resend)
Posted by Jilles Oldenbeuving <oj...@hotmail.com>.
"Benjamin Pflugmann" <be...@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
Re: [PATCH] 'svn st -v' looks weird on an empty repository (Issue:708) (resend)
Posted by Benjamin Pflugmann <be...@pflugmann.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.
[...]
> # 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