Re: [PATCHv3 2/2] builtin/commit.c: switch to xstrfmt(), instead of snprintf,

From: Elia Pinto <gitter.spiros@gmail.com>
Date: Jan 14, 2017, 12:42 UTC
Message-ID: <CA+EOSBm_ciQ-7bXuzn4Ba7Q5qqihaYH3Sdkkv+0M0VKWbhk=7w@mail.gmail.com>
In-Reply-To: <20170113183309.GA28002@google.com>

Ok. I agree. But  is it strictly necessary to resend for this ?

Thanks

2017-01-13 19:33 GMT+01:00 Brandon Williams <bmwill@google.com>:

Show 71 quoted lines

> On 01/13, Elia Pinto wrote:
>> In this patch, instead of using xnprintf instead of snprintf, which asserts
>> that we don't truncate, we are switching to dynamic allocation with  xstrfmt(),
>> , so we can avoid dealing with magic numbers in the code and reduce the cognitive burden from
>> the programmers, because they no longer have to count bytes needed for static allocation.
>> As a side effect of this patch we have also reduced the snprintf() calls, that may silently truncate
>> results if the programmer is not careful.
>>
>> Helped-by: Junio C Hamano <gitster@pobox.com>
>> Helped-by: Jeff King <peff@peff.net>
>> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
>
> Small nit's with the commit message:
> * Stray comma ',' of on its own
> * lines are longer than 80 characters
>
>> ---
>> This is the third  version of the patch.
>>
>> Changes from the first version: I have split the original commit in two, as discussed here
>> http://public-inbox.org/git/20161213132717.42965-1-gitter.spiros@gmail.com/.
>>
>> Changes from the second version:
>> - Changed the commit message to clarify the purpose of the patch (
>> as suggested by Junio)
>> https://public-inbox.org/git/xmqqtw95mfo3.fsf@gitster.mtv.corp.google.com/T/#m2e6405a8a78a8ca1ed770614c91398290574c4a1
>>
>>
>>
>>  builtin/commit.c | 10 ++++------
>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 09bcc0f13..37228330c 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1526,12 +1526,10 @@ static int git_commit_config(const char *k, const char *v, void *cb)
>>  static int run_rewrite_hook(const unsigned char *oldsha1,
>>                           const unsigned char *newsha1)
>>  {
>> -     /* oldsha1 SP newsha1 LF NUL */
>> -     static char buf[2*40 + 3];
>> +     char *buf;
>>       struct child_process proc = CHILD_PROCESS_INIT;
>>       const char *argv[3];
>>       int code;
>> -     size_t n;
>>
>>       argv[0] = find_hook("post-rewrite");
>>       if (!argv[0])
>> @@ -1547,11 +1545,11 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
>>       code = start_command(&proc);
>>       if (code)
>>               return code;
>> -     n = snprintf(buf, sizeof(buf), "%s %s\n",
>> -                  sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
>> +     buf = xstrfmt("%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
>>       sigchain_push(SIGPIPE, SIG_IGN);
>> -     write_in_full(proc.in, buf, n);
>> +     write_in_full(proc.in, buf, strlen(buf));
>>       close(proc.in);
>> +     free(buf);
>>       sigchain_pop(SIGPIPE);
>>       return finish_command(&proc);
>>  }
>> --
>> 2.11.0.154.g5f5f154
>>
>
> --
> Brandon Williams