On Wed, Mar 16 2022, brian m. carlson wrote:

[[PGP Signed Part:Undecided]]
On 2022-03-14 at 21:19:10, Phillip Wood wrote:

quoted

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.

quoted

On 11/03/2022 02:08, Ævar Arnfjörð Bjarmason wrote:

quoted

On Thu, Mar 10 2022, brian m. carlson wrote:

quoted

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

quoted

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

quoted

quoted

quoted

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

quoted

quoted

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

quoted

+		for (size_t i = 0;; i++, nitems++) {

Do we need i and nitems?

I can look into removing them.

quoted

quoted

quoted

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

quoted

quoted

quoted

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