Patch attached.
Nice finding, thanks for the patch!
Just more context for our better understanding.
There’re 2 sets of capabilities in the context of svn://. General capabilities that exist across protocols and svn://-specific capabilities.
General capabilities are defined in {cut “svn_ra.h”}
{code}
/**
/**
/**
/**
/**
/**
/**
/**
/**
/**
svn://-specific capabilities are defined in {cut “svn_ra_svn.h”}
{code}
/** Currently-defined capabilities. /
#define SVN_RA_SVN_CAP_EDIT_PIPELINE “edit-pipeline”
#define SVN_RA_SVN_CAP_SVNDIFF1 “svndiff1”
#define SVN_RA_SVN_CAP_SVNDIFF2_ACCEPTED “accepts-svndiff2”
#define SVN_RA_SVN_CAP_ABSENT_ENTRIES “absent-entries”
/ maps to SVN_RA_CAPABILITY_COMMIT_REVPROPS: /
#define SVN_RA_SVN_CAP_COMMIT_REVPROPS “commit-revprops”
/ maps to SVN_RA_CAPABILITY_MERGEINFO: /
#define SVN_RA_SVN_CAP_MERGEINFO “mergeinfo”
/ maps to SVN_RA_CAPABILITY_DEPTH: /
#define SVN_RA_SVN_CAP_DEPTH “depth”
/ maps to SVN_RA_CAPABILITY_LOG_REVPROPS /
#define SVN_RA_SVN_CAP_LOG_REVPROPS “log-revprops”
/ maps to SVN_RA_CAPABILITY_PARTIAL_REPLAY /
#define SVN_RA_SVN_CAP_PARTIAL_REPLAY “partial-replay”
/ maps to SVN_RA_CAPABILITY_ATOMIC_REVPROPS /
#define SVN_RA_SVN_CAP_ATOMIC_REVPROPS “atomic-revprops”
/ maps to SVN_RA_CAPABILITY_INHERITED_PROPERTIES: /
#define SVN_RA_SVN_CAP_INHERITED_PROPS “inherited-props”
/ maps to SVN_RA_CAPABILITY_EPHEMERAL_TXNPROPS /
#define SVN_RA_SVN_CAP_EPHEMERAL_TXNPROPS “ephemeral-txnprops”
/ maps to SVN_RA_CAPABILITY_GET_FILE_REVS_REVERSE /
#define SVN_RA_SVN_CAP_GET_FILE_REVS_REVERSE “file-revs-reverse”
/ maps to SVN_RA_CAPABILITY_LIST */
#define SVN_RA_SVN_CAP_LIST “list”
{code}
{cut}
When comparing both sets, it becomes clear that for some unknown reason “get-file-revs-reversed” is named in a different way: “file-revs-reverse”.
In “libsvn_ra_svn/client.c” they are mapped one-to-one using this {cut “table”}
{code}
static const char* capabilities[][2] =
{
/* { ra capability string, svn:// wire capability string} */
{SVN_RA_CAPABILITY_DEPTH, SVN_RA_SVN_CAP_DEPTH},
{SVN_RA_CAPABILITY_MERGEINFO, SVN_RA_SVN_CAP_MERGEINFO},
{SVN_RA_CAPABILITY_LOG_REVPROPS, SVN_RA_SVN_CAP_LOG_REVPROPS},
{SVN_RA_CAPABILITY_PARTIAL_REPLAY, SVN_RA_SVN_CAP_PARTIAL_REPLAY},
{SVN_RA_CAPABILITY_COMMIT_REVPROPS, SVN_RA_SVN_CAP_COMMIT_REVPROPS},
{SVN_RA_CAPABILITY_ATOMIC_REVPROPS, SVN_RA_SVN_CAP_ATOMIC_REVPROPS},
{SVN_RA_CAPABILITY_INHERITED_PROPS, SVN_RA_SVN_CAP_INHERITED_PROPS},
{SVN_RA_CAPABILITY_EPHEMERAL_TXNPROPS,
SVN_RA_SVN_CAP_EPHEMERAL_TXNPROPS},
{SVN_RA_CAPABILITY_GET_FILE_REVS_REVERSE,
SVN_RA_SVN_CAP_GET_FILE_REVS_REVERSE},
{SVN_RA_CAPABILITY_LIST, SVN_RA_SVN_CAP_LIST},
{NULL, NULL} /* End of list marker */
};
{code}
{cut}
So Marat’s patch fixes SVN_RA_CAPABILITY_GET_FILE_REVS_REVERSE case.
Also some explanation to
{code}
if (startRevision > endRevision && !hasCapability(SVNCapability.GET_FILE_REVS_REVERSED)) {
SVNErrorManager.error(SVNErrorMessage.create(SVNErrorCode.RA_NOT_IMPLEMENTED), SVNLogType.NETWORK);
}
{code}
The corresponding native SVN code looks like:
{code}
if (start > end || !SVN_IS_VALID_REVNUM(start))
SVN_ERR(
svn_ra__assert_capable_server(session,
SVN_RA_CAPABILITY_GET_FILE_REVS_REVERSE,
NULL,
pool));
{code}
and is the same for all protocols, not just for svn://-specific implementation.
But in SVNKit getFileRevisionsImpl() is abstract. We could add this check to its every implementation: FSRepository, DAVRepository, and SVNRepositoryImpl (like in the patch).
It doesn’t make sense to add it to FSRepository as it is supported there whenever SVNCapability has the corresponding constant: SVNCapability.GET_FILE_REVS_REVERSED.
The ‘’‘open question’’’ is whether we should add the same check to DAVRepository.
In either case I’ll apply the patch and add “SVNRevision.isValidRevisionNumber()” check to it as well to match native SVN code.
The patch is applied to SVNRepositoryImpl at r10769 of trunk.
I didn’t touch DAVRepository code yet. Do you think I should add the same piece there?
Dunno about DAVRepository, I haven’t checked how getFileRevisions is implemented there. If native svn does this logic for all repository types, I guess svnkit should do the same.
for some unknown reason “get-file-revs-reversed” is named in a different way
Hehe, that triggered a huge WTF when I discovered that.