> Nice work gesha.
> I have one general proposal: The license text should be extracted from
> the build info file and put in the separate file in the same directory
> as the build info file.

We've agreed to have a license text inside of the BUILD-INFO.txt when made discussed the format of that file.

> 
> Few comments on the code also:
> 
> > +    public function readFile($fn)
> > +    {
> > +        $this->search_path = dirname($fn);
> > +        $field = '';
> > +        $fp_read = 0;
> > +        if (is_dir($fn) or !is_file($fn) or filesize($fn) == 0) return
> false; is
> > +        $file = fopen($fn, "r") or exit("Unable to open file $fn!");
> 
> There are exceptions in PHP as well, I started using them in the project
> so I think it should be good idea that we stick with that.

Actually it is usual to open files such way. There is no suitable exception for that.

> > +                $this->text_array[$fields[0]] = trim($fields[1]);
> > +                $fp_read = 0;
> > +            }
> > +            $line = fgets($file);
> > +            if (trim($line) == "")
> > +                continue;
> > +            $fields = explode(":", $line, "2");
> > +            if ($fields[0] == "Files-Pattern") {
> > +                $tmp_arr = array();
> > +                $fp = trim($fields[1]);
> > +                while(!feof($file)) {
> > +                    $line = fgets($file);
> > +                    if (trim($line) == "")
> > +                        continue;
> > +                    $fields = explode(":", $line, "2");
> > +                    if (in_array($fields[0], $this->multiline_vars)) {
> > +                        $field = $fields[0];
> > +                        if (isset($fields[1]))
> > +                            $tmp_arr[$fields[0]] = trim($fields[1]);
> > +                        while(!feof($file)) {
> > +                            $line = fgets($file);
> > +                            if (trim($line) == "")
> > +                                continue;
> > +                            $fields = explode(":", $line, "2");
> > +                            if(in_array($fields[0], $this->fields_defined))
> {
> > +                                if ($fields[0] == "Files-Pattern") {
> > +                                    fseek($file, -(strlen($line)),
> SEEK_CUR);
> > +                                    break 2;
> > +                                }
> > +                                break;
> > +                            }
> > +                            $tmp_arr[$field] = $tmp_arr[$field].
> > +                                "\n".rtrim($line);
> > +                        }
> > +
> > +                    }
> > +                    if (isset($fields[1])) {
> > +                        $tmp_arr[$fields[0]] = trim($fields[1]);
> > +                    }
> > +                }
> > +                $this->text_array[$fp] = $tmp_arr;
> > +                unset($tmp_arr);
> > +            }
> > +        }
> > +        fclose($file);
> > +
> > +        return true;
> > +    }
> > +
> 
> First, the code above is not commented at all and was very hard to read.
> I think this will change anyway if you apply the license text extraction
> from file so I will not comment it further just now. Three nested
> 'while' loops certainly don't look good when you basically need to go
> through file only once.
> 


> >
> > === modified file 'licenses/license.php'
> > --- licenses/license.php      2012-05-11 08:23:42 +0000
> > +++ licenses/license.php      2012-05-28 09:45:23 +0000
> 
> Indentation changes makes the diff harder to read. Please keep in mind
> to do this kind of work in 2 check-in please.

There were not only indentation changes, but anyway your note is good.

> 
> 
> > +    /**
> > +     * Running readFile on a directory fnameh returns false.
> > +     */
> > +    public function test_readFile_nonFile()
> > +    {
> > +        $this->assertFalse(BuildInfo::readFile(dirname(__FILE__)));
> > +    }
> > +
> > +    /**
> > +     * Running readFile on a nonexistent file returns false.
> > +     */
> > +    public function test_readFile_nonexistentFile()
> > +    {
> > +        $this->assertFalse(BuildInfo::readFile("nonexistent.file"));
> > +    }
> > +
> > +    /**
> > +     * Running readFile on a regular file returns true.
> > +     */
> > +    public function test_readFile_file()
> > +    {
> > +        $bi = new BuildInfo();
> > +        $this->assertTrue($bi->readFile("tests/BUILD-INFO.txt"));
> > +    }
> > +
> Yes it does. But it also sets the 'text-array' variable with some very
> important data. This should be tested as well securing that array is not
> empty, the format of the array (it's multidimensional) etc.

The array can be empty. But anyway tests to check for correct values are added to verify that parsing works.

> 
> > +    /**
> > +     * Running 'get' functions on an empty fields returns false.
> > +     */
> > +    public function test_getFormatVersion_empty()
> > +    {
> > +        $this->assertFalse($this->bi->getFormatVersion());
> > +    }
> > +
> > +    /**
> > +     * Running 'get' functions on non-empty fields returns string value.
> > +     */
> > +    public function test_getFormatVersion()
> > +    {
> > +        $this->assertInternalType('string',
> $this->good_bi->getFormatVersion());
> > +    }
> 
> This is not enough. Testing that all the parsed values are not empty or
> strings will not make sure that our code is 100% working. One solution
> is to make a mock BUILD_INFO file and test the exact values you have put
> there. Or use the existing one from our trunk if it's possible.

Tests are added based on BUILD-INFO.txt