Re: [PATCH v3 1/2] sequencer: handle errors in read_author_ident()

[Junio C Hamano]

Eric Sunshine  writes:

> I think this patch can be simplified considerably by shifting one's
> perspective. If we admit that read_author_ident() is already correctly
> reporting an error by returning NULL (which is exactly what it is
> doing), then the bug is is purely on the calling side; namely, the
> caller is ignoring the error. (In fact, your commit message already
> states this.)

This approach looks quite sensible.

Re: [PATCH v3 1/2] sequencer: handle errors in read_author_ident()

[Eric Sunshine]

On Thu, Aug 2, 2018 at 7:20 AM Phillip Wood  wrote:
> The calling code did not treat NULL as an error. Instead NULL caused
> it to fallback to using the default author when creating the new
> commit. This changed the date and potentially the author of the
> commit which corrupted the author data compared to its expected
> value. Fix this by returning and integer and passing in a parameter to
> receive the author.
>
> Signed-off-by: Phillip Wood 
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -701,57 +701,59 @@ static char *get_author(const char *message)
> -static const char *read_author_ident(struct strbuf *buf)
> +static int read_author_ident(char **author)
>  {
> +   struct strbuf buf = STRBUF_INIT;
> -   if (strbuf_read_file(buf, rebase_path_author_script(), 256) <= 0)
> -   return NULL;
> +   if (strbuf_read_file(&buf, rebase_path_author_script(), 256) <= 0) {
> +   strbuf_release(&buf);
> +   return -1;
> +   }

[...much noisiness snipped...]

> @@ -794,12 +796,14 @@ static int run_git_commit(const char *defmsg, struct 
> replay_opts *opts,
> if ((flags & CREATE_ROOT_COMMIT) && !(flags & AMEND_MSG)) {
> -   struct strbuf msg = STRBUF_INIT, script = STRBUF_INIT;
> -   const char *author = is_rebase_i(opts) ?
> -   read_author_ident(&script) : NULL;
> +   struct strbuf msg = STRBUF_INIT;
> +   char *author = NULL;
>
> +   if (is_rebase_i(opts) && read_author_ident(&author))
> +   return -1;
> +

I think this patch can be simplified considerably by shifting one's
perspective. If we admit that read_author_ident() is already correctly
reporting an error by returning NULL (which is exactly what it is
doing), then the bug is is purely on the calling side; namely, the
caller is ignoring the error. (In fact, your commit message already
states this.)

So, if it's the caller which is buggy, then read_author_ident() does
not require _any_ changes (meaning all the noisiness in this patch can
be dropped), and only the caller needs a fix, and that change can be
quite tiny. To wit, instead of initializing 'author' like this (as
done in the current "buggy" code):

const char *author = is_rebase_i(opts) ?
read_author_ident(&script) : NULL;

Do this instead:

const char *author = NULL;
...
if (is_rebase_i(opts)) {
author = read_author_ident(&script);
if (!author) {
strbuf_release(&script);
return -1;
}
}

That's it, a minimal fix giving the same result, without all the code
churn, thus safer (less opportunity, for instance, to introduce a leak
as in v1). Of course, you can collapse the above even further to:

if (is_rebase_i(opts) &&
!(author = read_author_ident(&script)) {
strbuf_release(&script);
return -1;
}

Though, I think that is less readable.

[PATCH v3 1/2] sequencer: handle errors in read_author_ident()

[Phillip Wood]

From: Phillip Wood 

The calling code did not treat NULL as an error. Instead NULL caused
it to fallback to using the default author when creating the new
commit. This changed the date and potentially the author of the
commit which corrupted the author data compared to its expected
value. Fix this by returning and integer and passing in a parameter to
receive the author.

Signed-off-by: Phillip Wood 
---

Notes:
changes since v2:

 - Improved commit message
 - Fixed memory leak
 - Translated a couple of error messages

 sequencer.c | 52 
 1 file changed, 28 insertions(+), 24 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 944dea6997..1bf8b0c431 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -701,57 +701,59 @@ static char *get_author(const char *message)
 }
 
 /* Read author-script and return an ident line (author  timestamp) */
-static const char *read_author_ident(struct strbuf *buf)
+static int read_author_ident(char **author)
 {
const char *keys[] = {
"GIT_AUTHOR_NAME=", "GIT_AUTHOR_EMAIL=", "GIT_AUTHOR_DATE="
};
+   struct strbuf buf = STRBUF_INIT;
struct strbuf out = STRBUF_INIT;
char *in, *eol;
const char *val[3];
int i = 0;
 
-   if (strbuf_read_file(buf, rebase_path_author_script(), 256) <= 0)
-   return NULL;
+   if (strbuf_read_file(&buf, rebase_path_author_script(), 256) <= 0) {
+   strbuf_release(&buf);
+   return -1;
+   }
 
-   /* dequote values and construct ident line in-place */
-   for (in = buf->buf; i < 3 && in - buf->buf < buf->len; i++) {
+   for (in = buf.buf; i < 3 && in - buf.buf < buf.len; i++) {
if (!skip_prefix(in, keys[i], (const char **)&in)) {
-   warning("could not parse '%s' (looking for '%s'",
-   rebase_path_author_script(), keys[i]);
-   return NULL;
+   strbuf_release(&buf);
+   return error(_("could not parse '%s' (looking for 
'%s')"),
+  rebase_path_author_script(), keys[i]);
}
-
eol = strchrnul(in, '\n');
*eol = '\0';
if (!sq_dequote(in)) {
-   warning(_("bad quoting on %s value in '%s'"),
-   keys[i], rebase_path_author_script());
-   return NULL;
+   strbuf_release(&buf);
+   return error(_("bad quoting on %s value in '%s'"),
+keys[i], rebase_path_author_script());
}
val[i] = in;
in = eol + 1;
}
 
if (i < 3) {
-   warning("could not parse '%s' (looking for '%s')",
-   rebase_path_author_script(), keys[i]);
-   return NULL;
+   strbuf_release(&buf);
+   return error(_("could not parse '%s' (looking for '%s')"),
+rebase_path_author_script(), keys[i]);
}
 
/* validate date since fmt_ident() will die() on bad value */
if (parse_date(val[2], &out)){
-   warning(_("invalid date format '%s' in '%s'"),
+   error(_("invalid date format '%s' in '%s'"),
val[2], rebase_path_author_script());
strbuf_release(&out);
-   return NULL;
+   strbuf_release(&buf);
+   return -1;
}
 
strbuf_reset(&out);
strbuf_addstr(&out, fmt_ident(val[0], val[1], val[2], 0));
-   strbuf_swap(buf, &out);
-   strbuf_release(&out);
-   return buf->buf;
+   *author = strbuf_detach(&out, NULL);
+   strbuf_release(&buf);
+   return 0;
 }
 
 static const char staged_changes_advice[] =
@@ -794,12 +796,14 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
const char *value;
 
if ((flags & CREATE_ROOT_COMMIT) && !(flags & AMEND_MSG)) {
-   struct strbuf msg = STRBUF_INIT, script = STRBUF_INIT;
-   const char *author = is_rebase_i(opts) ?
-   read_author_ident(&script) : NULL;
+   struct strbuf msg = STRBUF_INIT;
+   char *author = NULL;
struct object_id root_commit, *cache_tree_oid;
int res = 0;
 
+   if (is_rebase_i(opts) && read_author_ident(&author))
+   return -1;
+
if (!defmsg)
BUG("root commit without message");
 
@@ -817,7 +821,7 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
  opts->gpg_sign);
 
strbuf_release(&msg);
-   strbuf_release(&script);
+   free(author);
if (!res) {