* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).