Re: Index format v5

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

On 05/15/2012 03:49 PM, Thomas Gummerer wrote:
> Thanks again for your feedback. I've refactored the code again,
> thanks to your suggestions.

I just reviewed version 1369bd855b86 of your script, and it is MUCH 
better.  It's easy to read and review.  The functions that it defines 
are now self-contained and could therefore be reused for other purposes. 
  There are fewer magic numbers (though there are still a few; I wonder 
if there is a way to get rid of those?)  You've done a nice job 
polishing up the code.

I have only a few remaining niggles::

1. The struct module can handle fixed-length strings, so you could read 
and parse the SHA1s as part of FILE_DATA_STRUCT and DIR_DATA_STRUCT 
rather than handling them separately.

2. At least some of the functions deserve docstrings, especially when 
they are nontrivial.  For example, what arguments read_files() needs and 
how they are used is far from obvious.

3. It would be easier to read the multiline string formatting templates 
if they are written as multiline strings (even though this kindof 
requires that they be made file-level constants); e.g.,

FILE_FORMAT = """\
%(name)s (%(objhash)s)
mtime: %(mtimes)s:%(mtimens)s
mode: %(mode)s flags: %(flags)s
statcrc: """

In the future, please try to commit one self-contained change at a time 
and make your commit messages really describe what is changed in the 
commit.  For example, commit fb2654c648a does at least three things, 
only one of which is mentioned in its commit message.  It would be 
better to break this into three commits with three commit messages.  Use 
"git rebase -i" and the other commit-rewriting tools liberally to tidy 
up commits before publishing them (but not after publishing them!); for 
example, commit be8d01c22c should really have been squashed on top of 
"Changed main to a function" so that the rest of the world doesn't have 
to see the broken latter commit.

But I don't want to detract from the fact that altogether you have made 
very nice improvements to the script, and I hope to see more like this 
from you in the future!

Michael

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