# Python script for a git repository

I am a newbie regarding Python and I would like to improve my Python-fu. As a learning exercise, I decided to write a script for helping me maintain my Unix configuration/dot-files under a git repository.

Before the code, some background on how it works: I check out a copy of the repository in my home directory called .dotfiles, and, inside that, I have the Python script called linkify, which deals with creating links from my home directory to the real files inside the .dotfiles directory.

So, without further ado, here is the code (for your reference, the respective code is in my github repository):

# New version after incorporation of suggestions

Just for the record, I have incorporated the changes received in the answers in my repository and I would love to receive further comments if that's not good. Of course, proper credit is given there for the changes suggested. Further comments are still appreciated, as I am a newbie with Python.

#!/usr/bin/env python

import errno
import os

to_keep_private = [
'cifs-credentials',
'config',
'fetchmailrc',
'gitconfig',
'gnupg',
'mpoprc',
'msmtprc',
'mutt',
'netrc',
'offlineimaprc',
'pim',
'purple',
'quodlibet',
'ssh',
]

to_ignore = [
'.',
'.git',
'.gitignore',
]

def make_private(files=[]):
if len(files) == 0:
files = os.listdir('.')
else:
# Use the list of files passed by the user, if it is not empty
pass
for filename in files:
if os.path.isdir(filename):
os.chmod(filename, 0700)
else:
os.chmod(filename, 0600)

if len(files) == 0:
files = os.listdir('.')
else:
# Use the list of files passed by the user, if it is not empty
pass

os.chdir(os.path.join(os.environ['HOME'], '.dotfiles'))
for source in files:
if source not in to_ignore:
target = os.path.join('..', '.' + source)
source = os.path.join('.dotfiles', source)
try:
except OSError, e:
if e.errno == errno.ENOENT:
pass
elif e.errno == errno.EISDIR:
print("%s is a directory. Ignoring." % target)
else:
print errno.errorcode[e.errno]
finally:

f = open('.git/hooks/post-commit', 'w')
f.write('#!/bin/sh\ngit push\n')
f.close()
os.chmod('.git/hooks/post-commit', 0755)

def parse_cli():
import optparse
parser = optparse.OptionParser()

'--install-hook',
help='Install git hook for automatic push.',
action='store_true',
default=False)
action='store_true',
default=False)
'--no-fix-perms',
help='Disable fix of permission for private files.',
action='store_true',
default=False)

return parser.parse_args()

if __name__ == '__main__':
opts, args = parse_cli()

if not opts.no_fix_perms:
make_private(args)
if opts.install_hook:

-

### Some guidelines to follow:

• If possible, as it is in your case, work with absolute path.
You'll have at the beginning to do more or less something like:

dir = '/home/rik/'
files = [os.path.join(dir,file) for file in os.listdir(dir)]


But everything should be easier and straighforward later on.

• You need to move the default behaviour - files = os.listdir('.') - one level up.
I can see one symptom and one reason for that:

1. You've wrote two times the exact same code.
2. The two different functions with that code should ignore what's your default directory, since they operate on list of files.

So, if the user does not provide his own custom list, you need to generate the list of files in . and pass it to both your functions. This also means that the functions should not have a default argument value.

• If you can, use argparse instead of optparse.
I'm not sure from which version optparse was deprecated, but the main point is that if you have argpare in your standard library use that. You have only a couple of arguments so switching shouldn't take long.

• Be consistent with your variable names.

def make_private(files=[]):
# [...]
for filename in files:
# [...]

# [...]
for source in files:
# [...]


Why one time files is a list of filenames and another a list of sources?
If you strongly believe that one function works on filenames and the other on sources maybe you need to change the argument name, otherwise being consistent will help the readability of your code.

• As Winston Ewert already told you, you should use capital letter for your constants names. Quoting from PEP8:

Constants are usually defined on a module level and written in all capital letters with underscores separating words. Examples include MAX_OVERFLOW and TOTAL.

Don't do this:

def function(lst=[]):
# ...


def function(lst=None):
if lst is None:
lst = []
# ...


The reason for that is well explained here (or google it, because it's a very well known problem in which everyone stumbles sooner or later).

I'd say that by doing this you'll also remove the useless else, but as I explained it above you should remove the whole check and move it one level up against the user arguments.

-
Thanks for the comments. Both yours and @Winston's were very helpful and I chose yours as an answer because you have fewer reputation points (I hope that he doesn't care too much here). –  rbrito Mar 4 '12 at 9:28
thanks for the suggestions. They are very much appreciated. One thing that I didn't incorporate was the change to argparse from optparse, as I have systems with old versions of Python. (And, unfortunately, since I don't even have enough reputation points, I can't upvote your answer.) –  rbrito Mar 6 '12 at 2:13
#!/usr/bin/env python

