Re: [PATCH 4/6] builtin/stash: provide a way to export stashes to a ref

["brian m. carlson" <sandals@crustytoothpaste.net>]

[-- Attachment #1: Type: text/plain, Size: 3430 bytes --]

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.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]