Re: [PATCH 16/22] sequencer: prepare for rebase -i's GPG settings
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Sep 1, 2016, 13:33 UTC
- Message-ID
- <alpine.DEB.2.20.1609011516080.129229@virtualbox>
- In-Reply-To
- <104de15c-77e8-9474-9579-4934ca8a4323@gmail.com>
Hi Kuba,
On Wed, 31 Aug 2016, Jakub Narębski wrote:
Show 6 quoted lines
> 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
Show 5 quoted lines
> > +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.
Show 15 quoted lines
> > @@ -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 ;-)
Show 17 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"
Cute. But would this not drive the l10ners insane?
> So shell quoting is required only for error output.
Indeed.
Show 18 quoted lines
> > @@ -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.
Show 6 quoted lines
> > + 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