From: Scott Lamb <slamb@slamb.org>
Date: Thu, 31 May 2007 16:47:51 GMT
Subject: git-p4import.py robustness changes
Message-ID: <4ACE2ABC-8D73-4097-87AC-F3B27EDA97DE@slamb.org>

I'm trying out git-p4import.py (and git itself) for the first time.  
I'm frustrated with its error behavior. For example, it's saying this:

     $ git-p4import.py //my/path/... master
     Setting perforce to  //my/path/...
     Already up to date...

when it should be saying this:

     $ git-p4import.py //my/path/... master
     Setting perforce to  //my/path/...
     git-p4import fatal error: p4 changes //my/path/...@1,#head:  
Request too large (over 150000); see 'p4 help maxresults'.

There's a logfile option, but that's a poor excuse for no error  
handling. I'd like to fix it. A couple questions, though:


First, is it acceptable to switch from os.popen to the subprocess  
module? I ask because the latter was only introduced with Python 2.4  
on. The subprocess module does work with earlier versions of Python  
(definitely 2.3) and is GPL-compatible, so maybe it could be thrown  
into the distribution if desired.

I could make do with popen2.Popen3, but subprocess is actually  
pleasant to use:

         git = subprocess.Popen(cmdlist,
                                stdin=subprocess.PIPE,
                                stdout=subprocess.PIPE,
                                stderr=subprocess.PIPE)
         stdout, stderr = git.communicate(stdin)
         if git.wait() != 0:
             raise GitException("'git %s' failed: %s" % (cmd, stderr))

vs. the popen2 way, which is longer and uglier. It'd probably involve  
tempfiles rather than reimplementing subprocess.Popen.communicate().


Second, this crowd seems to want sequences of tiny patches. How does  
this sound?

* patch 1 - use subprocess to make git_command.git() and p4_command.p4 
() throw properly-typed exceptions on error, fix caller exception  
handling to match.

* patch 2 - remove the use of the shell and pipelines (fix some  
escaping problems).

* patch 3 - use lists instead of space separation for the commandline  
arguments (fix more escaping problems).

* patch 4 - allow grabbing partial history (make my error go away).


Cheers,
Scott

-- 
Scott Lamb <http://www.slamb.org/>