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

> 
> On Wed, Mar 16 2022, brian m. carlson wrote:
> 
>> [[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.
> 
> FWIW I dug into my logs and I ran into it with %zu (not %z), but that's
> what you're using.
> 
> Sorry about being inaccurate, it seems %z's portability isn't the same
> as %z.
> 
> I ran into it in mid-2021 in the GitHub CI, but those logs are deleted
> now (and I didn't re-push that specific OID):
> 
>      https://github.com/avar/git/runs/2298653913
> 
> Where it would emit output like:
> 
>      builtin/log.c: In function 'gen_message_id':
>      311
>      builtin/log.c:1047:29: error: unknown conversion type character 'z' in format [-Werror=format=]
>      312
>       1047 |  strbuf_addf(&fmt, "%%s-%%0%zud.%%0%zud-%%s-%%s-%%s", tmp.len, tmp.len);
> 
> This SO post, whose accuracy I can't verify, claims it is supported in
> VS 2013 or later, and that the way to check for it is with "_MSC_VER >=
> 1800":
> 
>      https://stackoverflow.com/questions/44382862/how-to-printf-a-size-t-without-warning-in-mingw-w64-gcc-7-1