Ramkumar Ramachandra wrote on Sat, Aug 07, 2010 at 20:00:28 +0530:
> 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.
>
I've grown the habit of reviewing the 'svn diff' before committing.
(I have a Vim function to do that, in fact.)
> > The line reordering makes it harder to spot the functional change done here:
> > passing nb->base_checksum instead of NULL.
(this)
>
> 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.
(and this)
>
> Oh, I thought "changed tense of LDR_DBG messages" would suffice. Have
> to be more explicit/ careful.
>
The marked two points of mine may have been more nit-picky than normal; but
it's hard for me to explain exactly what I mean here (without getting into
long paragraphs of trivialities). There's a balance to be made, and I think
this commit was on the wrong side thereof, but we certainly don't go all the
way to the other extreme either (e.g., it's okay to combine several
non-functional changes).
Your best bet is to review your patches more carefully before committing
them, and seeing how others do it. With ~50k revisions in our history, you
have plenty of examples to follow :-)
> -- Ram
Received on 2010-08-07 16:49:16 CEST