Re: [PATCH v4 4/4] builtin/stash: provide a way to import stashes from a ref

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 13, 2022, 18:33 UTC
Message-ID: <220413.861qy0g6yk.gmgdl@evledraar.gmail.com>
In-Reply-To: <20220407215352.3491567-5-sandals@crustytoothpaste.net>

On Thu, Apr 07 2022, brian m. carlson wrote:

[Finding time to go over this in a few passes, so some disjointed replies, sorry]

Show 9 quoted lines

> +	for (i = 0;; i++) {
> +		struct object_id tree, oid;
> +		char revision[GIT_MAX_HEXSZ + 1];
> +
> +		oid_to_hex_r(revision, &chain);
> +
> +		if (get_oidf(&tree, "%s:", revision) ||
> +		    !oideq(&tree, the_hash_algo->empty_tree)) {
> +			return error(_("%s is not a valid exported stash commit"), revision);

I think you're leaking memory here, i.e. you're in the for-loop and doing oid_array_append()< but here you won't clear that or do other "out" free-ing at the end.

But I also checked if your tests leaked with SANITIZE=leak, and (after omitting the existing leaks) they didn't, so either I'm wrong or it's a test blindspot.

Have you tried "make coverage-report" with this?

Show 25 quoted lines

> +		}
> +		if (get_oidf(&chain, "%s^1", revision) ||
> +		    get_oidf(&oid, "%s^2", revision))
> +			break;
> +		oid_array_append(&items, &oid);
> +	}
> +
> +	/*
> +	 * Now, walk each entry, adding it to the stash as a normal stash
> +	 * commit.
> +	 */
> +	for (i = items.nr - 1; i >= 0; i--) {
> +		unsigned long bufsize;
> +		const char *p;
> +		const struct object_id *oid = items.oid + i;
> +
> +		this = lookup_commit_reference(the_repository, oid);
> +		buffer = get_commit_buffer(this, &bufsize);
> +		if (!buffer) {
> +			res = -1;
> +			error(_("cannot read commit buffer for %s"), oid_to_hex(oid));
> +			goto out;
> +		}
> +
> +		p = memmem(buffer, bufsize, "\n\n", 2);

Nit: Grepping in-tree all other API users of get_commit_buffer() just
use strstr(buffer, "\n\n"), if this one needs to handle \0 specially
(for reasons I'm missing) perhaps a comment here discussing why?

Show 14 quoted lines

> +		if (!p) {
> +			res = -1;
> +			error(_("cannot parse commit %s"), oid_to_hex(oid));
> +			goto out;
> +		}
> +
> +		p += 2;
> +		msg = xmemdupz(p, bufsize - (p - buffer));
> +		unuse_commit_buffer(this, buffer);
> +		buffer = NULL;
> +
> +		if (do_store_stash(oid, msg, 1)) {
> +			res = -1;
> +			error(_("cannot save the stash for %s"), oid_to_hex(oid));

Maybe just "res = error" for these? You use that in 3/4, would be good to continue the same pattern consistently in 4/4.

Show 25 quoted lines

> +			goto out;
> +		}
> +		FREE_AND_NULL(msg);
> +	}
> +out:
> +	if (this && buffer)
> +		unuse_commit_buffer(this, buffer);
> +	oid_array_clear(&items);
> +	free(msg);
> +
> +	return res;
> +}
> +
> +static int import_stash(int argc, const char **argv, const char *prefix)
> +{
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options,
> +			     git_stash_import_usage,
> +			     PARSE_OPT_KEEP_DASHDASH);
> +
> +	if (argc != 1) {
> +		usage_with_options(git_stash_import_usage, options);

This function is a NORETURN....

> +		return -1;

...so this code isn't reachable, and will warn on some compilers (suncc at least).

But consider using usage_msg_opt() instead, i.e. tell the user what went wrong.