You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@codematters.co.uk> on 2002/01/25 21:07:32 UTC
revision/date handling in the client library
Hello
Revision/date handling in the client library interface is less than
perfect. At present we get four values from the command line
svn_revnum_t start_revision;
svn_revnum_t end_revision;
apr_time_t start_date;
apr_time_t end_date;
Sometimes both a number and a date are passed to the client library to
define a revision (e.g. svn_client_diff), and sometimes only a number
(e.g. svn_client_log). Those that take both tend to return an error if
both are specified. In addition since all apr_time_t values are valid
times it is not possible to properly determine whether a date has been
specified. Adding a validity flag and passing three paramaters to
define one revision is silly. The application cannot do the
date-to-number conversion since that requires an ra session, which it
doesn't at present have.
Based on an earlier idea from Branko, how about
enum svn_client_revision_type {
svn_client_revision_unspecified,
svn_client_revision_number, /* -r NUMBER */
svn_client_revision_date, /* -D DATE */
svn_client_revision_commited, /* .svn/entries commited-rev */
svn_client_revision_previous, /* .svn/entries commited-rev-1 Is this useful? */
svn_client_revision_current, /* .svn/entries revision */
svn_client_revision_head /* -r HEAD, RA youngest */
};
typedef struct svn_client_revision_s {
enum svn_client_revision_type type;
svn_revnum_t number;
apr_time_t date;
} svn_client_revision_t;
#define SVN_CLIENT_REVISION_IS_UNSPECIFIED(r) \
((r).type == svn_client_revision_unspecified)
/* return revision number in *number using ra if required */
svn_error_t *
svn_cl__get_revision_number (svn_ra_plugin_t *ra_lib,
void *session,
svn_client_revision_t *revision,
const svn_stringbuf_t *path,
svn_revnum_t *number);
Now have the application parse the command line into two
svn_client_revision_t structures, perhaps complaining about attempts
to specify both a date and time for the same revision. Then always
pass svn_client_revision_t* into the client library routines to
define a revision. Handling in the client is along the lines
if (SVN_CLIENT_REVISION_IS_UNSPECIFIED(*rev))
complain if appropriate
SVN_ERR (svn_cl__get_revision_number (ra_lib, session, path, rev, &num));
use num and forget all about dates/HEAD/whatever
Look at the ugly revision handling in svn_client_diff to see how bad
it gets at present.
Comments?
--
Philip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: revision/date handling in the client library
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Philip Martin <ph...@codematters.co.uk> writes:
> Originally, I introduced svn_client_revision_unspecified in
> svn_client_revision_type because svn_client_diff needs to know whether
> there was a -r on the command line. This is ugly. I now think that the
> svn_client_revision_type should not have this setting, unspecified
> revisions should be limited to the application.
>
> The function svn_client_diff should be split into three functions,
> svn_client_diff_text_base_to_wc with no revision parameters,
> svn_client_diff_repository_to_wc with one revision parameter, and
> svn_client_diff_repository_to_repository with two revision parameters,
> and svn_cl__diff should decide which one to call based on the number
> of revisions specified on the command line.
Mmmm. I think I'm going to keep that logic in libsvn_client. If
every client would have to implement it anyway, then we'd be making
more work for people; but also, I feel it's libsvn_client's job to
understand what things require contacting the repository and which
things can be done with the wc. The client itself shouldn't have to
know this stuff -- it should be as independent as it can from the
questions of wc vs repository.
I do understand your motivation, but keep in mind that this logic
would all have to be repeated again with merge. Let's make it all
happen at one place in libsvn_client, rather than force all clients to
go through the formalities.
> The advantage of doing this is that the client functions do not need
> to deal with unspecified revisions. It also solves the current diff
> problem, where if -DDATE1:DATE2 has two dates that resolve to the same
> revision then diff compares the revision against the wc which is
> wrong.
+1 I'll find some other way to solve this problem, yes.
> Next, svn_client_revision_commited and svn_client_revision_current
> (and svn_client_revision_previous if included) are a little tricky. I
> was assuming that svn_client__get_revision_number would extract the
> relevant revision from the entries file, and then that revision number
> would be used as if specified explicitly on the command line. When
> operating recursively this would mean that "current" referred to the
> directory and not to the elements within which may have different
> current revision numbers. Personally I would find this acceptable, but
> it would need to be made clear to the users.
Gotcha.
> The application should reject attempts to specify two revisions for
> commands that only accept one, 'svn -r1:2 up' should fail for example.
> It should probably also reject -rREV:REV a repeated revision.
+1
> The application currently initialises start_revision to
> SVN_INVALID_REVNUM and end_revision to 1. In the case of the log
> command I see no reason for the HEAD:1 default, HEAD:0 would be
> better.
Yup, will do.
Philip, thanks so much for all the thought you've been putting into
this, it's really helpful. Please review my [upcoming] commits if you
have time to.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: revision/date handling in the client library
Posted by Philip Martin <ph...@codematters.co.uk>.
Karl Fogel <kf...@newton.ch.collab.net> writes:
> Philip Martin <ph...@codematters.co.uk> writes:
> > I may give it a go in the next few days, but if you want to do it I
> > have no strong attachment to the task :)
>
> I'm not likely to do it before Monday, so if you start it before then,
> it's all yours. :-) Otherwise, I probably will start it then.
I'm not going to do before then, so it's all yours. I have thought a
bit more about the problem, so I'll add some further comments.
My original idea was for a libsvn_client interface (the mail included
an svn_cl__ prototype but that was a typo), however I would now use
something like:
enum svn_client_revision_type {
svn_client_revision_number, /* -r NUMBER */
svn_client_revision_date, /* -D DATE */
svn_client_revision_commited, /* .svn/entries commited-rev */
svn_client_revision_previous, /* .svn/entries commited-rev-1 ??? */
svn_client_revision_current, /* .svn/entries revision */
svn_client_revision_head /* -r HEAD, RA youngest */
};
typedef struct svn_client_revision_t {
enum svn_client_revision_type type;
union {
svn_revnum_t number;
apr_time_t date;
} value;
} svn_client_revision_t;
svn_error_t *
svn_client__get_revision_number (svn_ra_plugin_t *ra_lib,
void *session,
svn_client_revision_t *revision,
const svn_stringbuf_t *path,
svn_revnum_t *number,
apr_pool_t *pool);
struct svn_cl__revision_t {
svn_client_revision_t revision;
svn_boolean_t specified;
};
Originally, I introduced svn_client_revision_unspecified in
svn_client_revision_type because svn_client_diff needs to know whether
there was a -r on the command line. This is ugly. I now think that the
svn_client_revision_type should not have this setting, unspecified
revisions should be limited to the application.
The function svn_client_diff should be split into three functions,
svn_client_diff_text_base_to_wc with no revision parameters,
svn_client_diff_repository_to_wc with one revision parameter, and
svn_client_diff_repository_to_repository with two revision parameters,
and svn_cl__diff should decide which one to call based on the number
of revisions specified on the command line.
The advantage of doing this is that the client functions do not need
to deal with unspecified revisions. It also solves the current diff
problem, where if -DDATE1:DATE2 has two dates that resolve to the same
revision then diff compares the revision against the wc which is
wrong.
Next, svn_client_revision_commited and svn_client_revision_current
(and svn_client_revision_previous if included) are a little tricky. I
was assuming that svn_client__get_revision_number would extract the
relevant revision from the entries file, and then that revision number
would be used as if specified explicitly on the command line. When
operating recursively this would mean that "current" referred to the
directory and not to the elements within which may have different
current revision numbers. Personally I would find this acceptable, but
it would need to be made clear to the users.
Finally, a couple of minor points:
The application should reject attempts to specify two revisions for
commands that only accept one, 'svn -r1:2 up' should fail for example.
It should probably also reject -rREV:REV a repeated revision.
The application currently initialises start_revision to
SVN_INVALID_REVNUM and end_revision to 1. In the case of the log
command I see no reason for the HEAD:1 default, HEAD:0 would be
better.
--
Philip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: revision/date handling in the client library
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Philip Martin <ph...@codematters.co.uk> writes:
> I may give it a go in the next few days, but if you want to do it I
> have no strong attachment to the task :)
I'm not likely to do it before Monday, so if you start it before then,
it's all yours. :-) Otherwise, I probably will start it then.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: revision/date handling in the client library
Posted by Philip Martin <ph...@codematters.co.uk>.
Karl Fogel <kf...@newton.ch.collab.net> writes:
> Philip Martin <ph...@codematters.co.uk> writes:
> > typedef struct svn_client_revision_s {
> >
> > enum svn_client_revision_type type;
> >
> > svn_revnum_t number;
> > apr_time_t date;
> >
> > } svn_client_revision_t;
[snip]
> I like! Would you like me to do it, or were you planning to do it?
> I'm fine with either way.
I may give it a go in the next few days, but if you want to do it I
have no strong attachment to the task :)
--
Philip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: revision/date handling in the client library
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Philip Martin <ph...@codematters.co.uk> writes:
> typedef struct svn_client_revision_s {
>
> enum svn_client_revision_type type;
>
> svn_revnum_t number;
> apr_time_t date;
>
> } svn_client_revision_t;
I think we could `union' those last two fields, no?
(Btw, is "_s" your struct suffix? We usually just use _t for both...)
> Look at the ugly revision handling in svn_client_diff to see how bad
> it gets at present.
>
> Comments?
I like! Would you like me to do it, or were you planning to do it?
I'm fine with either way.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org