On Sun, 2011-05-22 at 09:30 -0400, Greg Stein wrote:
> On Fri, May 20, 2011 at 12:38, <julianfoad_at_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
Received on 2011-05-23 14:11:09 CEST