8
\$\begingroup\$

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 = "removed@myemail.com"

    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://sanfx@bitbucket.org/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)
\$\endgroup\$
  • \$\begingroup\$ OOps I forgot to run pylint before posting, I'll do that next. \$\endgroup\$ – Ciasto piekarz Jan 26 '14 at 11:42
  • 2
    \$\begingroup\$ What's the point of checking for the valid email address? You exclude a fair amount of valid ones. \$\endgroup\$ – ChrisWue Jan 26 '14 at 11:50
  • \$\begingroup\$ @ChrisWue: Like what ? \$\endgroup\$ – Ciasto piekarz Jan 28 '14 at 16:52
  • \$\begingroup\$ 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. \$\endgroup\$ – ChrisWue Jan 28 '14 at 18:53
  • \$\begingroup\$ Don't forget address extensions! (user+service@example.com) \$\endgroup\$ – nyuszika7h Jul 21 '14 at 22:07
10
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Does git hook push allow's colour diff in email ? \$\endgroup\$ – Ciasto piekarz Jan 26 '14 at 11:39
  • \$\begingroup\$ 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. \$\endgroup\$ – Ciasto piekarz Jan 26 '14 at 16:11
  • 1
    \$\begingroup\$ 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]) \$\endgroup\$ – David Harkness Jan 26 '14 at 19:12
  • 2
    \$\begingroup\$ ''.join(map(format_color_html, diffData)) is a shorter way to compute the same result. \$\endgroup\$ – Gareth Rees Jan 27 '14 at 18:20
  • 1
    \$\begingroup\$ @DavidHarkness : I think this way I can come up with whole module of colour and styling for HTML formatting !!! something like pygmentize is doing. \$\endgroup\$ – Ciasto piekarz Jan 28 '14 at 1:58
8
\$\begingroup\$

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)
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Ciasto piekarz Jan 28 '14 at 16:33
  • \$\begingroup\$ also your regex for validating email address is wrong \$\endgroup\$ – Ciasto piekarz Jan 28 '14 at 16:45
  • \$\begingroup\$ (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. \$\endgroup\$ – Gareth Rees Jan 28 '14 at 16:55
  • \$\begingroup\$ 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 \$\endgroup\$ – Ciasto piekarz Jan 28 '14 at 17:00
  • \$\begingroup\$ I'm not talking about uncommitted changes. I'm talking about committed changes that have not been pushed. \$\endgroup\$ – Gareth Rees Jan 28 '14 at 17:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.