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

[Jakub Narębski]

Hello Johannes,

W dniu 01.09.2016 o 15:33, Johannes Schindelin pisze:
> On Wed, 31 Aug 2016, Jakub Narębski wrote:
>> W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:

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

After writing this email, I got an idea on how it could work:
git-rebase script calls some C helper, which outputs above, and
output of this helper is eval'ed by script (with gpg_sign_opt_quoted
variable present in the environment)...

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

... but it might have been simply copy'n'pasted from shell script
to C, literally.

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

Shouldn't, as l10ners need to deal with arg reordering, because in different
languages the order of words might be different: %s %s in English may be
%2$s %1$s in other language, see example in
  https://www.gnu.org/software/gettext/manual/gettext.html#c_002dformat-Flag

Best,
-- 
Jakub Narębski

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

[Johannes Schindelin]

Hi Junio,

On Wed, 31 Aug 2016, Junio C Hamano wrote:

> Jakub Narębski  writes:
> 
> >> +  else {
> >> +  opts->gpg_sign = buf.buf + 2;
> >> +  strbuf_detach(, NULL);
> >
> > Wouldn't we leak 2 characters that got skipped?  Maybe xstrdup would
> > be better (if it is leaked, and not reattached)?
> 
> An attempt to avoid leaking by calling free(opts->gpg_sign) would
> make it crash, which would be even worse ;-).

As I pointed out in a couple of replies yesterday: we cannot assume that
gpg_sign is free()able. That's the entire reason behind the
sequencer_entrust() dance.

Ciao,
Dscho

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

[Johannes Schindelin]

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(, 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();
> 
> 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(, 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

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

[Johannes Schindelin]

Hi Kuba,

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

> Note that if the last line was
> 
> + sequencer_entrust(opts, strbuf_detach(, 
> NULL));
> 
> we can make it not leak.  A bit tricksy, though.

Heh... I made it that tricksy, then.

Ciao,
Dscho

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

[Junio C Hamano]

Jakub Narębski  writes:

> Though xstrdup(buf.buf + 2) followed by strbuf_release() would
> make free(opts->gpg_sign) possible without crash.  That is we can
> work without *_entrust() mechanism at the cost of strdups.

Absolutely.

It is not like entrust() thing is free of allocation cost (it needs
to allocate an array of pointers to keep track of what to free) or
programmer's mental burden (you need to be careful what to entrust()
and what not to), so "at the cost of strdup(3)" is reasonable cost
of doing business in the way normal people expect the code to work.

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

[Jakub Narębski]

W dniu 31.08.2016 o 22:12, Junio C Hamano pisze:
> Jakub Narębski  writes:
>> Johannes Schindelin wrote:

>>> +   else {
>>> +   opts->gpg_sign = buf.buf + 2;
>>> +   strbuf_detach(, NULL);
>>
>> Wouldn't we leak 2 characters that got skipped?  Maybe xstrdup would
>> be better (if it is leaked, and not reattached)?
> 
> An attempt to avoid leaking by calling free(opts->gpg_sign) would
> make it crash, which would be even worse ;-).
 
Actually, from C standard:

"If ptr [in free(ptr)] does not point to a block of memory allocated
 with the above functions [malloc(), etc.], it causes undefined behavior."
  ^

Which is even worse if it does not lead to crash.


Note that if the last line was

+   sequencer_entrust(opts, strbuf_detach(, 
NULL));

we can make it not leak.  A bit tricksy, though.


Though xstrdup(buf.buf + 2) followed by strbuf_release() would
make free(opts->gpg_sign) possible without crash.  That is we can
work without *_entrust() mechanism at the cost of strdups.

-- 
Jakub Narębski

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

[Junio C Hamano]

Jakub Narębski  writes:

>> +else {
>> +opts->gpg_sign = buf.buf + 2;
>> +strbuf_detach(, NULL);
>
> Wouldn't we leak 2 characters that got skipped?  Maybe xstrdup would
> be better (if it is leaked, and not reattached)?

An attempt to avoid leaking by calling free(opts->gpg_sign) would
make it crash, which would be even worse ;-).

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

[Jakub Narębski]

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

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

>  
>  /* 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();
> + if (opts->gpg_sign)
> + sq_quotef(, "-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().

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

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

>   }
>  
>   argv_array_init();
> @@ -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(, 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();

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

> + else {
> + opts->gpg_sign = buf.buf