Re: [PATCH 07/17] commit: increase commit message buffer size

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Jul 9, 2018, 23:39 UTC
Message-ID: <20180709233952.GB535220@genre.crustytoothpaste.net>
In-Reply-To: <xmqqva9oe20y.fsf@gitster-ct.c.googlers.com>

On Mon, Jul 09, 2018 at 10:45:33AM -0700, Junio C Hamano wrote:

Show 18 quoted lines

> Derrick Stolee <stolee@gmail.com> writes:
> 
> > On 7/8/2018 7:36 PM, brian m. carlson wrote:
> >> diff --git a/refs/files-backend.c b/refs/files-backend.c
> >> index a9a066dcfb..252f835bae 100644
> >> --- a/refs/files-backend.c
> >> +++ b/refs/files-backend.c
> >> @@ -1587,7 +1587,7 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,
> >>   	char *logrec;
> >>     	msglen = msg ? strlen(msg) : 0;
> >> -	maxlen = strlen(committer) + msglen + 100;
> >> +	maxlen = strlen(committer) + msglen + 200;
> >>   	logrec = xmalloc(maxlen);
> >>   	len = xsnprintf(logrec, maxlen, "%s %s %s\n",
> >>   			oid_to_hex(old_oid),
> >
> > nit: 100 is not enough anymore, but wasn't a very descriptive
> > value. 200 may be enough now, but I'm not sure why.

200 is definitely enough. Suppose we had a message consisting entirely of SHA-1 hashes (5, at 20 bytes a piece). If our new hash is 32 bytes long, then it would require at most 160 bytes.

I only noticed this because the old code segfaulted. My approach to using a 32-byte hash was to set it up, do some basic tests, find out what crashed, and fix it. Most of this series is the basics necessary to get the most rudimentary functionality out of a 32-byte Git, excluding the index pieces, which are necessarily inelegant.

I didn't include them because there are other ways to implement the changes which are more elegant in some ways and less elegant in other ways, and I want to think more about it before I send them in.

> As Brandon alludes to downthread, we probably should use strbuf for
> things like this these days, so a preliminary clean-up to do so is
> probably a welcome change to sneak in and rebase this series on top
> of.

Sure, I agree that would be a better change, and I'm happy to reroll with that.

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204