{"thread":{"id":"63817","subject":"[PATCH 0/4] Fix resource leaks in various helpers and builtin commands","startedAt":"2025-07-22T08:12:37Z","lastAt":"2025-07-22T14:53:39Z","messageCount":12,"participants":["Hoyoung Lee","Jeff King","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"522402","messageId":"20250722081219.1086866-1-lhywkd22@gmail.com","threadId":"63817","inReplyTo":null,"subject":"[PATCH 0/4] Fix resource leaks in various helpers and builtin commands","fromName":"Hoyoung Lee","fromEmail":"lhywkd22@gmail.com","sentAt":"2025-07-22T08:12:15Z","receivedAt":"2025-07-22T08:12:37Z","isPatch":true,"sender":{"key":"lhywkd22@gmail.com","avatar":"https://avatars.githubusercontent.com/u/204378075?v=4"},"body":"This patch series fixes several cases where file descriptors were not\nproperly closed on error paths. The changes affect helper programs and\na builtin command, and ensure that system resources are correctly\nreleased before returning from the function.\n\nEach fix is minimal and follows the existing style of the surrounding\ncode. These changes help improve the robustness of the code by avoiding\npotential file descriptor leaks.\n\nHoyoung Lee (4):\n  t/helper/test-truncate: close file descriptor after truncation\n  builtin/archive: close file descriptor on dup2() failure\n  t/helper/test-delta: close fd if fstat() fails after open()\n  t/helper/test-delta: close fd if fstat() fails after second open()\n\n builtin/archive.c        | 4 +++-\n t/helper/test-delta.c    | 3 +++\n t/helper/test-truncate.c | 3 +++\n 3 files changed, 9 insertions(+), 1 deletion(-)\n\n-- \n2.34.1\n\n"},{"id":"522403","messageId":"20250722081219.1086866-2-lhywkd22@gmail.com","threadId":"63817","inReplyTo":"20250722081219.1086866-1-lhywkd22@gmail.com","subject":"[PATCH v2 1/4] t/helper/test-truncate: close file descriptor after truncation","fromName":"Hoyoung Lee","fromEmail":"lhywkd22@gmail.com","sentAt":"2025-07-22T08:12:16Z","receivedAt":"2025-07-22T08:12:40Z","isPatch":true,"sender":{"key":"lhywkd22@gmail.com","avatar":"https://avatars.githubusercontent.com/u/204378075?v=4"},"body":"Fix a resource leak where the file descriptor was not closed after\ntruncating a file in t/helper/test-truncate.c.\n\nSigned-off-by: Hoyoung Lee <lhywkd22@gmail.com>\n---\n t/helper/test-truncate.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/t/helper/test-truncate.c b/t/helper/test-truncate.c\nindex 3931deaec7..104bc36cc0 100644\n--- a/t/helper/test-truncate.c\n+++ b/t/helper/test-truncate.c\n@@ -21,5 +21,8 @@ int cmd__truncate(int argc, const char **argv)\n \n \tif (ftruncate(fd, (off_t) sz) < 0)\n \t\tdie_errno(\"failed to truncate file\");\n+\t\n+\tclose(fd);\n+\n \treturn 0;\n }\n-- \n2.34.1\n\n"},{"id":"522404","messageId":"20250722081219.1086866-3-lhywkd22@gmail.com","threadId":"63817","inReplyTo":"20250722081219.1086866-1-lhywkd22@gmail.com","subject":"[PATCH v2 2/4] builtin/archive: close file descriptor on dup2() failure","fromName":"Hoyoung Lee","fromEmail":"lhywkd22@gmail.com","sentAt":"2025-07-22T08:12:17Z","receivedAt":"2025-07-22T08:12:42Z","isPatch":true,"sender":{"key":"lhywkd22@gmail.com","avatar":"https://avatars.githubusercontent.com/u/204378075?v=4"},"body":"In create_output_file(), the file descriptor returned by xopen()\nwas not closed if dup2() failed. This leads to a potential resource\nleak. Ensure the file descriptor is closed regardless of whether\ndup2() succeeds or fails.\n\nSigned-off-by: Hoyoung Lee <lhywkd22@gmail.com>\n---\n builtin/archive.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex 13ea7308c8..c919a39f90 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -14,6 +14,7 @@ static void create_output_file(const char *output_file)\n \tint output_fd = xopen(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666);\n \tif (output_fd != 1) {\n \t\tif (dup2(output_fd, 1) < 0)\n+\t\t\tclose(output_fd);\n \t\t\tdie_errno(_(\"could not redirect output\"));\n \t\telse\n \t\t\tclose(output_fd);\n-- \n2.34.1\n\n"},{"id":"522405","messageId":"20250722081219.1086866-4-lhywkd22@gmail.com","threadId":"63817","inReplyTo":"20250722081219.1086866-1-lhywkd22@gmail.com","subject":"[PATCH v2 3/4] t/helper/test-delta: close fd if fstat() fails after open()","fromName":"Hoyoung Lee","fromEmail":"lhywkd22@gmail.com","sentAt":"2025-07-22T08:12:18Z","receivedAt":"2025-07-22T08:12:43Z","isPatch":true,"sender":{"key":"lhywkd22@gmail.com","avatar":"https://avatars.githubusercontent.com/u/204378075?v=4"},"body":"If open() succeeds but fstat() fails, the file descriptor is not\nclosed, causing a resource leak. This patch adds a close(fd) call\nin the failure path after fstat() to ensure proper resource cleanup.\n\nSigned-off-by: Hoyoung Lee <lhywkd22@gmail.com>\n---\n t/helper/test-delta.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/helper/test-delta.c b/t/helper/test-delta.c\nindex 6bc787a474..103bf7f3e9 100644\n--- a/t/helper/test-delta.c\n+++ b/t/helper/test-delta.c\n@@ -31,6 +31,7 @@ int cmd__delta(int argc, const char **argv)\n \tfd = open(argv[2], O_RDONLY);\n \tif (fd < 0 || fstat(fd, &st)) {\n \t\tperror(argv[2]);\n+\t\tclose(fd);\n \t\treturn 1;\n \t}\n \tfrom_size = st.st_size;\n-- \n2.34.1\n\n"},{"id":"522406","messageId":"20250722081219.1086866-5-lhywkd22@gmail.com","threadId":"63817","inReplyTo":"20250722081219.1086866-1-lhywkd22@gmail.com","subject":"[PATCH v2 4/4] t/helper/test-delta: close fd if fstat() fails after second open()","fromName":"Hoyoung Lee","fromEmail":"lhywkd22@gmail.com","sentAt":"2025-07-22T08:12:19Z","receivedAt":"2025-07-22T08:12:45Z","isPatch":true,"sender":{"key":"lhywkd22@gmail.com","avatar":"https://avatars.githubusercontent.com/u/204378075?v=4"},"body":"When opening argv[3], if open() succeeds but fstat() fails,\nthe file descriptor is not closed, resulting in a resource leak.\nThis patch ensures that the descriptor is closed on failure.\n\nSigned-off-by: Hoyoung Lee <lhywkd22@gmail.com>\n---\n builtin/archive.c     | 3 ++-\n t/helper/test-delta.c | 2 ++\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex c919a39f90..951fc2e444 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -13,9 +13,10 @@ static void create_output_file(const char *output_file)\n {\n \tint output_fd = xopen(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666);\n \tif (output_fd != 1) {\n-\t\tif (dup2(output_fd, 1) < 0)\n+\t\tif (dup2(output_fd, 1) < 0) {\n \t\t\tclose(output_fd);\n \t\t\tdie_errno(_(\"could not redirect output\"));\n+\t\t}\n \t\telse\n \t\t\tclose(output_fd);\n \t}\ndiff --git a/t/helper/test-delta.c b/t/helper/test-delta.c\nindex 103bf7f3e9..ba2d897aa3 100644\n--- a/t/helper/test-delta.c\n+++ b/t/helper/test-delta.c\n@@ -46,6 +46,8 @@ int cmd__delta(int argc, const char **argv)\n \tfd = open(argv[3], O_RDONLY);\n \tif (fd < 0 || fstat(fd, &st)) {\n \t\tperror(argv[3]);\n+\t\tif (fd >= 0)\n+\t\t\tclose(fd);\n \t\tgoto cleanup;\n \t}\n \tdata_size = st.st_size;\n-- \n2.34.1\n\n"},{"id":"522408","messageId":"20250722082451.GB862006@coredump.intra.peff.net","threadId":"63817","inReplyTo":"20250722081219.1086866-3-lhywkd22@gmail.com","subject":"Re: [PATCH v2 2/4] builtin/archive: close file descriptor on dup2() failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-22T08:24:51Z","receivedAt":"2025-07-22T08:24:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 22, 2025 at 08:12:17AM +0000, Hoyoung Lee wrote:\n\n> In create_output_file(), the file descriptor returned by xopen()\n> was not closed if dup2() failed. This leads to a potential resource\n> leak. Ensure the file descriptor is closed regardless of whether\n> dup2() succeeds or fails.\n\nThe other patches in this series made sense to me, but I don't think\nthis one does. It is not really a \"leak\" in the sense that output_fd is\nstill on the stack when we exit (via the die_errno() call).\n\nThat may sound pedantic, but we have run into the same question with\nmemory leaks. If we are not unwinding the stack, then we can never fully\nclean up the program state. E.g., with:\n\n  void foo(void)\n  {\n\tint fd = open(...);\n\tif (bar() < 0)\n\t\tdie(...);\n  }\n\nit is tempting to think that you should close(fd) to clean up your\nstate. But now imagine that bar(), instead of returning an error, itself\nexits the program, like this:\n\n  void bar(void)\n  {\n\tif (baz() < 0)\n\t\tdie(...);\n  }\n\nIt _can't_ close \"fd\" because it doesn't even know about it!\n\nSo when we consider whether something is leaking, I think it only makes\nsense in terms of unwinding the stack. And likewise any automated tools\nwe use should consider that.\n\n> diff --git a/builtin/archive.c b/builtin/archive.c\n> index 13ea7308c8..c919a39f90 100644\n> --- a/builtin/archive.c\n> +++ b/builtin/archive.c\n> @@ -14,6 +14,7 @@ static void create_output_file(const char *output_file)\n>  \tint output_fd = xopen(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666);\n>  \tif (output_fd != 1) {\n>  \t\tif (dup2(output_fd, 1) < 0)\n> +\t\t\tclose(output_fd);\n>  \t\t\tdie_errno(_(\"could not redirect output\"));\n>  \t\telse\n>  \t\t\tclose(output_fd);\n\nSo I don't think this patch makes sense, but also...does this even\ncompile? You did not add braces around the conditional block, so we'll\ncall close() on failure but we won't call die_errno() anymore. And the\nelse is no longer coupled to the if (because of the unrelated die\nstatement in between), which should cause the compiler to barf.\n\n-Peff\n"},{"id":"522409","messageId":"20250722082557.GC862006@coredump.intra.peff.net","threadId":"63817","inReplyTo":"20250722081219.1086866-5-lhywkd22@gmail.com","subject":"Re: [PATCH v2 4/4] t/helper/test-delta: close fd if fstat() fails after second open()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-22T08:25:57Z","receivedAt":"2025-07-22T08:25:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 22, 2025 at 08:12:19AM +0000, Hoyoung Lee wrote:\n\n> When opening argv[3], if open() succeeds but fstat() fails,\n> the file descriptor is not closed, resulting in a resource leak.\n> This patch ensures that the descriptor is closed on failure.\n> \n> Signed-off-by: Hoyoung Lee <lhywkd22@gmail.com>\n> ---\n>  builtin/archive.c     | 3 ++-\n>  t/helper/test-delta.c | 2 ++\n>  2 files changed, 4 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/archive.c b/builtin/archive.c\n> index c919a39f90..951fc2e444 100644\n> --- a/builtin/archive.c\n> +++ b/builtin/archive.c\n> @@ -13,9 +13,10 @@ static void create_output_file(const char *output_file)\n>  {\n>  \tint output_fd = xopen(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666);\n>  \tif (output_fd != 1) {\n> -\t\tif (dup2(output_fd, 1) < 0)\n> +\t\tif (dup2(output_fd, 1) < 0) {\n>  \t\t\tclose(output_fd);\n>  \t\t\tdie_errno(_(\"could not redirect output\"));\n> +\t\t}\n>  \t\telse\n>  \t\t\tclose(output_fd);\n>  \t}\n\nAh, I guess you found the problem from patch 3. But it should have been\nsquashed in there, not to this patch. (But as I said there, I think we\nshould just drop patch 3 entirely).\n\n-Peff\n"},{"id":"522410","messageId":"20250722082643.GD862006@coredump.intra.peff.net","threadId":"63817","inReplyTo":"20250722082557.GC862006@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/4] t/helper/test-delta: close fd if fstat() fails after second open()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-22T08:26:43Z","receivedAt":"2025-07-22T08:26:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 22, 2025 at 04:25:57AM -0400, Jeff King wrote:\n\n> >  \tint output_fd = xopen(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666);\n> >  \tif (output_fd != 1) {\n> > -\t\tif (dup2(output_fd, 1) < 0)\n> > +\t\tif (dup2(output_fd, 1) < 0) {\n> >  \t\t\tclose(output_fd);\n> >  \t\t\tdie_errno(_(\"could not redirect output\"));\n> > +\t\t}\n> >  \t\telse\n> >  \t\t\tclose(output_fd);\n> >  \t}\n> \n> Ah, I guess you found the problem from patch 3. But it should have been\n> squashed in there, not to this patch. (But as I said there, I think we\n> should just drop patch 3 entirely).\n\nEr, sorry, I meant patch 2. Counting is hard.\n\n-Peff\n"},{"id":"522411","messageId":"20250722083516.GE862006@coredump.intra.peff.net","threadId":"63817","inReplyTo":"20250722081219.1086866-1-lhywkd22@gmail.com","subject":"Re: [PATCH 0/4] Fix resource leaks in various helpers and builtin commands","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-22T08:35:16Z","receivedAt":"2025-07-22T08:35:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 22, 2025 at 08:12:15AM +0000, Hoyoung Lee wrote:\n\n> This patch series fixes several cases where file descriptors were not\n> properly closed on error paths. The changes affect helper programs and\n> a builtin command, and ensure that system resources are correctly\n> released before returning from the function.\n> \n> Each fix is minimal and follows the existing style of the surrounding\n> code. These changes help improve the robustness of the code by avoiding\n> potential file descriptor leaks.\n> \n> Hoyoung Lee (4):\n>   t/helper/test-truncate: close file descriptor after truncation\n>   builtin/archive: close file descriptor on dup2() failure\n>   t/helper/test-delta: close fd if fstat() fails after open()\n>   t/helper/test-delta: close fd if fstat() fails after second open()\n\nI looked through these and I think patches 1, 3, and 4 are all good\n(minus the fixup for patch 2 that snuck into patch 4). I responded to\npatch 2 in the v1 thread, and i think it should be dropped.\n\nIn each case the descriptor is leaked before exiting from the main\nfunction, so they're not practical leaks (the OS will close them for us\nanyway). But it seems reasonable to me to close them anyway. It's a good\ngeneral practice, and it could help with any tools that try to\nauto-detect leaked descriptors.\n\nOut of curiosity, are you using such a tool, or just finding these\nmanually? I could imagine coverity or some other static analysis tool\nfinding them. I've never seen a tool which finds descriptor leaks the\nway we find memory leaks with LSan, etc. I'd guess it would be hard to\ndistinguish \"reachable\" global descriptors that are meant to be held\nopen versus true leaks (doubly so when we call die() in the middle of a\ndeep call chain).\n\nOne non-obvious thing to consider in reviewing this: sometimes calling\nclose(), or any other syscall, in an error code path may clobber errno,\nbreaking a message we may print (or even confusing our caller if they\ncheck errno). But I don't see any problems in these three sites.\n\n-Peff\n"},{"id":"522412","messageId":"CAPig+cT1hzz4_7JqesqFmgYdpsNezurcsF9XF1ep8KzvgMUc2Q@mail.gmail.com","threadId":"63817","inReplyTo":"20250722081219.1086866-4-lhywkd22@gmail.com","subject":"Re: [PATCH v2 3/4] t/helper/test-delta: close fd if fstat() fails after open()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-07-22T08:36:14Z","receivedAt":"2025-07-22T08:36:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jul 22, 2025 at 4:12 AM Hoyoung Lee <lhywkd22@gmail.com> wrote:\n> If open() succeeds but fstat() fails, the file descriptor is not\n> closed, causing a resource leak. This patch adds a close(fd) call\n> in the failure path after fstat() to ensure proper resource cleanup.\n>\n> Signed-off-by: Hoyoung Lee <lhywkd22@gmail.com>\n> ---\n> diff --git a/t/helper/test-delta.c b/t/helper/test-delta.c\n> @@ -31,6 +31,7 @@ int cmd__delta(int argc, const char **argv)\n>         fd = open(argv[2], O_RDONLY);\n>         if (fd < 0 || fstat(fd, &st)) {\n>                 perror(argv[2]);\n> +               close(fd);\n>                 return 1;\n>         }\n\nOne condition under which this block is entered is if `fd` is less\nthan 0, which means close() is now being called with a negative file\ndescriptor, which seems quite suspect. I'd think you would want to\neither restructure it into two `if` statements:\n\n    if (fd < 0) {\n        ...\n        return 1;\n    }\n    if (fstat(fd, ...)) {\n        ...\n        return 1;\n    }\n\nor at least protect the call to close():\n\n    if (fd < 0 || fstat(fd, ...)) {\n        ...\n        if (fd >= 0)\n            close(fd);\n        return 1;\n    }\n\nI think the separate `if` statements are probably a bit easier to\nreason about, but it is of course subjective.\n"},{"id":"522413","messageId":"20250722084059.GA863857@coredump.intra.peff.net","threadId":"63817","inReplyTo":"20250722081219.1086866-4-lhywkd22@gmail.com","subject":"Re: [PATCH v2 3/4] t/helper/test-delta: close fd if fstat() fails after open()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-22T08:40:59Z","receivedAt":"2025-07-22T08:41:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 22, 2025 at 08:12:18AM +0000, Hoyoung Lee wrote:\n\n> If open() succeeds but fstat() fails, the file descriptor is not\n> closed, causing a resource leak. This patch adds a close(fd) call\n> in the failure path after fstat() to ensure proper resource cleanup.\n> \n> Signed-off-by: Hoyoung Lee <lhywkd22@gmail.com>\n> ---\n>  t/helper/test-delta.c | 1 +\n>  1 file changed, 1 insertion(+)\n> \n> diff --git a/t/helper/test-delta.c b/t/helper/test-delta.c\n> index 6bc787a474..103bf7f3e9 100644\n> --- a/t/helper/test-delta.c\n> +++ b/t/helper/test-delta.c\n> @@ -31,6 +31,7 @@ int cmd__delta(int argc, const char **argv)\n>  \tfd = open(argv[2], O_RDONLY);\n>  \tif (fd < 0 || fstat(fd, &st)) {\n>  \t\tperror(argv[2]);\n> +\t\tclose(fd);\n>  \t\treturn 1;\n\nThis will result in close(-1) if the open() call failed (rather than the\nfstat() call). Not the end of the world, but you do the correct check in\nthe same function for patch 4.\n\nSince there are two spots here (and it looks like a third for argv[4]\nlater in the function!), perhaps it would make more sense to attach the\nclose to the cleanup label, like this:\n\ndiff --git a/t/helper/test-delta.c b/t/helper/test-delta.c\nindex 6bc787a474..d291d6f02d 100644\n--- a/t/helper/test-delta.c\n+++ b/t/helper/test-delta.c\n@@ -31,13 +31,12 @@ int cmd__delta(int argc, const char **argv)\n \tfd = open(argv[2], O_RDONLY);\n \tif (fd < 0 || fstat(fd, &st)) {\n \t\tperror(argv[2]);\n-\t\treturn 1;\n+\t\tgoto cleanup;\n \t}\n \tfrom_size = st.st_size;\n \tfrom_buf = xmalloc(from_size);\n \tif (read_in_full(fd, from_buf, from_size) < 0) {\n \t\tperror(argv[2]);\n-\t\tclose(fd);\n \t\tgoto cleanup;\n \t}\n \tclose(fd);\n@@ -51,7 +50,6 @@ int cmd__delta(int argc, const char **argv)\n \tdata_buf = xmalloc(data_size);\n \tif (read_in_full(fd, data_buf, data_size) < 0) {\n \t\tperror(argv[3]);\n-\t\tclose(fd);\n \t\tgoto cleanup;\n \t}\n \tclose(fd);\n@@ -81,5 +79,8 @@ int cmd__delta(int argc, const char **argv)\n \tfree(data_buf);\n \tfree(out_buf);\n \n+\tif (fd >= 0)\n+\t\tclose(fd);\n+\n \treturn ret;\n }\n\nIt is a little hard to see from just the diff that this always does the\nright thing, since we reuse \"fd\" over and over. But it is OK because we\nalways close() it right before calling open() again (and never jump to\ncleanup in between).\n\nProbably the whole thing would be more readable with three separate\ndescriptors initialized to -1, but I'm not sure how much effort it is\nworth putting into polishing this test helper.\n\n-Peff\n"},{"id":"522445","messageId":"xmqq34aouvim.fsf@gitster.g","threadId":"63817","inReplyTo":"20250722082643.GD862006@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/4] t/helper/test-delta: close fd if fstat() fails after second open()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-22T14:53:37Z","receivedAt":"2025-07-22T14:53:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Jul 22, 2025 at 04:25:57AM -0400, Jeff King wrote:\n>\n>> >  \tint output_fd = xopen(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666);\n>> >  \tif (output_fd != 1) {\n>> > -\t\tif (dup2(output_fd, 1) < 0)\n>> > +\t\tif (dup2(output_fd, 1) < 0) {\n>> >  \t\t\tclose(output_fd);\n>> >  \t\t\tdie_errno(_(\"could not redirect output\"));\n>> > +\t\t}\n>> >  \t\telse\n>> >  \t\t\tclose(output_fd);\n>> >  \t}\n>> \n>> Ah, I guess you found the problem from patch 3. But it should have been\n>> squashed in there, not to this patch. (But as I said there, I think we\n>> should just drop patch 3 entirely).\n>\n> Er, sorry, I meant patch 2. Counting is hard.\n\n;-)\n\nThanks for taking a look on these patches before I wake up, so I\ndidn't have to ;-)\n"}]}