svn_client__get_revision_number() returning -1

[neels <ne...@gmail.com>]

fyi,

I found that _get_revision_number()'s -1 revision for added nodes
would leak at least into an error message from 'svn log'.

Checking that out I noticed that -1 is the revnum returned for
_revision_unspecified; 'svn log' in that case assumes HEAD. Probably
other callers do the same or similar things, not checking _unspecified
explicitly.

The problem is that an added node returning SVN_INVALID_REVNUM for
_revision_base would look like it was _revision_unspecified. Instead,
svn_client__get_revision_number() asked for an added node's _base
should probably rather throw an error.

~Neels

Re: svn_client__get_revision_number() returning -1

[neels <ne...@gmail.com>]

I've committed my patch to get entry_t out of
svn_client__get_revision_number() in r935095 -- review would be much
appreciated.

~Neels

Re: svn_client__get_revision_number() returning -1

[Greg Stein <gs...@gmail.com>]

On Thu, Apr 15, 2010 at 07:55, Julian Foad <ju...@wandisco.com> wrote:
> On Wed, 2010-04-14 at 23:06 +0200, neels wrote:
>...
>> The problem is that an added node returning SVN_INVALID_REVNUM for
>> _revision_base would look like it was _revision_unspecified. Instead,
>> svn_client__get_revision_number() asked for an added node's _base
>> should probably rather throw an error.
>
> Yes, that might be better, as it seems to be useful to keep the "INVALID
> means HEAD" semantic.

Well... is that _base in terms of the cmdline semantic? What is that exactly?

In terms of the BASE tree, an added node is not present there, so
"give me the BASE revision for ADDED-FILE" is non-sensical and an
error is appropriate.

Cheers,
-g

Re: svn_client__get_revision_number() returning -1

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

On Wed, 2010-04-14 at 23:06 +0200, neels wrote:
> fyi,
> 
> I found that _get_revision_number()'s -1 revision for added nodes
> would leak at least into an error message from 'svn log'.

I wouldn't count the content of an error message as an API: I'm happy
for that to change.

> Checking that out I noticed that -1 is the revnum returned for
> _revision_unspecified; 'svn log' in that case assumes HEAD. Probably
> other callers do the same or similar things, not checking _unspecified
> explicitly.

It's quite common throughout Subversion to treat SVN_INVALID_REVNUM as
meaning HEAD.  The docs should always make it clear whether that's the
case.  (If they don't, they need fixing.)

> The problem is that an added node returning SVN_INVALID_REVNUM for
> _revision_base would look like it was _revision_unspecified. Instead,
> svn_client__get_revision_number() asked for an added node's _base
> should probably rather throw an error.

Yes, that might be better, as it seems to be useful to keep the "INVALID
means HEAD" semantic.

- Julian