@matheusbsbs / matheus-bagliano / commits / 3d8a3038bc

git-p4: don't select shell mode using the type of the command argument

Previously, the script would invoke subprocess functions setting the shell argument True if the command argument was a string, setting it False otherwise. This patch replaces this implicit type-driven behaviour with explicit shell arguments specified by the caller. The apparent motive for the implict behaviour is that the subprocess functions do not divide command strings into args. Invoking subprocess.call("echo hello") will attempt to execute a program by the name "echo hello". With subprocess.call("echo hello", shell=True), sh -c "echo hello" will be executed instead, which will cause the command and args to be divided by spaces. Eventually, all usage of shell=True, that is not necessary for some purpose beyond parsing command strings, should be removed. For now, this patch makes the usage of shells explicit. Signed-off-by: Joel Holdsworth <jholdsworth@nvidia.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Joel Holdsworth committed Jan 6, 2022 at 21:40 UTC 3d8a3038bce2b13faeca0b776efa896d1c0d0b49

1 file changed +63 -63

git-p4.py

+63 -63

@@ -276,12 +276,11 @@ def run_hook_command(cmd, param):

276 return subprocess.call(cli, shell=use_shell)

277

278

279 -def write_pipe(c, stdin):

279 +def write_pipe(c, stdin, *k, **kw):

280 if verbose:

281 sys.stderr.write('Writing pipe: %s\n' % str(c))

282

283 - expand = not isinstance(c, list)

284 - p = subprocess.Popen(c, stdin=subprocess.PIPE, shell=expand)

283 + p = subprocess.Popen(c, stdin=subprocess.PIPE, *k, **kw)

284 pipe = p.stdin

285 val = pipe.write(stdin)

286 pipe.close()

@@ -290,13 +289,13 @@ def write_pipe(c, stdin):

289

290 return val

291

293 -def p4_write_pipe(c, stdin):

292 +def p4_write_pipe(c, stdin, *k, **kw):

293 real_cmd = p4_build_cmd(c)

294 if bytes is not str and isinstance(stdin, str):

295 stdin = encode_text_stream(stdin)

297 - return write_pipe(real_cmd, stdin)

296 + return write_pipe(real_cmd, stdin, *k, **kw)

297

299 -def read_pipe_full(c):

298 +def read_pipe_full(c, *k, **kw):

299 """ Read output from command. Returns a tuple

300 of the return status, stdout text and stderr

301 text.

@@ -304,19 +303,19 @@ def read_pipe_full(c):

303 if verbose:

304 sys.stderr.write('Reading pipe: %s\n' % str(c))

305

307 - expand = not isinstance(c, list)

308 - p = subprocess.Popen(c, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=expand)

306 + p = subprocess.Popen(

307 + c, stdout=subprocess.PIPE, stderr=subprocess.PIPE, *k, **kw)

308 (out, err) = p.communicate()

309 return (p.returncode, out, decode_text_stream(err))

310

312 -def read_pipe(c, ignore_error=False, raw=False):

311 +def read_pipe(c, ignore_error=False, raw=False, *k, **kw):

312 """ Read output from command. Returns the output text on

313 success. On failure, terminates execution, unless

314 ignore_error is True, when it returns an empty string.

315

316 If raw is True, do not attempt to decode output text.

317 """

319 - (retcode, out, err) = read_pipe_full(c)

318 + (retcode, out, err) = read_pipe_full(c, *k, **kw)

319 if retcode != 0:

320 if ignore_error:

321 out = ""

@@ -326,26 +325,25 @@ def read_pipe(c, ignore_error=False, raw=False):

325 out = decode_text_stream(out)

326 return out

327

329 -def read_pipe_text(c):

328 +def read_pipe_text(c, *k, **kw):

329 """ Read output from a command with trailing whitespace stripped.

330 On error, returns None.

