In https://lore.kernel.org/git/xmqqv99i4ck2.fsf@gitster.g/ Junio and Christian talked about the problem of using strbuf_replace() to replace $ARG: 1. if the user's script has more than one $ARG, only the first one will be replaced, which is incorrected. 2. $ARG is textually replaced without shell syntax, which may result a broken command when $ARG include some unmatching single quote, very unsafe. Now pass trailer value as $1 to the trailer command with another trailer.<token>.cmd config, to solve these above problems. We are now writing documents that are more readable and correct than before. ZheNing Hu (2): [GSOC] docs: correct description of .command [GSOC] trailer: add new .cmd config option Documentation/git-interpret-trailers.txt | 111 ++++++++++++++++++--- t/t7513-interpret-trailers.sh | 122 +++++++++++++++++++++++ trailer.c | 79 ++++++++++++--- 3 files changed, 280 insertions(+), 32 deletions(-) base-commit: 142430338477d9d1bb25be66267225fb58498d92 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-913%2Fadlternative%2Ftrailer-pass-ARG-env-v11 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-913/adlternative/trailer-pass-ARG-env-v11 Pull-Request: https://github.com/gitgitgadget/git/pull/913 Range-diff vs v10: 1: 8129ef6c476b ! 1: 34210e5bd3da [GSOC] docs: correct descript of trailer.<token>.command @@ Metadata Author: ZheNing Hu <adlternative@xxxxxxxxx> ## Commit message ## - [GSOC] docs: correct descript of trailer.<token>.command + [GSOC] docs: correct description of .command In the original documentation of `trailer.<token>.command`, some descriptions are easily misunderstood. So let's modify 2: daa889bd0ade ! 2: 9c0fc91aba24 [GSOC] trailer: add new .cmd config option @@ Commit message replaced with the value given to the `interpret-trailer` command for the token in a '--trailer <token>=<value>' argument. - This has three downsides: + This has two downsides: * The use of $ARG in the mechanism misleads the users that the value is passed in the shell variable, and tempt them @@ Commit message a broken command that is not syntactically correct (or worse). - * The first occurrence of substring `$ARG` will be replaced - with the empty string, in the command when the command is - first called to add a trailer with the specified <token>. - This is a bad design, the nature of automatic execution - causes it to add a trailer that we don't expect. - Introduce a new `trailer.<token>.cmd` configuration that takes higher precedence to deprecate and eventually remove `trailer.<token>.command`, which passes the value as an @@ Commit message refer to the value as positional argument, $1, in their scripts. At the same time, in order to allow `git interpret-trailers` to better simulate the behavior - of `git command -s`, 'trailer.<token>.cmd' will not - automatically execute. + of `git command -s`, the first implicitly executed command + will not pass positional parameters, users can use this + feature to suppress its output. Helped-by: Junio C Hamano <gitster@xxxxxxxxx> Helped-by: Christian Couder <christian.couder@xxxxxxxxx> @@ Documentation/git-interpret-trailers.txt: leading and trailing whitespace trimme -occurrence of substring `$ARG` in the command. This way the -command can produce a <value> computed from the <value> passed -in the '--trailer <token>=<value>' argument. --+ ++of these arguments, if any, will be passed to the command as its ++first argument. This way the command can produce a <value> computed ++from the <value> passed in the '--trailer <token>=<value>' argument. + + -For consistency, the first occurrence of substring `$ARG` is -also replaced, this time with the empty string, in the command -when the command is first called to add a trailer with the -specified <token>. -+of these arguments, if any, will be passed to the command as its -+first argument. This way the command can produce a <value> computed -+from the <value> passed in the '--trailer <token>=<value>' argument. ++It is worth mentioning that the command is first called to add a ++trailer with the specified <token> and without positional argument. ++Users can make use of this output when they need automatically add ++some trailers. On the other hand, users can use a trick to suppress ++this output by judging whether the number of positional parameters ++is equal to one, if it is true, execute the commands, otherwise exit ++with non-zero to suppress the output. EXAMPLES -------- @@ Documentation/git-interpret-trailers.txt: subject +------------ +$ cat ~/bin/gcount +#!/bin/sh -+test -n "$1" && git shortlog -s --author="$1" HEAD || true ++if test "$#" != 1 ++then ++ exit 1 ++else ++ test -n "$1" && git shortlog -s --author="$1" HEAD || true ++fi +$ git config trailer.cnt.key "Commit-count: " +$ git config trailer.cnt.ifExists "addIfDifferentNeighbor" +$ git config trailer.cnt.cmd "~/bin/gcount" +$ git interpret-trailers --trailer="cnt:Junio" --trailer="cnt:Linus Torvalds"<<EOF +> subject -+> ++> +> message -+> ++> +> EOF +subject + @@ Documentation/git-interpret-trailers.txt: subject +------------ +$ cat ~/bin/glog-grep +#!/bin/sh -+test -n "$1" && git log --grep "$1" --pretty=reference -1 || true ++if test "$#" != 1 ++then ++ exit 1 ++else ++ test -n "$1" && git log --grep "$1" --pretty=reference -1 || true ++fi +$ git config trailer.ref.key "Reference-to: " +$ git config trailer.ref.ifExists "replace" +$ git config trailer.ref.cmd "~/bin/glog-grep" +$ git interpret-trailers --trailer="ref:Add copyright notices." <<EOF +> subject -+> ++> +> message -+> ++> +> EOF +subject + @@ t/t7513-interpret-trailers.sh: test_expect_success 'setup' ' + git config trailer.bug.cmd "echo \"maybe is\"" && + cat >expected2 <<-EOF && + ++ Bug-maker: maybe is + Bug-maker: maybe is him + Bug-maker: maybe is me + EOF @@ t/t7513-interpret-trailers.sh: test_expect_success 'setup' ' + git config trailer.bug.cmd "echo \"\$1\" is" && + cat >expected2 <<-EOF && + ++ Bug-maker: is + Bug-maker: him is him + Bug-maker: me is me + EOF @@ t/t7513-interpret-trailers.sh: test_expect_success 'setup' ' + EOF + cat >echoscript <<-EOF && + #!/bin/sh -+ echo who is "\$1" ++ if test "\$#" != 1 ++ then ++ exit 1 ++ else ++ echo who is "\$1" ++ fi + EOF + chmod +x echoscript && + git interpret-trailers --trailer "bug: him" --trailer "bug:me" \ + >actual2 && + test_cmp expected2 actual2 +' ++ ++test_expect_success 'with cmd, $1 and without --trailer' ' ++ test_when_finished "git config --remove-section trailer.bug" && ++ test_when_finished "git config --remove-section trailer.gub" && ++ git config trailer.bug.key "Bug-maker: " && ++ git config trailer.bug.ifExists "replace" && ++ git config trailer.bug.cmd "./echoscript" && ++ git config trailer.gub.key "Gub-maker: " && ++ git config trailer.gub.ifExists "replace" && ++ git config trailer.gub.cmd "./echoscript2" && ++ cat >expected2 <<-EOF && ++ ++ Gub-maker: si ohw ++ EOF ++ cat >echoscript <<-EOF && ++ #!/bin/sh ++ if test "\$#" != 1 ++ then ++ exit 1 ++ else ++ echo who is "\$1" ++ fi ++ EOF ++ cat >echoscript2 <<-EOF && ++ echo si ohw "\$1" ++ EOF ++ chmod +x echoscript && ++ chmod +x echoscript2 && ++ git interpret-trailers >actual2 && ++ test_cmp expected2 actual2 ++' + test_expect_success 'without config' ' sed -e "s/ Z\$/ /" >expected <<-\EOF && @@ trailer.c: static void free_arg_item(struct arg_item *item) free(item->conf.command); + free(item->conf.cmd); free(item->token); - free(item->value); +- free(item->value); ++ if (item->value) ++ FREE_AND_NULL(item->value); free(item); + } + @@ trailer.c: static int check_if_different(struct trailer_item *in_tok, return 1; } @@ trailer.c: static int check_if_different(struct trailer_item *in_tok, cp.env = local_repo_env; cp.no_stdin = 1; cp.use_shell = 1; + + if (capture_command(&cp, &buf, 1024)) { +- error(_("running trailer command '%s' failed"), cmd.buf); + strbuf_release(&buf); +- result = xstrdup(""); ++ if (!conf->cmd || arg) { ++ error(_("running trailer command '%s' failed"), cmd.buf); ++ result = xstrdup(""); ++ } else ++ result = NULL; + } else { + strbuf_trim(&buf); + result = strbuf_detach(&buf, NULL); @@ trailer.c: static char *apply_command(const char *command, const char *arg) static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg_tok) { - if (arg_tok->conf.command) { +- const char *arg; +- if (arg_tok->value && arg_tok->value[0]) { + if (arg_tok->conf.command || arg_tok->conf.cmd) { - const char *arg; - if (arg_tok->value && arg_tok->value[0]) { ++ const char *arg = NULL; ++ ++ if ((arg_tok->value && arg_tok->value[0]) || ++ (arg_tok->conf.cmd && !arg_tok->value)) { arg = arg_tok->value; + } else { + if (in_tok && in_tok->value) @@ trailer.c: static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg else arg = xstrdup(""); } - arg_tok->value = apply_command(arg_tok->conf.command, arg); +- free((char *)arg); + arg_tok->value = apply_command(&arg_tok->conf, arg); - free((char *)arg); ++ if (arg) ++ free((char *)arg); } } + +@@ trailer.c: static void apply_arg_if_exists(struct trailer_item *in_tok, + break; + case EXISTS_REPLACE: + apply_item_command(in_tok, arg_tok); ++ if (!arg_tok->value) { ++ free_arg_item(arg_tok); ++ return; ++ } + add_arg_to_input_list(on_tok, arg_tok); + list_del(&in_tok->list); + free_trailer_item(in_tok); + break; + case EXISTS_ADD: + apply_item_command(in_tok, arg_tok); ++ if (!arg_tok->value) { ++ free_arg_item(arg_tok); ++ return; ++ } + add_arg_to_input_list(on_tok, arg_tok); + break; + case EXISTS_ADD_IF_DIFFERENT: + apply_item_command(in_tok, arg_tok); ++ if (!arg_tok->value) { ++ free_arg_item(arg_tok); ++ return; ++ } + if (check_if_different(in_tok, arg_tok, 1, head)) + add_arg_to_input_list(on_tok, arg_tok); + else +@@ trailer.c: static void apply_arg_if_exists(struct trailer_item *in_tok, + break; + case EXISTS_ADD_IF_DIFFERENT_NEIGHBOR: + apply_item_command(in_tok, arg_tok); ++ if (!arg_tok->value) { ++ free_arg_item(arg_tok); ++ return; ++ } + if (check_if_different(on_tok, arg_tok, 0, head)) + add_arg_to_input_list(on_tok, arg_tok); + else +@@ trailer.c: static void apply_arg_if_missing(struct list_head *head, + case MISSING_ADD: + where = arg_tok->conf.where; + apply_item_command(NULL, arg_tok); ++ if (!arg_tok->value) { ++ free_arg_item(arg_tok); ++ return; ++ } + to_add = trailer_from_arg(arg_tok); + if (after_or_end(where)) + list_add_tail(&to_add->list, head); @@ trailer.c: static void duplicate_conf(struct conf_info *dst, const struct conf_info *src) dst->name = xstrdup_or_null(src->name); dst->key = xstrdup_or_null(src->key); @@ trailer.c: static int git_trailer_config(const char *conf_key, const char *value case TRAILER_WHERE: if (trailer_set_where(&conf->where, value)) warning(_("unknown value '%s' for key '%s'"), value, conf_key); +@@ trailer.c: static void process_command_line_args(struct list_head *arg_head, + xstrdup(token_from_item(item, NULL)), + xstrdup(""), + &item->conf, NULL); ++ else if (item->conf.cmd) ++ add_arg_item(arg_head, ++ xstrdup(token_from_item(item, NULL)), ++ NULL, ++ &item->conf, NULL); + } + + /* Add an arg item for each trailer on the command line */ -- gitgitgadget
