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