Re: [PATCH 4/6] builtin/stash: provide a way to export stashes to a ref
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Mar 11, 2022, 02:08 UTC
- Message-ID
- <220311.86bkydi65v.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <20220310173236.4165310-5-sandals@crustytoothpaste.net>
On Thu, Mar 10 2022, brian m. carlson wrote:
Show 8 quoted lines
> + 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..
Show 8 quoted lines
> + 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)?
Show 5 quoted lines
> + 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.
Show 9 quoted lines
> +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")Show 10 quoted lines
> + 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 _().