Junio C Hamano wrote: > Jim Meyering <jim@xxxxxxxxxxxx> writes: > >> diff --git a/transport-helper.c b/transport-helper.c >> index f57e84c..0bbd014 100644 >> --- a/transport-helper.c >> +++ b/transport-helper.c >> @@ -49,6 +49,7 @@ static struct child_process *get_helper(struct transport *transport) >> if (!strcmp(buf.buf, "fetch")) >> data->fetch = 1; >> } >> + fclose (file); >> return data->helper; >> } >> >> @@ -88,6 +89,7 @@ static int fetch_with_fetch(struct transport *transport, >> if (strbuf_getline(&buf, file, '\n') == EOF) >> exit(128); /* child died, message supplied already */ >> } >> + fclose (file); >> return 0; >> } > > The callchain of fetch_with_fetch() looks like: > > fetch_with_fetch() > helper = get_helper(); > --> get_helper() > - start helper with start_command(); > - read from helper->out until it sees an empty line; > - break out of the loop; > <-- return helper > - file = xfdopen(helper->out) to get another FILE on the fd > - read the rest of the output from helper->out via file > > It seems to me that the fclose() in get_helper() will close the underlying > fd and would break the caller, no? I confess that my sole test was to run "make test", which passed. If the fd must live on, a slightly less invasive change would be to xdup each descriptor just before each of the three xfdopen calls, e.g., - file = xfdopen(helper->out, "r"); + file = xfdopen(xdup(helper->out), "r"); > I think "struct helper_data" should get a new FILE* field and once > somebody creates a FILE* out of its helper->out, that FILE* can be passed > around without a new xfdopen(). > > Or something like that. That's probably best. > Who is responsible for closing the underlying helper->out fd in the > start_command() API, by the way? -- 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