Johannes Schindelin [off-list ref] writes:
quoted
quoted
+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.
Hi Junio,
On Tue, 30 Aug 2016, Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:
quoted
quoted
quoted
+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