Am 26.06.2011 21:37, schrieb Junio C Hamano: > Fredrik Gustafsson <iveqy@xxxxxxxxx> 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) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html