Hi Junio,
On Tue, 30 Aug 2016, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:
>
> >> > +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. What is this loop doing? Is it decoding a sq-quoted buffer
> >> or something? Don't we have a helper function to do that?
> >
> > Well, it is not just decoding an sq-quoted buffer, but several lines with
> > definitions we sq-quoted ourselves, individually.
> >
> > The quote.[ch] code currently has no code to dequote lines individually.
>
> There is a function with exactly the same name in builtin/am.c and I
> assume that it is reading from a file with the same format, which
> uses a helper to read one variable line at a time. Hopefully that
> can be refactored so that more parsing is shared between the two
> users.
>
> Two functions with the same name reading from the same format, even
> when they expect to produce the same result in different internal
> format, without even being aware of each other is a bad enough
> "regression" in maintainability of the code. One of them not even
> using sq_dequote() helper and reinventing is even worse.
First of all, builtin/am's read_author_script() really, really, really
only wants to read stuff into the am_state struct.
That alone is already so incompatible that I do not think it can be
repaired.
Further, builtin/am's read_author_script() reads *only* the
GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL and GIT_AUTHOR_DATE lines (opening the
file three times for the task). And then it *requires* the corresponding
values to be sq-quoted.
I do not have a use case for this myself, but rebase -i explicitly eval's
the author script, so it is conceivable that some enterprisey user makes
use of this feature to set other environment variables. The thing is that
rebase -i cannot necessarily expect all of those files in its state
directory to be under tight control -- in stark contrast to git-am.
I could imagine that this could be fixed by abstracting the functionality
more, and use your favored sq_dequote() (which may actually fail in case
of an enterprisey usage as outlined above), and adapting builtin/am's
read_author_script() to make use of that refactored approach.
This is a huge task, make no mistake, in particular because we need to
ensure compatibility with non-standard usage. So I do not think I can
tackle that any time soon. Certainly not as part of an iteration of this
here patch series.
Ciao,
Dscho