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:
artagnon_at_apache.org wrote on Fri, Aug 06, 2010 at 19:10:20 -0000:
> Author: artagnon
> Date: Fri Aug 6 19:10:20 2010
> New Revision: 983096
>
> URL: http://svn.apache.org/viewvc?rev=983096&view=rev
> Log:
> svnrdump: Fix a bug involving svn_node_action_delete
>
> * subversion/svnrdump/load_editor.c
> (new_node_record, apply_textdelta, close_node): Fix the tense of the
> LDR_DBG messages.
> (new_node_record): Extract base_checksum header. Also, no Node-Kind
> information is present during a svn_node_action_delete, so don't
> look for it and blindly delete the given entry.
> (apply_textdelta): Use the extracted base_checksum while applying
> the textdelta.
>
> * subversion/svnrdump/load_editor.h
> (node_baton): Add a new base_checksum field.
>
> Modified:
> subversion/trunk/subversion/svnrdump/load_editor.c
> subversion/trunk/subversion/svnrdump/load_editor.h
>
> Modified: subversion/trunk/subversion/svnrdump/load_editor.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svnrdump/load_editor.c?rev=983096&r1=983095&r2=983096&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/svnrdump/load_editor.c (original)
> +++ subversion/trunk/subversion/svnrdump/load_editor.c Fri Aug 6 19:10:20 2010
> @@ -189,13 +189,15 @@ new_node_record(void **node_baton,
> if (strcmp(hval, "replace") == 0)
> nb->action = svn_node_action_replace;
> }
> + if (strcmp(hname, SVN_REPOS_DUMPFILE_TEXT_DELTA_BASE_MD5) == 0)
> + nb->base_checksum = apr_pstrdup(rb->pool, hval);
This part is okay.
> 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.)
> }
>
> 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).
> @@ -288,21 +290,9 @@ new_node_record(void **node_baton,
> }
> break;
> case svn_node_action_delete:
> - switch (nb->kind)
> - {
> - case svn_node_file:
> - LDR_DBG(("Deleting file %s in %p\n", nb->path, rb->db->baton));
> - SVN_ERR(commit_editor->delete_entry(nb->path, rb->rev,
> - rb->db->baton, rb->pool));
> - break;
> - case svn_node_dir:
> - LDR_DBG(("Deleting dir %s in %p\n", nb->path, rb->db->baton));
> - SVN_ERR(commit_editor->delete_entry(nb->path, rb->rev,
> - rb->db->baton, rb->pool));
> - break;
> - default:
> - break;
> - }
> + LDR_DBG(("Deleting entry %s in %p\n", nb->path, rb->db->baton));
> + SVN_ERR(commit_editor->delete_entry(nb->path, rb->rev,
> + rb->db->baton, rb->pool));
Okay.
> 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.
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.
> }
>
> /* The svn_node_dir case is handled in close_revision */
>
> Modified: subversion/trunk/subversion/svnrdump/load_editor.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svnrdump/load_editor.h?rev=983096&r1=983095&r2=983096&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/svnrdump/load_editor.h (original)
> +++ subversion/trunk/subversion/svnrdump/load_editor.h Fri Aug 6 19:10:20 2010
> @@ -66,6 +66,8 @@ struct node_baton
> const char *copyfrom_path;
>
> void *file_baton;
> + const char *base_checksum;
> +
> struct revision_baton *rb;
> };
>
>
>
Received on 2010-08-07 14:59:20 CEST