Re: [PATCH] git-p4: avoid shell interpretation of commit ids in applyCommit

[Junio C Hamano <gitster@pobox.com>]

"Anupam Mediratta via GitGitGadget" <gitgitgadget@gmail.com> writes:

> @@ -2279,7 +2290,7 @@ class P4Submit(Command, P4UserMap):
>  
>              if fixed_rcs_keywords:
>                  print("Retrying the patch with RCS keywords cleaned up")
> -                if os.system(tryPatchCmd) == 0:
> +                if diffTreeApply(id, tryPatchArgs) == 0:
>                      patch_succeeded = True
>                      print("Patch succeesed this time with RCS keywords cleaned")

Both of these check the result of running diff|apply pipeline and
react to a failure.

> @@ -2291,7 +2302,7 @@ class P4Submit(Command, P4UserMap):
>          #
>          # Apply the patch for real, and do add/delete/+x handling.
>          #
> -        system(applyPatchCmd, shell=True)
> +        diffTreeApply(id, applyPatchArgs)

It is a bit hard to discover, but the original code catches a failed
"diff|apply" pipeline invocation, because the "system()" used here
is what git-p4.py defines for itself.  When the pipeline fails, this
system() raises subprocess.CalledProcessError().

The new one ignores the exit status from the pipeline, so even after
a failure to apply the change, the program continues.

Which may not be what you want to see.

>  
>          for f in filesToChangeType:
>              p4_edit(f, "-t", "auto")
> diff --git a/t/t9803-git-p4-shell-metachars.sh b/t/t9803-git-p4-shell-metachars.sh
> index 2913277013..ef8fd6e094 100755
> --- a/t/t9803-git-p4-shell-metachars.sh
> +++ b/t/t9803-git-p4-shell-metachars.sh
> @@ -105,4 +105,20 @@ test_expect_success 'branch with shell char' '
>  	)
>  '
>  
> +test_expect_success 'git p4 submit --commit does not execute shell metachars in commit id' '
> +	git p4 clone --dest="$git" //depot &&
> +	test_when_finished cleanup_git &&
> +	(
> +		cd "$git" &&
> +		git config git-p4.skipSubmitEditCheck true &&
> +		echo f3 >file3 &&
> +		git add file3 &&
> +		git commit -m "add file3" &&
> +		name='"'"'$(touch${IFS}injection-marker)'"'"' &&
> +		git branch "$name" HEAD &&
> +		P4EDITOR="test-tool chmtime +5" git p4 submit --commit "$name"
> +	) &&
> +	test_path_is_missing "$cli/injection-marker"
> +'
> +
>  test_done
>
> base-commit: d38352cd43ab9745686d697872408bc3249a153f