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

[Phillip Wood <phillip.wood123@gmail.com>]

On 18/03/2022 13:34, Ævar Arnfjörð Bjarmason wrote:
> 
> 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

Oh, so it is mingw that is the problem, not MSVC. Indeed using "%zu" 
(see the diff below) fails for the "win build" job[1] but the "win+VS 
build" succeeds[2]. mingw config.mak.uname to uses 
-D__MINGW_USE_ANSI_STDIO=0. Even if we could change that to =1 it would 
be insufficient as it does not affect the format specifiers allowed by 
__attribute__((format (printf, 3, 4))). I think to do that we would have 
to change "printf" to "__MINGW_PRINTF_FORMAT" for each attribute 
declaration[3].

In short unfortunately I don't think we can easily use "%zu"

Best wishes

Phillip

diff --git a/add-interactive.c b/add-interactive.c
index e1ab39cce3..1790ad6359 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -193,9 +193,8 @@ static ssize_t find_unique(const char *string, 
struct prefix_item_list *list)
         struct string_list_item *item;

         if (list->items.nr != list->sorted.nr)
-               BUG("prefix_item_list in inconsistent state (%"PRIuMAX
-                   " vs %"PRIuMAX")",
-                   (uintmax_t)list->items.nr, (uintmax_t)list->sorted.nr);
+               BUG("prefix_item_list in inconsistent state (%zu vs %zu)",
+                   list->items.nr, list->sorted.nr);

         if (index < 0)
                 item = list->sorted.items[-1 - index].util;

[1] 
https://github.com/phillipwood/git/runs/5601840748?check_suite_focus=true
[2] 
https://github.com/phillipwood/git/runs/5601840528?check_suite_focus=true
[3] https://sourceforge.net/p/mingw-w64/wiki2/gnu%20printf

> So if we are going to use it and that's true (which would be great!) it
> would IMO make sense to introduce it in some prep commit where we delete
> e.g. "_MSC_VER>=1310" and other things you'll see in-tree if you look
> through:
> 
>      git grep _MSC_VER
> 
> I.e. to push out some canary commit for using that specific feature, and
> along with it delete the old MSVC compatibility shims (which presumably
> we can't use if we're going to hard depend on %zu).