Re: [PATCH v2 3/4] sequencer: fix "rebase -i --root" corrupting author header timestamp

[Phillip Wood <phillip.wood@talktalk.net>]

On 31/07/18 08:33, Eric Sunshine wrote:
> When "git rebase -i --root" creates a new root commit, it corrupts the
> "author" header's timestamp by prepending a "@":
> 
>      author A U Thor <author@example.com> @1112912773 -0700
> 
> The commit parser is very strict about the format of the "author"
> header, and does not allow a "@" in that position.
> 
> The "@" comes from GIT_AUTHOR_DATE in "rebase-merge/author-script",
> signifying a Unix epoch-based timestamp, however, read_author_ident()
> incorrectly allows it to slip into the commit's "author" header, thus
> corrupting it.
> 
> One possible fix would be simply to filter out the "@" when constructing
> the "author" header timestamp, however, a more correct fix is to parse
> the GIT_AUTHOR_DATE date (via parse_date()) and format the parsed result
> into the "author" header. Since "rebase-merge/author-script" may be
> edited by the user, this approach has the extra benefit of catching
> other potential timestamp corruption due to hand-editing.
> 
> We can do better than calling parse_date() ourselves and constructing
> the "author" header manually, however, by instead taking advantage of
> fmt_ident() which does this work for us.
> 
> The benefits of using fmt_ident() are twofold. First, it simplifies the
> logic considerably by allowing us to avoid the complexity of building
> the "author" header in parallel with and in the same buffer from which
> "rebase-merge/author-script" is being parsed. Instead, fmt_ident() is
> invoked to compose the header after parsing is complete.
> 
> Second, fmt_ident() is careful to prevent "crud" from polluting the
> composed ident. As with validating GIT_AUTHOR_DATE, this "crud"
> avoidance prevents other (possibly hand-edited) bogus author information
> from "rebase-merge/author-script" from corrupting the commit object.
> 
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>   sequencer.c                   | 23 +++++++++--------------
>   t/t3404-rebase-interactive.sh |  2 +-
>   2 files changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 1008f6d71a..15a66a334c 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -709,14 +709,16 @@ static const char *read_author_ident(struct strbuf *buf)
>   	const char *keys[] = {
>   		"GIT_AUTHOR_NAME=", "GIT_AUTHOR_EMAIL=", "GIT_AUTHOR_DATE="
>   	};
> -	char *in, *out, *eol;
> -	int i = 0, len;
> +	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;
>   
>   	/* dequote values and construct ident line in-place */
> -	for (in = out = 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]);
> @@ -730,16 +732,7 @@ static const char *read_author_ident(struct strbuf *buf)
>   				keys[i], rebase_path_author_script());
>   			return NULL;
>   		}
> -		len = strlen(in);
> -
> -		if (i > 0) /* separate values by spaces */
> -			*(out++) = ' ';
> -		if (i == 1) /* email needs to be surrounded by <...> */
> -			*(out++) = '<';
> -		memmove(out, in, len);
> -		out += len;
> -		if (i == 1) /* email needs to be surrounded by <...> */
> -			*(out++) = '>';
> +		val[i] = in;
>   		in = eol + 1;
>   	}
>   
> @@ -749,7 +742,9 @@ static const char *read_author_ident(struct strbuf *buf)
>   		return NULL;
>   	}
>   
> -	strbuf_setlen(buf, out - buf->buf);
> +	strbuf_addstr(&out, fmt_ident(val[0], val[1], val[2], 0));
> +	strbuf_swap(buf, &out);
> +	strbuf_release(&out);
>   	return buf->buf;
>   }

This is a welcome simplification

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index fd3a18154e..d340018781 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh

Now that the author is correct, can we test_cmp() it against its 
expected value to make sure there are no hidden surprises in the name 
and email in the future. (It would be reassuring to test an author with 
"'" in the name as well but that is out of scope for this series.)

> @@ -1420,7 +1420,7 @@ test_expect_success 'valid author header after --root swap' '
+	git cat-file commit HEAD^ |grep ^author >expected &&
>   	set_fake_editor &&
>   	FAKE_LINES="2 1" git rebase -i --root &&
>   	git cat-file commit HEAD^ >out &&
-	git cat-file commit HEAD^ >out &&
> -	grep "^author ..*> @[0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$" out
+	git cat-file commit HEAD^ |grep ^author >out &&
+	test_cmp expected out	
> +	grep "^author ..*> [0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$" out
>   ' >   test_done
>