Re: [PATCHv3 2/2] builtin/commit.c: switch to xstrfmt(), instead of snprintf,
2017-01-15 3:42 GMT+01:00 Junio C Hamano : > Elia Pinto writes: > >> Ok. I agree. But is it strictly necessary to resend for this ? > > FWIW, the attacched is what I queued locally, after complaining > "both have the same title? They need to be explained better." > > In any case, I sense that 2/2 will be redone using strbuf, from the > looks of what is discussed in a subthread nearby? Yes, it seems to me correct, I resend the patch 2/2 based on the review thank you all > > Thanks. > > commit 8d7aa4ba6a00b3ff69261e88b4842c0df5662125 > Author: Elia Pinto > Date: Fri Jan 13 17:58:00 2017 + > > builtin/commit.c: remove the PATH_MAX limitation via dynamic allocation > > Remove the PATH_MAX limitation from the environment setting that > points to a filename by switching to dynamic allocation. > > As a side effect of this change, we also reduce the snprintf() > calls, that may silently truncate results if the programmer is not > careful. > > Helped-by: Junio C Hamano > Helped-by: Jeff King > Signed-off-by: Elia Pinto > Signed-off-by: Junio C Hamano > > commit 2a7e328877982557d921a398af9442093290c613 > Author: Elia Pinto > Date: Fri Jan 13 17:58:01 2017 + > > builtin/commit.c: switch to xstrfmt(), instead of snprintf() > > Switch to dynamic allocation with xstrfmt(), so we can avoid dealing > with magic numbers in the code and reduce the cognitive burden from > the programmers. The original code is correct, but programmers no > longer have to count bytes needed for static allocation to know that. > > As a side effect of this change, we also reduce the snprintf() > calls, that may silently truncate results if the programmer is not > careful. > > Helped-by: Junio C Hamano > Helped-by: Jeff King > Signed-off-by: Elia Pinto > Signed-off-by: Junio C Hamano
Re: [PATCHv3 2/2] builtin/commit.c: switch to xstrfmt(), instead of snprintf,
Elia Pinto writes: > Ok. I agree. But is it strictly necessary to resend for this ? FWIW, the attacched is what I queued locally, after complaining "both have the same title? They need to be explained better." In any case, I sense that 2/2 will be redone using strbuf, from the looks of what is discussed in a subthread nearby? Thanks. commit 8d7aa4ba6a00b3ff69261e88b4842c0df5662125 Author: Elia Pinto Date: Fri Jan 13 17:58:00 2017 + builtin/commit.c: remove the PATH_MAX limitation via dynamic allocation Remove the PATH_MAX limitation from the environment setting that points to a filename by switching to dynamic allocation. As a side effect of this change, we also reduce the snprintf() calls, that may silently truncate results if the programmer is not careful. Helped-by: Junio C Hamano Helped-by: Jeff King Signed-off-by: Elia Pinto Signed-off-by: Junio C Hamano commit 2a7e328877982557d921a398af9442093290c613 Author: Elia Pinto Date: Fri Jan 13 17:58:01 2017 + builtin/commit.c: switch to xstrfmt(), instead of snprintf() Switch to dynamic allocation with xstrfmt(), so we can avoid dealing with magic numbers in the code and reduce the cognitive burden from the programmers. The original code is correct, but programmers no longer have to count bytes needed for static allocation to know that. As a side effect of this change, we also reduce the snprintf() calls, that may silently truncate results if the programmer is not careful. Helped-by: Junio C Hamano Helped-by: Jeff King Signed-off-by: Elia Pinto Signed-off-by: Junio C Hamano
Re: [PATCHv3 2/2] builtin/commit.c: switch to xstrfmt(), instead of snprintf,
On Sat, Jan 14, 2017 at 05:31:39PM +0100, René Scharfe wrote: > Perhaps I missed it from the discussion, but why not use strbuf? It > would avoid counting the generated string's length. That's probably > not going to make a measurable difference performance-wise, but it's > easy to avoid and doesn't even take up more lines: It was mentioned in the review of the very first patch, but I think it just got dropped. I agree the strbuf way is nicer (not because of efficiency, but because it's simply easier to read and follow). -Peff
Re: [PATCHv3 2/2] builtin/commit.c: switch to xstrfmt(), instead of snprintf,
Am 13.01.2017 um 18:58 schrieb Elia Pinto:
> 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
> Helped-by: Jeff King
> Signed-off-by: Elia Pinto
> ---
> 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.spi...@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@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);
> }
>
Perhaps I missed it from the discussion, but why not use strbuf? It
would avoid counting the generated string's length. That's probably
not going to make a measurable difference performance-wise, but it's
easy to avoid and doesn't even take up more lines:
---
builtin/commit.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 711f96cc43..73bb72016f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1525,12 +1525,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];
+ struct strbuf sb = STRBUF_INIT;
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])
@@ -1546,11 +1544,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));
+ strbuf_addf(&sb, "%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, sb.buf, sb.len);
close(proc.in);
+ strbuf_release(&sb);
sigchain_pop(SIGPIPE);
return finish_command(&proc);
}
--
2.11.0
Re: [PATCHv3 2/2] builtin/commit.c: switch to xstrfmt(), instead of snprintf,
Ok. I agree. But is it strictly necessary to resend for this ?
Thanks
2017-01-13 19:33 GMT+01:00 Brandon Williams :
> 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
>> Helped-by: Jeff King
>> Signed-off-by: Elia Pinto
>
> 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.spi...@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@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
Re: [PATCHv3 2/2] builtin/commit.c: switch to xstrfmt(), instead of snprintf,
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
> Helped-by: Jeff King
> Signed-off-by: Elia Pinto
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.spi...@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@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
[PATCHv3 2/2] builtin/commit.c: switch to xstrfmt(), instead of snprintf,
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
Helped-by: Jeff King
Signed-off-by: Elia Pinto
---
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.spi...@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@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
