Re: Index format v5

[Michael Haggerty <mhagger@alum.mit.edu>]

On 05/14/2012 05:01 PM, Thomas Gummerer wrote:
> Thanks a lot for your feedback. I've now refactored the code and
> thanks to your suggestions hopefully made it simpler and easier
> to read. The reader should now read exactly the data from the
> spec.

Yes, the style is much better now.  Here is the next round of feedback:

1. f is still a global variable.  This is unnecessary.

2. read_name() is indented incorrectly.

3. The signature and version number should be checked within 
read_header() *before* reading anything else.  Otherwise, if some random 
file is given to the script, it will read some random, probably very 
large number for "nextensions" and then try to read that number of 
extension offsets into RAM.  It would be a pretty innocent error to have 
the wrong version of the index file lying around, and the script should 
detect such an error quickly and painlessly rather than triggering a lot 
of pointless disk activity (and likely an out-of-memory error) before 
figuring out that it shouldn't be reading the file in the first place.

4. For readability reasons, it is better to raise an exception 
immediately in an error situation rather than put the error-handling 
code in the "else" part of an if statement; i.e., instead of

     if not error_condition:
         non-error-actions
     else:
         raise exception

use

     if error_condition:
         raise exception

     non-error-actions

The reasons are: (a) the reader can see immediately what will happen in 
the error case, then forget it and continue on with the "normal" flow 
instead of having to remember the "if" condition through the whole long 
"then" block before finally seeing the point in the "else" block; (b) 
then the "normal" case doesn't have to be within the if statement at 
all, thereby requiring less block nesting and less indentation.  For 
example:

> -    if crc == datacrc:
> -        return dict(signature=signature, vnr=vnr, ndir=ndir, nfile=nfile,
> -                nextensions=nextensions, extoffsets=extoffsets)
> -    else:
> -        raise Exception("Wrong header crc")
> +    if crc != datacrc:
> +        raise Exception("Wrong header crc")
> +
> +    return dict(signature=signature, vnr=vnr, ndir=ndir, nfile=nfile,
> +                nextensions=nextensions, extoffsets=extoffsets)

5. If the first limit of a range() or xrange() is zero, it is usually 
omitted; e.g., "xrange(0, nextensions)" -> "xrange(nextensions)".

6. It is possible to precompile "struct" patterns, which should be 
faster and also allows you to ask the Struct object its size, reducing 
the number of magic numbers needed in the code.  And fixed-length 
strings can also be read via struct.  For example:

> DIR_DATA_STRUCT = struct.Struct("!HIIIIII 20s")
>
>
> def read_dirs(f, ndir):
>     dirs = list()
>     for i in xrange(0, ndir):
>         (pathname, partialcrc) = read_name(f)
>
>         (filedata, partialcrc) = read_calc_crc(f, DIR_DATA_STRUCT.size, partialcrc)
>         (flags, foffset, cr, ncr, nsubtrees, nfiles,
>                 nentries, objname) = DIR_DATA_STRUCT.unpack(filedata)
>     # ...

7. The "if dirnr == 0" stuff in read_files() should probably be done in 
read_index_entries() (the first invocation is the only place dirnr==0, 
right?)

8. read_files() only returns a result "if len(directories) > dirnr". 
But actually I don't see the purpose for the check.  Earlier in the 
function you dereference directories[dirnr] several times, so it *must* 
be that dirnr < len(directories).  I think you can take out this test 
and unindent its block.

9. read_files() doesn't need to return "entries".  Since entries is an 
array that is only mutated in place, the return value will always be the 
same as the "entries" argument (albeit fuller).

10. It would make sense to extract a function read_file(), which reads a 
single file entry from the current position in the current file (using 
code from read_files()).  Similarly for read_dir()/read_dirs().

11. It is good form to move the file-level code into a main() function, 
then call that from the bottom of the file, something like this:

 > def main(args):
 >     ....
 >
 > main(sys.argv[1:])

This avoids creating global variables that are accidentally used within 
functions.


What is your plan for testing this code, and later the C version?  For 
example, you might want to have a suite of index files with various 
contents, and compare the "git ls-files --debug" output with the output 
that is expected.  How would you create index files like this?  Via git 
commands?  Or should one of your Python scripts be taught how to do it?

To make testing easier, you probably don't want to hard-code the name of 
the input file in git-read-index-v5.py, so that you can use it to read 
arbitrary files.  For example, you might want to honor the 
GIT_INDEX_FILES environment variable in some form, or to take the name 
of the index file as a command-line argument.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/