* [PATCH/RFCv1] git-p4: handle files with shell metacharacters
@ 2011-09-26 21:29 Luke Diamand
2011-09-26 21:29 ` Luke Diamand
0 siblings, 1 reply; 8+ messages in thread
From: Luke Diamand @ 2011-09-26 21:29 UTC (permalink / raw)
To: git; +Cc: pw, vitor.hda, Luke Diamand
git-p4 uses the shell to execute perforce and git. This leads to problems
where files contain shell metacharacters or spaces. I first hit this
when someone checked in files with dollars ($) in their name, but in theory
you could cause complete havoc with other characters: backticks in a
filename would be especially entertaining.
Make git-p4 use subprocess.Popen() and subprocess.call() instead, and
pass in argv[] style arrays instead, at least for cases where filenames
are involved. Add test cases.
Notes:
This patch is based on Pete Wyckoff's recent patch series for refactoring
the git-p4 test harness, so it won't apply to the current next or master
branches.
I tried testing it on Cygwin as well, but the test harness appears to be
very broken on that platform as it is unable to start p4d.
Luke Diamand (1):
git-p4: handle files with shell metacharacters
contrib/fast-import/git-p4 | 174 +++++++++++++++++++++++++---------------
t/t9800-git-p4.sh | 2 +-
t/t9803-git-shell-metachars.sh | 70 ++++++++++++++++
3 files changed, 179 insertions(+), 67 deletions(-)
create mode 100755 t/t9803-git-shell-metachars.sh
--
1.7.6.347.g4db0d
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH/RFCv1] git-p4: handle files with shell metacharacters
2011-09-26 21:29 [PATCH/RFCv1] git-p4: handle files with shell metacharacters Luke Diamand
@ 2011-09-26 21:29 ` Luke Diamand
2011-09-26 21:47 ` Pete Wyckoff
0 siblings, 1 reply; 8+ messages in thread
From: Luke Diamand @ 2011-09-26 21:29 UTC (permalink / raw)
To: git; +Cc: pw, vitor.hda, Luke Diamand
git-p4 used to simply pass strings into system() and popen(), and
relied on the shell doing the necessary expansion. This though meant
that shell metacharacters in file names would be corrupted - for
example files with $ or space in them.
Switch to using subprocess.Popen() and friends, and pass in explicit
arrays in the places where it matters. This then avoids needing shell
expansion.
Add trivial helper functions for some common perforce operations. Add
test case.
Signed-off-by: Luke Diamand <luke@diamand.org>
---
contrib/fast-import/git-p4 | 174 +++++++++++++++++++++++++---------------
t/t9800-git-p4.sh | 2 +-
t/t9803-git-shell-metachars.sh | 70 ++++++++++++++++
3 files changed, 179 insertions(+), 67 deletions(-)
create mode 100755 t/t9803-git-shell-metachars.sh
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 782b891..be67d30 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -14,7 +14,6 @@ import re
verbose = False
-
def p4_build_cmd(cmd):
"""Build a suitable p4 command line.
@@ -22,36 +21,40 @@ def p4_build_cmd(cmd):
location. It means that hooking into the environment, or other configuration
can be done more easily.
"""
- real_cmd = "%s " % "p4"
+ real_cmd = ["p4"]
user = gitConfig("git-p4.user")
if len(user) > 0:
- real_cmd += "-u %s " % user
+ real_cmd += ["-u",user]
password = gitConfig("git-p4.password")
if len(password) > 0:
- real_cmd += "-P %s " % password
+ real_cmd += ["-P", password]
port = gitConfig("git-p4.port")
if len(port) > 0:
- real_cmd += "-p %s " % port
+ real_cmd += ["-p", port]
host = gitConfig("git-p4.host")
if len(host) > 0:
- real_cmd += "-h %s " % host
+ real_cmd += ["-h", host]
client = gitConfig("git-p4.client")
if len(client) > 0:
- real_cmd += "-c %s " % client
+ real_cmd += ["-c", client]
+
- real_cmd += "%s" % (cmd)
+ if isinstance(cmd,basestring):
+ real_cmd = ' '.join(real_cmd) + ' ' + cmd
+ else:
+ real_cmd += cmd
if verbose:
- print real_cmd
+ print ' '.join(real_cmd)
return real_cmd
def chdir(dir):
- if os.name == 'nt':
- os.environ['PWD']=dir
+ # P4 uses the PWD environment variable rather than getcwd(), it would appear!
+ os.environ['PWD']=dir
os.chdir(dir)
def die(msg):
@@ -65,9 +68,12 @@ def write_pipe(c, str):
if verbose:
sys.stderr.write('Writing pipe: %s\n' % c)
- pipe = os.popen(c, 'w')
+ p = subprocess.Popen(c, shell=isinstance(c,basestring),
+ stdin=subprocess.PIPE)
+ pipe = p.stdin
val = pipe.write(str)
- if pipe.close():
+ pipe.close();
+ if p.wait():
die('Command failed: %s' % c)
return val
@@ -78,11 +84,13 @@ def p4_write_pipe(c, str):
def read_pipe(c, ignore_error=False):
if verbose:
- sys.stderr.write('Reading pipe: %s\n' % c)
+ sys.stderr.write('Reading pipe: %s\n' % str(c))
- pipe = os.popen(c, 'rb')
+ expand = isinstance(c,basestring)
+ p = subprocess.Popen(c, stdout=subprocess.PIPE, shell=expand)
+ pipe = p.stdout
val = pipe.read()
- if pipe.close() and not ignore_error:
+ if p.wait() and not ignore_error:
die('Command failed: %s' % c)
return val
@@ -93,11 +101,13 @@ def p4_read_pipe(c, ignore_error=False):
def read_pipe_lines(c):
if verbose:
- sys.stderr.write('Reading pipe: %s\n' % c)
- ## todo: check return status
- pipe = os.popen(c, 'rb')
+ sys.stderr.write('Reading pipe: %s\n' % str(c))
+
+ expand = isinstance(c, basestring)
+ p = subprocess.Popen(c, shell=expand, stdout=subprocess.PIPE)
+ pipe = p.stdout
val = pipe.readlines()
- if pipe.close():
+ if pipe.close() or p.wait():
die('Command failed: %s' % c)
return val
@@ -108,15 +118,37 @@ def p4_read_pipe_lines(c):
return read_pipe_lines(real_cmd)
def system(cmd):
+ expand = isinstance(cmd,basestring)
if verbose:
- sys.stderr.write("executing %s\n" % cmd)
- if os.system(cmd) != 0:
- die("command failed: %s" % cmd)
+ sys.stderr.write("executing %s\n" % str(cmd))
+ subprocess.check_call(cmd, shell=expand)
def p4_system(cmd):
"""Specifically invoke p4 as the system command. """
real_cmd = p4_build_cmd(cmd)
- return system(real_cmd)
+ expand = isinstance(real_cmd, basestring)
+ subprocess.check_call(real_cmd, shell=expand)
+
+def p4_integrate(src, dest):
+ p4_system(["integrate", "-Dt", src, dest])
+
+def p4_sync(path):
+ p4_system(["sync", path])
+
+def p4_add(f):
+ p4_system(["add", f])
+
+def p4_delete(f):
+ p4_system(["delete", f])
+
+def p4_edit(f):
+ p4_system(["edit", f])
+
+def p4_revert(f):
+ p4_system(["revert", f])
+
+def p4_reopen(type, file):
+ p4_system(["reopen", "-t", type, file])
#
# Canonicalize the p4 type and return a tuple of the
@@ -167,12 +199,12 @@ def setP4ExecBit(file, mode):
if p4Type[-1] == "+":
p4Type = p4Type[0:-1]
- p4_system("reopen -t %s %s" % (p4Type, file))
+ p4_reopen(p4Type, file)
def getP4OpenedType(file):
# Returns the perforce file type for the given file.
- result = p4_read_pipe("opened %s" % file)
+ result = p4_read_pipe(["opened", file])
match = re.match(".*\((.+)\)\r?$", result)
if match:
return match.group(1)
@@ -228,9 +260,17 @@ def isModeExecChanged(src_mode, dst_mode):
return isModeExec(src_mode) != isModeExec(dst_mode)
def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):
- cmd = p4_build_cmd("-G %s" % (cmd))
+
+ if isinstance(cmd,basestring):
+ cmd = "-G " + cmd
+ expand = True
+ else:
+ cmd = ["-G"] + cmd
+ expand = False
+
+ cmd = p4_build_cmd(cmd)
if verbose:
- sys.stderr.write("Opening pipe: %s\n" % cmd)
+ sys.stderr.write("Opening pipe: %s\n" % str(cmd))
# Use a temporary file to avoid deadlocks without
# subprocess.communicate(), which would put another copy
@@ -242,7 +282,9 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):
stdin_file.flush()
stdin_file.seek(0)
- p4 = subprocess.Popen(cmd, shell=True,
+ print cmd
+ p4 = subprocess.Popen(cmd,
+ shell=expand,
stdin=stdin_file,
stdout=subprocess.PIPE)
@@ -275,7 +317,7 @@ def p4Where(depotPath):
if not depotPath.endswith("/"):
depotPath += "/"
depotPath = depotPath + "..."
- outputList = p4CmdList("where %s" % depotPath)
+ outputList = p4CmdList(["where", depotPath])
output = None
for entry in outputList:
if "depotFile" in entry:
@@ -477,8 +519,10 @@ def originP4BranchesExist():
def p4ChangesForPaths(depotPaths, changeRange):
assert depotPaths
- output = p4_read_pipe_lines("changes " + ' '.join (["%s...%s" % (p, changeRange)
- for p in depotPaths]))
+ cmd = ['changes']
+ for p in depotPaths:
+ cmd += ["%s...%s" % (p, changeRange)]
+ output = p4_read_pipe_lines(cmd)
changes = {}
for line in output:
@@ -561,7 +605,7 @@ class P4Debug(Command):
def run(self, args):
j = 0
- for output in p4CmdList(" ".join(args)):
+ for output in p4CmdList(args):
print 'Element: %d' % j
j += 1
print output
@@ -715,7 +759,7 @@ class P4Submit(Command, P4UserMap):
break
if not client:
die("could not get client spec")
- results = p4CmdList("changes -c %s -m 1" % client)
+ results = p4CmdList(["changes", "-c", client, "-m", "1"])
for r in results:
if r.has_key('change'):
return r['change']
@@ -778,7 +822,7 @@ class P4Submit(Command, P4UserMap):
# remove lines in the Files section that show changes to files outside the depot path we're committing into
template = ""
inFilesSection = False
- for line in p4_read_pipe_lines("change -o"):
+ for line in p4_read_pipe_lines(['change', '-o']):
if line.endswith("\r\n"):
line = line[:-2] + "\n"
if inFilesSection:
@@ -835,7 +879,7 @@ class P4Submit(Command, P4UserMap):
modifier = diff['status']
path = diff['src']
if modifier == "M":
- p4_system("edit \"%s\"" % path)
+ p4_edit(path)
if isModeExecChanged(diff['src_mode'], diff['dst_mode']):
filesToChangeExecBit[path] = diff['dst_mode']
editedFiles.add(path)
@@ -850,21 +894,21 @@ class P4Submit(Command, P4UserMap):
filesToAdd.remove(path)
elif modifier == "C":
src, dest = diff['src'], diff['dst']
- p4_system("integrate -Dt \"%s\" \"%s\"" % (src, dest))
+ p4_integrate(src, dest)
if diff['src_sha1'] != diff['dst_sha1']:
- p4_system("edit \"%s\"" % (dest))
+ p4_edit(dest)
if isModeExecChanged(diff['src_mode'], diff['dst_mode']):
- p4_system("edit \"%s\"" % (dest))
+ p4_edit(dest)
filesToChangeExecBit[dest] = diff['dst_mode']
os.unlink(dest)
editedFiles.add(dest)
elif modifier == "R":
src, dest = diff['src'], diff['dst']
- p4_system("integrate -Dt \"%s\" \"%s\"" % (src, dest))
+ p4_integrate(src, dest)
if diff['src_sha1'] != diff['dst_sha1']:
- p4_system("edit \"%s\"" % (dest))
+ p4_edit(dest)
if isModeExecChanged(diff['src_mode'], diff['dst_mode']):
- p4_system("edit \"%s\"" % (dest))
+ p4_edit(dest)
filesToChangeExecBit[dest] = diff['dst_mode']
os.unlink(dest)
editedFiles.add(dest)
@@ -887,9 +931,9 @@ class P4Submit(Command, P4UserMap):
if response == "s":
print "Skipping! Good luck with the next patches..."
for f in editedFiles:
- p4_system("revert \"%s\"" % f);
+ p4_revert(f)
for f in filesToAdd:
- system("rm %s" %f)
+ os.remove(f)
return
elif response == "a":
os.system(applyPatchCmd)
@@ -910,10 +954,10 @@ class P4Submit(Command, P4UserMap):
system(applyPatchCmd)
for f in filesToAdd:
- p4_system("add \"%s\"" % f)
+ p4_add(f)
for f in filesToDelete:
- p4_system("revert \"%s\"" % f)
- p4_system("delete \"%s\"" % f)
+ p4_revert(f)
+ p4_delete(f)
# Set/clear executable bits
for f in filesToChangeExecBit.keys():
@@ -935,7 +979,7 @@ class P4Submit(Command, P4UserMap):
del(os.environ["P4DIFF"])
diff = ""
for editedFile in editedFiles:
- diff += p4_read_pipe("diff -du %r" % editedFile)
+ diff += p4_read_pipe(['diff', '-du', editedFile])
newdiff = ""
for newFile in filesToAdd:
@@ -987,7 +1031,7 @@ class P4Submit(Command, P4UserMap):
submitTemplate = message[:message.index(separatorLine)]
if self.isWindows:
submitTemplate = submitTemplate.replace("\r\n", "\n")
- p4_write_pipe("submit -i", submitTemplate)
+ p4_write_pipe(['submit', '-i'], submitTemplate)
if self.preserveUser:
if p4User:
@@ -998,10 +1042,10 @@ class P4Submit(Command, P4UserMap):
else:
for f in editedFiles:
- p4_system("revert \"%s\"" % f);
+ p4_revert(f)
for f in filesToAdd:
- p4_system("revert \"%s\"" % f);
- system("rm %s" %f)
+ p4_revert(f)
+ os.remove(f)
os.remove(fileName)
else:
@@ -1054,8 +1098,7 @@ class P4Submit(Command, P4UserMap):
chdir(self.clientPath)
print "Synchronizing p4 checkout..."
- p4_system("sync ...")
-
+ p4_sync("...")
self.check()
commits = []
@@ -1269,7 +1312,7 @@ class P4Sync(Command, P4UserMap):
# operations. utf16 is converted to ascii or utf8, perhaps.
# But ascii text saved as -t utf16 is completely mangled.
# Invoke print -o to get the real contents.
- text = p4_read_pipe('print -q -o - "%s"' % file['depotFile'])
+ text = p4_read_pipe(['print', '-q', '-o', '-', file['depotFile']])
contents = [ text ]
# Perhaps windows wants unicode, utf16 newlines translated too;
@@ -1365,9 +1408,9 @@ class P4Sync(Command, P4UserMap):
def streamP4FilesCbSelf(entry):
self.streamP4FilesCb(entry)
- p4CmdList("-x - print",
- '\n'.join(['%s#%s' % (f['path'], f['rev'])
- for f in filesToRead]),
+ fileArgs = ['%s#%s' % (f['path'], f['rev']) for f in filesToRead]
+
+ p4CmdList(["print"] + fileArgs,
cb=streamP4FilesCbSelf)
# do the last chunk
@@ -1429,7 +1472,7 @@ class P4Sync(Command, P4UserMap):
if self.verbose:
print "Change %s is labelled %s" % (change, labelDetails)
- files = p4CmdList("files " + ' '.join (["%s...@%s" % (p, change)
+ files = p4CmdList(["files"] + (["%s...@%s" % (p, change)
for p in branchPrefixes]))
if len(files) == len(labelRevisions):
@@ -1478,9 +1521,9 @@ class P4Sync(Command, P4UserMap):
newestChange = 0
if self.verbose:
print "Querying files for label %s" % label
- for file in p4CmdList("files "
- + ' '.join (["%s...@%s" % (p, label)
- for p in self.depotPaths])):
+ for file in p4CmdList(["files"] +
+ (["%s...@%s" % (p, label)
+ for p in self.depotPaths])):
revisions[file["depotFile"]] = file["rev"]
change = int(file["change"])
if change > newestChange:
@@ -1735,10 +1778,9 @@ class P4Sync(Command, P4UserMap):
newestRevision = 0
fileCnt = 0
- for info in p4CmdList("files "
- + ' '.join(["%s...%s"
- % (p, revision)
- for p in self.depotPaths])):
+ fileArgs = ["%s...%s" % (p,revision) for p in self.depotPaths]
+
+ for info in p4CmdList(["files"] + fileArgs):
if 'code' in info and info['code'] == 'error':
sys.stderr.write("p4 returned an error: %s\n"
diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
index bb89b63..12f374f 100755
--- a/t/t9800-git-p4.sh
+++ b/t/t9800-git-p4.sh
@@ -150,7 +150,7 @@ test_expect_success 'preserve users' '
git commit --author "Alice <alice@localhost>" -m "a change by alice" file1 &&
git commit --author "Bob <bob@localhost>" -m "a change by bob" file2 &&
git config git-p4.skipSubmitEditCheck true &&
- P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit --preserve-user &&
+ P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit --verbose --preserve-user &&
p4_check_commit_author file1 alice &&
p4_check_commit_author file2 bob
'
diff --git a/t/t9803-git-shell-metachars.sh b/t/t9803-git-shell-metachars.sh
new file mode 100755
index 0000000..1143491
--- /dev/null
+++ b/t/t9803-git-shell-metachars.sh
@@ -0,0 +1,70 @@
+#!/bin/sh
+
+test_description='git-p4 transparency to shell metachars in filenames'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+ kill_p4d || : &&
+ start_p4d &&
+ cd "$TRASH_DIRECTORY"
+'
+
+test_expect_success 'init depot' '
+ (
+ cd "$cli" &&
+ echo file1 >file1 &&
+ p4 add file1 &&
+ p4 submit -d "file1"
+ )
+'
+
+test_expect_success 'shell metachars in filenames' '
+ "$GITP4" clone --dest="$git" //depot &&
+ test_when_finished cleanup_git &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEditCheck true &&
+ echo f1 >foo\$bar &&
+ git add foo\$bar &&
+ echo f2 >"file with spaces" &&
+ git add "file with spaces" &&
+ P4EDITOR=touch git commit -m "add files" &&
+ "$GITP4" submit --verbose &&
+ cd "$cli" &&
+ p4 sync ... &&
+ ls -l "file with spaces" &&
+ ls -l "foo\$bar"
+ )
+'
+
+check_missing() {
+ for i in $*; do
+ if [ -f $i ]; then
+ echo $i found but should be missing 1>&2
+ exit 1
+ fi
+ done
+}
+
+test_expect_success 'deleting with shell metachars' '
+ "$GITP4" clone --dest="$git" //depot &&
+ test_when_finished cleanup_git &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEditCheck true &&
+ git rm foo\$bar &&
+ git rm file\ with\ spaces &&
+ P4EDITOR=touch git commit -m "remove files" &&
+ "$GITP4" submit --verbose
+ cd "$cli" &&
+ p4 sync ... &&
+ check_missing "file with spaces" foo\$bar
+ )
+'
+
+test_expect_success 'kill p4d' '
+ kill_p4d
+'
+
+test_done
--
1.7.6.347.g4db0d
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH/RFCv1] git-p4: handle files with shell metacharacters
2011-09-26 21:29 ` Luke Diamand
@ 2011-09-26 21:47 ` Pete Wyckoff
2011-09-27 8:40 ` [RFC/PATCHv2] " Luke Diamand
0 siblings, 1 reply; 8+ messages in thread
From: Pete Wyckoff @ 2011-09-26 21:47 UTC (permalink / raw)
To: Luke Diamand; +Cc: git, vitor.hda
luke@diamand.org wrote on Mon, 26 Sep 2011 22:29 +0100:
> git-p4 used to simply pass strings into system() and popen(), and
> relied on the shell doing the necessary expansion. This though meant
> that shell metacharacters in file names would be corrupted - for
> example files with $ or space in them.
>
> Switch to using subprocess.Popen() and friends, and pass in explicit
> arrays in the places where it matters. This then avoids needing shell
> expansion.
>
> Add trivial helper functions for some common perforce operations. Add
> test case.
This is great. Some code comments below. Could you resend an
RFCv2? I'll fixup your test case along with mine later, and send
you the patch just for that.
-- Pete
> Signed-off-by: Luke Diamand <luke@diamand.org>
> ---
> contrib/fast-import/git-p4 | 174 +++++++++++++++++++++++++---------------
> t/t9800-git-p4.sh | 2 +-
> t/t9803-git-shell-metachars.sh | 70 ++++++++++++++++
> 3 files changed, 179 insertions(+), 67 deletions(-)
> create mode 100755 t/t9803-git-shell-metachars.sh
>
> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
> index 782b891..be67d30 100755
> --- a/contrib/fast-import/git-p4
> +++ b/contrib/fast-import/git-p4
> @@ -14,7 +14,6 @@ import re
>
> verbose = False
>
> -
> def p4_build_cmd(cmd):
> """Build a suitable p4 command line.
>
> @@ -22,36 +21,40 @@ def p4_build_cmd(cmd):
> location. It means that hooking into the environment, or other configuration
> can be done more easily.
> """
> - real_cmd = "%s " % "p4"
> + real_cmd = ["p4"]
>
> user = gitConfig("git-p4.user")
> if len(user) > 0:
> - real_cmd += "-u %s " % user
> + real_cmd += ["-u",user]
>
> password = gitConfig("git-p4.password")
> if len(password) > 0:
> - real_cmd += "-P %s " % password
> + real_cmd += ["-P", password]
>
> port = gitConfig("git-p4.port")
> if len(port) > 0:
> - real_cmd += "-p %s " % port
> + real_cmd += ["-p", port]
>
> host = gitConfig("git-p4.host")
> if len(host) > 0:
> - real_cmd += "-h %s " % host
> + real_cmd += ["-h", host]
>
> client = gitConfig("git-p4.client")
> if len(client) > 0:
> - real_cmd += "-c %s " % client
> + real_cmd += ["-c", client]
> +
>
> - real_cmd += "%s" % (cmd)
> + if isinstance(cmd,basestring):
> + real_cmd = ' '.join(real_cmd) + ' ' + cmd
> + else:
> + real_cmd += cmd
> if verbose:
> - print real_cmd
> + print ' '.join(real_cmd)
> return real_cmd
Okay, so it returns a string if given a string, only. Would be
nice if we could kill off the string usages eventually?
The verbose print ' '.join(real_cmd) will look quite funny if
it real_cmd is a string.
> def chdir(dir):
> - if os.name == 'nt':
> - os.environ['PWD']=dir
> + # P4 uses the PWD environment variable rather than getcwd(), it would appear!
> + os.environ['PWD']=dir
> os.chdir(dir)
Interesting. Separate unrelated fix? Both "nt" and other OSes?
> def die(msg):
> @@ -65,9 +68,12 @@ def write_pipe(c, str):
> if verbose:
> sys.stderr.write('Writing pipe: %s\n' % c)
Needs str(c) like in read_pipe()?
> - pipe = os.popen(c, 'w')
> + p = subprocess.Popen(c, shell=isinstance(c,basestring),
> + stdin=subprocess.PIPE)
> + pipe = p.stdin
> val = pipe.write(str)
> - if pipe.close():
> + pipe.close();
Stray semicolon.
> + if p.wait():
> die('Command failed: %s' % c)
> return val
> @@ -78,11 +84,13 @@ def p4_write_pipe(c, str):
>
> def read_pipe(c, ignore_error=False):
> if verbose:
> - sys.stderr.write('Reading pipe: %s\n' % c)
> + sys.stderr.write('Reading pipe: %s\n' % str(c))
>
> - pipe = os.popen(c, 'rb')
> + expand = isinstance(c,basestring)
> + p = subprocess.Popen(c, stdout=subprocess.PIPE, shell=expand)
> + pipe = p.stdout
> val = pipe.read()
> - if pipe.close() and not ignore_error:
> + if p.wait() and not ignore_error:
> die('Command failed: %s' % c)
Can this be made more symmetric with read_pipe()? "expand",
order of args.
> return val
> @@ -93,11 +101,13 @@ def p4_read_pipe(c, ignore_error=False):
>
> def read_pipe_lines(c):
> if verbose:
> - sys.stderr.write('Reading pipe: %s\n' % c)
> - ## todo: check return status
> - pipe = os.popen(c, 'rb')
> + sys.stderr.write('Reading pipe: %s\n' % str(c))
> +
> + expand = isinstance(c, basestring)
> + p = subprocess.Popen(c, shell=expand, stdout=subprocess.PIPE)
> + pipe = p.stdout
> val = pipe.readlines()
> - if pipe.close():
> + if pipe.close() or p.wait():
> die('Command failed: %s' % c)
>
> return val
> @@ -108,15 +118,37 @@ def p4_read_pipe_lines(c):
> return read_pipe_lines(real_cmd)
>
> def system(cmd):
> + expand = isinstance(cmd,basestring)
> if verbose:
> - sys.stderr.write("executing %s\n" % cmd)
> - if os.system(cmd) != 0:
> - die("command failed: %s" % cmd)
> + sys.stderr.write("executing %s\n" % str(cmd))
> + subprocess.check_call(cmd, shell=expand)
>
> def p4_system(cmd):
> """Specifically invoke p4 as the system command. """
> real_cmd = p4_build_cmd(cmd)
> - return system(real_cmd)
> + expand = isinstance(real_cmd, basestring)
> + subprocess.check_call(real_cmd, shell=expand)
> +
> +def p4_integrate(src, dest):
> + p4_system(["integrate", "-Dt", src, dest])
> +
> +def p4_sync(path):
> + p4_system(["sync", path])
> +
> +def p4_add(f):
> + p4_system(["add", f])
> +
> +def p4_delete(f):
> + p4_system(["delete", f])
> +
> +def p4_edit(f):
> + p4_system(["edit", f])
> +
> +def p4_revert(f):
> + p4_system(["revert", f])
> +
> +def p4_reopen(type, file):
> + p4_system(["reopen", "-t", type, file])
These look cleaner. We almost need a class for this
p4 interface stuff. Maybe a job for a later refactoring.
> @@ -1365,9 +1408,9 @@ class P4Sync(Command, P4UserMap):
> def streamP4FilesCbSelf(entry):
> self.streamP4FilesCb(entry)
>
> - p4CmdList("-x - print",
> - '\n'.join(['%s#%s' % (f['path'], f['rev'])
> - for f in filesToRead]),
> + fileArgs = ['%s#%s' % (f['path'], f['rev']) for f in filesToRead]
> +
> + p4CmdList(["print"] + fileArgs,
> cb=streamP4FilesCbSelf)
I think this used "-x -" because when there are too many
fileArgs, the kernel command line couldn't hold them all. So
this trick feeds them on stdin instead.
> @@ -1429,7 +1472,7 @@ class P4Sync(Command, P4UserMap):
> if self.verbose:
> print "Change %s is labelled %s" % (change, labelDetails)
>
> - files = p4CmdList("files " + ' '.join (["%s...@%s" % (p, change)
> + files = p4CmdList(["files"] + (["%s...@%s" % (p, change)
> for p in branchPrefixes]))
Spurious parens around the list comprehension; confusing.
> @@ -1478,9 +1521,9 @@ class P4Sync(Command, P4UserMap):
> newestChange = 0
> if self.verbose:
> print "Querying files for label %s" % label
> - for file in p4CmdList("files "
> - + ' '.join (["%s...@%s" % (p, label)
> - for p in self.depotPaths])):
> + for file in p4CmdList(["files"] +
> + (["%s...@%s" % (p, label)
> + for p in self.depotPaths])):
Here too.
> diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
> index bb89b63..12f374f 100755
> --- a/t/t9800-git-p4.sh
> +++ b/t/t9800-git-p4.sh
> @@ -150,7 +150,7 @@ test_expect_success 'preserve users' '
> git commit --author "Alice <alice@localhost>" -m "a change by alice" file1 &&
> git commit --author "Bob <bob@localhost>" -m "a change by bob" file2 &&
> git config git-p4.skipSubmitEditCheck true &&
> - P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit --preserve-user &&
> + P4EDITOR=touch P4USER=alice P4PASSWD=secret "$GITP4" commit --verbose --preserve-user &&
> p4_check_commit_author file1 alice &&
> p4_check_commit_author file2 bob
> '
Debugging change?
> diff --git a/t/t9803-git-shell-metachars.sh b/t/t9803-git-shell-metachars.sh
> new file mode 100755
> index 0000000..1143491
> --- /dev/null
> +++ b/t/t9803-git-shell-metachars.sh
> @@ -0,0 +1,70 @@
> +#!/bin/sh
> +
> +test_description='git-p4 transparency to shell metachars in filenames'
> +
> +. ./lib-git-p4.sh
> +
> +test_expect_success 'start p4d' '
> + kill_p4d || : &&
> + start_p4d &&
> + cd "$TRASH_DIRECTORY"
> +'
Excellent, a test case. I'm still waiting 1.7.7 to resubmit my
series, but part of it includes fixing the test cases. I'll mail
a separate patch that does similar transformations to this.
Easier just to do it all at once.
> +test_expect_success 'init depot' '
> + (
> + cd "$cli" &&
> + echo file1 >file1 &&
> + p4 add file1 &&
> + p4 submit -d "file1"
> + )
> +'
> +
> +test_expect_success 'shell metachars in filenames' '
> + "$GITP4" clone --dest="$git" //depot &&
> + test_when_finished cleanup_git &&
> + (
> + cd "$git" &&
> + git config git-p4.skipSubmitEditCheck true &&
> + echo f1 >foo\$bar &&
> + git add foo\$bar &&
> + echo f2 >"file with spaces" &&
> + git add "file with spaces" &&
> + P4EDITOR=touch git commit -m "add files" &&
> + "$GITP4" submit --verbose &&
> + cd "$cli" &&
> + p4 sync ... &&
> + ls -l "file with spaces" &&
> + ls -l "foo\$bar"
> + )
> +'
> +
> +check_missing() {
> + for i in $*; do
> + if [ -f $i ]; then
> + echo $i found but should be missing 1>&2
> + exit 1
> + fi
> + done
> +}
> +
> +test_expect_success 'deleting with shell metachars' '
> + "$GITP4" clone --dest="$git" //depot &&
> + test_when_finished cleanup_git &&
> + (
> + cd "$git" &&
> + git config git-p4.skipSubmitEditCheck true &&
> + git rm foo\$bar &&
> + git rm file\ with\ spaces &&
> + P4EDITOR=touch git commit -m "remove files" &&
> + "$GITP4" submit --verbose
> + cd "$cli" &&
> + p4 sync ... &&
> + check_missing "file with spaces" foo\$bar
> + )
> +'
> +
> +test_expect_success 'kill p4d' '
> + kill_p4d
> +'
> +
> +test_done
> --
> 1.7.6.347.g4db0d
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC/PATCHv2] git-p4: handle files with shell metacharacters
2011-09-26 21:47 ` Pete Wyckoff
@ 2011-09-27 8:40 ` Luke Diamand
2011-09-27 8:40 ` Luke Diamand
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Luke Diamand @ 2011-09-27 8:40 UTC (permalink / raw)
To: git; +Cc: pw, vitor.hda, Luke Diamand
Updated git-p4 changes incorporating Pete's comments.
- p4CmdList's stdin argument can now be a list.
- Getting rid of the string option entirely is very hard; there are
places where currently git-p4 creates a pipeline.
- I wonder if verbose should actually be enabled for all the test
cases?
- The $ENV{PWD} is needed now because the shell used to set that; now
that the shell isn't in use git-p4 has to set it.
Pete - I wasn't sure whether you were saying I should rework
my patch against next (and you would then rework your series) or
something else. That sounds complicated though - let me know!
Thanks!
Luke
Luke Diamand (1):
git-p4: handle files with shell metacharacters
contrib/fast-import/git-p4 | 200 ++++++++++++++++++++++++---------------
t/t9803-git-shell-metachars.sh | 70 ++++++++++++++
2 files changed, 193 insertions(+), 77 deletions(-)
create mode 100755 t/t9803-git-shell-metachars.sh
--
1.7.6.347.g4db0d
^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC/PATCHv2] git-p4: handle files with shell metacharacters
2011-09-27 8:40 ` [RFC/PATCHv2] " Luke Diamand
@ 2011-09-27 8:40 ` Luke Diamand
2011-09-27 9:32 ` Luke Diamand
2011-09-27 13:03 ` Pete Wyckoff
2 siblings, 0 replies; 8+ messages in thread
From: Luke Diamand @ 2011-09-27 8:40 UTC (permalink / raw)
To: git; +Cc: pw, vitor.hda, Luke Diamand
git-p4 used to simply pass strings into system() and popen(), and
relied on the shell doing the necessary expansion. This though meant
that shell metacharacters in file names would be corrupted - for
example files with $ or space in them.
Switch to using subprocess.Popen() and friends, and pass in explicit
arrays in the places where it matters. This then avoids needing shell
expansion.
Add trivial helper functions for some common perforce operations. Add
test case.
Signed-off-by: Luke Diamand <luke@diamand.org>
---
contrib/fast-import/git-p4 | 200 ++++++++++++++++++++++++---------------
t/t9803-git-shell-metachars.sh | 70 ++++++++++++++
2 files changed, 193 insertions(+), 77 deletions(-)
create mode 100755 t/t9803-git-shell-metachars.sh
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 782b891..9770304 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -22,36 +22,39 @@ def p4_build_cmd(cmd):
location. It means that hooking into the environment, or other configuration
can be done more easily.
"""
- real_cmd = "%s " % "p4"
+ real_cmd = ["p4"]
user = gitConfig("git-p4.user")
if len(user) > 0:
- real_cmd += "-u %s " % user
+ real_cmd += ["-u",user]
password = gitConfig("git-p4.password")
if len(password) > 0:
- real_cmd += "-P %s " % password
+ real_cmd += ["-P", password]
port = gitConfig("git-p4.port")
if len(port) > 0:
- real_cmd += "-p %s " % port
+ real_cmd += ["-p", port]
host = gitConfig("git-p4.host")
if len(host) > 0:
- real_cmd += "-h %s " % host
+ real_cmd += ["-h", host]
client = gitConfig("git-p4.client")
if len(client) > 0:
- real_cmd += "-c %s " % client
+ real_cmd += ["-c", client]
- real_cmd += "%s" % (cmd)
- if verbose:
- print real_cmd
+
+ if isinstance(cmd,basestring):
+ real_cmd = ' '.join(real_cmd) + ' ' + cmd
+ else:
+ real_cmd += cmd
return real_cmd
def chdir(dir):
- if os.name == 'nt':
- os.environ['PWD']=dir
+ # P4 uses the PWD environment variable rather than getcwd(). Since we're
+ # not using the shell, we have to set it ourselves.
+ os.environ['PWD']=dir
os.chdir(dir)
def die(msg):
@@ -61,29 +64,34 @@ def die(msg):
sys.stderr.write(msg + "\n")
sys.exit(1)
-def write_pipe(c, str):
+def write_pipe(c, stdin):
if verbose:
- sys.stderr.write('Writing pipe: %s\n' % c)
+ sys.stderr.write('Writing pipe: %s\n' % str(c))
- pipe = os.popen(c, 'w')
- val = pipe.write(str)
- if pipe.close():
- die('Command failed: %s' % c)
+ expand = isinstance(c,basestring)
+ p = subprocess.Popen(c, stdin=subprocess.PIPE, shell=expand)
+ pipe = p.stdin
+ val = pipe.write(stdin)
+ pipe.close()
+ if p.wait():
+ die('Command failed: %s' % str(c))
return val
-def p4_write_pipe(c, str):
+def p4_write_pipe(c, stdin):
real_cmd = p4_build_cmd(c)
- return write_pipe(real_cmd, str)
+ return write_pipe(real_cmd, stdin)
def read_pipe(c, ignore_error=False):
if verbose:
- sys.stderr.write('Reading pipe: %s\n' % c)
+ sys.stderr.write('Reading pipe: %s\n' % str(c))
- pipe = os.popen(c, 'rb')
+ expand = isinstance(c,basestring)
+ p = subprocess.Popen(c, stdout=subprocess.PIPE, shell=expand)
+ pipe = p.stdout
val = pipe.read()
- if pipe.close() and not ignore_error:
- die('Command failed: %s' % c)
+ if p.wait() and not ignore_error:
+ die('Command failed: %s' % str(c))
return val
@@ -93,12 +101,14 @@ def p4_read_pipe(c, ignore_error=False):
def read_pipe_lines(c):
if verbose:
- sys.stderr.write('Reading pipe: %s\n' % c)
- ## todo: check return status
- pipe = os.popen(c, 'rb')
+ sys.stderr.write('Reading pipe: %s\n' % str(c))
+
+ expand = isinstance(c, basestring)
+ p = subprocess.Popen(c, stdout=subprocess.PIPE, shell=expand)
+ pipe = p.stdout
val = pipe.readlines()
- if pipe.close():
- die('Command failed: %s' % c)
+ if pipe.close() or p.wait():
+ die('Command failed: %s' % str(c))
return val
@@ -108,15 +118,37 @@ def p4_read_pipe_lines(c):
return read_pipe_lines(real_cmd)
def system(cmd):
+ expand = isinstance(cmd,basestring)
if verbose:
- sys.stderr.write("executing %s\n" % cmd)
- if os.system(cmd) != 0:
- die("command failed: %s" % cmd)
+ sys.stderr.write("executing %s\n" % str(cmd))
+ subprocess.check_call(cmd, shell=expand)
def p4_system(cmd):
"""Specifically invoke p4 as the system command. """
real_cmd = p4_build_cmd(cmd)
- return system(real_cmd)
+ expand = isinstance(real_cmd, basestring)
+ subprocess.check_call(real_cmd, shell=expand)
+
+def p4_integrate(src, dest):
+ p4_system(["integrate", "-Dt", src, dest])
+
+def p4_sync(path):
+ p4_system(["sync", path])
+
+def p4_add(f):
+ p4_system(["add", f])
+
+def p4_delete(f):
+ p4_system(["delete", f])
+
+def p4_edit(f):
+ p4_system(["edit", f])
+
+def p4_revert(f):
+ p4_system(["revert", f])
+
+def p4_reopen(type, file):
+ p4_system(["reopen", "-t", type, file])
#
# Canonicalize the p4 type and return a tuple of the
@@ -167,12 +199,12 @@ def setP4ExecBit(file, mode):
if p4Type[-1] == "+":
p4Type = p4Type[0:-1]
- p4_system("reopen -t %s %s" % (p4Type, file))
+ p4_reopen(p4Type, file)
def getP4OpenedType(file):
# Returns the perforce file type for the given file.
- result = p4_read_pipe("opened %s" % file)
+ result = p4_read_pipe(["opened", file])
match = re.match(".*\((.+)\)\r?$", result)
if match:
return match.group(1)
@@ -228,9 +260,17 @@ def isModeExecChanged(src_mode, dst_mode):
return isModeExec(src_mode) != isModeExec(dst_mode)
def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):
- cmd = p4_build_cmd("-G %s" % (cmd))
+
+ if isinstance(cmd,basestring):
+ cmd = "-G " + cmd
+ expand = True
+ else:
+ cmd = ["-G"] + cmd
+ expand = False
+
+ cmd = p4_build_cmd(cmd)
if verbose:
- sys.stderr.write("Opening pipe: %s\n" % cmd)
+ sys.stderr.write("Opening pipe: %s\n" % str(cmd))
# Use a temporary file to avoid deadlocks without
# subprocess.communicate(), which would put another copy
@@ -238,11 +278,16 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):
stdin_file = None
if stdin is not None:
stdin_file = tempfile.TemporaryFile(prefix='p4-stdin', mode=stdin_mode)
- stdin_file.write(stdin)
+ if isinstance(stdin,basestring):
+ stdin_file.write(stdin)
+ else:
+ for i in stdin:
+ stdin_file.write(i + '\n')
stdin_file.flush()
stdin_file.seek(0)
- p4 = subprocess.Popen(cmd, shell=True,
+ p4 = subprocess.Popen(cmd,
+ shell=expand,
stdin=stdin_file,
stdout=subprocess.PIPE)
@@ -275,7 +320,7 @@ def p4Where(depotPath):
if not depotPath.endswith("/"):
depotPath += "/"
depotPath = depotPath + "..."
- outputList = p4CmdList("where %s" % depotPath)
+ outputList = p4CmdList(["where", depotPath])
output = None
for entry in outputList:
if "depotFile" in entry:
@@ -477,8 +522,10 @@ def originP4BranchesExist():
def p4ChangesForPaths(depotPaths, changeRange):
assert depotPaths
- output = p4_read_pipe_lines("changes " + ' '.join (["%s...%s" % (p, changeRange)
- for p in depotPaths]))
+ cmd = ['changes']
+ for p in depotPaths:
+ cmd += ["%s...%s" % (p, changeRange)]
+ output = p4_read_pipe_lines(cmd)
changes = {}
for line in output:
@@ -561,7 +608,7 @@ class P4Debug(Command):
def run(self, args):
j = 0
- for output in p4CmdList(" ".join(args)):
+ for output in p4CmdList(args):
print 'Element: %d' % j
j += 1
print output
@@ -715,7 +762,7 @@ class P4Submit(Command, P4UserMap):
break
if not client:
die("could not get client spec")
- results = p4CmdList("changes -c %s -m 1" % client)
+ results = p4CmdList(["changes", "-c", client, "-m", "1"])
for r in results:
if r.has_key('change'):
return r['change']
@@ -778,7 +825,7 @@ class P4Submit(Command, P4UserMap):
# remove lines in the Files section that show changes to files outside the depot path we're committing into
template = ""
inFilesSection = False
- for line in p4_read_pipe_lines("change -o"):
+ for line in p4_read_pipe_lines(['change', '-o']):
if line.endswith("\r\n"):
line = line[:-2] + "\n"
if inFilesSection:
@@ -835,7 +882,7 @@ class P4Submit(Command, P4UserMap):
modifier = diff['status']
path = diff['src']
if modifier == "M":
- p4_system("edit \"%s\"" % path)
+ p4_edit(path)
if isModeExecChanged(diff['src_mode'], diff['dst_mode']):
filesToChangeExecBit[path] = diff['dst_mode']
editedFiles.add(path)
@@ -850,21 +897,21 @@ class P4Submit(Command, P4UserMap):
filesToAdd.remove(path)
elif modifier == "C":
src, dest = diff['src'], diff['dst']
- p4_system("integrate -Dt \"%s\" \"%s\"" % (src, dest))
+ p4_integrate(src, dest)
if diff['src_sha1'] != diff['dst_sha1']:
- p4_system("edit \"%s\"" % (dest))
+ p4_edit(dest)
if isModeExecChanged(diff['src_mode'], diff['dst_mode']):
- p4_system("edit \"%s\"" % (dest))
+ p4_edit(dest)
filesToChangeExecBit[dest] = diff['dst_mode']
os.unlink(dest)
editedFiles.add(dest)
elif modifier == "R":
src, dest = diff['src'], diff['dst']
- p4_system("integrate -Dt \"%s\" \"%s\"" % (src, dest))
+ p4_integrate(src, dest)
if diff['src_sha1'] != diff['dst_sha1']:
- p4_system("edit \"%s\"" % (dest))
+ p4_edit(dest)
if isModeExecChanged(diff['src_mode'], diff['dst_mode']):
- p4_system("edit \"%s\"" % (dest))
+ p4_edit(dest)
filesToChangeExecBit[dest] = diff['dst_mode']
os.unlink(dest)
editedFiles.add(dest)
@@ -887,9 +934,9 @@ class P4Submit(Command, P4UserMap):
if response == "s":
print "Skipping! Good luck with the next patches..."
for f in editedFiles:
- p4_system("revert \"%s\"" % f);
+ p4_revert(f)
for f in filesToAdd:
- system("rm %s" %f)
+ os.remove(f)
return
elif response == "a":
os.system(applyPatchCmd)
@@ -910,10 +957,10 @@ class P4Submit(Command, P4UserMap):
system(applyPatchCmd)
for f in filesToAdd:
- p4_system("add \"%s\"" % f)
+ p4_add(f)
for f in filesToDelete:
- p4_system("revert \"%s\"" % f)
- p4_system("delete \"%s\"" % f)
+ p4_revert(f)
+ p4_delete(f)
# Set/clear executable bits
for f in filesToChangeExecBit.keys():
@@ -935,7 +982,7 @@ class P4Submit(Command, P4UserMap):
del(os.environ["P4DIFF"])
diff = ""
for editedFile in editedFiles:
- diff += p4_read_pipe("diff -du %r" % editedFile)
+ diff += p4_read_pipe(['diff', '-du', editedFile])
newdiff = ""
for newFile in filesToAdd:
@@ -987,7 +1034,7 @@ class P4Submit(Command, P4UserMap):
submitTemplate = message[:message.index(separatorLine)]
if self.isWindows:
submitTemplate = submitTemplate.replace("\r\n", "\n")
- p4_write_pipe("submit -i", submitTemplate)
+ p4_write_pipe(['submit', '-i'], submitTemplate)
if self.preserveUser:
if p4User:
@@ -998,10 +1045,10 @@ class P4Submit(Command, P4UserMap):
else:
for f in editedFiles:
- p4_system("revert \"%s\"" % f);
+ p4_revert(f)
for f in filesToAdd:
- p4_system("revert \"%s\"" % f);
- system("rm %s" %f)
+ p4_revert(f)
+ os.remove(f)
os.remove(fileName)
else:
@@ -1054,8 +1101,7 @@ class P4Submit(Command, P4UserMap):
chdir(self.clientPath)
print "Synchronizing p4 checkout..."
- p4_system("sync ...")
-
+ p4_sync("...")
self.check()
commits = []
@@ -1269,7 +1315,7 @@ class P4Sync(Command, P4UserMap):
# operations. utf16 is converted to ascii or utf8, perhaps.
# But ascii text saved as -t utf16 is completely mangled.
# Invoke print -o to get the real contents.
- text = p4_read_pipe('print -q -o - "%s"' % file['depotFile'])
+ text = p4_read_pipe(['print', '-q', '-o', '-', file['depotFile']])
contents = [ text ]
# Perhaps windows wants unicode, utf16 newlines translated too;
@@ -1365,10 +1411,11 @@ class P4Sync(Command, P4UserMap):
def streamP4FilesCbSelf(entry):
self.streamP4FilesCb(entry)
- p4CmdList("-x - print",
- '\n'.join(['%s#%s' % (f['path'], f['rev'])
- for f in filesToRead]),
- cb=streamP4FilesCbSelf)
+ fileArgs = ['%s#%s' % (f['path'], f['rev']) for f in filesToRead]
+
+ p4CmdList(["-x", "-", "print"],
+ stdin=fileArgs,
+ cb=streamP4FilesCbSelf)
# do the last chunk
if self.stream_file.has_key('depotFile'):
@@ -1429,8 +1476,8 @@ class P4Sync(Command, P4UserMap):
if self.verbose:
print "Change %s is labelled %s" % (change, labelDetails)
- files = p4CmdList("files " + ' '.join (["%s...@%s" % (p, change)
- for p in branchPrefixes]))
+ files = p4CmdList(["files"] + ["%s...@%s" % (p, change)
+ for p in branchPrefixes])
if len(files) == len(labelRevisions):
@@ -1478,9 +1525,9 @@ class P4Sync(Command, P4UserMap):
newestChange = 0
if self.verbose:
print "Querying files for label %s" % label
- for file in p4CmdList("files "
- + ' '.join (["%s...@%s" % (p, label)
- for p in self.depotPaths])):
+ for file in p4CmdList(["files"] +
+ ["%s...@%s" % (p, label)
+ for p in self.depotPaths]):
revisions[file["depotFile"]] = file["rev"]
change = int(file["change"])
if change > newestChange:
@@ -1735,10 +1782,9 @@ class P4Sync(Command, P4UserMap):
newestRevision = 0
fileCnt = 0
- for info in p4CmdList("files "
- + ' '.join(["%s...%s"
- % (p, revision)
- for p in self.depotPaths])):
+ fileArgs = ["%s...%s" % (p,revision) for p in self.depotPaths]
+
+ for info in p4CmdList(["files"] + fileArgs):
if 'code' in info and info['code'] == 'error':
sys.stderr.write("p4 returned an error: %s\n"
diff --git a/t/t9803-git-shell-metachars.sh b/t/t9803-git-shell-metachars.sh
new file mode 100755
index 0000000..c166603
--- /dev/null
+++ b/t/t9803-git-shell-metachars.sh
@@ -0,0 +1,70 @@
+#!/bin/sh
+
+test_description='git-p4 transparency to shell metachars in filenames'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+ kill_p4d || : &&
+ start_p4d &&
+ cd "$TRASH_DIRECTORY"
+'
+
+test_expect_success 'init depot' '
+ (
+ cd "$cli" &&
+ echo file1 >file1 &&
+ p4 add file1 &&
+ p4 submit -d "file1"
+ )
+'
+
+test_expect_success 'shell metachars in filenames' '
+ "$GITP4" clone --dest="$git" //depot &&
+ test_when_finished cleanup_git &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEditCheck true &&
+ echo f1 >foo\$bar &&
+ git add foo\$bar &&
+ echo f2 >"file with spaces" &&
+ git add "file with spaces" &&
+ P4EDITOR=touch git commit -m "add files" &&
+ "$GITP4" submit --verbose &&
+ cd "$cli" &&
+ p4 sync ... &&
+ ls -l "file with spaces" &&
+ ls -l "foo\$bar"
+ )
+'
+
+check_missing () {
+ for i in $*; do
+ if [ -f $i ]; then
+ echo $i found but should be missing 1>&2
+ exit 1
+ fi
+ done
+}
+
+test_expect_success 'deleting with shell metachars' '
+ "$GITP4" clone --dest="$git" --verbose //depot &&
+ test_when_finished cleanup_git &&
+ (
+ cd "$git" &&
+ git config git-p4.skipSubmitEditCheck true &&
+ git rm foo\$bar &&
+ git rm file\ with\ spaces &&
+ P4EDITOR=touch git commit -m "remove files" &&
+ "$GITP4" submit --verbose
+ cd "$cli" &&
+ p4 sync ... &&
+ check_missing "file with spaces" foo\$bar
+ )
+'
+
+test_expect_success 'kill p4d' '
+ kill_p4d
+'
+
+test_done
--
1.7.6.347.g4db0d
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC/PATCHv2] git-p4: handle files with shell metacharacters
2011-09-27 8:40 ` [RFC/PATCHv2] " Luke Diamand
2011-09-27 8:40 ` Luke Diamand
@ 2011-09-27 9:32 ` Luke Diamand
2011-09-27 13:03 ` Pete Wyckoff
2 siblings, 0 replies; 8+ messages in thread
From: Luke Diamand @ 2011-09-27 9:32 UTC (permalink / raw)
To: Luke Diamand; +Cc: git, pw, vitor.hda
On 27/09/11 09:40, Luke Diamand wrote:
> Updated git-p4 changes incorporating Pete's comments.
>
> - p4CmdList's stdin argument can now be a list.
- the only purpose of this is to avoid constructing large strings. I
should probably submit as a separate patch.
>
> - Getting rid of the string option entirely is very hard; there are
> places where currently git-p4 creates a pipeline.
>
> - I wonder if verbose should actually be enabled for all the test
> cases?
>
> - The $ENV{PWD} is needed now because the shell used to set that; now
> that the shell isn't in use git-p4 has to set it.
>
> Pete - I wasn't sure whether you were saying I should rework
> my patch against next (and you would then rework your series) or
> something else. That sounds complicated though - let me know!
>
> Thanks!
> Luke
>
> Luke Diamand (1):
> git-p4: handle files with shell metacharacters
>
> contrib/fast-import/git-p4 | 200 ++++++++++++++++++++++++---------------
> t/t9803-git-shell-metachars.sh | 70 ++++++++++++++
> 2 files changed, 193 insertions(+), 77 deletions(-)
> create mode 100755 t/t9803-git-shell-metachars.sh
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC/PATCHv2] git-p4: handle files with shell metacharacters
2011-09-27 8:40 ` [RFC/PATCHv2] " Luke Diamand
2011-09-27 8:40 ` Luke Diamand
2011-09-27 9:32 ` Luke Diamand
@ 2011-09-27 13:03 ` Pete Wyckoff
2011-09-30 9:06 ` Luke Diamand
2 siblings, 1 reply; 8+ messages in thread
From: Pete Wyckoff @ 2011-09-27 13:03 UTC (permalink / raw)
To: Luke Diamand; +Cc: git, vitor.hda
luke@diamand.org wrote on Tue, 27 Sep 2011 09:40 +0100:
> Updated git-p4 changes incorporating Pete's comments.
>
> - p4CmdList's stdin argument can now be a list.
I think this fits in with the rest of the patch and can stay.
> - Getting rid of the string option entirely is very hard; there are
> places where currently git-p4 creates a pipeline.
Yeah, thanks for checking though. Best to leave it consistent
like you did.
> - I wonder if verbose should actually be enabled for all the test
> cases?
It is way too verbose, even for test, but I see the argument.
One easy place to change it would be in the definition in
t/lib-git-p4.sh. You could do this by hand when testing the
tests perhaps.
> - The $ENV{PWD} is needed now because the shell used to set that; now
> that the shell isn't in use git-p4 has to set it.
>
> Pete - I wasn't sure whether you were saying I should rework
> my patch against next (and you would then rework your series) or
> something else. That sounds complicated though - let me know!
If you don't mind, I'll just queue it up with the utf16 and
test-refactor stuff I have, and send it all to Junio post-1.7.7.
Here's how I plan to adjust your tests, given the feedback that
Junio gave earlier and from reading other tests in t/.
-- Pete
-----------8<------------------
From 6b4bd671df338210ffd0348358420f0feb6f35c0 Mon Sep 17 00:00:00 2001
From: Pete Wyckoff <pw@padd.com>
Date: Tue, 27 Sep 2011 08:53:25 -0400
Subject: [PATCH] git-p4 t9803: align syntax with other tests
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
t/t9803-git-shell-metachars.sh | 30 ++++++++++++------------------
1 files changed, 12 insertions(+), 18 deletions(-)
diff --git a/t/t9803-git-shell-metachars.sh b/t/t9803-git-shell-metachars.sh
index c166603..6cf4298 100755
--- a/t/t9803-git-shell-metachars.sh
+++ b/t/t9803-git-shell-metachars.sh
@@ -5,9 +5,7 @@ test_description='git-p4 transparency to shell metachars in filenames'
. ./lib-git-p4.sh
test_expect_success 'start p4d' '
- kill_p4d || : &&
- start_p4d &&
- cd "$TRASH_DIRECTORY"
+ start_p4d
'
test_expect_success 'init depot' '
@@ -30,25 +28,18 @@ test_expect_success 'shell metachars in filenames' '
echo f2 >"file with spaces" &&
git add "file with spaces" &&
P4EDITOR=touch git commit -m "add files" &&
- "$GITP4" submit --verbose &&
+ "$GITP4" submit
+ ) &&
+ (
cd "$cli" &&
p4 sync ... &&
- ls -l "file with spaces" &&
- ls -l "foo\$bar"
+ test -e "file with spaces" &&
+ test -e "foo\$bar"
)
'
-check_missing () {
- for i in $*; do
- if [ -f $i ]; then
- echo $i found but should be missing 1>&2
- exit 1
- fi
- done
-}
-
test_expect_success 'deleting with shell metachars' '
- "$GITP4" clone --dest="$git" --verbose //depot &&
+ "$GITP4" clone --dest="$git" //depot &&
test_when_finished cleanup_git &&
(
cd "$git" &&
@@ -56,10 +47,13 @@ test_expect_success 'deleting with shell metachars' '
git rm foo\$bar &&
git rm file\ with\ spaces &&
P4EDITOR=touch git commit -m "remove files" &&
- "$GITP4" submit --verbose
+ "$GITP4" submit
+ ) &&
+ (
cd "$cli" &&
p4 sync ... &&
- check_missing "file with spaces" foo\$bar
+ test ! -e "file with spaces" &&
+ test ! -e foo\$bar
)
'
--
1.7.6.3
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC/PATCHv2] git-p4: handle files with shell metacharacters
2011-09-27 13:03 ` Pete Wyckoff
@ 2011-09-30 9:06 ` Luke Diamand
0 siblings, 0 replies; 8+ messages in thread
From: Luke Diamand @ 2011-09-30 9:06 UTC (permalink / raw)
To: Pete Wyckoff; +Cc: git
On 27/09/11 14:03, Pete Wyckoff wrote:
<snip>
>
> If you don't mind, I'll just queue it up with the utf16 and
> test-refactor stuff I have, and send it all to Junio post-1.7.7.
> Here's how I plan to adjust your tests, given the feedback that
> Junio gave earlier and from reading other tests in t/.
>
> -- Pete
Pete - I've just noticed that t9803 sets P4EDITOR up for the wrong commit.
It works fine for me in my the setup I've got at home but on another
setup hangs trying to run vi from within the test.
It looks like two or possibly three bugs combine with each other.
(a) my misplacement of P4EDITOR
(b) git-p4 doesn't check the return code from system(editor + fileName)
at around 1018, so it just carries blithely on.
(c) "git var GIT_EDITOR" called from within the test harness gives
differing results. In one setup I get "vi", in another, nothing. The
tests pass in the latter case.
I'll submit a new patch series to fix (a) and (b). I'm not sure if (c)
is a bug or a feature. If I get very keen I might also include a tidied
up version of the recent patch for turning off the editor explicitly.
Luke
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-09-30 9:06 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-09-26 21:29 [PATCH/RFCv1] git-p4: handle files with shell metacharacters Luke Diamand
2011-09-26 21:29 ` Luke Diamand
2011-09-26 21:47 ` Pete Wyckoff
2011-09-27 8:40 ` [RFC/PATCHv2] " Luke Diamand
2011-09-27 8:40 ` Luke Diamand
2011-09-27 9:32 ` Luke Diamand
2011-09-27 13:03 ` Pete Wyckoff
2011-09-30 9:06 ` Luke Diamand
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.