Re: [PATCH 4/6] builtin/stash: provide a way to export stashes to a ref
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Mar 14, 2022, 21:19 UTC
- Message-ID
- <2422376f-79aa-2d35-2646-c3611e2ef8d6@gmail.com>
- In-Reply-To
- <220311.86bkydi65v.gmgdl@evledraar.gmail.com>
Hi Brian and Ævar
Firstly I think this is a useful feature to add to git stash, thanks for working on it Brian
On 11/03/2022 02:08, Ævar Arnfjörð Bjarmason wrote:
Show 22 quoted lines
> > 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);
You could start searching from orig_committer rather than buffer but I'm sure it doesn't make any real difference. The sequencer does something similar to this to replay commits when rebasing - is there any scope for sharing code between the two?
Show 6 quoted lines
> ...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++) {Do we need i and nitems?
Show 9 quoted lines
>> + 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?ALLOC_GROW_BY() zeros out the memory which would mean we could remove the memset() calls in the loops. I noticed in some other loops we know the size in advance and could use CALLOC_ARRAY().
Show 6 quoted lines
>> + } >> + 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...
According to [1] it has been available since at least 2015. It is certainly much nicer than casting every size_t to uintmax_t and having to use PRIuMAX.
>> + 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...I'm not really convinced by this ssize_t can be small argument[2], do you know of any platforms where it is true?
Best Wishes
Phillip
[1] https://docs.microsoft.com/en-us/cpp/c-runtime-library/format-specification-syntax-printf-and-wprintf-functions?view=msvc-140#size
[2] https://lore.kernel.org/git/e881a455-88b5-9c87-03a8-caaee68bb344@gmail.com/