Re: [PATCH 14/22] sequencer: prepare for rebase -i's commit functionality

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 30, 2016, 18:18 UTC
Message-ID: <alpine.DEB.2.20.1608302005440.129229@virtualbox>
In-Reply-To: <xmqqfupm5fix.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Tue, 30 Aug 2016, Junio C Hamano wrote:

Show 41 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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