Git command: push the latest commit and email the diff in colour

Question

This is my first attempt a for git command I can use to push the latest commit and email the diff. Story behind how it came up: http://goo.gl/3NEHvn

Revision 1 (Scroll down for 2nd Revision)

import argparse
import subprocess
import os
import re
import send
import sys
class INVALIDEMAIL(Exception):
 pass
def main():
 parser = argparse.ArgumentParser(prog='ipush', description='Utility to push the last commit and email the color diff')
 parser.add_argument('-v', '--verbose', action='store_true', help='if enabled will spit every command and its resulting data.')
 parser.add_argument('-c', '--compose', help='text message to be sent with diff')
 parser.add_argument('-to', type=str, help='enter a valid email you want to send to.')
 parser.add_argument('-ver', '--version', action='version', version='%(prog)s 1.0')
 parser.add_argument('-d', '--diff', required=False, help='if present pass arguments to it as you will do to git diff in inverted commas')
 args = parser.parse_args()
 VERBOSE = args.verbose
 optArgs = vars(args)
 if optArgs['to']:
 sendTo = optArgs['to']
 pattern = "^([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})$"
 if not re.match(pattern, sendTo) != None:
 raise INVALIDEMAIL("You have entered an invalid email to send to.")
 else:
 sendto = "[email protected]"
 diffCmd = 'git diff HEAD^ HEAD' if not optArgs['diff'] else "git diff %s" % optArgs['diff']
 branchName, _ = execGitCommand('git rev-parse --abbrev-ref HEAD')
 # stripping newline character which got appended when pulling branch name
 branchName = branchName.split("\n")[0]
 commitComment, _ = execGitCommand('git log -1 --pretty=%B')
 subject = "%s: %s" % (branchName, commitComment)
 # check for fatal error when executing git command
 diffData, error = execGitCommand(diffCmd, VERBOSE)
 if 'fatal' not in error.split(":"):
 modifiedData, error = execGitCommand('git status', VERBOSE)
 if any([re.search(word, modifiedData) for word in ['modified', 'untracked']]):
 print "\x1b[31m You have uncommited changes, Commit and try again:\x1b[m"
 return
 # only push that is displayed 
 if diffCmd in ['git diff HEAD^ HEAD', 'git diff HEAD~ HEAD']:
 pushBranch(VERBOSE)
 if diffData:
 htmlDiff = getHtml(diffData.split("\n"))
 message = htmlDiff if not optArgs['compose'] else "%s<br><br>%s" % (
 optArgs['compose'], htmlDiff)
 emailDiff(subject, sendto, message)
 else:
 print error.capitalize()
def getHtml(diffData):
 """ This method convertes git diff data to html color code
 """
 lines = ""
 openTag = "<span style='font-size: .80em; color: "
 openTagEnd = ";font-family: courier, arial, helvetica, sans-serif;'>"
 nbsp = '&nbsp;&nbsp;&nbsp;&nbsp;'
 for eachLine in diffData:
 if eachLine.startswith('-'):
 tabs = eachLine.count('\t')
 lines += "%s#ff0000%s%s%s</span><br>" % (openTag, openTagEnd, nbsp*tabs ,eachLine)
 elif eachLine.startswith('+'):
 tabs = eachLine.count('\t')
 lines += "%s#007900%s%s%s</span><br>" % (openTag, openTagEnd, nbsp*tabs, eachLine)
 else:
 tabs = eachLine.count('\t')
 lines += "%s#000000%s%s%s</span><br>" % (openTag, openTagEnd, nbsp*tabs, eachLine)
 return lines
