[PATCHv3 1/2] builtin/commit.c: removes the PATH_MAX limitation via dynamic allocation

[PATCHv3 1/2] builtin/commit.c: removes the PATH_MAX limitation via dynamic allocation

[Elia Pinto]

This patch removes the PATH_MAX limitation from the environment 
setting that points to a filename, we'd want to handle larger
paths anyway, so we switch to dynamic 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>
---
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)
- drop the arg_array_clear call after exit(1)
https://public-inbox.org/git/xmqqtw95mfo3.fsf@gitster.mtv.corp.google.com/T/#m2e6405a8a78a8ca1ed770614c91398290574c4a1


 builtin/commit.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 0ed634b26..09bcc0f13 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -960,15 +960,15 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		return 0;
 
 	if (use_editor) {
-		char index[PATH_MAX];
-		const char *env[2] = { NULL };
-		env[0] =  index;
-		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
-		if (launch_editor(git_path_commit_editmsg(), NULL, env)) {
+		struct argv_array env = ARGV_ARRAY_INIT;
+
+		argv_array_pushf(&env, "GIT_INDEX_FILE=%s", index_file);
+		if (launch_editor(git_path_commit_editmsg(), NULL, env.argv)) {
 			fprintf(stderr,
 			_("Please supply the message using either -m or -F option.\n"));
 			exit(1);
 		}
+		argv_array_clear(&env);
 	}
 
 	if (!no_verify &&
@@ -1557,23 +1557,22 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
 
 int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...)
 {
-	const char *hook_env[3] =  { NULL };
-	char index[PATH_MAX];
+	struct argv_array hook_env = ARGV_ARRAY_INIT;
 	va_list args;
 	int ret;
 
-	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
-	hook_env[0] = index;
+	argv_array_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);
 
 	/*
 	 * Let the hook know that no editor will be launched.
 	 */
 	if (!editor_is_used)
-		hook_env[1] = "GIT_EDITOR=:";
+		argv_array_push(&hook_env, "GIT_EDITOR=:");
 
 	va_start(args, name);
-	ret = run_hook_ve(hook_env, name, args);
+	ret = run_hook_ve(hook_env.argv,name, args);
 	va_end(args);
+	argv_array_clear(&hook_env);
 
 	return ret;
 }
-- 
2.11.0.154.g5f5f154

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

[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 <gitster@pobox.com>
Helped-by: Jeff King <peff@peff.net> 
Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
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

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

[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 <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

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

[Elia Pinto]

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>:
> 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

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

[René Scharfe]

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 <gitster@pobox.com>
> Helped-by: Jeff King <peff@peff.net> 
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
> 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);
>  }
> 

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,

[Jeff King]

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,

[Junio C Hamano]

Elia Pinto <gitter.spiros@gmail.com> 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 <gitter.spiros@gmail.com>
Date:   Fri Jan 13 17:58:00 2017 +0000

    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 <gitster@pobox.com>
    Helped-by: Jeff King <peff@peff.net>
    Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

commit 2a7e328877982557d921a398af9442093290c613
Author: Elia Pinto <gitter.spiros@gmail.com>
Date:   Fri Jan 13 17:58:01 2017 +0000

    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 <gitster@pobox.com>
    Helped-by: Jeff King <peff@peff.net>
    Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

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

[Elia Pinto]

2017-01-15 3:42 GMT+01:00 Junio C Hamano <gitster@pobox.com>:
> Elia Pinto <gitter.spiros@gmail.com> 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 <gitter.spiros@gmail.com>
> Date:   Fri Jan 13 17:58:00 2017 +0000
>
>     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 <gitster@pobox.com>
>     Helped-by: Jeff King <peff@peff.net>
>     Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> commit 2a7e328877982557d921a398af9442093290c613
> Author: Elia Pinto <gitter.spiros@gmail.com>
> Date:   Fri Jan 13 17:58:01 2017 +0000
>
>     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 <gitster@pobox.com>
>     Helped-by: Jeff King <peff@peff.net>
>     Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>