You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by kf...@collab.net on 2003/08/12 20:59:31 UTC
Re: svn commit: rev 6716 - in branches/fs-schema-changes/subversion: include libsvn_fs libsvn_repos svnadmin tests/libsvn_fs tests/libsvn_repos
cmpilato@tigris.org writes:
> --- branches/fs-schema-changes/subversion/libsvn_fs/tree.c (original)
> +++ branches/fs-schema-changes/subversion/libsvn_fs/tree.c Tue Aug 12 15:08:47 2003
> @@ -3604,6 +3604,85 @@
>
>
>
> +/* Finding Changes */
> +
> +struct paths_changed_args
> +{
> + apr_hash_t *changes;
> + svn_fs_root_t *root;
> +};
I realize it wasn't documented in the original code either, but might
be nice to say what are the keys and values of the 'changes' hash.
(It wasn't obvious to me from looking.)
> +/* Our coolio opaque history object. */
> +struct svn_fs_history_t
> +{
> + svn_fs_t *fs;
> + const char *path;
> + svn_revnum_t revision;
> + const char *path_hint;
> + svn_revnum_t rev_hint;
> + int is_interesting;
> +};
So coolio that none of its fields need to be documented :-) ?
(Seriously -- I try not to be a stickler about internal structs when
the fields are obvious, but in this case some docs might clarify,
particularly the hint fields and is_interesting).
> +/* Return a new history object (marked as "interesting") for PATH and
> + REVISION, allocated in POOL. Note that paths are not duped into
> + POOL -- it is the responsibility of the caller to ensure that this
> + happens. */
> +static svn_fs_history_t *
> +assemble_history (svn_fs_t *fs,
> + const char *path,
> + svn_revnum_t revision,
> + int is_interesting,
> + const char *path_hint,
> + svn_revnum_t rev_hint,
> + apr_pool_t *pool)
I guess it's sort of implied, but you might as well just say that
arguments get copied to the corresponding fields in the structure.
The phrase 'marked as "interesting"' doesn't convey anything, for
example.
Call me "Doc Monster", just don't call me late for dinner.
I assume the doc string is referring to both the 'path' and
'path_hint' fields when it talks about allocation? Would be good to
name them explicitly, though.
> +/* Derive the oldest of a set of copies which took place in filesystem
> + FS -- bounded by the transactions START_TXN_ID and END_TXN_ID, and
> + by the copy ids START_COPY_ID and END_COPY_ID -- which resulted in
> + the creation of END_PATH. Return the previous location of the
> + END_PATH as *SRC_REV/SRC_PATH, and the revision in which the copy
> + occured as *DST_REV. Do all of this as part of TRAIL.
>
> +find_youngest_copy (svn_revnum_t *src_rev, /* return */
> + const char **src_path, /* return */
> + svn_revnum_t *dst_rev, /* return */
> + const char *dst_path,
> + svn_fs_t *fs,
> + svn_revnum_t start_rev,
> + const char *start_copy_id,
> + svn_revnum_t end_rev,
> + const char *end_copy_id, /* may be NULL */
> + trail_t *trail)
> +{
How are 'dst_path', 'start_rev' and 'end_rev' used?
> if (! (txn->copies && txn->copies->nelts))
> - continue;
> + goto loop_inc;
Sure you don't want to call that 'loop_decr', since it's a decrement
at the label?
> + loop_inc:
> + cur_rev--;
...here.
> +struct history_prev_args
> {
> - apr_hash_t *revs;
> - svn_fs_t *fs;
> - svn_fs_root_t *root;
> - const char *path;
> - const svn_fs_id_t *id;
> - int cross_copy_history;
> + svn_fs_history_t **prev_history_p;
> + svn_fs_history_t *history;
> + int cross_copies;
> + apr_pool_t *pool;
> };
You might think I'm going to complain here, but I'm not :-). These
fields are all obvious. Documentation would only obscure (except
perhaps for the 'pool' field).
> +svn_error_t *
> +svn_repos_revisions_changed (apr_array_header_t **revs,
> + svn_fs_t *fs,
> + const char *path,
> + svn_revnum_t start,
> + svn_revnum_t end,
> + svn_boolean_t cross_copies,
> + apr_pool_t *pool)
> +{
> + svn_fs_history_t *history;
> + apr_pool_t *subpool1 = svn_pool_create (pool);
> + apr_pool_t *subpool2 = svn_pool_create (pool);
> + apr_pool_t *oldpool, *newpool;
> + const char *history_path;
> + svn_revnum_t history_rev;
> + svn_fs_root_t *root;
> +
> + /* Validate the revisions. */
> + if ((! SVN_IS_VALID_REVNUM (start)) || (! SVN_IS_VALID_REVNUM (end)))
> + return svn_error_create (SVN_ERR_FS_NO_SUCH_REVISION, 0, "");
Do we have a function for manufacturing a better error string here
(one that specifies the invalid revision?) If not, we could still do
it by hand, and also distinguish between 'start' being invalid and
'end' being invalid...
> + /* Ensure that the input is ordered. */
> + if (start > end)
> + {
> + svn_revnum_t tmprev = start;
> + start = end;
> + end = tmprev;
> + }
A professor once showed me
start = start + end
end = start - end
start = start - end
It's needlessly obfuscatory, risks overflow, and is in no way
preferable to what you did. I don't know why I brought it up.
> + /* Allocate our return array. */
> + *revs = apr_array_make (pool, 4, sizeof (history_rev));
> +
> + /* Get a revision root for END, and an initial HISTORY baton. */
> + SVN_ERR (svn_fs_revision_root (&root, fs, end, pool));
> + SVN_ERR (svn_fs_node_history (&history, root, path, subpool1));
> + oldpool = subpool1;
> + newpool = subpool2;
I got baffled by the 'subpool1, subpool2' vs 'oldpool, newpool'
distinction. Why are there two pairs of variables, instead of just
two variables? I saw 'tmppool' and the comment later about "crazy
pool work", but wow -- is there some simpler way to do this? I
haven't considered the problem deeply, just a hunch. Or did you start
out thinking that too, and then learn better? :-)
Anyway, above very minor nits aside, nice job! The new interfaces are
clean & beautiful, and the code is extremely clear.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 6716 - in branches/fs-schema-changes/subversion:
include libsvn_fs libsvn_repos svnadmin tests/libsvn_fs tests/libsvn_repos
Posted by Branko Čibej <br...@xbc.nu>.
kfogel@collab.net wrote:
>Branko Čibej <br...@xbc.nu> writes:
>
>
>>And sometimes I wonder why we have svn_boolean_t, then people keep using
>>int...
>>
>>
>
>I was going to say something about that, but then decided to leave the
>tangled history of svn_fs alone :-).
>
>Back in the day, Jim Blandy hated svn_boolean_t. He's not writing
>code in libsvn_fs anymore, so if someone wanted to change all those
>'int's to booleans, it would be a fine thing.
>
>Should we introduce them and live with the inconsistency? Yeah,
>probably. I'd certainly be fine with it.
>
>
Heh, actually, we should start following C99, #include <stdbool.h> and
use "bool" everywhere. :-)
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 6716 - in branches/fs-schema-changes/subversion: include libsvn_fs libsvn_repos svnadmin tests/libsvn_fs tests/libsvn_repos
Posted by kf...@collab.net.
Branko Čibej <br...@xbc.nu> writes:
> And sometimes I wonder why we have svn_boolean_t, then people keep using
> int...
I was going to say something about that, but then decided to leave the
tangled history of svn_fs alone :-).
Back in the day, Jim Blandy hated svn_boolean_t. He's not writing
code in libsvn_fs anymore, so if someone wanted to change all those
'int's to booleans, it would be a fine thing.
Should we introduce them and live with the inconsistency? Yeah,
probably. I'd certainly be fine with it.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 6716 - in branches/fs-schema-changes/subversion:
include libsvn_fs libsvn_repos svnadmin tests/libsvn_fs tests/libsvn_repos
Posted by Branko Čibej <br...@xbc.nu>.
kfogel@collab.net wrote:
>cmpilato@tigris.org writes:
>
>
>>+/* Our coolio opaque history object. */
>>+struct svn_fs_history_t
>>+{
>>+ svn_fs_t *fs;
>>+ const char *path;
>>+ svn_revnum_t revision;
>>+ const char *path_hint;
>>+ svn_revnum_t rev_hint;
>>+ int is_interesting;
>>+};
>>
>>
>
>So coolio that none of its fields need to be documented :-) ?
>(Seriously -- I try not to be a stickler about internal structs when
>the fields are obvious, but in this case some docs might clarify,
>particularly the hint fields and is_interesting).
>
And sometimes I wonder why we have svn_boolean_t, then people keep using
int...
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 6716 - in branches/fs-schema-changes/subversion: include libsvn_fs libsvn_repos svnadmin tests/libsvn_fs tests/libsvn_repos
Posted by cm...@collab.net.
kfogel@collab.net writes:
> ? I understood my question, and I understood your answer, but I
> didn't understand how the answer connects to the question :-)
>
> I was just pointing out that this function takes arguments that its
> doc string does not mention. The bit you quote above contains none of
> the variables I asked about...
And it is that very problem that I'm talking about when I say, "I've
looked at enough text for the day. Goodnight." :-)
Sorry, dude. Brain was out to ... dinner.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 6716 - in branches/fs-schema-changes/subversion: include libsvn_fs libsvn_repos svnadmin tests/libsvn_fs tests/libsvn_repos
Posted by kf...@collab.net.
cmpilato@collab.net writes:
> > > +find_youngest_copy (svn_revnum_t *src_rev, /* return */
> > > + const char **src_path, /* return */
> > > + svn_revnum_t *dst_rev, /* return */
> > > + const char *dst_path,
> > > + svn_fs_t *fs,
> > > + svn_revnum_t start_rev,
> > > + const char *start_copy_id,
> > > + svn_revnum_t end_rev,
> > > + const char *end_copy_id, /* may be NULL */
> > > + trail_t *trail)
> > > +{
> >
> > How are 'dst_path', 'start_rev' and 'end_rev' used?
>
> "Return the previous location of the END_PATH as *SRC_REV/SRC_PATH,
> and the revision in which the copy occured as *DST_REV."
? I understood my question, and I understood your answer, but I
didn't understand how the answer connects to the question :-)
I was just pointing out that this function takes arguments that its
doc string does not mention. The bit you quote above contains none of
the variables I asked about...
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 6716 - in branches/fs-schema-changes/subversion: include libsvn_fs libsvn_repos svnadmin tests/libsvn_fs tests/libsvn_repos
Posted by cm...@collab.net.
kfogel@collab.net writes:
Am fixing other items in this review. Thanks for it.
> > +/* Derive the oldest of a set of copies which took place in filesystem
> > + FS -- bounded by the transactions START_TXN_ID and END_TXN_ID, and
> > + by the copy ids START_COPY_ID and END_COPY_ID -- which resulted in
> > + the creation of END_PATH. Return the previous location of the
> > + END_PATH as *SRC_REV/SRC_PATH, and the revision in which the copy
> > + occured as *DST_REV. Do all of this as part of TRAIL.
> >
> > +find_youngest_copy (svn_revnum_t *src_rev, /* return */
> > + const char **src_path, /* return */
> > + svn_revnum_t *dst_rev, /* return */
> > + const char *dst_path,
> > + svn_fs_t *fs,
> > + svn_revnum_t start_rev,
> > + const char *start_copy_id,
> > + svn_revnum_t end_rev,
> > + const char *end_copy_id, /* may be NULL */
> > + trail_t *trail)
> > +{
>
> How are 'dst_path', 'start_rev' and 'end_rev' used?
"Return the previous location of the END_PATH as *SRC_REV/SRC_PATH,
and the revision in which the copy occured as *DST_REV."
> > if (! (txn->copies && txn->copies->nelts))
> > - continue;
> > + goto loop_inc;
>
> Sure you don't want to call that 'loop_decr', since it's a decrement
> at the label?
Heh. Yep.
> > + /* Allocate our return array. */
> > + *revs = apr_array_make (pool, 4, sizeof (history_rev));
> > +
> > + /* Get a revision root for END, and an initial HISTORY baton. */
> > + SVN_ERR (svn_fs_revision_root (&root, fs, end, pool));
> > + SVN_ERR (svn_fs_node_history (&history, root, path, subpool1));
> > + oldpool = subpool1;
> > + newpool = subpool2;
>
> I got baffled by the 'subpool1, subpool2' vs 'oldpool, newpool'
> distinction. Why are there two pairs of variables, instead of just
> two variables? I saw 'tmppool' and the comment later about "crazy
> pool work", but wow -- is there some simpler way to do this? I
> haven't considered the problem deeply, just a hunch. Or did you start
> out thinking that too, and then learn better? :-)
The problem is that I have a loop which is generating a "current
thing" from a "previous thing", meaning I can't clear the subpool of
the previous thing until I have used it to get the "current thing".
The easiest way I could think of to do this was to have two subpools,
and just toggle back and forth between them.
But yeah, I think I can drop the subpool1 and subpool2 and just have
the old and new.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org