def execGitCommand(command=None, verbose=False):
 """ Function used to get data out of git commads
 and errors in case of failure.
 Args:
 command(string): string of a git command
 verbose(bool): whether to display every command
 and its resulting data.
 Return:
 (tuple): string of Data and error if present
 """
 if command:
 pr = subprocess.Popen(command, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
 msg = pr.stdout.read()
 err = pr.stderr.read()
 txt = "\x1b[32mResult:\x1b[m\n"+msg if msg else "\x1b[31mError:\x1b[m\n"+err
 if verbose:
 print "Executing '%s'\n%s" % (command, txt)
 return msg, err
def emailDiff(subject, emailTo, htmlDiff):
 """ This function send color diff via email
 Args:
 subject(string): name of the branch with commit message
 htmlDiff(string): html formatted string
 """
 mail = send.EMail(
 mailFrom=os.getenv('myemail'), 
 server='smtp.gmail.com', 
 usrname=os.getenv('myemail').split('@')[0],
 password=os.getenv('PASS'), 
 files=None, 
 debug=False
 )
 mail.sendMessage(subject, htmlDiff, None, emailTo)
 print "\x1b[31m Diff of branch, %s sent to email: %s .\x1b[m" % (subject, emailTo)
def pushBranch(VERBOSE):
 """ Pushes the branch to remote repository
 Args:
 VERBOSE(bool): defines whether to spit out which command
 is being executed and result of command.
 Return:
 (bool) True if push Succesfull else False 
 """
 _, err = execGitCommand('git push')
 if re.search(r'rejected', err):
 print '%s\n\x1b[31mDo you want to try force push?\x1b[m\n\x1b[31mEnter yes to force Push else any key to cancel\x1b[m' % err
 # default value yes and hit enter without entering anything
 answer = raw_input('[YES]')
 if answer and answer.lower() not in ['yes', 'y', 'f', 'git push -f']:
 print "Cancelled !!!"
 else:
 execGitCommand('git push -f', VERBOSE)
 print "\x1b[33mPush Succesfull !\x1b[m" if VERBOSE else ""
 return True
 else:
 print "\x1b[33mPush Succesfull !\x1b[m" if VERBOSE else ""
 return True
 return False
if __name__ == '__main__':
 sys.exit(main())

It has dependency file that sends the email [send.py]

from email.MIMEMultipart import MIMEMultipart
from email.MIMEBase import MIMEBase
from email.MIMEText import MIMEText
from email.Utils import COMMASPACE, formatdate
from email import Encoders
import smtplib
class EMail(object):
 """ Class defines method to send email
 """
 def __init__(self, mailFrom, server, usrname, password, files, debug=False):
 self.debug = debug
 self.mailFrom = mailFrom
 self.smtpserver = server
 self.EMAIL_PORT = 587
 self.usrname = usrname
 self.password = password
 def sendMessage(self, subject, msgContent, files, mailto):
 """ Send the email message
 Args:
 subject(string): subject for the email
 msgContent(string): email message Content
 files(List): list of files to be attached
 mailto(string): email address to be sent to
 """
 msg = self.prepareMail(subject, msgContent, files, mailto)
 # connect to server and send email
 server=smtplib.SMTP(self.smtpserver, port=self.EMAIL_PORT)
 server.ehlo()
 # use encrypted SSL mode
 server.starttls()
 # to make starttls work
 server.ehlo()
 server.login(self.usrname, self.password)
 server.set_debuglevel(self.debug)
 try:
 failed = server.sendmail(self.mailFrom, mailto, msg.as_string())
 except Exception as er:
 print er
 finally:
 server.quit()
 def prepareMail(self, subject, msgHTML, attachments, mailto):
 """ Prepare the email to send
 Args:
 subject(string): subject of the email.
 msgHTML(string): HTML formatted email message Content.
 attachments(List): list of file paths to be attached with email. 
 """
 msg = MIMEMultipart()
 msg['From'] = self.mailFrom
 msg['To'] = mailto
 msg['Date'] = formatdate(localtime=True)
 msg['Subject'] = subject
 #the Body message
 msg.attach(MIMEText(msgHTML, 'html'))
 msg.attach(MIMEText("Sent using git ipush\n git clone https://[email protected]/sanfx/git-ipush.git"))
 if attachments:
 for phile in attachments:
 # we could check for MIMETypes here
 part = MIMEBase('application',"octet-stream")
 part.set_payload(open(phile, "rb").read())
 Encoders.encode_base64(part)
 part.add_header('Content-Disposition', 'attachment; filename="%s"' % os.path.basename(phile))
 msg.attach(part)
 return msg

I would really appreciate if you can help me make it more better. How neat is this first attempt?

Revision 2: I have updated the following methods:

def getHtml(diffData):
 """ This method convertes git diff data to html color code
 Args:
 diffData(sting): diff between commits in simple text
 """
 openTag = "<span style='font-size: .80em; color: "
 openTagEnd = "00;font-family: courier, arial, helvetica, sans-serif;'>"
 nbsp = '&nbsp;&nbsp;&nbsp;&nbsp;'
 lines = []
 for line in diffData:
 color = "#ff00" if line.startswith('-') else ('#0079' if line.startswith('+') else '#0000')
 tabs = line.count('\t')
 lines.append("%s%s%s%s%s</span><br>" % 
 ((openTag, color, openTagEnd, nbsp*tabs ,line)))
 return ''.join(lines)

and this too:

def validate_address(address):
 """ If address looks like a valid e-mail address, return it. Otherwise
 raise ArgumentTypeError.
 Args:
 address(string): email address to send to
 """
 if re.match('^([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})$', address):
 return address
 raise argparse.ArgumentTypeError('Invalid e-mail address: %s' % address)

• Ciasto piekarz: OOps I forgot to run pylint before posting, I'll do that next.
• ChrisWue: What's the point of checking for the valid email address? You exclude a fair amount of valid ones.
• Ciasto piekarz: @ChrisWue: Like what ?
• ChrisWue: like user@domain or "Jon Doe"@example.com. Admittedly not extremely likely (except maybe for the first one). It's much more likely the user has a typo in the address - in which case it's still valid just the wrong address.
• user688: Don't forget address extensions! ([email protected] )

Answer 1

Here's my review on the Python code :

As a general comment, your code does not follow this. I'll try to hilight the different points as I see them but for the most obvious one, the namming convention is not respected. You can find tools to check automatically that everything is fine. For instance, http://pep8online.com/ .

This can take a default value as an argument. I guess you could use this to remove the if optArgs['to'] logic.

Comparisons to singletons like None should always be done with is or is not, never the equality operators.

if not re.match(pattern, sendTo) != None:

becomes

if re.match(pattern, sendTo) is None:

You could remove duplication from

diffCmd = 'git diff HEAD^ HEAD' if not optArgs['diff'] else "git diff %s" % optArgs['diff']

by writing it

diffCmd = "git diff %s" % (optArgs['diff'] if optArgs['diff'] else "HEAD^ HEAD")

Purely personal but I find

message = htmlDiff if not optArgs['compose'] else "%s<br><br>%s" % (optArgs['compose'], htmlDiff)

easier to read when conditions are not inverted:

message = "%s<br><br>%s" % (optArgs['compose'], htmlDiff) if optArgs['compose'] else htmlDiff

I do not think eachLine is a very good variable name. line seems to be simple enough (and it does follow the naming conventions).

def getHtml(diffData):
 """ This method convertes git diff data to html color code
 """
 lines = ""
 openTag = "<span style='font-size: .80em; color: "
 openTagEnd = ";font-family: courier, arial, helvetica, sans-serif;'>"
 nbsp = '&nbsp;&nbsp;&nbsp;&nbsp;'
 for eachLine in diffData:
 if eachLine.startswith('-'):
 tabs = eachLine.count('\t')
 lines += "%s#ff0000%s%s%s</span><br>" % (openTag, openTagEnd, nbsp*tabs ,eachLine)
 elif eachLine.startswith('+'):
 tabs = eachLine.count('\t')
 lines += "%s#007900%s%s%s</span><br>" % (openTag, openTagEnd, nbsp*tabs, eachLine)
 else:
 tabs = eachLine.count('\t')
 lines += "%s#000000%s%s%s</span><br>" % (openTag, openTagEnd, nbsp*tabs, eachLine)
 return lines

can be enhanced by removing duplicated code :

def getHtml(diffData):
 """ This method convertes git diff data to html color code
 """
 lines = ""
 openTag = "<span style='font-size: .80em; color: "
 openTagEnd = ";font-family: courier, arial, helvetica, sans-serif;'>"
 nbsp = '&nbsp;&nbsp;&nbsp;&nbsp;'
 for line in diffData:
 if line.startswith('-'):
 value = '#ff0000'
 elif line.startswith('+'):
 value = '#007900'
 else:
 value = '#000000'
 tabs = line.count('\t')
 lines += "%s%s%s%s%s</span><br>" % (openTag, value, openTagEnd, nbsp*tabs ,line)
 return lines

Then, PEP 8 advices to use ''.join() instead of multiple concatenation which can become a bit expensive. Here's how you could do.

def getHtml(diffData):
 """ This method convertes git diff data to html color code
 """
 lines = []
 openTag = "<span style='font-size: .80em; color: "
 openTagEnd = ";font-family: courier, arial, helvetica, sans-serif;'>"
 nbsp = '&nbsp;&nbsp;&nbsp;&nbsp;'
 for line in diffData:
 if line.startswith('-'):
 value = '#ff0000'
 elif line.startswith('+'):
 value = '#007900'
 else:
 value = '#000000'
 tabs = line.count('\t')
 lines.append("%s%s%s%s%s</span><br>" % (openTag, value, openTagEnd, nbsp*tabs ,line))
 return ''.join(lines)

Now, if we want to go further, one can notice that the pattern l=[]. for x in X: l.append(f(x)) looks like it could be some kind of list comprehension/generator expression. Thus :

def getHtml(diffData):
 """ This method convertes git diff data to html color code
 """
 openTag = "<span style='font-size: .80em; color: "
 openTagEnd = ";font-family: courier, arial, helvetica, sans-serif;'>"
 nbsp = '&nbsp;&nbsp;&nbsp;&nbsp;'
 lines = []
 for line in diffData:
 value = '#ff0000' if line.startswith('-') else ('#007900' if line.startswith('+') else '#000000')
 tabs = line.count('\t')
 lines.append("%s%s%s%s%s</span><br>" % (openTag, value, openTagEnd, nbsp*tabs ,line))
 return ''.join(lines)

becomes (but it might be a bit too excessive):

def getHtml(diffData):
 """ This method convertes git diff data to html color code
 """
 openTag = "<span style='font-size: .80em; color: "
 openTagEnd = ";font-family: courier, arial, helvetica, sans-serif;'>"
 nbsp = '&nbsp;&nbsp;&nbsp;&nbsp;'
 return ''.join([("%s%s%s%s%s</span><br>" % (openTag, '#ff0000' if line.startswith('-') else ('#007900' if line.startswith('+') else '#000000'), openTagEnd, nbsp*line.count('\t') ,line)) for line in diffData])

You do not need to check if attachments: before doing for phile in attachments:.

That's pretty much all I have to say about the code itself for the time being.

Now for the functional side of it, I am not quite sure what was intended but you might want to have a look at git hooks as they would be a nice way to send the email automatically as you commit/push.

• Ciasto piekarz: Does git hook push allow's colour diff in email ?
• Ciasto piekarz: thanks for the review, I preferred not to use list comprehension though just to leave code more readable for anyone who looks although list comprehension are my personal favourites.
• David Harkness: If you pull the line-formatting out into a function, the list comprehension becomes much cleaner: return ''.join([format_color_html(line) for line in diffData])
• Gareth Rees: ''.join(map(format_color_html, diffData)) is a shorter way to compute the same result.
• Ciasto piekarz: @DavidHarkness : I think this way I can come up with whole module of colour and styling for HTML formatting !!! something like pygmentize is doing.

Answer 2

1. Introduction

I'm just going to comment on the part of your program (the first few lines of the main function) where you parse the command-line arguments. You'll see that there's plenty here for one answer.

2. Comments on your code

1. I don't like the specification of this program. It does two things: (i) push all commits on the current branch to the remote origin; (ii) e-mail the diff from the last commit (only) on the current branch to a specified e-mail address.

   The problem is that these two things do not match. You push all the commits but then e-mail the diff for the last commit only. This more or less ensures that at some point a user will be confused or misled because they had made multiple commits locally but when they ran your script they only saw the diff for the last commit they made.

   My proposal is that you separate these two pieces of functionality. Piece number (i) is so simple (just run git push, and if that fails, ask the user if they want to try git push -f) that it doesn't deserve a special script. So I would revise the script so that it just e-mail a diff.

2. I don't like the casual way you run git push -f. It says in the manual that the -f option "can cause the remote repository to lose commits; use it with care." Running it if the user presses return in response to the prompt "Enter yes to force Push else any key to cancel" does not seem like an appropriate level of care.

3. In Unix, users expect options that start with a single hyphen to be single characters, and for options without arguments to be combinable. In particular, -ver would normally mean the same thing as -v -e -r. So I would recommend replacing -ver with -V and getting rid of the -to option (you can make this a positional argument, as in the sendmail command).

4. There's no need to write "in inverted commas" in the help text. It's reasonable to assume that the user knows how to use the shell.

5. There's no need to pass type=str or required=False to add_argument: these are the defaults.

6. If you had kept your code to 79 columns (as recommended by the Python style guide, PEP8) then we could read your code here without having to scroll it horizontally.

7. It's conventional to use CamelCase for classes, so I would name the exception class InvalidEmail.

8. There's no need to call vars(args): instead of optArgs['to'] you could just write args.to and so on.

9. By specifying a default value for the --diff argument, you can avoid the need to check whether this argument was supplied when assembling the diffCmd.

10. The default e-mail address should come from the environment, not be hard-coded into the script.

11. The validation of the e-mail address is too restrictive: many e-mail addresses will be rejected. It's good practive just to check for the presence of an @ (to avoid typos) and let your mail server reject bad addresses.

12. Since you are using re.match, which only matches at the start of the string, there's no need to anchor the pattern with ^.

13. It's not a good idea to raise exceptions for user errors (reserve them for programming errors). For user errors, it's best to print a usage message instead. This is most easily done by passing a function to the type= parameter for the e-mail address argument.

14. For testing purposes, it's a good idea for main to take one parameter argv, and to pass this to parser.parse_args. This allows you to test the program from the interactive interpreter. In the __name__ == '__main__' clause, write main(sys.argv).

15. You write sys.exit(main()) but the function main doesn't return an exit code.

3. Revised code

import argparse
import os
import re
import sys
def validate_address(address):
 """If address looks like a valid e-mail address, return it. Otherwise
 raise ArgumentTypeError.
 """
 if re.match('[^@]+@[^@]+$', address):
 return address
 raise argparse.ArgumentTypeError('Invalid e-mail address: {}'.format(address))
def main(argv=()):
 parser = argparse.ArgumentParser(prog='git-mail-diff',
 description='E-mail a git diff')
 parser.add_argument(dest='address', nargs='?',
 type=validate_address,
 default=os.getenv('myemail'),
 help='E-mail address to send the diff to')
 parser.add_argument('-d', '--diff',
 default='HEAD^ HEAD',
 help='arguments to pass to git diff '
 '(default: HEAD^ HEAD)')
 parser.add_argument('-c', '--compose',
 help='message to be sent along with the diff')
 parser.add_argument('-v', '--verbose', action='store_true',
 help='print commands and responses to standard output')
 parser.add_argument('-V', '--version', action='version',
 version='%(prog)s 1.0')
 args = parser.parse_args(argv)
 # ... etc ...
if __name__ == '__main__':
 main(sys.argv)

• Ciasto piekarz: no it pushes the last commit only which is why I have a check for if git diff HEAD^ HEAD then do pushBranch() and email the same only. But if the diff is of something else it will just diff and display in verbose mode and email but not push.
• Ciasto piekarz: also your regex for validating email address is wrong
• Gareth Rees: (1) Suppose that a user has multiple local commits that have not yet been pushed to the remote repository, and they run your script (without passing any options). The check on diffCmd passes, so you call pushBranch, which runs git push, which pushes all commits. But then you e-mail only the output of git diff HEAD^ HEAD (that is, the last commit only). (2) See my point #11.
• Ciasto piekarz: for that there is a check if any([re.search(word, modifiedData) for word in ['modified', 'untracked']]):print "You have uncommited changes, Commit and try again" return
• Gareth Rees: I'm not talking about uncommitted changes. I'm talking about committed changes that have not been pushed.