Re: [PATCH v13] [GSOC] commit: add --trailer option

Christian Couder [off-list ref] writes:

If you want nothing to happen when $ARG isn't set, you can change the
config option to something like:

$ git config trailer.sign.command "NAME='\$ARG'; test -n \"\$NAME\" &&
git log --author=\"\$NAME\" -1 --format='format:%aN <%aE>' || true"

(This is because it looks like $ARG is replaced only once with the
actual value, which is perhaps a bug. Otherwise something like the
following might work:

I do not know the origin of that code in trailers.c but it feels
quite confused and error prone to use textual replacement with
strbuf_replace().  Why doesn't the code, which knows it will use
shell to execute the command line given by the end user in the
configuration, to just export ARG as an environment variable and
be done with it?  It would also avoid quoting problem etc.

On Tue, Mar 23, 2021 at 7:19 AM Junio C Hamano [off-list ref] wrote:

Christian Couder [off-list ref] writes:

quoted

If you want nothing to happen when $ARG isn't set, you can change the
config option to something like:

$ git config trailer.sign.command "NAME='\$ARG'; test -n \"\$NAME\" &&
git log --author=\"\$NAME\" -1 --format='format:%aN <%aE>' || true"

(This is because it looks like $ARG is replaced only once with the
actual value, which is perhaps a bug. Otherwise something like the
following might work:

I do not know the origin of that code in trailers.c but it feels
quite confused and error prone to use textual replacement with
strbuf_replace().  Why doesn't the code, which knows it will use
shell to execute the command line given by the end user in the
configuration, to just export ARG as an environment variable and
be done with it?  It would also avoid quoting problem etc.

Yeah, I agree that would be better.

Junio C Hamano [off-list ref] 于2021年3月23日周二 下午2:19写道：

Christian Couder [off-list ref] writes:

quoted

If you want nothing to happen when $ARG isn't set, you can change the
config option to something like:

$ git config trailer.sign.command "NAME='\$ARG'; test -n \"\$NAME\" &&
git log --author=\"\$NAME\" -1 --format='format:%aN <%aE>' || true"

(This is because it looks like $ARG is replaced only once with the
actual value, which is perhaps a bug. Otherwise something like the
following might work:

I do not know the origin of that code in trailers.c but it feels
quite confused and error prone to use textual replacement with
strbuf_replace().  Why doesn't the code, which knows it will use
shell to execute the command line given by the end user in the
configuration, to just export ARG as an environment variable and
be done with it?  It would also avoid quoting problem etc.

Maybe like this?

-#define TRAILER_ARG_STRING "$ARG"
-
static const char *git_generated_prefixes[] = {
       "Signed-off-by: ",
       "(cherry picked from commit ",

@@ -222,13 +220,17 @@ 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);
-       if (arg)
-               strbuf_replace(&cmd, TRAILER_ARG_STRING, arg);
+       for (var = local_repo_env; *var; var++)
+               strvec_push(&cp.env_array, *var);
+       if (arg) {
+               strvec_pushf(&cp.env_array, "ARG=%s", arg);
+       }

       strvec_push(&cp.args, cmd.buf);
-       cp.env = local_repo_env;
+
       cp.no_stdin = 1;
       cp.use_shell = 1;

On Tue, Mar 23, 2021 at 11:35 AM ZheNing Hu [off-list ref] wrote:

Junio C Hamano [off-list ref] 于2021年3月23日周二 下午2:19写道：

Maybe like this?

Yeah, it's a good idea to work on this, but please make it an
independent patch unrelated to adding --trailer to `git commit`.

Also the documentation might need a bit of tweeking to tell that ARG
is now an exported environment variable.

-#define TRAILER_ARG_STRING "$ARG"

It might still be better to use a #define or a const char [] to avoid
hard coding "ARG" below.

You can drop the "{" and "}" above.

Thanks!