From: Tor Arvid Lund Date: Thu, 14 Aug 2008 17:00:12 GMT Subject: Re: [PATCH 00/12] Allow users to access Perforce repositories requiring authentication Message-ID: <1a6be5fa0808141000w78b54bc2x1262b814f36ecdd6@mail.gmail.com> In-Reply-To: <1218392795-4084-1-git-send-email-wildfire@progsoc.org> Hi, On Sun, Aug 10, 2008 at 8:26 PM, Anand Kumria wrote: > Hi, > > This series of patches refactors a few function calls into git-p4 > so that they all go via the same function to build up the command > line. > > It also then allows users to specify any particular user / password > required to access their Perforce repository (plus a few other parameters). > > I have specifically tested this agains the public Zimbra repository and > it works for me fine. Any feedback would be welcomed. I had an issue when using git p4 submit, and I had newly added files in the changelist. This, I think, is caused by a call to "p4 opened" that was not converted to use your new p4_build_cmd function. So I don't think your patches should be merged to master before this is fixed. However, when applying the simple diff below, this works for me. Otherwise, I think your patch series were very nice, since I often use different perforce servers. Btw, my diff works correctly, I think, but it is probably nicer to wrap read_pipe inside a p4_read_pipe - similar to what you did with p4_read_pipe_lines. All in all: good work! :) -Tor Arvid- diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4 index 6c64224..8705ec9 100755 --- a/contrib/fast-import/git-p4 +++ b/contrib/fast-import/git-p4 @@ -134,7 +134,8 @@ def setP4ExecBit(file, mode): def getP4OpenedType(file): # Returns the perforce file type for the given file. - result = read_pipe("p4 opened %s" % file) + real_cmd = p4_build_cmd("opened %s" % file) + result = read_pipe(real_cmd) match = re.match(".*\((.+)\)\r?$", result) if match: return match.group(1)