From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 14 Mar 2022 21:19:10 GMT
Subject: Re: [PATCH 4/6] builtin/stash: provide a way to export stashes to a ref
Message-ID: <2422376f-79aa-2d35-2646-c3611e2ef8d6@gmail.com>
In-Reply-To: <220311.86bkydi65v.gmgdl@evledraar.gmail.com>

Hi Brian and Ævar

Firstly I think this is a useful feature to add to git stash, thanks for 
working on it Brian

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

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

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

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

>> +	for (ssize_t i = nitems - 1; i >= 0; i--) {
> 
> The ssize_t type can be really small (it's not a signed size_t), so this
> is unportable, but in practice maybe it's OK...

I'm not really convinced by this ssize_t can be small argument[2], do 
you know of any platforms where it is true?

Best Wishes

Phillip

[1] 
https://docs.microsoft.com/en-us/cpp/c-runtime-library/format-specification-syntax-printf-and-wprintf-functions?view=msvc-140#size

[2] 
https://lore.kernel.org/git/e881a455-88b5-9c87-03a8-caaee68bb344@gmail.com/