threads / patch / 27631

patchUse correct value when hinting strbuf_read()

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

## tl;dr

3 messages between Jun 15, 2011 and Jun 26, 2011. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Fredrik Gustafsson· Jun 15, 2011, 18:08 UTC · lore

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(-)

Show changes to submodule.c +1 −1

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

Junio C Hamano· Jun 26, 2011, 19:37 UTC · re: Fredrik Gustafsson · lore

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

Fredrik Gustafsson <iveqy@iveqy.com> writes:

Show 11 quoted lines

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

Jens Lehmann· Jun 26, 2011, 20:32 UTC · re: Junio C Hamano · lore

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

Am 26.06.2011 21:37, schrieb Junio C Hamano:

Show 20 quoted lines

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

Show 16 quoted lines

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