Re: [PATCH 07/17] commit: increase commit message buffer size
"brian m. carlson" writes: > On Tue, Jul 10, 2018 at 02:08:28PM -0400, Ben Peart wrote: >> >> >> On 7/9/2018 7:39 PM, brian m. carlson wrote: >> > On Mon, Jul 09, 2018 at 10:45:33AM -0700, Junio C Hamano wrote: >> > > 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. >> > >> >> I've put together a patch to update log_ref_write_fd() to use strbuf and >> will submit it shortly. > > Excellent. I'll drop this patch in that case. OK, thanks for working well together. In the integration run I did yesterday evening, this part was resolved by taking what Ben did, dropping the s/100/200/ change from this series, and the result looked reasonable, of course.
Re: [PATCH 07/17] commit: increase commit message buffer size
On Tue, Jul 10, 2018 at 02:08:28PM -0400, Ben Peart wrote: > > > On 7/9/2018 7:39 PM, brian m. carlson wrote: > > On Mon, Jul 09, 2018 at 10:45:33AM -0700, Junio C Hamano wrote: > > > 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. > > > > I've put together a patch to update log_ref_write_fd() to use strbuf and > will submit it shortly. Excellent. I'll drop this patch in that case. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [PATCH 07/17] commit: increase commit message buffer size
On 7/9/2018 7:39 PM, brian m. carlson wrote: On Mon, Jul 09, 2018 at 10:45:33AM -0700, Junio C Hamano wrote: Derrick Stolee 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. I've put together a patch to update log_ref_write_fd() to use strbuf and will submit it shortly.
Re: [PATCH 07/17] commit: increase commit message buffer size
"brian m. carlson" writes: >> 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. Or we can do the clean-up after this 17-patch series settles, which probably is a better order to do things. It is usually better to make a longer but more mechanical topic like this pass through the system rather quickly, which tends to minimize disruption on other topics.
Re: [PATCH 07/17] commit: increase commit message buffer size
On Mon, Jul 09, 2018 at 10:45:33AM -0700, Junio C Hamano wrote: > Derrick Stolee 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 signature.asc Description: PGP signature
Re: [PATCH 07/17] commit: increase commit message buffer size
Derrick Stolee writes:
> On 7/8/2018 7:36 PM, brian m. carlson wrote:
>> 100 bytes is not sufficient to ensure we can write a commit message
>> buffer when using a 32-byte hash algorithm. Increase the buffer size to
>> ensure we have sufficient space.
>>
>> Signed-off-by: brian m. carlson
>> ---
>> refs/files-backend.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> 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.
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.
"%s %s %s\n" with old and new commit object name and the message
will be "2 * len(hash_in_hex) + 4" bytes long (counting the three
whitespaces and the terminating NUL), and Shawn's original in
6de08ae6 ("Log ref updates to logs/refs/", 2006-05-17) actually
computed this one as "strlen(...) + 2*40+4".
100 was merely me being sloppier than Shawn at 8ac65937 ("Make sure
we do not write bogus reflog entries.", 2007-01-26), preferring
being sufficient over not wasting even a single byte.
Re: [PATCH 07/17] commit: increase commit message buffer size
On 07/09, Junio C Hamano wrote:
> Brandon Williams writes:
>
> >> > > 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.
> >>
> >> That line was touched in by Michael in 7bd9bcf372d (refs: split
> >> filesystem-based
> >> refs code into a new file, 2015-11-09) and before that by Ronnie in
> >> 2c6207abbd6
> >> (refs.c: add a function to append a reflog entry to a fd, 2014-12-12)
> >> and introduced
> >> by Junio in 8ac65937d03 (Make sure we do not write bogus reflog
> >> entries., 2007-01-26)
> >> and it appears to me that 2*40 + 5 ought to be sufficient, but no
> >> comments or commit
> >> messages are found as to why we rather choose 100.
> >
> > Whats the reason for not using a strbuf here so that we don't have to
> > play with magic numbers?
>
> Quite a legitimate question.
>
> I suspect that the reason is because the code (even though it now
> sits in a file that was relatively recently creted) predates either
> the introduction or wide adoption of strbuf.
>
> Back when 6de08ae6 ("Log ref updates to logs/refs/",
> 2006-05-17) was done, we already had strbuf.c, but it only had
> read_line() and nothing else back then, so it wouldn't have been
> possible to use a strbuf there.
Fair enough, having never working in the code back then I don't know
what life was like without strbuf.
--
Brandon Williams
Re: [PATCH 07/17] commit: increase commit message buffer size
Brandon Williams writes:
>> > > 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.
>>
>> That line was touched in by Michael in 7bd9bcf372d (refs: split
>> filesystem-based
>> refs code into a new file, 2015-11-09) and before that by Ronnie in
>> 2c6207abbd6
>> (refs.c: add a function to append a reflog entry to a fd, 2014-12-12)
>> and introduced
>> by Junio in 8ac65937d03 (Make sure we do not write bogus reflog
>> entries., 2007-01-26)
>> and it appears to me that 2*40 + 5 ought to be sufficient, but no
>> comments or commit
>> messages are found as to why we rather choose 100.
>
> Whats the reason for not using a strbuf here so that we don't have to
> play with magic numbers?
Quite a legitimate question.
I suspect that the reason is because the code (even though it now
sits in a file that was relatively recently creted) predates either
the introduction or wide adoption of strbuf.
Back when 6de08ae6 ("Log ref updates to logs/refs/",
2006-05-17) was done, we already had strbuf.c, but it only had
read_line() and nothing else back then, so it wouldn't have been
possible to use a strbuf there.
Re: [PATCH 07/17] commit: increase commit message buffer size
On 07/09, Stefan Beller wrote: > On Mon, Jul 9, 2018 at 6:09 AM Derrick Stolee wrote: > > > > On 7/8/2018 7:36 PM, brian m. carlson wrote: > > > 100 bytes is not sufficient to ensure we can write a commit message > > > buffer when using a 32-byte hash algorithm. Increase the buffer size to > > > ensure we have sufficient space. > > > > > > Signed-off-by: brian m. carlson > > > --- > > > refs/files-backend.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > 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. > > That line was touched in by Michael in 7bd9bcf372d (refs: split > filesystem-based > refs code into a new file, 2015-11-09) and before that by Ronnie in > 2c6207abbd6 > (refs.c: add a function to append a reflog entry to a fd, 2014-12-12) > and introduced > by Junio in 8ac65937d03 (Make sure we do not write bogus reflog > entries., 2007-01-26) > and it appears to me that 2*40 + 5 ought to be sufficient, but no > comments or commit > messages are found as to why we rather choose 100. Whats the reason for not using a strbuf here so that we don't have to play with magic numbers? -- Brandon Williams
Re: [PATCH 07/17] commit: increase commit message buffer size
On Mon, Jul 9, 2018 at 6:09 AM Derrick Stolee wrote: > > On 7/8/2018 7:36 PM, brian m. carlson wrote: > > 100 bytes is not sufficient to ensure we can write a commit message > > buffer when using a 32-byte hash algorithm. Increase the buffer size to > > ensure we have sufficient space. > > > > Signed-off-by: brian m. carlson > > --- > > refs/files-backend.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > 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. That line was touched in by Michael in 7bd9bcf372d (refs: split filesystem-based refs code into a new file, 2015-11-09) and before that by Ronnie in 2c6207abbd6 (refs.c: add a function to append a reflog entry to a fd, 2014-12-12) and introduced by Junio in 8ac65937d03 (Make sure we do not write bogus reflog entries., 2007-01-26) and it appears to me that 2*40 + 5 ought to be sufficient, but no comments or commit messages are found as to why we rather choose 100.
Re: [PATCH 07/17] commit: increase commit message buffer size
On 7/8/2018 7:36 PM, brian m. carlson wrote: 100 bytes is not sufficient to ensure we can write a commit message buffer when using a 32-byte hash algorithm. Increase the buffer size to ensure we have sufficient space. Signed-off-by: brian m. carlson --- refs/files-backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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. Thanks, -Stolee
[PATCH 07/17] commit: increase commit message buffer size
100 bytes is not sufficient to ensure we can write a commit message buffer when using a 32-byte hash algorithm. Increase the buffer size to ensure we have sufficient space. Signed-off-by: brian m. carlson --- refs/files-backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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),