331 """

333 - (retcode, out, err) = read_pipe_full(c)

332 + (retcode, out, err) = read_pipe_full(c, *k, **kw)

333 if retcode != 0:

334 return None

335 else:

336 return decode_text_stream(out).rstrip()

337

339 -def p4_read_pipe(c, ignore_error=False, raw=False):

338 +def p4_read_pipe(c, ignore_error=False, raw=False, *k, **kw):

339 real_cmd = p4_build_cmd(c)

341 - return read_pipe(real_cmd, ignore_error, raw=raw)

340 + return read_pipe(real_cmd, ignore_error, raw=raw, *k, **kw)

341

343 -def read_pipe_lines(c, raw=False):

342 +def read_pipe_lines(c, raw=False, *k, **kw):

343 if verbose:

344 sys.stderr.write('Reading pipe: %s\n' % str(c))

345

347 - expand = not isinstance(c, list)

348 - p = subprocess.Popen(c, stdout=subprocess.PIPE, shell=expand)

346 + p = subprocess.Popen(c, stdout=subprocess.PIPE, *k, **kw)

347 pipe = p.stdout

348 lines = pipe.readlines()

349 if not raw:

@@ -354,10 +352,10 @@ def read_pipe_lines(c, raw=False):

352 die('Command failed: %s' % str(c))

353 return lines

354

357 -def p4_read_pipe_lines(c):

355 +def p4_read_pipe_lines(c, *k, **kw):

356 """Specifically invoke p4 on the command supplied. """

357 real_cmd = p4_build_cmd(c)

360 - return read_pipe_lines(real_cmd)

358 + return read_pipe_lines(real_cmd, *k, **kw)

359

360 def p4_has_command(cmd):

361 """Ask p4 for help on this command. If it returns an error, the

@@ -388,21 +386,19 @@ def p4_has_move_command():

386 # assume it failed because @... was invalid changelist

387 return True

388

391 -def system(cmd, ignore_error=False):

392 - expand = not isinstance(cmd, list)

389 +def system(cmd, ignore_error=False, *k, **kw):

390 if verbose:

391 sys.stderr.write("executing %s\n" % str(cmd))

395 - retcode = subprocess.call(cmd, shell=expand)

392 + retcode = subprocess.call(cmd, *k, **kw)

393 if retcode and not ignore_error:

394 raise CalledProcessError(retcode, cmd)

395

396 return retcode

397

401 -def p4_system(cmd):

398 +def p4_system(cmd, *k, **kw):

399 """Specifically invoke p4 as the system command. """

400 real_cmd = p4_build_cmd(cmd)

404 - expand = not isinstance(real_cmd, list)

405 - retcode = subprocess.call(real_cmd, shell=expand)

401 + retcode = subprocess.call(real_cmd, *k, **kw)

402 if retcode:

403 raise CalledProcessError(retcode, real_cmd)

404

@@ -723,14 +719,12 @@ def isModeExecChanged(src_mode, dst_mode):

719 return isModeExec(src_mode) != isModeExec(dst_mode)

720

721 def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,

726 - errors_as_exceptions=False):

722 + errors_as_exceptions=False, *k, **kw):

723

724 if not isinstance(cmd, list):

725 cmd = "-G " + cmd

730 - expand = True

726 else:

727 cmd = ["-G"] + cmd

733 - expand = False

728

729 cmd = p4_build_cmd(cmd)

730 if verbose:

@@ -751,10 +745,8 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,

745 stdin_file.flush()

746 stdin_file.seek(0)

747

754 - p4 = subprocess.Popen(cmd,

755 - shell=expand,

756 - stdin=stdin_file,

757 - stdout=subprocess.PIPE)

748 + p4 = subprocess.Popen(

749 + cmd, stdin=stdin_file, stdout=subprocess.PIPE, *k, **kw)

750

751 result = []

752 try:

@@ -807,8 +799,8 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,

799

800 return result

801

810 -def p4Cmd(cmd):

811 - list = p4CmdList(cmd)

802 +def p4Cmd(cmd, *k, **kw):

803 + list = p4CmdList(cmd, *k, **kw)

804 result = {}

805 for entry in list:

806 result.update(entry)

@@ -857,7 +849,7 @@ def isValidGitDir(path):

