C99 %zu support (on MSVC) (was: [PATCH 4/6] builtin/stash: provide a way to export stashes to a ref)

> [[PGP Signed Part:Undecided]]
> On 2022-03-14 at 21:19:10, Phillip Wood wrote:
>> Hi Brian and Ævar
>> 
>> Firstly I think this is a useful feature to add to git stash, thanks for
>> working on it Brian
>
> Thanks.  I'm glad folks other than me will find it useful.
>
>> On 11/03/2022 02:08, Ævar Arnfjörð Bjarmason wrote:
>> > 
>> > 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.
>
> This function got hoisted out of what would otherwise be duplicated
> code, and that's why they're all initialized (because we would otherwise
> have called free on an uninitialized value).  I can remove the ones that
> aren't strictly needed.
>
>> > > +	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?
>
> I can look into it.  The amount of code that would be duplicated here is
> very minimal, so I'm okay with just adding a few lines here.
>
>> > ...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?
>
> I can look into removing them.
>
>> > > +			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().
>
> Yeah, I can switch to that.  I was looking for that, but I was thinking
> of a function and not a macro, so I missed it.
>
>> > > +			}
>> > > +			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.
>
> If we're relying on a new enough MSVC for C11, then it's much newer than
> 2015, so we should be fine.  It's mandatory on POSIX systems.