You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@gmail.com> on 2011/05/22 15:30:12 UTC
Re: svn commit: r1125455 - in /subversion/trunk/subversion:
libsvn_wc/upgrade.c libsvn_wc/wc.h libsvn_wc/wc_db_pristine.c tests/cmdline/svntest/wc.py
On Fri, May 20, 2011 at 12:38, <ju...@apache.org> wrote:
> Author: julianfoad
> Date: Fri May 20 16:38:24 2011
> New Revision: 1125455
>
> URL: http://svn.apache.org/viewvc?rev=1125455&view=rev
> Log:
> Prepare to give pristine text files a filename extension of ".svn-base"
> after the string of 40 hex digits. This will help users who search the disk
> using a tool that can easily exclude matches to a given filename extension
> but cannot easily exclude pristine files any other way.
>
> This change will not be active until SVN_WC__VERSION is bumped to 29, at
> which time svntest/wc.py:text_base_path() will need to be adjusted as
> indicated in its comment.
Did you actually test this by bumping your local Subversion to 29? I
don't see how this code can possibly work. See below.
>...
> +++ subversion/trunk/subversion/libsvn_wc/upgrade.c Fri May 20 16:38:24 2011
> @@ -1213,13 +1213,47 @@ bump_to_28(void *baton, svn_sqlite__db_t
> return SVN_NO_ERROR;
> }
>
> +/* If FINFO indicates that PATH names a file, rename it to '<PATH>.svn-base'.
> + * Ignore any file whose name is not the expected length, in order to make
> + * life easier for any developer who runs this code twice or has some
> + * non-standard files in the pristine directory.
> + *
> + * A callback for bump_to_29(), implementing #svn_io_walk_func_t. */
> +static svn_error_t *
> +rename_pristine_file(void *baton,
> + const char *path,
> + const apr_finfo_t *finfo,
> + apr_pool_t *pool)
> +{
> + const char *new_path;
Please use abspath in these variable names for clarity. The walk began
with an abspath, so the svn_dirent_join() calls in svn_io_dir_walk2()
will also product abspaths.
> +
> + /* The old pristine file name was 40 hex digits. */
> + if (finfo->filetype == APR_REG && strlen(path) == 40)
Because you have abspath values, this strlen() condition can never be true.
> + {
> + new_path = apr_pstrcat(pool, path, ".svn-base", (char *)NULL);
> + SVN_ERR(svn_io_file_rename(path, new_path, pool));
If they were NOT abspaths, then the above rename would never function
because the walk does not change the current directory. But
whatever... we know they are abspaths, but the above condition doesn't
work trigger anyways.
I would also suggest using a #define for the ".svn-base" extension.
upgrade.c has plenty of these at the top. (I wouldn't bother to try
and share it from wc_db.c)
> + }
> + return SVN_NO_ERROR;
> +}
> +
> static svn_error_t *
> bump_to_29(void *baton, svn_sqlite__db_t *sdb, apr_pool_t *scratch_pool)
> {
> + const char *wcroot_abspath = ((struct bump_baton *)baton)->wcroot_abspath;
> + const char *pristine_dir;
Having abspath in the pristine_dir localvar would be nice. That would
increase clarity which would find problems like the strlen() above.
>...
Cheers,
-g
Re: svn commit: r1125455 - in /subversion/trunk/subversion:
libsvn_wc/upgrade.c libsvn_wc/wc.h libsvn_wc/wc_db_pristine.c
tests/cmdline/svntest/wc.py
Posted by Julian Foad <ju...@wandisco.com>.
Greg Stein wrote:
> On May 23, 2011 8:10 AM, "Julian Foad" <ju...@wandisco.com> wrote:
> >...
> > > > + /* The old pristine file name was 40 hex digits. */
> > > > + if (finfo->filetype == APR_REG && strlen(path) == 40)
> > >
> > > Because you have abspath values, this strlen() condition can never be
> true.
> >
> > ... I added this line, and aargh, you're right. I only tested the
> > negative condition: after introducing this check, it no longer
> > doubled-renamed the already-renamed files. But didn't test that it
> > still works.
> >
> > Thanks for the _abspath and #define suggestions. Will do.
>
> Thought it was something like that. The "innocuous change before commit" ...
> that isn't :-)
In r1126553 I have fixed the bug and the _abspath and #define
suggestions and also added a simple test for this upgrade, which I
should have done in the first place.
- Julian
Re: svn commit: r1125455 - in /subversion/trunk/subversion:
libsvn_wc/upgrade.c libsvn_wc/wc.h libsvn_wc/wc_db_pristine.c tests/cmdline/svntest/wc.py
Posted by Greg Stein <gs...@gmail.com>.
On May 23, 2011 8:10 AM, "Julian Foad" <ju...@wandisco.com> wrote:
>...
> > > + /* The old pristine file name was 40 hex digits. */
> > > + if (finfo->filetype == APR_REG && strlen(path) == 40)
> >
> > Because you have abspath values, this strlen() condition can never be
true.
>
> ... I added this line, and aargh, you're right. I only tested the
> negative condition: after introducing this check, it no longer
> doubled-renamed the already-renamed files. But didn't test that it
> still works.
>
> Thanks for the _abspath and #define suggestions. Will do.
Thought it was something like that. The "innocuous change before commit" ...
that isn't :-)
Cheers,
-g
Re: svn commit: r1125455 - in /subversion/trunk/subversion:
libsvn_wc/upgrade.c libsvn_wc/wc.h libsvn_wc/wc_db_pristine.c
tests/cmdline/svntest/wc.py
Posted by Julian Foad <ju...@wandisco.com>.
On Sun, 2011-05-22 at 09:30 -0400, Greg Stein wrote:
> On Fri, May 20, 2011 at 12:38, <ju...@apache.org> wrote:
> > Author: julianfoad
> > Date: Fri May 20 16:38:24 2011
> > New Revision: 1125455
> >
> > URL: http://svn.apache.org/viewvc?rev=1125455&view=rev
> > Log:
> > Prepare to give pristine text files a filename extension of ".svn-base"
> > after the string of 40 hex digits. This will help users who search the disk
> > using a tool that can easily exclude matches to a given filename extension
> > but cannot easily exclude pristine files any other way.
> >
> > This change will not be active until SVN_WC__VERSION is bumped to 29, at
> > which time svntest/wc.py:text_base_path() will need to be adjusted as
> > indicated in its comment.
>
> Did you actually test this by bumping your local Subversion to 29? I
> don't see how this code can possibly work. See below.
Oops, I see what I did. I tested a version of this patch with format
29, but then ...
> > + /* The old pristine file name was 40 hex digits. */
> > + if (finfo->filetype == APR_REG && strlen(path) == 40)
>
> Because you have abspath values, this strlen() condition can never be true.
... I added this line, and aargh, you're right. I only tested the
negative condition: after introducing this check, it no longer
doubled-renamed the already-renamed files. But didn't test that it
still works.
Thanks for the _abspath and #define suggestions. Will do.
- Julian
> > + {
> > + new_path = apr_pstrcat(pool, path, ".svn-base", (char *)NULL);
> > + SVN_ERR(svn_io_file_rename(path, new_path, pool));
>
> If they were NOT abspaths, then the above rename would never function
> because the walk does not change the current directory. But
> whatever... we know they are abspaths, but the above condition doesn't
> work trigger anyways.
>
> I would also suggest using a #define for the ".svn-base" extension.
> upgrade.c has plenty of these at the top. (I wouldn't bother to try
> and share it from wc_db.c)
>
>
> > + }
> > + return SVN_NO_ERROR;
> > +}
> > +
> > static svn_error_t *
> > bump_to_29(void *baton, svn_sqlite__db_t *sdb, apr_pool_t *scratch_pool)
> > {
> > + const char *wcroot_abspath = ((struct bump_baton *)baton)->wcroot_abspath;
> > + const char *pristine_dir;
>
> Having abspath in the pristine_dir localvar would be nice. That would
> increase clarity which would find problems like the strlen() above.
>
> >...
>
> Cheers,
> -g