import errno
import os

to_keep_private = [


By convention, global constants are in ALL_CAPS in python

    'cifs-credentials',
'config',
'fetchmailrc',
'gitconfig',
'gnupg',
'mpoprc',
'msmtprc',
'mutt',
'netrc',
'offlineimaprc',
'pim',
'purple',
'quodlibet',
'ssh',
]

to_ignore = [
'.',
'.git',
'.gitignore',
]

def make_private(files=[]):


Generally, its dangerous to have mutable objects as defaults. They don't behave like we might expect.

    if len(files) == 0:
files = os.listdir('.')


I'd expect an empty list of files to do nothing, not operate on the current directory. For this sort of case, its more usual to use None. Have None be the default and check for that.

    else:
# Use the list of files passed by the user, if it is not empty
pass


uhh.... this has no effect.

    for filename in files:
if os.path.isdir(filename):
os.chmod(filename, 0700)
else:
os.chmod(filename, 0600)

if len(files) == 0:
files = os.listdir('.')
else:
# Use the list of files passed by the user, if it is not empty
pass


Again. Don't do this.

    os.chdir(os.path.join(os.environ['HOME'], '.dotfiles'))


I avoid changing the current directory. It gives the function unintended semantics. For example all your os.listdir calls will return different results after this. Instead, use absolute paths.

    for source in files:
if source not in to_ignore:
target = os.path.join('..', '.' + source)
source = os.path.join('.dotfiles', source)
try:
except OSError, e:
if e.errno == errno.ENOENT:
pass
elif e.errno == errno.EISDIR:
print("%s is a directory. Ignoring." % target)
else:
print errno.errorcode[e.errno]


Do you really want to try carrying on after an unknown error code?

            finally:

f = open('.git/hooks/post-commit', 'w')
f.write('#!/bin/sh\ngit push\n')
f.close()


avoid one letter variable names. They make code harder to follow. Also you should use with

 with open('.git/hooks/post-commit', 'w') as hook:
hook.write('#1/bin/sh\ngitush\n')


This will take of closing files even in the face of an exception.

    os.chmod('.git/hooks/post-commit', 0755)

def parse_cli():
import optparse


Usually better to import everything at the top of the file, not in functions

    parser = optparse.OptionParser()

'--install-hook',
help='Install git hook for automatic push.',
action='store_true',
default=False)
action='store_true',
default=False)
'--no-fix-perms',
help='Disable fix of permission for private files.',
action='store_true',
default=False)

return parser.parse_args()


This is the bit where you actually interpreting the command line. I'd do the os.listdir() here as to my mind, its part of the command line interpretation.

if __name__ == '__main__':
opts, args = parse_cli()


I'd call args: filenames to get a better idea of what's going on

    if not opts.no_linkify:
if not opts.no_fix_perms:
make_private(args)
if opts.install_hook:


## Issues of else:

The original has the following code:

    if len(files) == 0:
files = os.listdir('.')
else:
# Use the list of files passed by the user, if it is not empty
pass


and I said this was a bad idea because the else did nothing. You've asked for clarification on this point. I think you put the else block in there to make it more explicit what you are doing, that way you have clearly delinated the two cases. I'd say if you want it to be explicit, make it explicit via code not comments

if len(files_) == 0:
files = os.listdir(.)
else:
files = files_


I think this is better because the code conveys the message, rather then inline english text.

However, I'm not sure that much is gained for explicitness here. If I see:

if len(files) == 0:
files == os.listdir('.')


I can see that its implementing the logic for a default parameter, because that's a pretty common practice. I don't see that you gain much by making it more explict what the other case is. The other case is pretty obvious.

-
Thanks for comments @Winston. There are some points where I would appreciate some clarification, though., but there are some points where I would appreciate clarification. The most important of these are your comments regarding the else with a pass, even if it has a comment to not let the reader know that that's deliberate. –  rbrito Mar 4 '12 at 4:47
@rbrito, I've added to the answer trying to clarify a bit about my else/pass comments –  Winston Ewert Mar 5 '12 at 21:02
thanks for the additional comments regarding the else/pass issue. This is just a long-time habit that I carried over, as I (still) feel bad whenever I see an if without the corresponding else. BTW, I have implemented most of the issues you both pointed out in a new version of the code. Not everything, but almost all points. –  rbrito Mar 6 '12 at 2:06