Re: [PATCH v5 3/4] builtin/stash: provide a way to export stashes to a ref

From: Junio C Hamano <gitster@pobox.com>
Date: May 9, 2025, 16:38 UTC
Message-ID: <xmqqr00x20zv.fsf@gitster.g>
In-Reply-To: <20250508234458.3665894-4-sandals@crustytoothpaste.net>

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

Show 31 quoted lines

> +static int write_commit_with_parents(struct repository *r,
> +				     struct object_id *out,
> +				     const struct object_id *oid,
> +				     struct commit_list *parents)
> +{
> +	size_t author_len, committer_len;
> +	struct commit *this;
> +	const char *orig_author, *orig_committer;
> +	char *author = NULL, *committer = NULL;
> +	const char *buffer;
> +	unsigned long bufsize;
> +	const char *p;
> +	struct strbuf msg = STRBUF_INIT;
> +	int ret = 0;
> +	struct ident_split id;
> +
> +	this = lookup_commit_reference(r, oid);
> +	buffer = repo_get_commit_buffer(r, this, &bufsize);
> +	orig_author = find_commit_header(buffer, "author", &author_len);
> +	orig_committer = find_commit_header(buffer, "committer", &committer_len);
> +	if (split_ident_line(&id, orig_author, author_len) < 0 ||
> +	    split_ident_line(&id, orig_committer, committer_len) < 0) {
> +		ret = error(_("invalid author or committer for %s"), oid_to_hex(oid));
> +		goto out;
> +	}
> +	p = strstr(buffer, "\n\n");
> +
> +	if (!orig_author || !orig_committer || !p) {
> +		ret = error(_("cannot parse commit %s"), oid_to_hex(oid));
> +		goto out;
> +	}

It is too late to check the NULL-ness of orig_author and orig_committer here. They have already been used without checking for their NULL-ness to call split_ident_line() that happily will dereference its second "const char *line" parameter, so we would have already segfaulted.

As fsck.c::verify_headers() say, it is not a crime to lack the "\n\n" after the last header item, if the commit truly lacks any message. So '!p' is a bit overly strict, but in practice I do not think our tools saved a byte by omitting the empty line after the header even when creating a commit with an empty message for a long time, so this may be OK. On the other hand, preparing for a stash entry a third-party reimplementation prepared would not be too hard to do here.

	if (!orig_author || !orig_committer) {
		ret = error(_("cannot parse..."));
		goto out;
	}
	if (split_ident_line(...) < 0 ||
	    split_ident_line(...) < 0) {
		ret = error(_("invalid au..."));
		goto out;
	}

	p = strstr(buffer, "\n\n");
	strbuf_addstr(&msg, "git stash: ");

	if (p)
		strbuf_add(&msg, p+2, bufsize - (p + 2 - buffer));
	strbuf_complete_line(&mesg);

or something.

> +	/* Jump to message. */
> +	p += 2;
> +	strbuf_addstr(&msg, "git stash: ");
> +	strbuf_add(&msg, p, bufsize - (p - buffer));