Erroneous handling of broken links to the config file

[Jari Häkkinen]

related to ticket:351

There should be a message telling the user about the problematic config file, i.e., a link that is broken. Havind done

cd .svndigest
ln -s ../.svndigest/config

This is a link to itself! Doing a cat on this file returns

# cat .svndigest/config
cat: .svndigest/config: Too many levels of symbolic links

whereas svndigest happily ignores the problem. Even if the link is to a non-existing file svndigest doesn't mind

# cat .svndigest/config 
cat: .svndigest/config: No such file or directory
# svndigest --no-report --copyright --verbose
Done parsing parameters
Initializing SVN singleton.
Checking target directory
Acquiring repository information
Building directory tree
Parsing directory tree

This is how we want svndigest to handle config

1. If config file is given with --config-file complain if node does not exist (see ticket:351)
2. If node exists, complain if node is not a file, or a symlink pointing to a file, or a symlink pointing to a symlink pointing to a file, etc. This also includes complaining if node is a dir or any other funky type of node (remember to catch the case where a link is a link to itself).
3. If node exists but is not readable, complain (I think this already taken care of in main).
4. If node exists but is not a svndigest config file, complain. Well this is not a clear-cut case, but incorrectly formatted config files should be taken care of by Configuration class. If the load function finds something funky, throw an exception.
5. If no --config-file switch is given and there exists no /path/to/root/.svndigest/config, then silently use a default configuration.

[Peter Johansson]

Reading a config file from a symbolic link works for me. Can you please add more details.

[Jari Häkkinen]

In my root WC directory I have a directory .svndigest with the config file

/path/to/WC/.svndigest/config

I want to run svndigest only on a branch of my repo but still use the same config like this

cd /path/to/WC/bin
mkdir .svndigest
cd .svndigest
ln -s ../../.svndigest/config .
cd ..
svndigest --copyright

The program runs but complains about missing aliases. If I change the link command above to copy instead then svndigest does not complain.

[Peter Johansson]

What if you issue svndigest --generate-config? Is the output similar to the link or is it the default config file?

And you are using trunk version?

[Jari Häkkinen]

Replying to peter:

What if you issue svndigest --generate-config? Is the output similar to the link or is it the default config file?

The same configuration file I link to works if I instead use a copy of it. My config file contains only two sections

### Section for setting aliases used in copyright update
[copyright-alias]

### Section for setting trac environment
[trac]
# If trac-root is set, svndigest will create anchors to the Trac page.

And you are using trunk version?

Yes.

The problem may be that the configuration reader opens the symbolic link entry in the operating system file table as an ordinary file. The file content of an symbolic link is a pointer to the file to be used instead of the symbolic link entry. I think the way forward is to print whatever the configuration reads.

[Jari Häkkinen]

So, I found the problem. It was a user error ... the link was not pointing on the configuration file.

[Jari Häkkinen]

I closed the ticket prematurely. There should be a message telling the user about the problematic config file, i.e., a link that is broken. In my case I had

cd .svndigest
ln -s ../.svndigest/config

This is a link to itself! Doing a cat on this file returns

# cat .svndigest/config
cat: .svndigest/config: Too many levels of symbolic links

Even if the link is to a non-existing file svndigest doesn't mind

# cat .svndigest/config 
cat: .svndigest/config: No such file or directory
# svndigest --no-report --copyright --verbose
Done parsing parameters
Initializing SVN singleton.
Checking target directory
Acquiring repository information
Building directory tree
Parsing directory tree

So, I simply change the description of this issue.

[Peter Johansson]

ticket:351 was marked as related

[Peter Johansson]

On line 71 in svndigest.cc there is a call

if (node_exist(option->config_file())) {

which probably returns false, and loading is skipped.

I think we should add a test for your case in Parameter::analyse.

[Peter Johansson]

The question is: how to differentiate between node does not exists and node exist in form of a broken link.

[Jari Häkkinen]

Replying to peter:

The question is: how to differentiate between node does not exists and node exist in form of a broken link.

The things it that the node exists, there is a link to something. The problem is that a link can point to it self, to a file that is not a config file, or to nothing (a broken link). I am excluding the case where the link actually points to a proper config file (this works in the current trunk version). The 'cat' program can detect a link that points to itself and also a broken link, so we can look into that program. The third case when a proper link exists but points to a file that is not a svndigest configuration file should be detected as a badly formatted config file is done now ... and now I understand your question and my answer; do we need to differentiate? Just say file not found in both cases. So, now there is only the case of a link to it self that must be resolved. Of course everything must also be coded into svndigest.

You don't agree this is a defect?

[Peter Johansson]

if you insist I could start think this is a defect

I think we could solve this by combining calls to stat and lstat

[Peter Johansson]

btw previously we have not required that there is a config file. If there is no config file we use default settings. So I disagree with your statement that we don't need to differentiate and can complain about missing config.

[Peter Johansson]

so to clarify, this is how I think it should work

1) If config file is given with --config-file complain if node does not exist (see ticket:351)

2) If node exists complain if node is not a file, or a symlink pointing to a file, or a symlink pointing to a symlink pointing to a file, etc. This also includes complaining if node is a dir or any other funky type of node.

3) If node exists but is not readable, complain (I think this already taken care of in main).

4) If node exists but is not a svndigest config file, complain. Well this is not a clear-cut case, but incorrectly formatted config files should be taken care of by Configuration class. If the load function finds something funky, throw an exception.

5) If no --config-file switch is given and there exists no /path/to/root/.svndigest/config, then silently use a default configuration.

[Jari Häkkinen]

Perfect, I was just writing on this but could not submit. I moved your last entry do the description, and yes I made some modifications.

[Peter Johansson]

ok if you have a symlink -> symlink, then

struct stat nodestat;
std::cout << stat("symlink",&nodestat) << "\n";
std::cout << lstat("symlink",&nodestat) << "\n";

will give you

-1
0

in other words, lstat tells us that there is a node and stat tells us that the link is not pointing to anything valid. I quote from man lstat:

Lstat() is like stat() except in the case where the named file is a symbolic link, in which case lstat() returns information about the link, while stat() returns information about the file the link references.

So I guess this gives us the recipe on how to differentiate.

[Peter Johansson]

(In [705]) importing classes for commandline parsing from ​yat. This fixes #349 and #265

[Peter Johansson]

(In [708]) refs #349. Fixed so svndigest complains about broken symlink for config file also when no argument was given at cmd, i.e., default file is a broken symlink