svndigest aborts on the Proteios repository

[Jari Häkkinen]

The WC is a copy of ​http://www.proteios.org/svn/trunk

Generating output
terminate called after throwing an instance of 'theplu::svndigest::SVNException'
  what():  ???/
Aborted

svndigest 0.7.4pre (r1036 compiled 01:19:38, Feb 16 2010)

[Peter Johansson]

I assume this is with assertions disabled. It would be interesting to see the behavior of a binary built with enable-debug. Could any assertion hint what the problem is?

[Peter Johansson]

It might be the bug fixed in r1075. Can you please test if you can reproduce the error with version 0.7.4.

[Jari Häkkinen]

Replying to peter:

It might be the bug fixed in r1075. Can you please test if you can reproduce the error with version 0.7.4.

The problem is still there with

svndigest 0.7.5pre (r1104 compiled 01:19:36, Jun 16 2010)                      

[Jari Häkkinen]

Replying to peter:

I assume this is with assertions disabled. It would be interesting to see the behavior of a binary built with enable-debug. Could any assertion hint what the problem is?

configure with --enable-debug does not say anything more than without debugging.

[Jari Häkkinen]

I have traced the problem to revision 3718 in the Proteios repository:

svn log http://www.proteios.org/svn

[snip]
r3719 | gregory | 2010-05-31 08:56:45 +0200 (Mon, 31 May 2010) | 1 line

Refs #688. Refactoring variables in mascot search plugin
------------------------------------------------------------------------
r3718 | (no author) | (no date) | 1 line


------------------------------------------------------------------------
r3717 | gregory | 2010-05-25 12:40:51 +0200 (Tue, 25 May 2010) | 1 line

Fixes #686. ListUsers now uses ConfigureTableFactory2 to display table of users.
[snip]

The svn log date and author is empty. Curiously trac reports a date and author, see ​http://dev.thep.lu.se/proteios/log/branches?rev=3737 and ​http://dev.thep.lu.se/proteios/changeset/3718/branches

svndigest fails on near line ​http://dev.thep.lu.se/svndigest/browser/trunk/lib/SVNlog.cc?rev=847#L94 where the empty date is catched. However, why is the exception message malformed?

The tests was done on a 64bit SuSE linux machine. svn devel libs are version 1.6.6 where function svn_client_log3 is used by svndigest. Is there another svn_client_log function that could be used?

[Jari Häkkinen]

Replying to jari:

The tests was done on a 64bit SuSE linux machine. svn devel libs are version 1.6.6 where function svn_client_log3 is used by svndigest. Is there another svn_client_log function that could be used?

Yes there is svn_client_log4 and the latest svn_client_log5. We should test these to see if the problem goes away. Of course, changing to a later log-call will change the required svn version from 1.4 to later. Do we want this in 0.7?

[Peter Johansson]

Replying to jari:

Yes there is svn_client_log4 and the latest svn_client_log5. We should test these to see if the problem goes away. Of course, changing to a later log-call will change the required svn version from 1.4 to later. Do we want this in 0.7?

What is the difference between svn_client_log3, svn_client_log4, and svn_client_log5? Typically, in the svn api, a newer version of a function only has more parameters, and the old version is just implemented by calling the newer version. We need to know more what the difference between different svn_log functions. And does the newer version solve the problem?

I'm against bumping svn requirement in a patch release. We could, however, still use the new version and fall back to old version by using some autoconf magic. I'm not sure that is the best idea, though?

[Peter Johansson]

Replying to jari:

svndigest fails on near line ​http://dev.thep.lu.se/svndigest/browser/trunk/lib/SVNlog.cc?rev=847#L94 where the empty date is catched. However, why is the exception message malformed?

Looking at the code

throw SVNException("No date defined for revision: " + rev); 

rev is an svn_revnum_t and I'm surprised that code even compiles. Is rev converted to a char before appended to the string? This code should be changed to, e.g., use a stringstream. I won't have time Monday or Tuesday, so if you feel like it please go ahead.

[Peter Johansson]

This thread ​http://subversion.open.collab.net/ds/viewMessage.do?dsForumId=3&dsMessageId=361675

hints that the problem might be that (one of) the modified path(s) is not readable by svndigest, i.e., password is needed to access path. Indeed

svn log -r3718 http://www.proteios.org/svn/branches/gregorys

fails after asking for authentication. I don't understand why when the path is accessible via trac ​http://dev.thep.lu.se/proteios/browser/branches/gregorys

