Re: [PATCH 16/22] sequencer: prepare for rebase -i's GPG settings
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
Hi Junio, On Wed, 31 Aug 2016, Junio C Hamano wrote: > Jakub Narębski writes: > > >> + 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)? > > 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
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
Re: [PATCH 16/22] sequencer: prepare for rebase -i's GPG settings
Hi Kuba, On Wed, 31 Aug 2016, Jakub Narębski wrote: > Note that if the last line was > > + sequencer_entrust(opts, strbuf_detach(&buf, > 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
Jakub Narębski writes: > Though xstrdup(buf.buf + 2) followed by strbuf_release(&buf) 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
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(&buf, 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(&buf, NULL)); we can make it not leak. A bit tricksy, though. Though xstrdup(buf.buf + 2) followed by strbuf_release(&buf) 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
Jakub Narębski writes: >> +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)? 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
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(&buf); > + if (opts->gpg_sign) > + sq_quotef(&buf, "-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(&array); > @@ -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... > + > + if (!starts_with(buf.buf, "-S")) > + strbuf_reset(&buf); Should we signal that there was problem with a file contents? > + else { > + opts->gpg_sign = buf.buf + 2; >
[PATCH 16/22] sequencer: prepare for rebase -i's GPG settings
The rebase command sports a `--gpg-sign` option that is heeded by the interactive rebase. 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). + */ +static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/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(&buf); + if (opts->gpg_sign) + sq_quotef(&buf, "-S%s", opts->gpg_sign); + return buf.buf; +} + 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" + " 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); + } } argv_array_init(&array); @@ -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'; + } + + if (!starts_with(buf.buf, "-S")) + strbuf_reset(&buf); + else { + opts->gpg_sign = buf.buf + 2; + strbuf_detach(&buf, NULL); + } + } + return 0; + } if (!file_exists(git_path_opts_file())) return 0; -- 2.10.0.rc1.114.g2bd6b38