[PATCH] Use correct value when hinting strbuf_read()

[PATCH] Use correct value when hinting strbuf_read()

[Fredrik Gustafsson]

The git strbuf allows for each read to hint about the size of the
string. In this case the the string can never be longer than 41
characters, as it cannot contain more than a single hex-sha1 and a
newline.

So let's use 41 instead of 1024 to reduce the memory footprint.

Signed-off-by: Fredrik Gustafsson <iveqy@iveqy.com>
Mentored-by: Jens Lehmann <Jens.Lehmann@web.de>
Mentored-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 submodule.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/submodule.c b/submodule.c
index b6dec70..86baf42 100644
--- a/submodule.c
+++ b/submodule.c
@@ -326,7 +326,7 @@ static int is_submodule_commit_present(const char *path, unsigned char sha1[20])
 		cp.no_stdin = 1;
 		cp.out = -1;
 		cp.dir = path;
-		if (!run_command(&cp) && !strbuf_read(&buf, cp.out, 1024))
+		if (!run_command(&cp) && !strbuf_read(&buf, cp.out, 41))
 			is_present = 1;
 
 		close(cp.out);
-- 
1.7.5.1.229.g455f

Re: [PATCH] Use correct value when hinting strbuf_read()

[Junio C Hamano]

Fredrik Gustafsson <iveqy@iveqy.com> writes:

> diff --git a/submodule.c b/submodule.c
> index b6dec70..86baf42 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -326,7 +326,7 @@ static int is_submodule_commit_present(const char *path, unsigned char sha1[20])
>  		cp.no_stdin = 1;
>  		cp.out = -1;
>  		cp.dir = path;
> -		if (!run_command(&cp) && !strbuf_read(&buf, cp.out, 1024))
> +		if (!run_command(&cp) && !strbuf_read(&buf, cp.out, 41))
>  			is_present = 1;

The change is not incorrect per-se. If the original used 41 and a patch
tried to update it to 1024, we wouldn't accept such a patch, but on the
other hand, I do not think this patch would make much difference (any
value would do here as it is merely a hint, and if the patch does make a
difference, we would have a bigger problem, either by forking too often
with run_command(), or by leaking buf.buf every time we do so).

It however raises a more interesting question.

This function tries to see, even a commit object name, if it is fully
connected to any ref (IOW the tip of a branch or a tag). There are three
outcomes:

 - It is reachable from a ref, and we get nothing from the command;

 - It is not, and we get the commit object name back (and nothing else);

 - We get something unexpected. Perhaps an error found in the repository
   (strictly speaking I do not think we would catch this as we are not
   capturing stderr at all).

The first one is what this if() condition catches, but we do not tell the
second and the third apart. I wonder if we should.

Re: [PATCH] Use correct value when hinting strbuf_read()

[Jens Lehmann]

Am 26.06.2011 21:37, schrieb Junio C Hamano:
> Fredrik Gustafsson <iveqy@iveqy.com> writes:
> 
>> diff --git a/submodule.c b/submodule.c
>> index b6dec70..86baf42 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -326,7 +326,7 @@ static int is_submodule_commit_present(const char *path, unsigned char sha1[20])
>>  		cp.no_stdin = 1;
>>  		cp.out = -1;
>>  		cp.dir = path;
>> -		if (!run_command(&cp) && !strbuf_read(&buf, cp.out, 1024))
>> +		if (!run_command(&cp) && !strbuf_read(&buf, cp.out, 41))
>>  			is_present = 1;
> 
> The change is not incorrect per-se. If the original used 41 and a patch
> tried to update it to 1024, we wouldn't accept such a patch, but on the
> other hand, I do not think this patch would make much difference (any
> value would do here as it is merely a hint, and if the patch does make a
> difference, we would have a bigger problem, either by forking too often
> with run_command(), or by leaking buf.buf every time we do so).

Makes sense.

> It however raises a more interesting question.
> 
> This function tries to see, even a commit object name, if it is fully
> connected to any ref (IOW the tip of a branch or a tag). There are three
> outcomes:
> 
>  - It is reachable from a ref, and we get nothing from the command;
> 
>  - It is not, and we get the commit object name back (and nothing else);
> 
>  - We get something unexpected. Perhaps an error found in the repository
>    (strictly speaking I do not think we would catch this as we are not
>    capturing stderr at all).
> 
> The first one is what this if() condition catches, but we do not tell the
> second and the third apart. I wonder if we should.

Hmm, maybe we should die() if run_command() doesn't return 0? (The intention
behind not catching stderr was that the reason for the failure of rev-list
should visible to the user)