{"thread":{"id":"55732","subject":"[PATCH] clone: clean up directory after transport_fetch_refs() failure","startedAt":"2021-05-19T11:17:20Z","lastAt":"2021-05-19T14:30:34Z","messageCount":2,"participants":["Jeff King","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"424938","messageId":"YKTzu96fN7XlpDoG@coredump.intra.peff.net","threadId":"55732","inReplyTo":null,"subject":"[PATCH] clone: clean up directory after transport_fetch_refs() failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-19T11:17:15Z","receivedAt":"2021-05-19T11:17:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"git-clone started respecting errors from the transport subsystem in\naab179d937 (builtin/clone.c: don't ignore transport_fetch_refs() errors,\n2020-12-03). However, that commit didn't handle the cleanup of the\nfilesystem quite right.\n\nThe cleanup of the directory that cmd_clone() creates is done by an\natexit() handler, which we control with a flag. It starts as\nJUNK_LEAVE_NONE (\"clean up everything\"), then progresses to\nJUNK_LEAVE_REPO when we know we have a valid repo but not working tree,\nand then finally JUNK_LEAVE_ALL when we have a successful checkout.\n\nMost errors cause us to die(), which then triggers the handler to do the\nright thing based on how far into cmd_clone() we got. But the checks\nadded by aab179d937 instead set the \"err\" variable and then jump to a\nnew \"cleanup\" label, which then returns our non-zero status. However,\nthe code after the cleanup label includes setting the flag to\nJUNK_LEAVE_ALL, and so we accidentally leave the repository and working\ntree in place.\n\nOne obvious option to fix this is to reorder the end of the function to\nset the flag first, before cleanup code, and put the label between them.\n\nBut we can observe another small bug: the error return from\ntransport_fetch_refs() is generally \"-1\", and we propagate that to the\nreturn value of cmd_clone(), which ultimately becomes the exit code of\nthe process. And we try to avoid transmitting negative values via exit\ncodes (only the low 8 bits are passed along as an unsigned value, though\nin practice for \"-1\" this at least retains the property that it's\nnon-zero).\n\nInstead, let's just die(). That makes us consistent with rest of the\ncode in the function. It does add a new \"fatal:\" line to the output, but\nI'd argue that's a good thing:\n\n  - in the rare case that the transport code didn't say anything, now\n    the user gets _some_ error message\n\n  - even if the transport code said something like \"error: ssh died of\n    signal 9\", it's nice to also say \"fatal\" to indicate that we\n    considered that to be a show-stopper.\n\nTriggering this in the test suite turns out to be surprisingly\ndifficult. Almost every error we'd encounter, including ones deep inside\nthe transport code, cause us to just die() right there! However, one way\nis to put a fake wrapper around git-upload-pack that sends the complete\npackfile but exits with a failure code.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nPulling this out of the discussion in:\n\n  https://lore.kernel.org/git/YKTg8nYjSGpKbq8W@coredump.intra.peff.net/\n\nThe problem is in v2.30.0, so this doesn't need to be part of the\nupcoming v2.32 release (but it's probably a good candidate for maint).\nNote that it isn't even a regression in v2.30. Before then, we ignored\nthe error completely! So the fix there simply didn't go far enough. :)\n\n builtin/clone.c               | 11 ++++-------\n t/t5600-clone-fail-cleanup.sh |  7 +++++++\n 2 files changed, 11 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex eeb74c0217..66fe66679c 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1320,9 +1320,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\t}\n \n \t\tif (!is_local && !complete_refs_before_fetch) {\n-\t\t\terr = transport_fetch_refs(transport, mapped_refs);\n-\t\t\tif (err)\n-\t\t\t\tgoto cleanup;\n+\t\t\tif (transport_fetch_refs(transport, mapped_refs))\n+\t\t\t\tdie(_(\"remote transport reported error\"));\n \t\t}\n \n \t\tremote_head = find_ref_by_name(refs, \"HEAD\");\n@@ -1380,9 +1379,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (is_local)\n \t\tclone_local(path, git_dir);\n \telse if (refs && complete_refs_before_fetch) {\n-\t\terr = transport_fetch_refs(transport, mapped_refs);\n-\t\tif (err)\n-\t\t\tgoto cleanup;\n+\t\tif (transport_fetch_refs(transport, mapped_refs))\n+\t\t\tdie(_(\"remote transport reported error\"));\n \t}\n \n \tupdate_remote_refs(refs, mapped_refs, remote_head_points_at,\n@@ -1410,7 +1408,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tjunk_mode = JUNK_LEAVE_REPO;\n \terr = checkout(submodule_progress);\n \n-cleanup:\n \tfree(remote_name);\n \tstrbuf_release(&reflog_msg);\n \tstrbuf_release(&branch_top);\ndiff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh\nindex 4a1a912e03..5bf10261d3 100755\n--- a/t/t5600-clone-fail-cleanup.sh\n+++ b/t/t5600-clone-fail-cleanup.sh\n@@ -97,4 +97,11 @@ test_expect_success 'failed clone into empty leaves directory (separate, wt)' '\n \ttest_dir_is_empty empty-wt\n '\n \n+test_expect_success 'transport failure cleans up directory' '\n+\ttest_must_fail git clone --no-local \\\n+\t\t-u \"f() { git-upload-pack \\\"\\$@\\\"; return 1; }; f\" \\\n+\t\tfoo broken-clone &&\n+\ttest_path_is_missing broken-clone\n+'\n+\n test_done\n-- \n2.32.0.rc0.424.g95d3bde94f\n"},{"id":"424964","messageId":"YKUhBdO1KSiLwqOZ@nand.local","threadId":"55732","inReplyTo":"YKTzu96fN7XlpDoG@coredump.intra.peff.net","subject":"Re: [PATCH] clone: clean up directory after transport_fetch_refs() failure","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-05-19T14:30:29Z","receivedAt":"2021-05-19T14:30:34Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, May 19, 2021 at 07:17:15AM -0400, Jeff King wrote:\n> One obvious option to fix this is to reorder the end of the function to\n> set the flag first, before cleanup code, and put the label between them.\n>\n> But we can observe another small bug: the error return from\n> transport_fetch_refs() is generally \"-1\", and we propagate that to the\n> return value of cmd_clone(), which ultimately becomes the exit code of\n> the process. And we try to avoid transmitting negative values via exit\n> codes (only the low 8 bits are passed along as an unsigned value, though\n> in practice for \"-1\" this at least retains the property that it's\n> non-zero).\n>\n> Instead, let's just die(). That makes us consistent with rest of the\n> code in the function. It does add a new \"fatal:\" line to the output, but\n> I'd argue that's a good thing:\n\nThis reasoning makes sense to me, and thanks for cleaning this up.\n\n    Reviewed-by: Taylor Blau <me@ttaylorr.com>\n\nThanks,\nTaylor\n"}]}