You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by David Glasser <gl...@davidglasser.net> on 2007/09/08 16:14:12 UTC
Re: svn commit: r26326 - branches/svnpatch-diff/subversion/libsvn_client
On 8/26/07, cacknin@tigris.org <ca...@tigris.org> wrote:
> Author: cacknin
> Date: Sun Aug 26 08:52:21 2007
> New Revision: 26326
>
> Log:
> * subversion/libsvn_client/patch.c:
> (get_empty_file, merge_file_added): fix cache issue as the empty file
> is moved.
>
>
> Modified:
> branches/svnpatch-diff/subversion/libsvn_client/patch.c
>
> Modified: branches/svnpatch-diff/subversion/libsvn_client/patch.c
> URL: http://svn.collab.net/viewvc/svn/branches/svnpatch-diff/subversion/libsvn_client/patch.c?pathrev=26326&r1=26325&r2=26326
> ==============================================================================
> --- branches/svnpatch-diff/subversion/libsvn_client/patch.c (original)
> +++ branches/svnpatch-diff/subversion/libsvn_client/patch.c Sun Aug 26 08:52:21 2007
> @@ -366,6 +366,11 @@
> SVN_ERR(svn_wc_add_repos_file2(mine, adm_access, yours, NULL,
> new_props, NULL, NULL,
> SVN_IGNORED_REVNUM, subpool));
> + /* The file 'yours' points to edit_baton's cached empty file. Since
> + * it was just moved, set it to empty so that later get_empty_file()
> + * calls don't get it wrong with the cache, i.e. create another
> + * empty file again. */
> + sprintf((char *)yours, ""); /* awful cast */
That strikes me as a strange use of sprintf; how about just
*yours = '\0';
?
> }
> if (content_state)
> *content_state = svn_wc_notify_state_changed;
> @@ -988,14 +993,13 @@
> get_empty_file(struct edit_baton *eb,
> const char **empty_file_path)
> {
> - /* Create the file if it does not exist */
> + /* Create the file if it does not exist or is empty path. */
> /* Note that we tried to use /dev/null in r17220, but
> that won't work on Windows: it's impossible to stat NUL */
> - if (!eb->empty_file)
> + if (!eb->empty_file || (eb->empty_file && strlen(eb->empty_file) < 1))
Similarly strlen is overkill here, and the short-circuit nature of ||
means you don't need to test empty_file twice::
if (!eb->empty_file || !*(eb->empty_file))
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r26326 - branches/svnpatch-diff/subversion/libsvn_client
Posted by Charles Acknin <ch...@gmail.com>.
On 9/8/07, David Glasser <gl...@davidglasser.net> wrote:
> On 8/26/07, cacknin@tigris.org <ca...@tigris.org> wrote:
> > Author: cacknin
> > Date: Sun Aug 26 08:52:21 2007
> > New Revision: 26326
> >
> > Log:
> > * subversion/libsvn_client/patch.c:
> > (get_empty_file, merge_file_added): fix cache issue as the empty file
> > is moved.
> >
> >
> > Modified:
> > branches/svnpatch-diff/subversion/libsvn_client/patch.c
> >
> > Modified: branches/svnpatch-diff/subversion/libsvn_client/patch.c
> > URL: http://svn.collab.net/viewvc/svn/branches/svnpatch-diff/subversion/libsvn_client/patch.c?pathrev=26326&r1=26325&r2=26326
> > ==============================================================================
> > --- branches/svnpatch-diff/subversion/libsvn_client/patch.c (original)
> > +++ branches/svnpatch-diff/subversion/libsvn_client/patch.c Sun Aug 26 08:52:21 2007
> > @@ -366,6 +366,11 @@
> > SVN_ERR(svn_wc_add_repos_file2(mine, adm_access, yours, NULL,
> > new_props, NULL, NULL,
> > SVN_IGNORED_REVNUM, subpool));
> > + /* The file 'yours' points to edit_baton's cached empty file. Since
> > + * it was just moved, set it to empty so that later get_empty_file()
> > + * calls don't get it wrong with the cache, i.e. create another
> > + * empty file again. */
> > + sprintf((char *)yours, ""); /* awful cast */
>
> That strikes me as a strange use of sprintf; how about just
> *yours = '\0';
> ?
>
> > }
> > if (content_state)
> > *content_state = svn_wc_notify_state_changed;
> > @@ -988,14 +993,13 @@
> > get_empty_file(struct edit_baton *eb,
> > const char **empty_file_path)
> > {
> > - /* Create the file if it does not exist */
> > + /* Create the file if it does not exist or is empty path. */
> > /* Note that we tried to use /dev/null in r17220, but
> > that won't work on Windows: it's impossible to stat NUL */
> > - if (!eb->empty_file)
> > + if (!eb->empty_file || (eb->empty_file && strlen(eb->empty_file) < 1))
>
> Similarly strlen is overkill here, and the short-circuit nature of ||
> means you don't need to test empty_file twice::
> if (!eb->empty_file || !*(eb->empty_file))
Thanks! Fixed in r26498.
Cheers,
Charles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r26326 - branches/svnpatch-diff/subversion/libsvn_client
Posted by Peter Hosey <bo...@gmail.com>.
On Sep 08, 2007, at 09:14:12, David Glasser wrote:
>> + /* The file 'yours' points to edit_baton's cached
>> empty file. Since
>> + * it was just moved, set it to empty so that later
>> get_empty_file()
>> + * calls don't get it wrong with the cache, i.e.
>> create another
>> + * empty file again. */
>> + sprintf((char *)yours, ""); /* awful cast */
>
> That strikes me as a strange use of sprintf; how about just
> *yours = '\0';
> ?
More importantly, it seems to me that “awful cast” is an understatement.
It ought to be no less than a #warning.
It's not obvious from the diff, since it doesn't cover the function
signature, but “yours” is declared as const char *:
static svn_error_t *
merge_file_added([etc.],
const char *mine,
const char *older,
----> const char *yours,
svn_revnum_t rev1,
svn_revnum_t rev2,
[etc.])
This is dangerous. It assumes that the storage behind “yours” will never
really be const. This *may* be true now (and I wouldn't know), but even
if it is, can you guarantee that the storage will never be const in
the future?
I apologize if I'm speaking out of turn, given that I'm not an svn
developer, have never submitted a patch for svn, and don't even know the
svn codebase very well. But the don't-write-to-const rule is universal,
so I felt I had to point this out, lest this potential bug become a real
bug at some point in the future.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org