Yoshiki Hayashi <yoshiki@xemacs.org> writes:
> fails, you just return conflict error. If it succeeds, you
> make nodes immutable and stable. I really don't understand
> what was wrong with my patch and why you're reinventing all
> this. :-(
That's a fair (and important) question, so I'll answer it in detail.
You've certainly earned a detailed response -- your contributions have
been huge, even in a short time. I hope it's okay that I'm answering
on the list; you asked on the list, and I think this response might be
generally useful.
Evaluating patches can be very time-consuming; it can be almost as
time-consuming as writing the code from scratch, really. If the patch
is self-documenting, that helps a *lot*. If it also contains no
obvious bugs, that helps yet more, because it makes me (or whoever's
receiving the patch) feel confident about continuing to devote time to
the evaluation.
When I read over your initial commit_txn patch, a few things stood
out:
1. The log message was complete, which helped a lot. Thanks!
2. The hard part of commits, doing the merge from youngest into the
txn, was not treated at all -- the word "merge" did not even
appear in the patch.
Now, to be fair, you had a different understanding of where the
merge would happen, and we hadn't talked about it much on the
list. It's disputable whether this can really be called a "bug"
-- the code you wrote did what it claimed to do, it just would
get out-of-date errors in circumstances where Subversion ought to
be able to commit successfully. It turned out you had a plan for
that, but I didn't know it at the time.
I was probably too hasty in letting the absence of merging code
discourage me about the rest of the patch; please accept my
apologies.
3. Internal static functions need documentation, too: there were no
doc strings for make_node_immutable and stabilize_node.
Particularly in the latter case, you wouldn't know from its name
*precisely* what it's supposed to do, or how it behaves when the
node is already stable, etc. (See the doc strings I added for
them in my most recent commit.) For example, due the timing of
its recursion, stabilize_node would always walk the entry list of
its argument even if that argument were immutable. Perhaps the
caller is encouraged to check mutability first, but without the
doc string, I wouldn't know that. :-)
By the way, I changed it to check mutability itself at the front.
This simplified the code of stabilize_node's own recursive case,
simplified callers, and guaranteed that stabilize_node would
never unnecessarily walk an entries list.
4. make_node_immutable didn't copy the node revision skel before
changing it, even though get_node_revision's doc string has a
warning about how one must always copy the skel before mutating
it. Had the txn commit failed, some node's cached skel would be
corrupt. This is a Bad Thing.
5. svn_fs__dag_commit_txn just returned success right away if the
root node of the txn was immutable -- without committing any new
revision or setting *new_rev to point to anything! This was
probably the thing that made me most worried, because it was
clearly not the semantics promised by svn_fs__dag_commit_txn's
doc string. That just seemed sloppy to me. The case of
committing a txn in which no changes have been made is complex;
one can't just bail because it's unexpected.
No one of these things alone would have put me off the patch; but
taken together, they force me to ask myself "What will be most
efficient for Subversion: continuing to try to bend this patch into
shape, or just writing the code from scratch?"
Quite possibly, the answer I gave myself -- which was to write the
code from scratch -- was wrong. It's always a gamble. I don't know
in advance how long it will take to write the code, and I also don't
know in advance how much more time I would have to spend on the patch.
So the two most important variables are unknown! :-)
Your patch contained a number of improvements to the way I was doing
things, and your code review from last night was a great help.
Largely because of your contributions, we're now able to commit the
Greek Tree into the repository.
But you've taken a very ambitious tack, writing huge swaths of code
quickly, to implement major functionality. It isn't entirely
surprising that the patches come out a little sloppy, with corners cut
(I can make similarly detailed commentary about some of the other
patches). Because of this starting uncertainty, it's easy for me, or
others working in the fs, to make a wrong decision about one of those
big patches.
Everyone writes bugs from time to time. I'm not saying I'm the
perfect programmer... In fact, I'm not even saying that I write fewer
bugs than you. I think I might write more. :-) But I have to manage
my time as best I can, and time spent patch reviewing is time spent
not coding. Therefore, I need to be confident that the patch
reviewing time will pay off at least as well as coding time does.
I'm definitely *not* trying to discourage you from sending in patches,
please don't misunderstand. The work you do is great, is much
appreciated, and I hope you will stay very much involved for as long
as you can. But is there any way to write smaller patches, more
carefully, with thorough test cases in the patch?
Thank you,
-Karl
> * dag.c (make_node_immutable): New helper function for
> stabilize_node.
> * dag.c (stabilize_node): New helper function for
> svn_fs__dag_commit_txn.
> * dag.c (svn_fs__dag_commit_txn): New function.
> * dag.h (svn_fs__dag_commit_txn): Add svn_revnum_t argument
> so that svn_fs_commit_txn can get new revision number.
> * rev-table.c (svn_fs__youngest_rev): New function.
> * rev-table.c (txn_body_youngest_rev): Call
> svn_fs__youngest_rev.
> * rev-table.h (svn_fs__youngest_rev): New function declaration.
> * txn.c (struct commit_txn_args): New struct.
> * txn.c (txn_body_commit_txn): New helper function for
> svn_fs_commit_txn.
> * txn.c (svn_fs_commit_txn): Fill in this function.
>
> Index: subversion/libsvn_fs/dag.c
> ===================================================================
> RCS file: /cvs/subversion/subversion/libsvn_fs/dag.c,v
> retrieving revision 1.70
> diff -u -r1.70 dag.c
> --- subversion/libsvn_fs/dag.c 2001/03/03 21:10:13 1.70
> +++ subversion/libsvn_fs/dag.c 2001/03/05 03:59:27
> @@ -574,6 +572,122 @@
> }
>
>
> +static svn_error_t *
> +make_node_immutable (dag_node_t *node, trail_t *trail)
> +{
> + skel_t *node_rev;
> + skel_t *header;
> + skel_t *flag, *prev = NULL;
> +
> + /* Go get a fresh NODE-REVISION for this node. */
> + SVN_ERR (get_node_revision (&node_rev, node, trail));
> + /* The node "header" is the first element of a node-revision skel,
> + itself a list. */
> + header = node_rev->children;
> +
> + /* Flag is the 3rd element of the header. */
> + for (flag = header->children->next->next; flag; flag = flag->next)
> + {
> + if (! flag->is_atom && svn_fs__matches_atom (flag->children, "mutable"))
> + {
> + /* We found it. */
> + if (prev)
> + prev->next = flag->next;
> + else
> + header->children->next->next = 0;
> +
> + SVN_ERR (set_node_revision (node, node_rev, trail));
> + return SVN_NO_ERROR;
> + }
> + prev = flag;
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> +
> +static svn_error_t *
> +stabilize_node (dag_node_t *node, trail_t *trail)
> +{
> + if (svn_fs__dag_is_directory (node))
> + {
> + skel_t *entries;
> + skel_t *entry;
> +
> + SVN_ERR (svn_fs__dag_dir_entries (&entries, node, trail));
> +
> + /* Each entry looks like (NAME ID). */
> + for (entry = entries->children; entry; entry = entry->next)
> + {
> + dag_node_t *child;
> + skel_t *id_skel = entry->children->next;
> + svn_boolean_t is_mutable;
> +
> + SVN_ERR (svn_fs__dag_get_node (&child, node->fs,
> + svn_fs_parse_id (id_skel->data,
> + id_skel->len,
> + trail->pool),
> + trail));
> + SVN_ERR (svn_fs__dag_check_mutable (&is_mutable, child, trail));
> +
> + if (is_mutable)
> + SVN_ERR (stabilize_node (child, trail));
> + }
> + }
> + else if (svn_fs__dag_is_file (node)
> + || svn_fs__dag_is_copy (node))
> + ;
> + else
> + abort ();
> +
> + SVN_ERR (make_node_immutable (node, trail));
> + SVN_ERR (svn_fs__stable_node (node->fs, node->id, trail));
> +
> + return SVN_NO_ERROR;
> +}
> +
> +
> +svn_error_t *
> +svn_fs__dag_commit_txn (svn_revnum_t *new_rev,
> + svn_fs_t *fs,
> + const char *svn_txn,
> + trail_t *trail)
> +{
> + dag_node_t *root;
> + svn_boolean_t is_mutable;
> +
> + SVN_ERR (svn_fs__dag_txn_root (&root, fs, svn_txn, trail));
> + SVN_ERR (svn_fs__dag_check_mutable (&is_mutable, root, trail));
> +
> + /* Nothing is changed in this transaction. */
> + if (! is_mutable)
> + return SVN_NO_ERROR;
> +
> + /* Make all mutable node immutable and stable. */
> + SVN_ERR (stabilize_node (root, trail));
> +
> + {
> + /* Add rew revision entry to `revisions' table. */
> + skel_t *new_revision_skel;
> + svn_string_t *id_string = svn_fs_unparse_id (root->id, trail->pool);
> +
> + new_revision_skel = svn_fs__make_empty_list (trail->pool);
> + svn_fs__prepend (svn_fs__make_empty_list (trail->pool),
> + new_revision_skel);
> + svn_fs__prepend (svn_fs__mem_atom (id_string->data,
> + id_string->len, trail->pool),
> + new_revision_skel);
> + svn_fs__prepend (svn_fs__str_atom ((char *) "revision", trail->pool),
> + new_revision_skel);
> + SVN_ERR (svn_fs__put_rev (new_rev, fs, new_revision_skel, trail));
> + }
> + /* Delete transaction from `transactions' table. */
> + SVN_ERR (svn_fs__delete_txn (fs, svn_txn, trail));
> +
> + return SVN_NO_ERROR;
> +}
> +
> +
> /* Helper function for svn_fs__dag_clone_child.
> Given a PARENT directory, and the NAME of an entry in that
> directory, update the PARENT's ENTRY list item for the NAMEd entry
> Index: subversion/libsvn_fs/dag.h
> ===================================================================
> RCS file: /cvs/subversion/subversion/libsvn_fs/dag.h,v
> retrieving revision 1.31
> diff -u -r1.31 dag.h
> --- subversion/libsvn_fs/dag.h 2001/03/02 20:32:36 1.31
> +++ subversion/libsvn_fs/dag.h 2001/03/05 03:59:27
> @@ -158,7 +158,8 @@
> trail_t *trail);
>
>
> -/* Commit the transaction SVN_TXN in FS, as part of TRAIL. This entails:
> +/* Commit the transaction SVN_TXN in FS, as part of TRAIL. Store the
> + new revision number in NEW_REV. This entails:
> - marking the tree of mutable nodes at SVN_TXN's root as immutable,
> and marking all their contents as stable
> - creating a new revision, with SVN_TXN's root as its root directory
> @@ -171,7 +172,8 @@
> Do any necessary temporary allocation in a subpool of TRAIL->pool.
> Consume temporary space at most proportional to the maximum depth
> of SVN_TXN's tree of mutable nodes. */
> -svn_error_t *svn_fs__dag_commit_txn (svn_fs_t *fs,
> +svn_error_t *svn_fs__dag_commit_txn (svn_revnum_t *new_rev,
> + svn_fs_t *fs,
> const char *svn_txn,
> trail_t *trail);
>
> Index: subversion/libsvn_fs/rev-table.c
> ===================================================================
> RCS file: /cvs/subversion/subversion/libsvn_fs/rev-table.c,v
> retrieving revision 1.8
> diff -u -r1.8 rev-table.c
> --- subversion/libsvn_fs/rev-table.c 2001/03/03 10:18:01 1.8
> +++ subversion/libsvn_fs/rev-table.c 2001/03/05 03:59:27
> @@ -161,25 +161,16 @@
> /* Getting the youngest revision. */
>
>
> -struct youngest_rev_args {
> - svn_revnum_t *youngest_p;
> - svn_fs_t *fs;
> -};
> -
> -
> -static svn_error_t *
> -txn_body_youngest_rev (void *baton,
> - trail_t *trail)
> +svn_error_t *
> +svn_fs__youngest_rev (svn_revnum_t *youngest_p,
> + svn_fs_t *fs,
> + trail_t *trail)
> {
> - struct youngest_rev_args *args = baton;
> -
> int db_err;
> DBC *cursor = 0;
> DBT key, value;
> db_recno_t recno;
>
> - svn_fs_t *fs = args->fs;
> -
> /* Create a database cursor. */
> SVN_ERR (DB_WRAP (fs, "getting youngest revision (creating cursor)",
> fs->revisions->cursor (fs->revisions, trail->db_txn,
> @@ -218,7 +209,24 @@
> /* Turn the record number into a Subversion revision number.
> Revisions are numbered starting with zero; Berkeley DB record
> numbers begin with one. */
> - *args->youngest_p = recno - 1;
> + *youngest_p = recno - 1;
> +
> + return SVN_NO_ERROR;
> +}
> +
> +struct youngest_rev_args {
> + svn_revnum_t *youngest_p;
> + svn_fs_t *fs;
> +};
> +
> +
> +static svn_error_t *
> +txn_body_youngest_rev (void *baton,
> + trail_t *trail)
> +{
> + struct youngest_rev_args *args = baton;
> + SVN_ERR (svn_fs__youngest_rev (args->youngest_p, args->fs, trail));
> +
> return SVN_NO_ERROR;
> }
>
> Index: subversion/libsvn_fs/rev-table.h
> ===================================================================
> RCS file: /cvs/subversion/subversion/libsvn_fs/rev-table.h,v
> retrieving revision 1.5
> diff -u -r1.5 rev-table.h
> --- subversion/libsvn_fs/rev-table.h 2001/03/03 10:18:01 1.5
> +++ subversion/libsvn_fs/rev-table.h 2001/03/05 03:59:27
> @@ -63,6 +63,13 @@
> trail_t *trail);
>
>
> +/* Set *YOUNGEST_P to the number of the youngest revision in filesystem FS,
> + as part of TRAIL. Use TRAIL->pool for all temporary allocation. */
> +svn_error_t *svn_fs__youngest_rev (svn_revnum_t *youngest_p,
> + svn_fs_t *fs,
> + trail_t *trail);
> +
> +
> #endif /* SVN_LIBSVN_FS_REV_TABLE_H */
>
>
> Index: subversion/libsvn_fs/txn.c
> ===================================================================
> RCS file: /cvs/subversion/subversion/libsvn_fs/txn.c,v
> retrieving revision 1.29
> diff -u -r1.29 txn.c
> --- subversion/libsvn_fs/txn.c 2001/03/03 17:25:38 1.29
> +++ subversion/libsvn_fs/txn.c 2001/03/05 05:34:51
> @@ -22,8 +22,10 @@
> #include "txn.h"
> #include "err.h"
> #include "trail.h"
> +#include "nodes-table.h"
> #include "rev-table.h"
> #include "txn-table.h"
> +#include "dag.h"
> #include "tree.h"
>
>
> @@ -146,12 +148,50 @@
> }
>
>
> +struct commit_txn_args {
> + svn_revnum_t *new_rev;
> + svn_fs_txn_t *txn;
> +};
> +
> +
> +static svn_error_t *
> +txn_body_commit_txn (void *baton, trail_t *trail)
> +{
> + struct commit_txn_args *args = baton;
> +
> + svn_fs_txn_t *txn = args->txn;
> + svn_revnum_t youngest_rev;
> + svn_fs_id_t *ignored, *base_root_id, *current_root_id;
> +
> + /* Is there already a new revision? */
> + SVN_ERR (svn_fs__get_txn (&ignored, &base_root_id, txn->fs,
> + txn->id, trail));
> + SVN_ERR (svn_fs__youngest_rev (&youngest_rev, txn->fs, trail));
> + SVN_ERR (svn_fs__rev_get_root (¤t_root_id, txn->fs,
> + youngest_rev, trail));
> + if (! svn_fs_id_eq (base_root_id, current_root_id))
> + return svn_error_createf (SVN_ERR_FS_CONFLICT, 0, 0, trail->pool,
> + "Trying to commit old transaction, ID `%s'",
> + txn->id);
> +
> + SVN_ERR (svn_fs__dag_commit_txn (args->new_rev, txn->fs, txn->id, trail));
> +
> + return SVN_NO_ERROR;
> +}
> +
> +
> svn_error_t *
> svn_fs_commit_txn (svn_revnum_t *new_rev,
> svn_fs_txn_t *txn)
> {
> + struct commit_txn_args args;
> +
> *new_rev = SVN_INVALID_REVNUM;
> - abort();
> + args.new_rev = new_rev;
> + args.txn = txn;
> +
> + SVN_ERR (svn_fs__retry_txn (txn->fs, txn_body_commit_txn, &args, txn->pool));
> +
> return SVN_NO_ERROR;
> }
Received on Sat Oct 21 14:36:25 2006