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
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).
