Re: [PATCH 16/22] sequencer: prepare for rebase -i's GPG settings

From: Jakub Narębski <jnareb@gmail.com>
Date: Aug 31, 2016, 20:10 UTC
Message-ID: <104de15c-77e8-9474-9579-4934ca8a4323@gmail.com>
In-Reply-To: <c6a37af958f59390c00429a5b38b34d34c9a5b95.1472457609.git.johannes.schindelin@gmx.de>

Hello Johannes,

W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:

> The rebase command sports a `--gpg-sign` option that is heeded by the
> interactive rebase.

Should it be "sports" or "supports"?

Show 29 quoted lines

> 
> This patch teaches the sequencer that trick, as part of the bigger
> effort to make the sequencer the work horse of the interactive rebase.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 48 +++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 43 insertions(+), 5 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 4204cc8..e094ac2 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -15,6 +15,7 @@
>  #include "merge-recursive.h"
>  #include "refs.h"
>  #include "argv-array.h"
> +#include "quote.h"
>  
>  #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>  
> @@ -33,6 +34,11 @@ static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
>   * being rebased.
>   */
>  static GIT_PATH_FUNC(rebase_path_author_script, "rebase-merge/author-script")
> +/*
> + * The following files are written by git-rebase just after parsing the
> + * command-line (and are only consumed, not modified, by the sequencer).
> + */

It is good to have this comment here.

> +static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")

I know it is not your fault, but I wonder why this file uses snake_case_name, while all other use kebab-case-names. That is, why it is gpg_sign_opt and not gpg-sign-opt.

Show 16 quoted lines

>  
>  /* We will introduce the 'interactive rebase' mode later */
>  #define IS_REBASE_I() 0
> @@ -129,6 +135,16 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
>  	return 1;
>  }
>  
> +static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
> +{
> +	static struct strbuf buf = STRBUF_INIT;
> +
> +	strbuf_reset(&buf);
> +	if (opts->gpg_sign)
> +		sq_quotef(&buf, "-S%s", opts->gpg_sign);
> +	return buf.buf;
> +}

All right, this function is quite clear.

Sidenote: it's a pity api-quote.txt is just a placeholder for proper
documentation (including sq_quotef()).  I also wonder why it is not
named sq_quotef_buf() or strbuf_addf_sq().

Show 16 quoted lines

> +
>  void *sequencer_entrust(struct replay_opts *opts, void *set_me_free_after_use)
>  {
>  	ALLOC_GROW(opts->owned, opts->owned_nr + 1, opts->owned_alloc);
> @@ -471,17 +487,20 @@ int sequencer_commit(const char *defmsg, struct replay_opts *opts,
>  
>  	if (IS_REBASE_I()) {
>  		env = read_author_script();
> -		if (!env)
> +		if (!env) {
> +			const char *gpg_opt = gpg_sign_opt_quoted(opts);
> +
>  			return error("You have staged changes in your working "
>  				"tree. If these changes are meant to be\n"
>  				"squashed into the previous commit, run:\n\n"
> -				"  git commit --amend $gpg_sign_opt_quoted\n\n"

How did this get expanded by error(), and why we want to replace it if it works?

Show 9 quoted lines

> +				"  git commit --amend %s\n\n"
>  				"If they are meant to go into a new commit, "
>  				"run:\n\n"
> -				"  git commit $gpg_sign_opt_quoted\n\n"
> +				"  git commit %s\n\n"
>  				"In both case, once you're done, continue "
>  				"with:\n\n"
> -				"  git rebase --continue\n");
> +				"  git rebase --continue\n", gpg_opt, gpg_opt);

Instead of passing option twice, why not make use of %1$s (arg reordering), that is

  +				"  git commit --amend %1$s\n\n"
[...]
  +				"  git commit %1$s\n\n"

> +		}

So shell quoting is required only for error output.

Show 18 quoted lines

>  	}
>  
>  	argv_array_init(&array);
> @@ -955,8 +974,27 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
>  
>  static int read_populate_opts(struct replay_opts *opts)
>  {
> -	if (IS_REBASE_I())
> +	if (IS_REBASE_I()) {
> +		struct strbuf buf = STRBUF_INIT;
> +
> +		if (read_oneliner(&buf, rebase_path_gpg_sign_opt(), 1)) {
> +			if (buf.len && buf.buf[buf.len - 1] == '\n') {
> +				if (--buf.len &&
> +				    buf.buf[buf.len - 1] == '\r')
> +					buf.len--;
> +				buf.buf[buf.len] = '\0';
> +			}

Isn't there some strbuf_chomp() / strbuf_strip_eof() function? Though as strbuf_getline() uses something similar...

> +
> +			if (!starts_with(buf.buf, "-S"))
> +				strbuf_reset(&buf);

Should we signal that there was problem with a file contents?

> +			else {
> +				opts->gpg_sign = buf.buf + 2;
> +				strbuf_detach(&buf, NULL);

Wouldn't we leak 2 characters that got skipped? Maybe xstrdup would be better (if it is leaked, and not reattached)?

Show 9 quoted lines

> +			}
> +		}
> +
>  		return 0;
> +	}
>  
>  	if (!file_exists(git_path_opts_file()))
>  		return 0;
>

-- 
Jakub Narębski