849 return git_dir(path) != None

850

851 def parseRevision(ref):

860 - return read_pipe("git rev-parse %s" % ref).strip()

852 + return read_pipe("git rev-parse %s" % ref, shell=True).strip()

853

854 def branchExists(ref):

855 rev = read_pipe(["git", "rev-parse", "-q", "--verify", ref],

@@ -969,7 +961,7 @@ def p4BranchesInGit(branchesAreInRemotes=True):

961 else:

962 cmdline += "--branches"

963

972 - for line in read_pipe_lines(cmdline):

964 + for line in read_pipe_lines(cmdline, shell=True):

965 line = line.strip()

966

967 # only import to p4/

@@ -1032,7 +1024,7 @@ def createOrUpdateBranchesFromOrigin(localRefPrefix = "refs/remotes/p4/", silent

1024

1025 originPrefix = "origin/p4/"

1026

1035 - for line in read_pipe_lines("git rev-parse --symbolic --remotes"):

1027 + for line in read_pipe_lines("git rev-parse --symbolic --remotes", shell=True):

1028 line = line.strip()

1029 if (not line.startswith(originPrefix)) or line.endswith("HEAD"):

1030 continue

@@ -1070,7 +1062,8 @@ def createOrUpdateBranchesFromOrigin(localRefPrefix = "refs/remotes/p4/", silent

1062 remoteHead, ','.join(settings['depot-paths'])))

1063

1064 if update:

1073 - system("git update-ref %s %s" % (remoteHead, originHead))

1065 + system("git update-ref %s %s" % (remoteHead, originHead),

1066 + shell=True)

1067

1068 def originP4BranchesExist():

1069 return gitBranchExists("origin") or gitBranchExists("origin/p4") or gitBranchExists("origin/p4/master")

@@ -1184,7 +1177,7 @@ def getClientSpec():

1177 """Look at the p4 client spec, create a View() object that contains

1178 all the mappings, and return it."""

1179

1187 - specList = p4CmdList("client -o")

1180 + specList = p4CmdList("client -o", shell=True)

1181 if len(specList) != 1:

1182 die('Output from "client -o" is %d lines, expecting 1' %

1183 len(specList))

@@ -1213,7 +1206,7 @@ def getClientSpec():

1206 def getClientRoot():

1207 """Grab the client directory."""

1208

1216 - output = p4CmdList("client -o")

1209 + output = p4CmdList("client -o", shell=True)

1210 if len(output) != 1:

1211 die('Output from "client -o" is %d lines, expecting 1' % len(output))

1212

@@ -1468,7 +1461,7 @@ class P4UserMap:

1461 if self.myP4UserId:

1462 return self.myP4UserId

1463

1471 - results = p4CmdList("user -o")

1464 + results = p4CmdList("user -o", shell=True)

1465 for r in results:

1466 if 'User' in r:

1467 self.myP4UserId = r['User']

@@ -1493,7 +1486,7 @@ class P4UserMap:

1486 self.users = {}

1487 self.emails = {}

1488

1496 - for output in p4CmdList("users"):

1489 + for output in p4CmdList("users", shell=True):

1490 if "User" not in output:

1491 continue

1492 self.users[output["User"]] = output["FullName"] + " <" + output["Email"] + ">"

@@ -1691,7 +1684,7 @@ class P4Submit(Command, P4UserMap):

1684 die("Large file system not supported for git-p4 submit command. Please remove it from config.")

1685

1686 def check(self):

1694 - if len(p4CmdList("opened ...")) > 0:

1687 + if len(p4CmdList("opened ...", shell=True)) > 0:

1688 die("You have files opened with perforce! Close them before starting the sync.")

1689

1690 def separate_jobs_from_description(self, message):

@@ -1795,7 +1788,7 @@ class P4Submit(Command, P4UserMap):

1788 # then gets used to patch up the username in the change. If the same

1789 # client spec is being used by multiple processes then this might go

1790 # wrong.

1798 - results = p4CmdList("client -o") # find the current client

1791 + results = p4CmdList("client -o", shell=True) # find the current client

1792 client = None

1793 for r in results:

1794 if 'Client' in r:

@@ -1811,7 +1804,7 @@ class P4Submit(Command, P4UserMap):

1804

1805 def modifyChangelistUser(self, changelist, newUser):

1806 # fixup the user field of a changelist after it has been submitted.

1814 - changes = p4CmdList("change -o %s" % changelist)

1807 + changes = p4CmdList("change -o %s" % changelist, shell=True)

1808 if len(changes) != 1:

1809 die("Bad output from p4 change modifying %s to user %s" %

1810 (changelist, newUser))

@@ -1822,7 +1815,7 @@ class P4Submit(Command, P4UserMap):

1815 # p4 does not understand format version 3 and above

1816 input = marshal.dumps(c, 2)

1817

1825 - result = p4CmdList("change -f -i", stdin=input)

1818 + result = p4CmdList("change -f -i", stdin=input, shell=True)

1819 for r in result:

1820 if 'code' in r:

1821 if r['code'] == 'error':

@@ -1928,7 +1921,7 @@ class P4Submit(Command, P4UserMap):

1921 if "P4EDITOR" in os.environ and (os.environ.get("P4EDITOR") != ""):

1922 editor = os.environ.get("P4EDITOR")

1923 else:

1931 - editor = read_pipe("git var GIT_EDITOR").strip()

1924 + editor = read_pipe("git var GIT_EDITOR", shell=True).strip()

1925 system(["sh", "-c", ('%s "$@"' % editor), editor, template_file])

1926

1927 # If the file was not saved, prompt to see if this patch should

@@ -1986,7 +1979,9 @@ class P4Submit(Command, P4UserMap):

1979

1980 (p4User, gitEmail) = self.p4UserForCommit(id)

1981

1989 - diff = read_pipe_lines("git diff-tree -r %s \"%s^\" \"%s\"" % (self.diffOpts, id, id))

1982 + diff = read_pipe_lines(

1983 + "git diff-tree -r %s \"%s^\" \"%s\"" % (self.diffOpts, id, id),

1984 + shell=True)

1985 filesToAdd = set()

1986 filesToChangeType = set()

1987 filesToDelete = set()

@@ -2122,7 +2117,7 @@ class P4Submit(Command, P4UserMap):

2117 #

2118 # Apply the patch for real, and do add/delete/+x handling.

2119 #

2125 - system(applyPatchCmd)

2120 + system(applyPatchCmd, shell=True)

2121

2122 for f in filesToChangeType:

2123 p4_edit(f, "-t", "auto")

@@ -3446,7 +3441,7 @@ class P4Sync(Command, P4UserMap):

3441 else:

3442 command = "branches"

3443

3449 - for info in p4CmdList(command):

3444 + for info in p4CmdList(command, shell=True):

3445 details = p4Cmd(["branch", "-o", info["branch"]])

3446 viewIdx = 0

3447 while "View%s" % viewIdx in details:

@@ -3537,7 +3532,9 @@ class P4Sync(Command, P4UserMap):

3532 while True:

3533 if self.verbose:

3534 print("trying: earliest %s latest %s" % (earliestCommit, latestCommit))

3540 - next = read_pipe("git rev-list --bisect %s %s" % (latestCommit, earliestCommit)).strip()

3535 + next = read_pipe(

3536 + "git rev-list --bisect %s %s" % (latestCommit, earliestCommit),

3537 + shell=True).strip()

3538 if len(next) == 0:

3539 if self.verbose:

3540 print("argh")

@@ -3692,7 +3689,7 @@ class P4Sync(Command, P4UserMap):

3689 if self.hasOrigin:

3690 if not self.silent:

3691 print('Syncing with origin first, using "git fetch origin"')

3695 - system("git fetch origin")

3692 + system("git fetch origin", shell=True)

3693

3694 def importHeadRevision(self, revision):

3695 print("Doing initial import of %s from revision %s into %s" % (' '.join(self.depotPaths), revision, self.branch))

@@ -3859,8 +3856,8 @@ class P4Sync(Command, P4UserMap):

3856 if len(self.branch) == 0:

3857 self.branch = self.refPrefix + "master"

3858 if gitBranchExists("refs/heads/p4") and self.importIntoRemotes:

3862 - system("git update-ref %s refs/heads/p4" % self.branch)

3863 - system("git branch -D p4")

3859 + system("git update-ref %s refs/heads/p4" % self.branch, shell=True)

3860 + system("git branch -D p4", shell=True)

3861

3862 # accept either the command-line option, or the configuration variable

3863 if self.useClientSpec:

@@ -4063,7 +4060,7 @@ class P4Sync(Command, P4UserMap):

4060 # Cleanup temporary branches created during import

4061 if self.tempBranches != []:

4062 for branch in self.tempBranches:

4066 - read_pipe("git update-ref -d %s" % branch)

4063 + read_pipe("git update-ref -d %s" % branch, shell=True)

4064 os.rmdir(os.path.join(os.environ.get("GIT_DIR", ".git"), self.tempBranchLocation))

4065

4066 # Create a symbolic ref p4/HEAD pointing to p4/<branch> to allow

@@ -4095,7 +4092,7 @@ class P4Rebase(Command):

4092 def rebase(self):

4093 if os.system("git update-index --refresh") != 0:

4094 die("Some files in your working directory are modified and different than what is in your index. You can use git update-index <filename> to bring the index up to date or stash away all your changes with git stash.");

4098 - if len(read_pipe("git diff-index HEAD --")) > 0:

4095 + if len(read_pipe("git diff-index HEAD --", shell=True)) > 0:

4096 die("You have uncommitted changes. Please commit them before rebasing or stash them away with git stash.");

4097

4098 [upstream, settings] = findUpstreamBranchPoint()

@@ -4106,9 +4103,10 @@ class P4Rebase(Command):

4103 upstream = re.sub("~[0-9]+$", "", upstream)

4104

4105 print("Rebasing the current branch onto %s" % upstream)

4109 - oldHead = read_pipe("git rev-parse HEAD").strip()

4110 - system("git rebase %s" % upstream)

4111 - system("git diff-tree --stat --summary -M %s HEAD --" % oldHead)

4106 + oldHead = read_pipe("git rev-parse HEAD", shell=True).strip()

4107 + system("git rebase %s" % upstream, shell=True)

4108 + system("git diff-tree --stat --summary -M %s HEAD --" % oldHead,

4109 + shell=True)

4110 return True

4111

4112 class P4Clone(P4Sync):

@@ -4185,7 +4183,7 @@ class P4Clone(P4Sync):

4183

4184 # auto-set this variable if invoked with --use-client-spec

4185 if self.useClientSpec_from_options:

4188 - system("git config --bool git-p4.useclientspec true")

4186 + system("git config --bool git-p4.useclientspec true", shell=True)

4187

4188 return True

4189

@@ -4319,7 +4317,7 @@ class P4Branches(Command):

4317 cmdline = "git rev-parse --symbolic "

4318 cmdline += " --remotes"

4319

4322 - for line in read_pipe_lines(cmdline):

4320 + for line in read_pipe_lines(cmdline, shell=True):

4321 line = line.strip()

4322

4323 if not line.startswith('p4/') or line == "p4/HEAD":

@@ -4404,9 +4402,11 @@ def main():

4402 cmd.gitdir = os.path.abspath(".git")

4403 if not isValidGitDir(cmd.gitdir):

4404 # "rev-parse --git-dir" without arguments will try $PWD/.git

4407 - cmd.gitdir = read_pipe("git rev-parse --git-dir").strip()

4405 + cmd.gitdir = read_pipe(

4406 + "git rev-parse --git-dir", shell=True).strip()

4407 if os.path.exists(cmd.gitdir):

4409 - cdup = read_pipe("git rev-parse --show-cdup").strip()

4408 + cdup = read_pipe(

4409 + "git rev-parse --show-cdup", shell=True).strip()

4410 if len(cdup) > 0:

4411 chdir(cdup);

4412