Anyway, svndigest should probably handle this case more gracefully. Since the changeset in question is outside the tree digested, we don't need the date and could simply set it to date-in-previous-rev or to some NULL-ish value. Thoughts?

It also raises the question: what happens if a node under --root lacks read permission? I suppose we will crash, but will we do it with or without style?

[Jari Häkkinen]

Replying to peter:

This thread ​http://subversion.open.collab.net/ds/viewMessage.do?dsForumId=3&dsMessageId=361675

hints that the problem might be that (one of) the modified path(s) is not readable by svndigest, i.e., password is needed to access path. Indeed

Nice catch. I suppose the Greg has not provided world read in the repository access file for that branch. It may or may not be a mistake from his side. I'll check with him (after we fixed svndigest, this is a perfect test bed) but as you say svndigest should be able to handle the situation.

svn log -r3718 http://www.proteios.org/svn/branches/gregorys

fails after asking for authentication. I don't understand why when the path is accessible via trac ​http://dev.thep.lu.se/proteios/browser/branches/gregorys

Ah, that is because trac accesses the repository directly (trac and svnrepo must reside on hte same machine, thus no http requests for trac) and no access rights restrictions.

Anyway, svndigest should probably handle this case more gracefully. Since the changeset in question is outside the tree digested, we don't need the date and could simply set it to date-in-previous-rev or to some NULL-ish value. Thoughts?

We need to treat this. Nullish may cause trouble when plots are generated. Probably fall back on previous date and issue warnings to stderr.

It also raises the question: what happens if a node under --root lacks read permission? I suppose we will crash, but will we do it with or without style?

I am for style as always.

Should we still target this for 0.7 or move it to next release?

[Jari Häkkinen]

Replying to jari:

Anyway, svndigest should probably handle this case more gracefully. Since the changeset in question is outside the tree digested, we don't need the date and could simply set it to date-in-previous-rev or to some NULL-ish value. Thoughts?

We need to treat this. Nullish may cause trouble when plots are generated. Probably fall back on previous date and issue warnings to stderr.

The solution should probably be that svndigest actually only requests logs for revisions that make up the checked out copy. In the proteios use case svndigest does not need to read the log for the unreadable branch.

[Peter Johansson]

Replying to jari:

The solution should probably be that svndigest actually only requests logs for revisions that make up the checked out copy. In the proteios use case svndigest does not need to read the log for the unreadable branch.

In principle yes, but...

It used to work like that but there were problems. Let me call the checked out directory root. You can check the exact history yourself, but I think that a couple of years ago we called log on root. It seemed to work most of the times but there are cases when a revision is not seen in the root log although it is seen in its daughter (or grand daughter) nodes. The solution was to call Node::log which returns the union of the Node's log and all logs of the sub-nodes. This works perfectly except that it is very expensive - rather than one call to log, there are N(=number of files) calls to log, i.e., expensive. The fix was to call log on the uber-root, i.e., the root of the entire project.

Given the history, I don't see a good and inexpensive way to implement your suggestion.

I think a much easier way would be to just replace empty date with date of previous revision. It's a bit ugly and it might bite us in the back, but for 0.7.x I think that is the way to go. I can whip up a patch (probably within a few days) and then we can discuss based on that.

If we feel like it, I think more complicated solutions belong in the trunk.

[Jari Häkkinen]

Replying to peter:

I think a much easier way would be to just replace empty date with date of previous revision. It's a bit ugly and it might bite us in the back, but for 0.7.x I think that is the way to go. I can whip up a patch (probably within a few days) and then we can discuss based on that.

You are right, let us go for the straightforward solution and hope we remember this discussion if needed. And I suppose that given my scenario these dates won't even be used in the plot, there is no data point to plot at the unreadable revision.

[Peter Johansson]

(In [1108]) refs #458. If we lack read permission for rev, no date is retrived and we use the date of the previous rev. This should be OK in the case when call the log for the entire repository, but might be problematic when calling logs of sub-nodes. Added an assertion to check that we don't use the workaround when calling log for a sub-node.

[Peter Johansson]

(In [1109]) use the date computed conditionally. refs #458

[Jari Häkkinen]

Replying to peter:

(In [1108]) refs #458. If we lack read permission for rev, no date is ...

I read the changeset and miss a warning message. I'd like svndigest to report problems reading the repository. Thoughts?

[Peter Johansson]

(in [1110]) refs #458. added message to stderr when date is empty, and fixed code in assertion (so it compiles)