Re: [PATCH] issue #3620: svn add ^/ triggers assertion failure

Greg Stein wrote:
> On Fri, Apr 23, 2010 at 11:58, C. Michael Pilato <cmpilato_at_collab.net> wrote:
>> Uwe Stuehler wrote:
>>> [[[
>>> Fix issue #3620: svn add ^/ triggers assertion failure
>>>
>>> * subversion/svn/add-cmd.c:
>>> (svn_cl__add): Raise an error if a target argument isn't a working
>>> copy path before calling svn_client_add4(), which would trigger an
>>> assertion.
>>>
>>> Found by: stsp
>>> Patch by: Uwe Stuehler (subversion-lists_at_bsdx.de)
>>> ]]]
>> Uwe,
>>
>> Thanks again for your patch. I took some time to look at it just now. The
>> patch, as patches go, is fine. But the approach isn't the best one. Here
>> are the notes I just recorded in issue #3620:
>>
>> ----------------------------------------------------------------------------
>> 'svn st ^/' suffers from the same thing.
>>
>> Subversion 1.6 handles these cases gracefully. Note the use of the
>> svn_cl__try() wrapper arounds the client's calls to svn_client_status5() and
>> svn_client_add4() -- it clearly is expecting those APIs to return a
>> particular error code when asked to operate on paths that aren't working
>> copies. The fix for this issue needs to preserve those behaviors (which
>> means it must live behind the libsvn_client API).
>>
>> I'm not confident, but I suspect that the root cause is the use of
>> svn_dirent_get_absolute() all over the place for WC-NG-ness. That function
>> asserts (ultimately) that its input isn't a URL. We need to either make it
>> return a graceful error in those conditions, or do this kind of checking in
>> its callers.
>> ----------------------------------------------------------------------------
>>
>> We need to figure out the right approach before moving on.
>
> I don't think this is rocket science. My comment in the issue:
>
> ----------------------------------------------------------------------------
> I think that svn_client_add4 should check if a URL is passed and
> return an error. Simple as that. Then we
> can proceed with the svn_dirent_get_absolute call.
>
> I don't really see the point about prior error returns and svn_cl__try
> and stuff. It is clear that a bad input
> was provided to svn_client_add4, and it should return an error stating such.
> ----------------------------------------------------------------------------
>
> Basically, put Uwe's patch right into svn_client_add4, rather than in
> the cmdline tool.

As I noted, the check needs to happen inside the client API. The question
becomes "Do we patch svn_client_addX() and svn_client_statusX() and
svn_client_updateX() and ..." or is there a more general fix to be made
(like perhaps a wrapper around svn_dirent_get_absolute() that gracefully
handles the my-caller-passed-a-url case)?

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