git-p4: handle files with shell metacharacters
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. [pw: test cleanup] Signed-off-by: Luke Diamand <luke@diamand.org> Signed-off-by: Pete Wyckoff <pw@padd.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Luke Diamand committed
Oct 16, 2011 at 10:47 UTC
6de040df566d4b4489366aa2ae5cc215ec449618
2 files changed
+187
-77
contrib/fast-import/git-p4
+123
-77
@@ -22,36 +22,39 @@ def p4_build_cmd(cmd):
22
location. It means that hooking into the environment, or other configuration
23
can be done more easily.
24
"""
25
- real_cmd = "%s " % "p4"
25
+ real_cmd = ["p4"]
26
27
user = gitConfig("git-p4.user")
28
if len(user) > 0:
29
- real_cmd += "-u %s " % user
29
+ real_cmd += ["-u",user]
30
31
password = gitConfig("git-p4.password")
32
if len(password) > 0:
33
- real_cmd += "-P %s " % password
33
+ real_cmd += ["-P", password]
34
35
port = gitConfig("git-p4.port")
36
if len(port) > 0:
37
- real_cmd += "-p %s " % port
37
+ real_cmd += ["-p", port]
38
39
host = gitConfig("git-p4.host")
40
if len(host) > 0:
41
- real_cmd += "-h %s " % host
41
+ real_cmd += ["-h", host]
42
43
client = gitConfig("git-p4.client")
44
if len(client) > 0:
45
- real_cmd += "-c %s " % client
45
+ real_cmd += ["-c", client]
46
47
- real_cmd += "%s" % (cmd)
48
- if verbose:
49
- print real_cmd
47
+
48
+ if isinstance(cmd,basestring):
49
+ real_cmd = ' '.join(real_cmd) + ' ' + cmd
50
+ else:
51
+ real_cmd += cmd
52
return real_cmd
53
54
def chdir(dir):
53
- if os.name == 'nt':
54
- os.environ['PWD']=dir
55
+ # P4 uses the PWD environment variable rather than getcwd(). Since we're
56
+ # not using the shell, we have to set it ourselves.
57
+ os.environ['PWD']=dir
58
os.chdir(dir)
59
60
def die(msg):
@@ -61,29 +64,34 @@ def die(msg):
64
sys.stderr.write(msg + "\n")
65
sys.exit(1)
66
64
-def write_pipe(c, str):
67
+def write_pipe(c, stdin):
68
if verbose:
66
- sys.stderr.write('Writing pipe: %s\n' % c)
69
+ sys.stderr.write('Writing pipe: %s\n' % str(c))
70
68
- pipe = os.popen(c, 'w')
69
- val = pipe.write(str)
70
- if pipe.close():
71
- die('Command failed: %s' % c)
71
+ expand = isinstance(c,basestring)
72
+ p = subprocess.Popen(c, stdin=subprocess.PIPE, shell=expand)
73
+ pipe = p.stdin
74
+ val = pipe.write(stdin)
75
+ pipe.close()
76
+ if p.wait():
77
+ die('Command failed: %s' % str(c))
78
79
return val
80
75
-def p4_write_pipe(c, str):
81
+def p4_write_pipe(c, stdin):
82
real_cmd = p4_build_cmd(c)
77
- return write_pipe(real_cmd, str)
83
+ return write_pipe(real_cmd, stdin)
84
85
def read_pipe(c, ignore_error=False):
86
if verbose:
81
- sys.stderr.write('Reading pipe: %s\n' % c)
87
+ sys.stderr.write('Reading pipe: %s\n' % str(c))
88
83
- pipe = os.popen(c, 'rb')
89
+ expand = isinstance(c,basestring)
90
+ p = subprocess.Popen(c, stdout=subprocess.PIPE, shell=expand)
91
+ pipe = p.stdout
92
val = pipe.read()
85
- if pipe.close() and not ignore_error:
86
- die('Command failed: %s' % c)
93
+ if p.wait() and not ignore_error:
94
+ die('Command failed: %s' % str(c))
95
96
return val
97
@@ -93,12 +101,14 @@ def p4_read_pipe(c, ignore_error=False):
101
102
def read_pipe_lines(c):
103
if verbose:
96
- sys.stderr.write('Reading pipe: %s\n' % c)
97
- ## todo: check return status
98
- pipe = os.popen(c, 'rb')
104
+ sys.stderr.write('Reading pipe: %s\n' % str(c))
105
+
106
+ expand = isinstance(c, basestring)
107
+ p = subprocess.Popen(c, stdout=subprocess.PIPE, shell=expand)
108
+ pipe = p.stdout
109
val = pipe.readlines()
100
- if pipe.close():
101
- die('Command failed: %s' % c)
110
+ if pipe.close() or p.wait():
111
+ die('Command failed: %s' % str(c))
112
113
return val
114
@@ -108,15 +118,37 @@ def p4_read_pipe_lines(c):
118
return read_pipe_lines(real_cmd)
119
120
def system(cmd):
121
+ expand = isinstance(cmd,basestring)
122
if verbose:
112
- sys.stderr.write("executing %s\n" % cmd)
113
- if os.system(cmd) != 0:
114
- die("command failed: %s" % cmd)
123
+ sys.stderr.write("executing %s\n" % str(cmd))
124
+ subprocess.check_call(cmd, shell=expand)
125
126
def p4_system(cmd):
127
"""Specifically invoke p4 as the system command. """
128
real_cmd = p4_build_cmd(cmd)
119
- return system(real_cmd)
129
+ expand = isinstance(real_cmd, basestring)
130
+ subprocess.check_call(real_cmd, shell=expand)
131
+
132
+def p4_integrate(src, dest):
133
+ p4_system(["integrate", "-Dt", src, dest])
134
+
135
+def p4_sync(path):
136
+ p4_system(["sync", path])
137
+
138
+def p4_add(f):
139
+ p4_system(["add", f])
140
+
141
+def p4_delete(f):
142
+ p4_system(["delete", f])
143
+
144
+def p4_edit(f):
145
+ p4_system(["edit", f])
146
+
147
+def p4_revert(f):
148
+ p4_system(["revert", f])
149
+
150
+def p4_reopen(type, file):
151
+ p4_system(["reopen", "-t", type, file])
152
153
#
154
# Canonicalize the p4 type and return a tuple of the
@@ -167,12 +199,12 @@ def setP4ExecBit(file, mode):
199
if p4Type[-1] == "+":
200
p4Type = p4Type[0:-1]
201
170
- p4_system("reopen -t %s %s" % (p4Type, file))
202
+ p4_reopen(p4Type, file)
203
204
def getP4OpenedType(file):
205
# Returns the perforce file type for the given file.
206
175
- result = p4_read_pipe("opened %s" % file)
207
+ result = p4_read_pipe(["opened", file])
208
match = re.match(".*\((.+)\)\r?$", result)
209
if match:
210
return match.group(1)
@@ -228,9 +260,17 @@ def isModeExecChanged(src_mode, dst_mode):
260
return isModeExec(src_mode) != isModeExec(dst_mode)
261
262
def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):
231
- cmd = p4_build_cmd("-G %s" % (cmd))
263
+
264
+ if isinstance(cmd,basestring):
265
+ cmd = "-G " + cmd
266
+ expand = True
267
+ else:
268
+ cmd = ["-G"] + cmd
269
+ expand = False
270
+
271
+ cmd = p4_build_cmd(cmd)
272
if verbose:
233
- sys.stderr.write("Opening pipe: %s\n" % cmd)
273
+ sys.stderr.write("Opening pipe: %s\n" % str(cmd))
274
275
# Use a temporary file to avoid deadlocks without
276
# subprocess.communicate(), which would put another copy
@@ -238,11 +278,16 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None):
278
stdin_file = None
279
if stdin is not None:
280
stdin_file = tempfile.TemporaryFile(prefix='p4-stdin', mode=stdin_mode)
241
- stdin_file.write(stdin)
281
+ if isinstance(stdin,basestring):
282
+ stdin_file.write(stdin)
283
+ else:
284
+ for i in stdin:
285
+ stdin_file.write(i + '\n')
286
stdin_file.flush()
287
stdin_file.seek(0)
288
245
- p4 = subprocess.Popen(cmd, shell=True,
289
+ p4 = subprocess.Popen(cmd,
290
+ shell=expand,
291
stdin=stdin_file,
292
stdout=subprocess.PIPE)
293
@@ -275,7 +320,7 @@ def p4Where(depotPath):
320
if not depotPath.endswith("/"):
321
depotPath += "/"
322
depotPath = depotPath + "..."
278
- outputList = p4CmdList("where %s" % depotPath)
323
+ outputList = p4CmdList(["where", depotPath])
324
output = None
325
for entry in outputList:
326
if "depotFile" in entry:
@@ -477,8 +522,10 @@ def originP4BranchesExist():
522
523
def p4ChangesForPaths(depotPaths, changeRange):
524
assert depotPaths
480
- output = p4_read_pipe_lines("changes " + ' '.join (["%s...%s" % (p, changeRange)
481
- for p in depotPaths]))
525
+ cmd = ['changes']
526
+ for p in depotPaths:
527
+ cmd += ["%s...%s" % (p, changeRange)]
528
+ output = p4_read_pipe_lines(cmd)
529
530
changes = {}
531
for line in output:
@@ -561,7 +608,7 @@ class P4Debug(Command):
608
609
def run(self, args):
610
j = 0
564
- for output in p4CmdList(" ".join(args)):
611
+ for output in p4CmdList(args):
612
print 'Element: %d' % j
613
j += 1
614
print output
@@ -715,7 +762,7 @@ class P4Submit(Command, P4UserMap):
762
break
763
if not client:
764
die("could not get client spec")
718
- results = p4CmdList("changes -c %s -m 1" % client)
765
+ results = p4CmdList(["changes", "-c", client, "-m", "1"])
766
for r in results:
767
if r.has_key('change'):
768
return r['change']
@@ -778,7 +825,7 @@ class P4Submit(Command, P4UserMap):
825
# remove lines in the Files section that show changes to files outside the depot path we're committing into
826
template = ""
827
inFilesSection = False
781
- for line in p4_read_pipe_lines("change -o"):
828
+ for line in p4_read_pipe_lines(['change', '-o']):
829
if line.endswith("\r\n"):
830
line = line[:-2] + "\n"
831
if inFilesSection:
@@ -835,7 +882,7 @@ class P4Submit(Command, P4UserMap):
882
modifier = diff['status']
883
path = diff['src']
884
if modifier == "M":
838
- p4_system("edit \"%s\"" % path)
885
+ p4_edit(path)
886
if isModeExecChanged(diff['src_mode'], diff['dst_mode']):
887
filesToChangeExecBit[path] = diff['dst_mode']
888
editedFiles.add(path)
@@ -850,21 +897,21 @@ class P4Submit(Command, P4UserMap):
897
filesToAdd.remove(path)
898
elif modifier == "C":
899
src, dest = diff['src'], diff['dst']
853
- p4_system("integrate -Dt \"%s\" \"%s\"" % (src, dest))
900
+ p4_integrate(src, dest)
901
if diff['src_sha1'] != diff['dst_sha1']:
855
- p4_system("edit \"%s\"" % (dest))
902
+ p4_edit(dest)
903
if isModeExecChanged(diff['src_mode'], diff['dst_mode']):
857
- p4_system("edit \"%s\"" % (dest))
904
+ p4_edit(dest)
905
filesToChangeExecBit[dest] = diff['dst_mode']
906
os.unlink(dest)
907
editedFiles.add(dest)
908
elif modifier == "R":
909
src, dest = diff['src'], diff['dst']
863
- p4_system("integrate -Dt \"%s\" \"%s\"" % (src, dest))
910
+ p4_integrate(src, dest)
911
if diff['src_sha1'] != diff['dst_sha1']:
865
- p4_system("edit \"%s\"" % (dest))
912
+ p4_edit(dest)
913
if isModeExecChanged(diff['src_mode'], diff['dst_mode']):
867
- p4_system("edit \"%s\"" % (dest))
914
+ p4_edit(dest)
915
filesToChangeExecBit[dest] = diff['dst_mode']
916
os.unlink(dest)
917
editedFiles.add(dest)
@@ -887,9 +934,9 @@ class P4Submit(Command, P4UserMap):
934
if response == "s":
935
print "Skipping! Good luck with the next patches..."
936
for f in editedFiles:
890
- p4_system("revert \"%s\"" % f);
937
+ p4_revert(f)
938
for f in filesToAdd:
892
- system("rm %s" %f)
939
+ os.remove(f)
940
return
941
elif response == "a":
942
os.system(applyPatchCmd)
@@ -910,10 +957,10 @@ class P4Submit(Command, P4UserMap):
957
system(applyPatchCmd)
958
959
for f in filesToAdd:
913
- p4_system("add \"%s\"" % f)
960
+ p4_add(f)
961
for f in filesToDelete:
915
- p4_system("revert \"%s\"" % f)
916
- p4_system("delete \"%s\"" % f)
962
+ p4_revert(f)
963
+ p4_delete(f)
964
965
# Set/clear executable bits
966
for f in filesToChangeExecBit.keys():
@@ -935,7 +982,7 @@ class P4Submit(Command, P4UserMap):
982
del(os.environ["P4DIFF"])
983
diff = ""
984
for editedFile in editedFiles:
938
- diff += p4_read_pipe("diff -du %r" % editedFile)
985
+ diff += p4_read_pipe(['diff', '-du', editedFile])
986
987
newdiff = ""
988
for newFile in filesToAdd:
@@ -987,7 +1034,7 @@ class P4Submit(Command, P4UserMap):
1034
submitTemplate = message[:message.index(separatorLine)]
1035
if self.isWindows:
1036
submitTemplate = submitTemplate.replace("\r\n", "\n")
990
- p4_write_pipe("submit -i", submitTemplate)
1037
+ p4_write_pipe(['submit', '-i'], submitTemplate)
1038
1039
if self.preserveUser:
1040
if p4User:
@@ -998,10 +1045,10 @@ class P4Submit(Command, P4UserMap):
1045
1046
else:
1047
for f in editedFiles:
1001
- p4_system("revert \"%s\"" % f);
1048
+ p4_revert(f)
1049
for f in filesToAdd:
1003
- p4_system("revert \"%s\"" % f);
1004
- system("rm %s" %f)
1050
+ p4_revert(f)
1051
+ os.remove(f)
1052
1053
os.remove(fileName)
1054
else:
@@ -1054,8 +1101,7 @@ class P4Submit(Command, P4UserMap):
1101
1102
chdir(self.clientPath)
1103
print "Synchronizing p4 checkout..."
1057
- p4_system("sync ...")
1058
-
1104
+ p4_sync("...")
1105
self.check()
1106
1107
commits = []
@@ -1269,7 +1315,7 @@ class P4Sync(Command, P4UserMap):
1315
# operations. utf16 is converted to ascii or utf8, perhaps.
1316
# But ascii text saved as -t utf16 is completely mangled.
1317
# Invoke print -o to get the real contents.
1272
- text = p4_read_pipe('print -q -o - "%s"' % file['depotFile'])
1318
+ text = p4_read_pipe(['print', '-q', '-o', '-', file['depotFile']])
1319
contents = [ text ]
1320
1321
# Perhaps windows wants unicode, utf16 newlines translated too;
@@ -1365,10 +1411,11 @@ class P4Sync(Command, P4UserMap):
1411
def streamP4FilesCbSelf(entry):
1412
self.streamP4FilesCb(entry)
1413
1368
- p4CmdList("-x - print",
1369
- '\n'.join(['%s#%s' % (f['path'], f['rev'])
1370
- for f in filesToRead]),
1371
- cb=streamP4FilesCbSelf)
1414
+ fileArgs = ['%s#%s' % (f['path'], f['rev']) for f in filesToRead]
1415
+
1416
+ p4CmdList(["-x", "-", "print"],
1417
+ stdin=fileArgs,
1418
+ cb=streamP4FilesCbSelf)
1419
1420
# do the last chunk
1421
if self.stream_file.has_key('depotFile'):
@@ -1429,8 +1476,8 @@ class P4Sync(Command, P4UserMap):
1476
if self.verbose:
1477
print "Change %s is labelled %s" % (change, labelDetails)
1478
1432
- files = p4CmdList("files " + ' '.join (["%s...@%s" % (p, change)
1433
- for p in branchPrefixes]))
1479
+ files = p4CmdList(["files"] + ["%s...@%s" % (p, change)
1480
+ for p in branchPrefixes])
1481
1482
if len(files) == len(labelRevisions):
1483
@@ -1478,9 +1525,9 @@ class P4Sync(Command, P4UserMap):
1525
newestChange = 0
1526
if self.verbose:
1527
print "Querying files for label %s" % label
1481
- for file in p4CmdList("files "
1482
- + ' '.join (["%s...@%s" % (p, label)
1483
- for p in self.depotPaths])):
1528
+ for file in p4CmdList(["files"] +
1529
+ ["%s...@%s" % (p, label)
1530
+ for p in self.depotPaths]):
1531
revisions[file["depotFile"]] = file["rev"]
1532
change = int(file["change"])
1533
if change > newestChange:
@@ -1735,10 +1782,9 @@ class P4Sync(Command, P4UserMap):
1782
newestRevision = 0
1783
1784
fileCnt = 0
1738
- for info in p4CmdList("files "
1739
- + ' '.join(["%s...%s"
1740
- % (p, revision)
1741
- for p in self.depotPaths])):
1785
+ fileArgs = ["%s...%s" % (p,revision) for p in self.depotPaths]
1786
+
1787
+ for info in p4CmdList(["files"] + fileArgs):
1788
1789
if 'code' in info and info['code'] == 'error':
1790
sys.stderr.write("p4 returned an error: %s\n"
t/t9803-git-shell-metachars.sh
new
+64
@@ -0,0 +1,64 @@
1
+#!/bin/sh
2
+
3
+test_description='git-p4 transparency to shell metachars in filenames'
4
+
5
+. ./lib-git-p4.sh
6
+
7
+test_expect_success 'start p4d' '
8
+ start_p4d
9
+'
10
+
11
+test_expect_success 'init depot' '
12
+ (
13
+ cd "$cli" &&
14
+ echo file1 >file1 &&
15
+ p4 add file1 &&
16
+ p4 submit -d "file1"
17
+ )
18
+'
19
+
20
+test_expect_success 'shell metachars in filenames' '
21
+ "$GITP4" clone --dest="$git" //depot &&
22
+ test_when_finished cleanup_git &&
23
+ (
24
+ cd "$git" &&
25
+ git config git-p4.skipSubmitEditCheck true &&
26
+ echo f1 >foo\$bar &&
27
+ git add foo\$bar &&
28
+ echo f2 >"file with spaces" &&
29
+ git add "file with spaces" &&
30
+ git commit -m "add files" &&
31
+ P4EDITOR=touch "$GITP4" submit
32
+ ) &&
33
+ (
34
+ cd "$cli" &&
35
+ p4 sync ... &&
36
+ test -e "file with spaces" &&
37
+ test -e "foo\$bar"
38
+ )
39
+'
40
+
41
+test_expect_success 'deleting with shell metachars' '
42
+ "$GITP4" clone --dest="$git" //depot &&
43
+ test_when_finished cleanup_git &&
44
+ (
45
+ cd "$git" &&
46
+ git config git-p4.skipSubmitEditCheck true &&
47
+ git rm foo\$bar &&
48
+ git rm file\ with\ spaces &&
49
+ git commit -m "remove files" &&
50
+ P4EDITOR=touch "$GITP4" submit
51
+ ) &&
52
+ (
53
+ cd "$cli" &&
54
+ p4 sync ... &&
55
+ test ! -e "file with spaces" &&
56
+ test ! -e foo\$bar
57
+ )
58
+'
59
+
60
+test_expect_success 'kill p4d' '
61
+ kill_p4d
62
+'
63
+
64
+test_done