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

Fredrik Gustafsson [off-list ref] writes:

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.

Am 26.06.2011 21:37, schrieb Junio C Hamano:

Fredrik Gustafsson [off-list ref] writes:

quoted

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)