[GSOC] trailer: change $ARG to environment variable

Commit Message

ZheNing Hu March 23, 2021, 2:53 p.m. UTC

From: ZheNing Hu <[e-mail]>

In the original implementation of `trailer.<token>.command`,
use `strbuf_replace` to replace `$ARG` in the <value> of the trailer,
but it have a problem:`strbuf_replace` replace the `$ARG` in command
only once, If the user's trailer command have used more then one `$ARG`,
error will occur.

So pass `$ARG` as an environment variable to the trailer command,
all `$ARG` in the command will be replaced, which will fix this problem.

Signed-off-by: ZheNing Hu <[e-mail]>
---
    [GSOC] trailer: change $ARG to environment variable
    
    In https://lore.kernel.org/git/xmqqv99i4ck2.fsf@gitster.g/ Junio and
    Christian talked about the problem of using strbuf_replace() to replace
    $ARG.
    
    The current new solution is to pass $ARG as an environment variable into
    the command.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-913%2Fadlternative%2Ftrailer-pass-ARG-env-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-913/adlternative/trailer-pass-ARG-env-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/913

 Documentation/git-interpret-trailers.txt |  3 ++-
 t/t7513-interpret-trailers.sh            | 17 +++++++++++++++++
 trailer.c                                |  9 ++++++---
 3 files changed, 25 insertions(+), 4 deletions(-)


base-commit: 142430338477d9d1bb25be66267225fb58498d92

Patch

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 96ec6499f001..7cf7c032a0e9 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -242,7 +242,8 @@  line, where <value> is taken to be the standard output of the
 specified command with any leading and trailing whitespace trimmed
 off.
 +
-If the command contains the `$ARG` string, this string will be
+If the command contains the `$ARG` string (`$ARG` is an exported
+environment variable), this string will be
 replaced with the <value> part of an existing trailer with the same
 <token>, if any, before the command is launched.
 +
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 6602790b5f4c..d303cf0e4b36 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -1291,6 +1291,23 @@  test_expect_success 'with command using $ARG' '
 	test_cmp expected actual
 '
 
+test_expect_success 'with command using more than one $ARG' '
+	git config trailer.fix.ifExists "replace" &&
+	git config trailer.fix.command "test -n $ARG && git log -1 --oneline --format=\"%h (%s)\" --abbrev-commit --abbrev=14 \$ARG || true" &&
+	FIXED=$(git log -1 --oneline --format="%h (%s)" --abbrev-commit --abbrev=14 HEAD) &&
+	cat complex_message_body >expected &&
+	sed -e "s/ Z\$/ /" >>expected <<-EOF &&
+		Fixes: $FIXED
+		Acked-by= Z
+		Reviewed-by:
+		Signed-off-by: Z
+		Signed-off-by: A U Thor <[e-mail]>
+	EOF
+	git interpret-trailers --trailer "review:" --trailer "fix=HEAD" \
+		<complex_message >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'with failing command using $ARG' '
 	git config trailer.fix.ifExists "replace" &&
 	git config trailer.fix.command "false \$ARG" &&
diff --git a/trailer.c b/trailer.c
index be4e9726421c..42e3b818327a 100644
--- a/trailer.c
+++ b/trailer.c
@@ -44,7 +44,7 @@  static char *separators = ":";
 
 static int configured;
 
-#define TRAILER_ARG_STRING "$ARG"
+#define TRAILER_ARG_STRING "ARG"
 
 static const char *git_generated_prefixes[] = {
 	"Signed-off-by: ",
@@ -222,13 +222,16 @@  static char *apply_command(const char *command, const char *arg)
 	struct strbuf buf = STRBUF_INIT;
 	struct child_process cp = CHILD_PROCESS_INIT;
 	char *result;
+	const char *const *var;
 
 	strbuf_addstr(&cmd, command);
+	for (var = local_repo_env; *var; var++)
+		strvec_push(&cp.env_array, *var);
 	if (arg)
-		strbuf_replace(&cmd, TRAILER_ARG_STRING, arg);
+		strvec_pushf(&cp.env_array, "%s=%s", TRAILER_ARG_STRING, arg);
 
 	strvec_push(&cp.args, cmd.buf);
-	cp.env = local_repo_env;
+
 	cp.no_stdin = 1;
 	cp.use_shell = 1;