path: root/t/
diff options
authorJeff King <>2021-05-19 11:17:15 (GMT)
committerJunio C Hamano <>2021-05-19 12:14:59 (GMT)
commit6aacb7d8619c50b3c92fe3e99b93ffa9b6065dfc (patch)
treebede78d8580144ad882c1fde30f2db402db43033 /t/
parent94f6e3e283f2adfc518b39cfc39291f1c2832ad0 (diff)
clone: clean up directory after transport_fetch_refs() failure
git-clone started respecting errors from the transport subsystem in aab179d937 (builtin/clone.c: don't ignore transport_fetch_refs() errors, 2020-12-03). However, that commit didn't handle the cleanup of the filesystem quite right. The cleanup of the directory that cmd_clone() creates is done by an atexit() handler, which we control with a flag. It starts as JUNK_LEAVE_NONE ("clean up everything"), then progresses to JUNK_LEAVE_REPO when we know we have a valid repo but not working tree, and then finally JUNK_LEAVE_ALL when we have a successful checkout. Most errors cause us to die(), which then triggers the handler to do the right thing based on how far into cmd_clone() we got. But the checks added by aab179d937 instead set the "err" variable and then jump to a new "cleanup" label, which then returns our non-zero status. However, the code after the cleanup label includes setting the flag to JUNK_LEAVE_ALL, and so we accidentally leave the repository and working tree in place. One obvious option to fix this is to reorder the end of the function to set the flag first, before cleanup code, and put the label between them. But we can observe another small bug: the error return from transport_fetch_refs() is generally "-1", and we propagate that to the return value of cmd_clone(), which ultimately becomes the exit code of the process. And we try to avoid transmitting negative values via exit codes (only the low 8 bits are passed along as an unsigned value, though in practice for "-1" this at least retains the property that it's non-zero). Instead, let's just die(). That makes us consistent with rest of the code in the function. It does add a new "fatal:" line to the output, but I'd argue that's a good thing: - in the rare case that the transport code didn't say anything, now the user gets _some_ error message - even if the transport code said something like "error: ssh died of signal 9", it's nice to also say "fatal" to indicate that we considered that to be a show-stopper. Triggering this in the test suite turns out to be surprisingly difficult. Almost every error we'd encounter, including ones deep inside the transport code, cause us to just die() right there! However, one way is to put a fake wrapper around git-upload-pack that sends the complete packfile but exits with a failure code. Signed-off-by: Jeff King <> Signed-off-by: Junio C Hamano <>
Diffstat (limited to 't/')
1 files changed, 7 insertions, 0 deletions
diff --git a/t/ b/t/
index 4a1a912..5bf1026 100755
--- a/t/
+++ b/t/
@@ -97,4 +97,11 @@ test_expect_success 'failed clone into empty leaves directory (separate, wt)' '
test_dir_is_empty empty-wt
+test_expect_success 'transport failure cleans up directory' '
+ test_must_fail git clone --no-local \
+ -u "f() { git-upload-pack \"\$@\"; return 1; }; f" \
+ foo broken-clone &&
+ test_path_is_missing broken-clone