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

[Jens Lehmann <Jens.Lehmann@web.de>]

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)