Re: Index format v5

On 05/15/2012 03:49 PM, Thomas Gummerer wrote:

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