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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Mar 10 2022, brian m. carlson wrote:

> +	size_t author_len, committer_len;
> +	struct commit *this = NULL;
> +	const char *orig_author = NULL, *orig_committer = NULL;
> +	char *author = NULL, *committer = NULL;
> +	const char *buffer = NULL;
> +	unsigned long bufsize;
> +	const char *p;
> +	char *msg = NULL;

These shouldn't be initialized unless they really need to..

> +	this = lookup_commit_reference(the_repository, &info->w_commit);

..and some are clobbered right away here, so all of these should not be initializzed.

> +	buffer = get_commit_buffer(this, &bufsize);
> +	orig_author = find_commit_header(buffer, "author", &author_len);
> +	orig_committer = find_commit_header(buffer, "committer", &committer_len);
> +	p = memmem(buffer, bufsize, "\n\n", 2);

...since by doing so we hide genuine "uninitialized"
warnings. E.g. "author_len" here isn't initialized, but is set by
find_commit_header(), but if that line was removed we'd warn below, but
not if it's initialized when the variables are declared..

> +		for (size_t i = 0;; i++, nitems++) {
> +			char buf[32];
> +			int ret;
> +
> +			if (nalloc <= i) {
> +				size_t new = nalloc * 3 / 2 + 5;
> +				items = xrealloc(items, new * sizeof(*items));
> +				nalloc = new;

Can't we just use the usual ALLOC_GROW() pattern here?

> +			}
> +			snprintf(buf, sizeof(buf), "%zu", i);

Aren't the %z formats unportable (even with our newly found reliance on
more C99)? I vaguely recall trying them recently and the windows CI jobs
erroring...

> +	for (ssize_t i = nitems - 1; i >= 0; i--) {

The ssize_t type can be really small (it's not a signed size_t), so this
is unportable, but in practice maybe it's OK...

In this case if you're just wanting to count down in a list maybe you
can use the pattern I used in 99d60545f87 (string-list API: change "nr"
and "alloc" to "size_t", 2022-03-07)?

> +	if (ref) {
> +		update_ref(NULL, ref, &prev->object.oid, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
> +	} else {
> +		puts(oid_to_hex(&prev->object.oid));
> +	}

Nit: braces can be gone.

> +out:
> +	for (size_t i = 0; i < nitems; i++) {
> +		free_stash_info(&items[i]);
> +	}

Ditto.

> +static int export_stash(int argc, const char **argv, const char *prefix)
> +{
> +	int ret = 0;
> +	int print = 0;
> +	const char *ref = NULL;
> +	struct option options[] = {
> +		OPT_BOOL(0, "print", &print,
> +			 N_("print the object ID instead of writing it to a ref")),
> +		OPT_STRING(0, "to-ref", &ref, "refname",

Needs _("refname")

> +			   N_("save the data to the given ref")),
> +		OPT_END()
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options,
> +			     git_stash_export_usage,
> +			     PARSE_OPT_KEEP_DASHDASH);
> +
> +	if (!(!!ref ^ print))
> +		return error("exactly one of --print or --to-ref is required");

Cute, but maybe we can just use OPT_CMDMODE(), and if it's "to-ref"
shift one off argv afterwards (which'll be your &ref).

I.e. it'll give you the option incompatibility check, and without a new
translaed string.

Which, if we're keeping this version should use _().