On Wed, Jun 24, 2009 at 04:00:15PM +0800, HuiHuang wrote:
> Hey,
>
> >I see from the patch you are adding a field to
> >svn_client_commit_item3_t. It wasn't obvious before r38160, but
> >adding new members of a public structure should be done at the end of
> >the structure. You should also update any constructors or copy
> >functions to account for the new member.
> I have moved the new member to the end of the structure.
>
> >Note that making a copy of and adm_access_t may not make sense.
> >So I think the _dup() function should *not* make a newly allocated
> >copy of the adm_access_t. Rather, it should just copy the pointer
> >value, and the docstring needs to be changed from:
> svn_client_commit_item3_create() and svn_client_commit_item3_dup()
> do not need to be changed according what you said. Original implement
> of svn_client_commit_item3_dup() already share adm_access. So I just
> modifed the docstring of it.
>
> >In the future, could you make sure your patches are attached
> >with the correct mime-type, as requested by HACKING? Using a mime-
> >type of text/x-diff, text/x-patch, or text/plain makes it possible to
> >view and review the patch inline in a mail reader, and increases the
> >likelihood it will get good review.
> following is new patch i make. Does this satisfies above requirement?
Committed in r38186.
Thanks,
Stefan
Received on 2009-06-24 16:50:51 CEST