Bugs item #1173768, was opened at 2005-03-31 04:05
Message generated for change (Comment added) made by broeker
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=104664&aid=1173768&group_id=4664
Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: inverted index handling
Group: None
Status: Open
>Resolution: Accepted
Priority: 5
Submitted By: Nobody/Anonymous (nobody)
>Assigned to: Hans-Bernhard Broeker (broeker)
Summary: -q mode skews database when directory is in cscope.files
Initial Comment:
I think this is similar to #1040690, only instead of
surfacing with an unreadable file in your filelist, it
occurs when there is a directory name in your filelist.
For example, if your filelist consists of:
a.c
b
c.c
(where b is a directory)
If you build the database using -q, and you search for
a symbol defined in c.c, the result will show up as
being in b instead.
I am attaching a patch that seems to have solved the
problem for me, but there may be more that needs to be
done to fix this. Please advise.
----------------------------------------------------------------------
>Comment By: Hans-Bernhard Broeker (broeker)
Date: 2006-04-19 14:45
Message:
Logged In: YES
user_id=27517
Neil: you never got round to actually posting that patch as
a patch...
As to the patch: I don't really like it the way it is. It
addes code to both instance of an already duplicated code
block. That's wasteful. This testing stuff should probably
all go directly into addsrcfile(). Or, better still, the
entire block should become a new function. I'll do that
myself, I think.
----------------------------------------------------------------------
Comment By: Neil Horman (nhorman)
Date: 2006-04-19 13:26
Message:
Logged In: YES
user_id=827328
Darlene, any comment on weather or not the patch works?
Hans do we have any consensus as to weather or not we should
include this? I personally think its a good idea. Broken
User input is bad, but goofing up our search results in
response to it, when we can easily avoid it is worse, IMHO.
----------------------------------------------------------------------
Comment By: Nobody/Anonymous (nobody)
Date: 2005-04-05 21:51
Message:
Logged In: NO
I have told the cscope user community here to place only
files into their cscope.files. However, this problem
surfaced with a user who has been using the same
cscope.files for years, including the directory names.
Though he obviously never benefitted from having the
directory name in there, it didn't harm his database. So
now I am trying to get him to move to 15.5, but he see is it
as "I have been using the same cscope.files for years but my
database build breaks with 15.5".
----------------------------------------------------------------------
Comment By: Neil Horman (nhorman)
Date: 2005-04-05 20:05
Message:
Logged In: YES
user_id=827328
ouch, my bad for the screwed up formatting. I'll repost it
as a patch file in a bit. But you get the idea, right?
----------------------------------------------------------------------
Comment By: Neil Horman (nhorman)
Date: 2005-04-05 20:04
Message:
Logged In: YES
user_id=827328
what about something like this?
===================================================================
RCS file: /cvsroot/cscope/cscope/src/dir.c,v
retrieving revision 1.22
diff -u -r1.22 dir.c
--- dir.c 8 Dec 2004 21:23:03 -0000 1.22
+++ dir.c 5 Apr 2005 18:01:11 -0000
@@ -272,6 +272,7 @@
char *file;
char *s;
int i;
+ struct stat path_stat;
makevpsrcdirs(); /* make the view source
directory list */
@@ -433,9 +434,15 @@
/* If an -I or -p arguments
was missing before,
* treat this name as the
argument: */
HANDLE_OPTION_ARGUMENT(unfinished_option,
newpath);
+
+ /* make sure the file is
really a file */
+ stat(newpath,&path_stat);
+
if (! done) {
- if ((s =
inviewpath(newpath)) != NULL) {
- addsrcfile(s);
+ if (((s =
inviewpath(newpath)) != NULL) &&
+
(!S_ISDIR(path_stat.st_mode)) &&
+
(access(newpath,R_OK) == 0)) {
+
addsrcfile(s);
} else {
(void)
fprintf(stderr,
"cscope: cannot find file %s\n",
@@ -450,9 +457,15 @@
/* If an -I or -p arguments
was missing before,
* treat this name as the
argument: */
HANDLE_OPTION_ARGUMENT(unfinished_option, path);
+
+ /* make sure the file is
really a file */
+ stat(path,&path_stat);
+
if (!done) {
- if ((s =
inviewpath(path)) != NULL) {
- addsrcfile(s);
+ if (((s =
inviewpath(path)) != NULL) &&
+
(!S_ISDIR(path_stat.st_mode)) &&
+
(access(path,R_OK) == 0)) {
+
addsrcfile(s);
} else {
(void)
fprintf(stderr, "cscope: cannot find file %s\n",
path);
----------------------------------------------------------------------
Comment By: Neil Horman (nhorman)
Date: 2005-04-05 20:02
Message:
Logged In: YES
user_id=827328
I tend to agree Hans. Its pretty dumb to put a directory in
a file that is obviously supposed to hold the names of files
that you want to scan, but allowing cscope to build a broken
database as a result when we could easily detect that a
given entry in the file is invalid doesn't seem like a very
robust design. Besides we should at least add in the same
access check for cscope.files that we use when we build the
list dynamically with scan_dir.
----------------------------------------------------------------------
Comment By: Darryl O (darrylo)
Date: 2005-04-05 15:55
Message:
Logged In: YES
user_id=27401
Well, something should be done, and I suspect that more than
documentation will be needed.
Foolish user input should never result in "silent data
corruption". Bad data should either be ignored, or result
in some kind of error/warning message.
----------------------------------------------------------------------
Comment By: Hans-Bernhard Broeker (broeker)
Date: 2005-04-05 15:39
Message:
Logged In: YES
user_id=27517
My tolerance limit to usage errors may be unusually small
today, but I don't really see why we need to fix this on the
source code side. If the documentation lead users to
believe that directories can meaningfully appear in a
cscope.files list, then let's fix the documentation. I
don't believe in trying to make software foolproof ---
nature will just make stronger fools.
Is it really that hard to guess that the correct way to
construct a cscope.files from scratch will be a variation on
find . -type f -name '*.[ch]' > cscope.files
?
----------------------------------------------------------------------
Comment By: Neil Horman (nhorman)
Date: 2005-04-05 14:39
Message:
Logged In: YES
user_id=827328
Hans, I just tried this, and it does appear to be a problem.
As it turns out the patch I did for 1040690 affects only
cscope database builds where the source tree is scanned
dynamically. In the event that a cscope.files file is
specified the makefilelist function is called from main, and
the in memory file list is build there by calling addsrcfile
for every appropriate entry in cscope.files (excluding the
option line). The problem is that makefilelist considers
directories to be legitimate entries, which as we saw with
inaccessible file in 1040690, they are not. It of course
begs the question, should anyone be putting directories in
cscope.files, but it probably wouldn't hurt to check for
this case. I'll post a patch shortly.
----------------------------------------------------------------------
Comment By: Darlene Wong (darlenew)
Date: 2005-04-05 00:44
Message:
Logged In: YES
user_id=199252
The "1.8" revision of crossref.c refers to the reversion of
the file since I imported it into my CVS repository. So I
believe when I first downloaded cscope-15.5 from SourceForge
and imported it into my repository, it got renumbered as
"1.1".
The problem does still exist with the current version, after
adding Neil Horman's patch. I just re-downloaded
cscope-15.5 and patched it. I downloaded cscope from
http://prdownloads.sourceforge.net/cscope/cscope-15.5.tar.gz?download
and took the patch from
http://sourceforge.net/tracker/download.php?group_id=4664&atid=104664&file_id=111733&aid=1040690
I am not claiming that writing the directory into the
filename list is the correct approach -- it was just
something that seemed to fix the problem for me. I welcome
any guidance you can provide towards a better solution.
I think that a lot of users place directory names into their
cscope.files, expecting cscope to index any source files in
the directory.
----------------------------------------------------------------------
Comment By: Hans-Bernhard Broeker (broeker)
Date: 2005-03-31 14:54
Message:
Logged In: YES
user_id=27517
Why does the patch appear to have been made against revision
1.8 of crossref.c? That's the source file revision
corresponding to cscope-15.3, 4 years ago.
Can you please confirm whether the problem still exists with
the curren CVS version of cscope (Neil Horman's patch to bug
#1040690 included)?
I don't think writing the directory into the filename list
is the right approach --- it should never have been passed
to crossref() in the first place. That's why we fixed
1040690 one layer above crossref().
----------------------------------------------------------------------
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=104664&aid=1173768&group_id=4664
|