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@xxxxxxxxxxxx
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html