[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Incorrectness in svn_opt__split_arg_at_peg_revision() [was: regression from 1.4.3 to 1.5.1 in handling filenames containg at (@) signs]

From: Karl Fogel <kfogel_at_red-bean.com>
Date: Wed, 27 Aug 2008 16:23:44 -0400

I think svn_opt__split_arg_at_peg_revision() is buggy. This discovery
was spurred by a bug report (quoted at the end of this mail).

Here's the svn_opt__split_arg_at_peg_revision API:

   /* Extract the peg revision, if any, from UTF8_TARGET. Return the peg
    * revision in *PEG_REVISION and the true target portion in *TRUE_TARGET.
    * *PEG_REVISION will be an empty string if no peg revision is found.
    *
    * UTF8_TARGET need not be canonical. *TRUE_TARGET will not be canonical
    * unless UTF8_TARGET is.
    *
    * Note that *PEG_REVISION will still contain the '@' symbol as the first
    * character if a peg revision was found.
    *
    * All allocations are done in POOL.
    */
   svn_error_t *
   svn_opt__split_arg_at_peg_revision(const char **true_target,
                                      const char **peg_revision,
                                      const char *utf8_target,
                                      apr_pool_t *pool);

And here's how we call it in svn_client_args_to_target_array() in
libsvn_client/cmdline.c (indentation adjusted for email):

    /*
     * This is needed so that the target can be properly canonicalized,
     * otherwise the canonicalization does not treat a "._at_BASE" as a "."
     * with a BASE peg revision, and it is not canonicalized to "@BASE".
     * If any peg revision exists, it is appended to the final
     * canonicalized path or URL. Do not use svn_opt_parse_path()
     * because the resulting peg revision is a structure that would have
     * to be converted back into a string. Converting from a string date
     * to the apr_time_t field in the svn_opt_revision_value_t and back to
     * a string would not necessarily preserve the exact bytes of the
     * input date, so its easier just to keep it in string form.
     */
    SVN_ERR(svn_opt__split_arg_at_peg_revision(&true_target, &peg_rev,
                                               utf8_target, pool));

And here's an annotated GDB session, starting from right before the
above call, showing the bugginess:

   (gdb) p utf8_target
   $6 = 0x9e47570 "dir/@file"

   ### Hmmm, I want to test what happens with "dir/@file@" instead,
   ### so I'll just set utf8_target to that:

   (gdb) p utf8_target = "dir/@file@"
   $7 = 0x9e49988 "dir/@file@"

   ### Great. Now, let's step into svn_opt__split_arg_at_peg_revision():

   (gdb) s
   svn_opt__split_arg_at_peg_revision (true_target=0xbfe058b8, \
                                       peg_revision=0xbfe058b4, \
                                       utf8_target=0x9e49988 "dir/@file@", \
                                       pool=0x9e45990) \
                                       at subversion/libsvn_subr/opt.c:1098
   (gdb) n
   (gdb) n
   (gdb) n
   (gdb) ...

   ### Etc, etc. Watch as we get to this code near the end:
   ###
   ### if (peg_start)
   ### {
   ### *true_target = apr_pstrmemdup(pool, utf8_target, j);
   ### *peg_revision = apr_pstrdup(pool, peg_start);
   ### }
   ###
   ### Whoa! Surprisingly, 'peg_start' is non-NULL and non-"" here:

   (gdb) p peg_start
   $8 = 0x9e49991 "@"

   ### Now, why is that? There shouldn't be any peg revision on this
   ### path; indeed, the whole point of the final '@' was to prevent
   ### there from being a peg revision. But if you look at the
   ### for-loop earlier in svn_opt__split_arg_at_peg_revision(), you
   ### can see exactly why it wrongly thinks there is a peg revision
   ### (that is, a 'peg_start'): on the very first iteration of the
   ### loop, the following condition will be true, so we'll break out
   ### with a real 'peg_start':
   ###
   ### if (utf8_target[j] == '@')
   ### {
   ### peg_start = &utf8_target[j];
   ### break;
   ### }
   ###
   ### And from this point on things are wrong, of course, though at
   ### least they're wrong in the expected way:

   (gdb) n
   (gdb) n
   (gdb) n
   svn_client_args_to_target_array (targets_p=0xbfe05988, \
                                    os=0x9e45b38, \
                                    known_targets=0x0, \
                                    ctx=0x9e46328, \
                                    pool=0x9e45990) \
                                    at subversion/libsvn_client/cmdline.c:229
   (gdb) p true_target
   $11 = 0x9e47598 "dir/@file"
   (gdb) p peg_rev
   $12 = 0x9e475a8 "@"

   ### Mmmm. Yup. So, 'peg_rev' should really be "", right?

Should I just make the obvious fix -- that is, set 'peg_start' to NULL
if we're on the first iteration of the loop -- or is there something
deeper going on here?

-Karl

scottnotrobot_at_gmail.com writes:
> in 1.4.3, this works:
>
> mkdir dir
> svn add dir
> svn ci -m test dir
> touch dir/@file
> svn add dir/@file
> svn ci -m test dir/@file
>
> but in 1.5.1, when trying to add @file, i get:
>
> svn: warning: 'dir_at_file' not found
>
> i also tried: "svn add dir/@file@", but i get:
>
> svn: warning: 'dir/@file@' not found

Yeah, there's a bug here, I think; see my long explanation above.

Also, note that the script below succeeds in adding and committing
"dir/@file@", because I adjust the 'touch' line to use that filename
too. However, I shouldn't have to adjust the 'touch' line to use
"dir/@file@"! Instead, Subversion should transform "dir/@file@" to
"dir/@file", as you understandably expected.

-----------------------------------------------------------------------
#!/bin/sh

# The next line is the only line you should need to adjust.
SVNDIR=/home/kfogel/src/subversion

SVN=${SVNDIR}/subversion/svn/svn
SVNADMIN=${SVNDIR}/subversion/svnadmin/svnadmin
URL=file:///`pwd`/repos

rm -rf repos wc
${SVNADMIN} create repos
${SVN} co -q ${URL} wc

cd wc
mkdir dir
${SVN} add dir
${SVN} ci -m test dir
touch dir/@file@
${SVN} add dir/@file@
${SVN} ci -m test dir/@file@
cd ..

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-08-27 22:24:00 CEST

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.