Hi Kuba, On Wed, 31 Aug 2016, Jakub Narębski wrote:
> 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"? 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 > > +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. > > @@ -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 ;-) > > > + " 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. > > @@ -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. > > + 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. > > + 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
