r1039140 (or, spineless code is for the birds).

["C. Michael Pilato" <cm...@collab.net>]

> r1039140 | julianfoad | 2010-11-25 13:51:34 -0500 (Thu, 25 Nov 2010) | 4 lines
> Changed paths:
>    M /subversion/trunk/subversion/svn/update-cmd.c
> 
> Fix assertion failures.
> 
> * subversion/svn/update-cmd.c
>   (print_update_summary): Don't allow a URL to reach svn_dirent_*().

Why?

First, you've prevented the call to svn_dirent_get_absolute, yes.  But
immediately below that are two more calls to svn_dirent_* APIs which are not
similarly avoided.  I'll copy the code context here for your convenience:

      /* Convert to an absolute path if it's not already. */
      /* (It shouldn't be URL, but don't call svn_dirent_* if it is.) */
      if (! svn_path_is_url(path) && ! svn_dirent_is_absolute(path))
        SVN_ERR(svn_dirent_get_absolute(&path, path, iter_pool));

      /* Remove the current working directory prefix from PATH (if
         PATH is at or under $CWD), and convert to local style for
         display. */
      path_local = svn_dirent_skip_ancestor(path_prefix, path);
      path_local = svn_dirent_local_style(path_local, iter_pool);

But that's not my biggest concern.  If, as you say, the path "shouldn't be
URL", then why is it a problem that an assertion verifies as much?  I'm
really down on spineless code of this sort.

Please revert this change or provide a reasonable explanation for it.

-- 
C. Michael Pilato <cm...@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

Re: r1039140 (or, spineless code is for the birds).

["C. Michael Pilato" <cm...@collab.net>]

On 11/29/2010 02:23 PM, Julian Foad wrote:
> Julian Foad wrote:
>> We agreed that C-Mike will clean up print_update_summary() and I will
>> make the caller reject URL targets in the first place, like Noorul has
>> done for other subcommands recently.
> 
> r1040232 (mine) and r1040233 (Mike's).  Thanks for raising this, Mike.

Thanks for reviewing the commits flying by so we can ultimately catch stuff
like this.

-- 
C. Michael Pilato <cm...@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

Re: r1039140 (or, spineless code is for the birds).

[Julian Foad <ju...@wandisco.com>]

Julian Foad wrote:
> We agreed that C-Mike will clean up print_update_summary() and I will
> make the caller reject URL targets in the first place, like Noorul has
> done for other subcommands recently.

r1040232 (mine) and r1040233 (Mike's).  Thanks for raising this, Mike.

- Julian

Re: r1039140 (or, spineless code is for the birds).

[Julian Foad <ju...@wandisco.com>]

C. Michael Pilato wrote:
> > r1039140 | julianfoad | 2010-11-25 13:51:34 -0500 (Thu, 25 Nov 2010) | 4 lines
> > Changed paths:
> >    M /subversion/trunk/subversion/svn/update-cmd.c
> > 
> > Fix assertion failures.
> > 
> > * subversion/svn/update-cmd.c
> >   (print_update_summary): Don't allow a URL to reach svn_dirent_*().
> 
> Why?

I should have explained in the log message.  I answered Mike on IRC, but
for the record:

The main issue is that update *shouldn't* be operating with a URL as a
target: it doesn't make sense - but the client impl doesn't reject URL
targets from being passed through.

So sometimes - even though the output is "Skip" - a URL does reach here.

  $ svn up http://...
  Skipped 'http://...'
  Summary of updates:
    *bang*

> First, you've prevented the call to svn_dirent_get_absolute, yes.  But
> immediately below that are two more calls to svn_dirent_* APIs which are not
> similarly avoided.  I'll copy the code context here for your convenience:
> 
>       /* Convert to an absolute path if it's not already. */
>       /* (It shouldn't be URL, but don't call svn_dirent_* if it is.) */
>       if (! svn_path_is_url(path) && ! svn_dirent_is_absolute(path))
>         SVN_ERR(svn_dirent_get_absolute(&path, path, iter_pool));
> 
>       /* Remove the current working directory prefix from PATH (if
>          PATH is at or under $CWD), and convert to local style for
>          display. */
>       path_local = svn_dirent_skip_ancestor(path_prefix, path);
>       path_local = svn_dirent_local_style(path_local, iter_pool);

Oops.  I think I just missed that in my haste to check in a fix for the
buildbot failures.  You're right, that's poor.

> But that's not my biggest concern.  If, as you say, the path "shouldn't be
> URL", then why is it a problem that an assertion verifies as much?  I'm
> really down on spineless code of this sort.
> 
> Please revert this change or provide a reasonable explanation for it.

We agreed that C-Mike will clean up print_update_summary() and I will
make the caller reject URL targets in the first place, like Noorul has
done for other subcommands recently.

- Julian