From: Jeff King <peff@peff.net>
Date: Tue, 14 Jan 2025 09:29:59 GMT
Subject: Re: [PATCH 02/14] combine-diff: add combine_diff_path_new()
Message-ID: <20250114092959.GB882468@coredump.intra.peff.net>
In-Reply-To: <Z4Uz43eByZHqW8UK@pks.im>

On Mon, Jan 13, 2025 at 04:40:19PM +0100, Patrick Steinhardt wrote:

> > +struct combine_diff_path *combine_diff_path_new(const char *path,
> > +						size_t path_len,
> > +						unsigned int mode,
> > +						const struct object_id *oid,
> > +						size_t num_parents)
> > +{
> > +	struct combine_diff_path *p;
> > +
> > +	p = xmalloc(combine_diff_path_size(num_parents, path_len));
> > +	p->path = (char *)&(p->parent[num_parents]);
> > +	memcpy(p->path, path, path_len);
> > +	p->path[path_len] = 0;
> > +	p->next = NULL;
> > +	p->mode = mode;
> > +	oidcpy(&p->oid, oid);
> > +
> > +	memset(p->parent, 0, sizeof(p->parent[0]) * num_parents);
> > +
> > +	return p;
> > +}
> 
> If I were to write this anew I'd probably use `xcalloc()` instead of
> manually `memset()`ing parts of it to zero. But it's a faithful
> transplant of the code from `intersect_paths()`, so that's probably
> okay.

Yeah, I actually wrote it that way originally (thinking the issue was
that we were leaving uninitialized fields all over), before realizing
that most callers were explicitly zero-ing the parents. So I went for
the minimal change.

From an efficiency standpoint, I don't know that it matters much between
the two (xcalloc would zero some fields which we're going to assign
anyway, but the zeroing may be more efficient on the backend). xcalloc
means you'd never forget to initialize any part of it, so maybe it's
more readable / less error prone?

We could do a patch on top, but I doubt it's a big deal either way.

-Peff