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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 26, 2011, 19:37 UTC
Message-ID: <7vzkl4z8nl.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1308161318-25637-1-git-send-email-iveqy@iveqy.com>

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.