Hi Daniel,
Daniel Shahaf writes:
> Ramkumar, I've noticed before in your patches that they tend to have
> multiple separable parts, and this commit is a good opportunity to explain:
Thanks for the review.
> > if (strcmp(hname, SVN_REPOS_DUMPFILE_NODE_COPYFROM_REV) == 0)
> > nb->copyfrom_rev = atoi(hval);
> > if (strcmp(hname, SVN_REPOS_DUMPFILE_NODE_COPYFROM_PATH) == 0)
> > - nb->copyfrom_path =
> > - svn_path_url_add_component2(rb->pb->root_url,
> > - apr_pstrdup(rb->pool, hval),
> > - rb->pool);
> > + nb->copyfrom_path =
> > + svn_path_url_add_component2(rb->pb->root_url,
> > + apr_pstrdup(rb->pool, hval),
> > + rb->pool);
>
> Whitespace-only change; should have been done in a separate commit. (Don't
> mix functional and non-functional change in the same commit.)
Yeah, this is a stray change.
> > }
> >
> > if (svn_path_compare_paths(svn_relpath_dirname(nb->path, pool),
> > @@ -252,14 +254,14 @@ new_node_record(void **node_baton,
> > nb->copyfrom_path,
> > nb->copyfrom_rev,
> > rb->pool, &(nb->file_baton)));
> > - LDR_DBG(("Adding file %s to dir %p as %p\n", nb->path, rb->db->baton, nb->file_baton));
> > + LDR_DBG(("Added file %s to dir %p as %p\n", nb->path, rb->db->baton, nb->file_baton));
>
> Non-functional change; should have been in a separate commit (if at all).
Right, I probably shouldn't do more than one thing at once. I'll try
to commit more often then at the risk of making many trivial commits.
> > break;
> > case svn_node_action_replace:
> > /* Absent in dumpstream; represented as a delete + add */
> > @@ -411,9 +401,9 @@ apply_textdelta(svn_txdelta_window_handl
> > nb = node_baton;
> > commit_editor = nb->rb->pb->commit_editor;
> > pool = nb->rb->pool;
> > - SVN_ERR(commit_editor->apply_textdelta(nb->file_baton, NULL /* base_checksum */,
> > - pool, handler, handler_baton));
> > LDR_DBG(("Applying textdelta to %p\n", nb->file_baton));
> > + SVN_ERR(commit_editor->apply_textdelta(nb->file_baton, nb->base_checksum,
> > + pool, handler, handler_baton));
> >
>
> The line reordering makes it harder to spot the functional change done here:
> passing nb->base_checksum instead of NULL.
Right. Should have probably been in two separate commits - I get the
point now.
> While here, how is this change related to the svn_node_action_delete bug?
> (The log message doesn't say.) Are the checksum-related changes and the
> delete-related changes logically part of one patch/bugfix? Or could you
> have committed them as two separate patches?
>
> > return SVN_NO_ERROR;
> > }
> > @@ -429,8 +419,8 @@ close_node(void *baton)
> >
> > if (nb->kind == svn_node_file)
> > {
> > - SVN_ERR(commit_editor->close_file(nb->file_baton, NULL, nb->rb->pool));
> > LDR_DBG(("Closing file %p\n", nb->file_baton));
> > + SVN_ERR(commit_editor->close_file(nb->file_baton, NULL, nb->rb->pool));
>
> This actually *is* a functional change (which is not mentioned in the log
> message), but should have been in a separate commit.
Oh, I thought "changed tense of LDR_DBG messages" would suffice. Have
to be more explicit/ careful.
-- Ram
Received on 2010-08-07 16:32:33 CEST