Hi Junio,
On Tue, 11 Oct 2016, Junio C Hamano wrote:
> > @@ -370,19 +383,79 @@ static int is_index_unchanged(void)
> > }
> >
> > /*
> > + * Read the author-script file into an environment block, ready for use in
> > + * run_command(), that can be free()d afterwards.
> > + */
> > +static char **read_author_script(void)
> > +{
> > + struct strbuf script = STRBUF_INIT;
> > + int i, count = 0;
> > + char *p, *p2, **env;
> > + size_t env_size;
> > +
> > + if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
> > + return NULL;
> > +
> > + for (p = script.buf; *p; p++)
> > + if (skip_prefix(p, "'\\\\''", (const char **)&p2))
> > + strbuf_splice(&script, p - script.buf, p2 - p, "'", 1);
> > + else if (*p == '\'')
> > + strbuf_splice(&script, p-- - script.buf, 1, "", 0);
> > + else if (*p == '\n') {
> > + *p = '\0';
> > + count++;
> > + }
>
> Hmph, didn't we recently add parse_key_value_squoted() to build
> read_author_script() in builtin/am.c on top of it, so that this
> piece of code can also take advantage of and share the parser?
I already pointed out that the author-script file may *not* be quoted.
sq_dequote() would return NULL and parse_key_value_squoted() would *fail*.
To complicate things further, the sequencer does not even need to access
the values at all. It needs to pass them to run_command() as an
environment block, which means that we would have to reconstruct the lines
after parse_key_value_squoted() painstakingly untangled the key names from
the values.
In short, this is another instance where using a function just because it
exists and is nominally related would make the resulting patch *more*
complicated than it currently is.
> > +/*
>
> Offtopic: this line and the beginning of the new comment block that
> begins with "Read the author-script" above show a suboptimal marking
> of what is added and what is left. I wonder "diff-indent-heuristic"
> topic by Michael can help to make it look better.
Maybe. I'll try to look into that once the more serious questions about
this patch series have been addressed.
Ciao,
Dscho