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

Hi Kuba,

On Wed, 31 Aug 2016, Jakub Narębski wrote:

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

quoted

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

Should it be "sports" or "supports"?

Funny. I got a PR last week that wanted to fix a similar expression.

I really meant "to sport", as in "To display; to have as a notable
feature.". See https://en.wiktionary.org/wiki/sport#Verb

quoted

+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.

Yes, you are correct: it is not my fault ;-)

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().

Heh. I did not even bother to check the documentation, it is my long-time
habit to dive right into the code.

quoted

@@ -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?

It did not work. It was a place-holder waiting for this patch ;-)

quoted

+				"  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"

Cute. But would this not drive the l10ners insane?

So shell quoting is required only for error output.

Indeed.

quoted

@@ -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...

Even worse. read_oneliner() *already* does that. I just forgot to delete
this code when I introduced and used read_oneliner().

Thanks.

quoted

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

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

Maybe. But probably not: this file is written by git-rebase itself. I
merely safe-guarded against empty files here.

quoted

+			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)?

We do not leak anything because I changed the code locally already to use
sequencer_entrust() (I guess in response to an earlier of your comments).

Ciao,
Dscho