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:
artag...@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; > }; > > >
