From patchwork Thu Jun 6 22:19:25 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Taylor Blau X-Patchwork-Id: 13689014 Received: from mail-oi1-f180.google.com (mail-oi1-f180.google.com [209.85.167.180]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id E880473444 for ; Thu, 6 Jun 2024 22:19:28 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.167.180 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1717712370; cv=none; b=MGtarzYDZPZiDw5YBH0bW1+MHbcG8STjh7Uvu7ZK27aqIZ05okI0rfgtRALmHA33scNYYQlkoB3tjRlrMcGmhw6WF+tPPqPctd6zF/czUQaXBkwa6H+uxjHDVSFYHPPr/FvngkDXxuzF2PcnW87KNjKIcACqC0Xz6HJk8LGBg+4= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1717712370; c=relaxed/simple; bh=JOAkrmUgXhuNd+/svUtRn0M3GA/394hWuIltutc4Fio=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=YM9TMVnZIVZtfWiipPipSxtjvpHGluX4ryO7x7pyNvpnFhG9GgcGa3C6piaDVj2hsisCu/QQ5bpQuM7EHy++3S6oPj1jiR2DvTxx+Np3hhvhNQNFE3pGn8r2+BdtgtklxV3ZX63anevEkD9sbgUjTP9nmK1HuQ4fDslXLazz3Jk= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ttaylorr.com; spf=none smtp.mailfrom=ttaylorr.com; dkim=pass (2048-bit key) header.d=ttaylorr-com.20230601.gappssmtp.com header.i=@ttaylorr-com.20230601.gappssmtp.com header.b=PLat2oNd; arc=none smtp.client-ip=209.85.167.180 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ttaylorr.com Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=ttaylorr.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=ttaylorr-com.20230601.gappssmtp.com header.i=@ttaylorr-com.20230601.gappssmtp.com header.b="PLat2oNd" Received: by mail-oi1-f180.google.com with SMTP id 5614622812f47-3d2062325fbso917117b6e.0 for ; Thu, 06 Jun 2024 15:19:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ttaylorr-com.20230601.gappssmtp.com; s=20230601; t=1717712367; x=1718317167; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=Wtre4JRs2W0SS0GSCyMPvBQPAD+pzKsUaqxsmxaVFlk=; b=PLat2oNdtcSJSxx56UA27Uf4LaVTPQTjH5XUn3KF5CrcRRaj1KoaHUDSNutTwITY6F nJ73kWxPEjwGRYHjDKE7TtrZQAtq8tO5Gie2dEfhFBrS9ZHDvLledfu8rfEaYipC2P1O Jd1QS8jv3jsnqaahIEW5m8xfGAu0Q8e/JQbXhoPzkuBk3b6hno7jlEFrGHTLXSi+GWMn RysE2khllp3kHN9qoUfbMvwj9Y5s/jTZr7QDRvXeKCCU9kqpbfGaaD0iR0Q41Fd1V3ff BGWxw3egDeJKe/XZzukZiz0txznykMwC4gptOASJxQkRQA4bvAnj5DIW1MV6HY8N9ARX jmuA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1717712367; x=1718317167; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=Wtre4JRs2W0SS0GSCyMPvBQPAD+pzKsUaqxsmxaVFlk=; b=ac8G3H5uIKzAwhLGP6rpQplAxUHdMk4u0A0tb0s35Eins5KwlFaiPWRBWaRyl97bHy uz0P9v6cpkd5aYPGpZY8qdES7zHRbCuVGDLmGZLB3q1lhaz19QT7wAVCv0ksFsnorBqV MYsQhe03CF94Qnltx5cYJ+cteuTUwmqlvN2J4O/rMer+Sau0eUXn9a/zjNdLhEIIeHc+ WA6z54BR2RTIEmOCODpqkpmQT8JdoNM4hNVkLKyzENJ+aUcgrm3ntjOIQUzQ3A4qx//x gmR6LJTpL+NcWGBd/hpSuJZzhmjyPF584tBlhu3A0xmuvsnJLQnNax75Y/nx6G89k78t OV9A== X-Gm-Message-State: AOJu0YwfzJB03PubE0Hgqs2BDixbYcHlQn5wk45b3RWzeQhyYBzcoR8E I7kxglVJBxjRr26uHllXV7wVg/ZxrGn0242jYB156V2BlyUIezF7VGfv8T9aaZEEh+56zicCU5u G/fc= X-Google-Smtp-Source: AGHT+IGZmwi7UK/MNI9xOlhBNz/PAI2oeVUAF+rMmxCQRdfUjvoRfriLIPmHv62DyEOgSETVtFv75g== X-Received: by 2002:a05:6808:249:b0:3d1:fdf5:a88e with SMTP id 5614622812f47-3d210d71220mr729825b6e.21.1717712367504; Thu, 06 Jun 2024 15:19:27 -0700 (PDT) Received: from localhost (104-178-186-189.lightspeed.milwwi.sbcglobal.net. [104.178.186.189]) by smtp.gmail.com with ESMTPSA id 6a1803df08f44-6b04fa39b1esm10112826d6.143.2024.06.06.15.19.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 06 Jun 2024 15:19:27 -0700 (PDT) Date: Thu, 6 Jun 2024 18:19:25 -0400 From: Taylor Blau To: git@vger.kernel.org Cc: Jeff King , Elijah Newren , Junio C Hamano Subject: [PATCH 1/2] commit-graph.c: remove temporary graph layers on exit Message-ID: <25324fea5b7c7f748d7f4e1e40299c0af04006e8.1717712358.git.me@ttaylorr.com> References: Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Since the introduction of split commit graph layers in 92b1ea66b9a (Merge branch 'ds/commit-graph-incremental', 2019-07-19), the function write_commit_graph_file() has done the following when writing an incremental commit-graph layer: - used a lock_file to control access to the commit-graph-chain file - used an auxiliary file (whose descriptor was stored in 'fd') to write the new commit-graph layer itself Using a lock_file to control access to the commit-graph-chain is sensible, since only one writer may modify it at a time. Likewise, when the commit-graph machinery is writing out a single layer, the lock_file structure is used to modify the commit-graph itself. This is also sensible, since the non-incremental commit-graph may also have at most one writer. However, using an auxiliary temporary file without using the tempfile.h API means that writes that fail after the temporary graph layer has been created will leave around a file in $GIT_DIR/objects/info/commit-graphs/tmp_graph_XXXXXX The commit-graph-chain file and non-incremental commit-graph do not suffer from this problem as the lockfile.h API uses the tempfile.h API transparently, so processes that died before moving those finals into their final location cleaned up after themselves. Ensure that the temporary file used to write incremental commit-graphs is also managed with the tempfile.h API, to ensure that we do not ever leave tmp_graph_XXXXXX files laying around. Signed-off-by: Taylor Blau --- commit-graph.c | 19 ++++++++----------- t/t5324-split-commit-graph.sh | 26 +++++++++++++++++++++++++- 2 files changed, 33 insertions(+), 12 deletions(-) diff --git a/commit-graph.c b/commit-graph.c index e5dd3553dfe..92c71637142 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -2002,8 +2002,8 @@ static int write_graph_chunk_base(struct hashfile *f, static int write_commit_graph_file(struct write_commit_graph_context *ctx) { uint32_t i; - int fd; struct hashfile *f; + struct tempfile *graph_layer; /* when ctx->split is non-zero */ struct lock_file lk = LOCK_INIT; const unsigned hashsz = the_hash_algo->rawsz; struct strbuf progress_title = STRBUF_INIT; @@ -2035,24 +2035,23 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx) LOCK_DIE_ON_ERROR, 0444); free(lock_name); - fd = git_mkstemp_mode(ctx->graph_name, 0444); - if (fd < 0) { + graph_layer = mks_tempfile_m(ctx->graph_name, 0444); + if (!graph_layer) { error(_("unable to create temporary graph layer")); return -1; } - if (adjust_shared_perm(ctx->graph_name)) { + if (adjust_shared_perm(get_tempfile_path(graph_layer))) { error(_("unable to adjust shared permissions for '%s'"), - ctx->graph_name); + get_tempfile_path(graph_layer)); return -1; } - f = hashfd(fd, ctx->graph_name); + f = hashfd(get_tempfile_fd(graph_layer), get_tempfile_path(graph_layer)); } else { hold_lock_file_for_update_mode(&lk, ctx->graph_name, LOCK_DIE_ON_ERROR, 0444); - fd = get_lock_file_fd(&lk); - f = hashfd(fd, get_lock_file_path(&lk)); + f = hashfd(get_lock_file_fd(&lk), get_lock_file_path(&lk)); } cf = init_chunkfile(f); @@ -2133,8 +2132,6 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx) char *final_graph_name; int result; - close(fd); - if (!chainf) { error(_("unable to open commit-graph chain file")); return -1; @@ -2169,7 +2166,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx) free(ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1]); ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1] = final_graph_name; - result = rename(ctx->graph_name, final_graph_name); + result = rename_tempfile(&graph_layer, final_graph_name); for (i = 0; i < ctx->num_commit_graphs_after; i++) fprintf(get_lock_file_fp(&lk), "%s\n", ctx->commit_graph_hash_after[i]); diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh index 281266f7883..77e91547ea3 100755 --- a/t/t5324-split-commit-graph.sh +++ b/t/t5324-split-commit-graph.sh @@ -13,7 +13,8 @@ test_expect_success 'setup repo' ' git init && git config core.commitGraph true && git config gc.writeCommitGraph false && - infodir=".git/objects/info" && + objdir=".git/objects" && + infodir="$objdir/info" && graphdir="$infodir/commit-graphs" && test_oid_cache <<-EOM shallow sha1:2132 @@ -718,4 +719,27 @@ test_expect_success 'write generation data chunk when commit-graph chain is repl ) ' +test_expect_success 'temporary graph layer is discarded upon failure' ' + git init layer-discard && + ( + cd layer-discard && + + test_commit A && + test_commit B && + + # Intentionally remove commit "A" from the object store + # so that the commit-graph machinery fails to parse the + # parents of "B". + # + # This takes place after the commit-graph machinery has + # initialized a new temporary file to store the contents + # of the new graph layer, so will allow us to ensure + # that the temporary file is discarded upon failure. + rm $objdir/$(test_oid_to_path $(git rev-parse HEAD^)) && + + test_must_fail git commit-graph write --reachable --split && + test_dir_is_empty $graphdir + ) +' + test_done From patchwork Thu Jun 6 22:19:31 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Taylor Blau X-Patchwork-Id: 13689015 Received: from mail-oi1-f173.google.com (mail-oi1-f173.google.com [209.85.167.173]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6D98273444 for ; Thu, 6 Jun 2024 22:19:34 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.167.173 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1717712375; cv=none; b=EOPri+cMhd+8Cde6qGjYgjAg30qE6Es4VJBhTPWfeWbmzfEBKLRCcZj6VwB8rsa3Vu2PMc0Zg1S4lmkVNZ8Zrkyo0HVJivS8QYnvCbE5ipiiOStUbG2uVs4kct4J8aZ7U4t/5zOwWDbaZ91PP+EDZP6f9Zy/rB/qJ/GN391jtMA= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1717712375; c=relaxed/simple; bh=Q7UBmu9GzPEFqjJJauo7s+5JBmtilcQsK5bzE0XMwbI=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=PXrydFUXAgR5o6oJv0K6HyTOtdw5X4WXavUamkgOky+aif3fL60aZTyTAE2wlHELiJiAJwRZASloiaOj7vboagEp7VjAQHdSUhnS1+ylxXaZ+cZiXscPcNQK/QXC7FuXnSQMzlrg7yNrMSjWGyQeirX2dFUL2y8RT86Jjt3VuTQ= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ttaylorr.com; spf=none smtp.mailfrom=ttaylorr.com; dkim=pass (2048-bit key) header.d=ttaylorr-com.20230601.gappssmtp.com header.i=@ttaylorr-com.20230601.gappssmtp.com header.b=rCxcZ2x6; arc=none smtp.client-ip=209.85.167.173 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ttaylorr.com Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=ttaylorr.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=ttaylorr-com.20230601.gappssmtp.com header.i=@ttaylorr-com.20230601.gappssmtp.com header.b="rCxcZ2x6" Received: by mail-oi1-f173.google.com with SMTP id 5614622812f47-3d1ffaf6091so824066b6e.3 for ; Thu, 06 Jun 2024 15:19:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ttaylorr-com.20230601.gappssmtp.com; s=20230601; t=1717712373; x=1718317173; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=CHL//jDFzqaeLoKKP5Z/bq5VZ3YyKc9m5Ot3xsEJyQQ=; b=rCxcZ2x6DF4QT/qzU+tUHnMZcrdF7oaygAcH8L6gRuD3LQEy8KdywrIhUsv9ei8TzE L6MuYK5dzet5HRRGzqL757KHeU4Wrsmqr9/m9tDKfbwI5cdlumCrpndlc/q8+5w0WntK i3LT+dVrjiZfO7wiV4p2O3r4nNrcUGWUAOCCUyvTmUPqhEmo8x4u3sjzuo/A78+pww+M ojqVNJKNlpeQSGI9VZQ4Hp3y0FjAKy4rjFu07/rav7wsjz+ukaQGZtdFBsPF/rWK2Pl/ jkqPnErX5gtkHA2RxuOEoxwrhuDL3+5qswTwewy0zvskSWWI6UBLJkW8wcHEjfcmGnx0 KDTA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1717712373; x=1718317173; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=CHL//jDFzqaeLoKKP5Z/bq5VZ3YyKc9m5Ot3xsEJyQQ=; b=aD8tjlYanKWs/qNz9zYUWo9YM28MPYj8vIP+c6V9ziw4s2tMKNovFlK41xMAushLu8 sMAKQdr3RVDP+dADzTvBWN/Xt3gbIW1dG51q7/9/Hwh1Di7QoGw6ureOVvTKqJQPM0LK oVdd7qJTpz+sjWrcW/lEhsJIhU+RjV0JT/NbHB64JBJqn/Mkd17uOKI1+5CRqokl6fWM GZbuuwPYlpN4Y2AJAtS3mI0IcBIsLxSI91OdwUdh48j5aAygnUkl+l3BZbpjKqviv5mC e6spss4iTjH2qFxh6CLBgkKYCPRqBGC2IegR4EA8T8SoyjMHFuZhgmeLOeRoDWCBJC3T tIGA== X-Gm-Message-State: AOJu0YwRC+4EUILGPwewPKk8GYgRIGgJEp6ChsI+JNM3uZ/s6s3eCCiL YeGslD77SaeXUrmQCIYlk4BGYpNs2BY6K/f8ovExq2U+s6UpDXWJ7rxb5tUokxcIRasnWE1Brfv 9rA4= X-Google-Smtp-Source: AGHT+IFZEgM+ir/7k6K4lxvvfl5xtP2KokHWTs8xdDHFGu/y2lG7SWc2ynmaOUjNxEsB7xq62l5U6w== X-Received: by 2002:a05:6808:23ca:b0:3d1:fd7e:2234 with SMTP id 5614622812f47-3d210d699bfmr1038825b6e.14.1717712373198; Thu, 06 Jun 2024 15:19:33 -0700 (PDT) Received: from localhost (104-178-186-189.lightspeed.milwwi.sbcglobal.net. [104.178.186.189]) by smtp.gmail.com with ESMTPSA id af79cd13be357-795331c9e2esm99888685a.101.2024.06.06.15.19.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 06 Jun 2024 15:19:32 -0700 (PDT) Date: Thu, 6 Jun 2024 18:19:31 -0400 From: Taylor Blau To: git@vger.kernel.org Cc: Jeff King , Elijah Newren , Junio C Hamano Subject: [PATCH 2/2] server-info.c: remove temporary info files on exit Message-ID: <2d5a0536af1a6d45835622e2c020266079fa0873.1717712358.git.me@ttaylorr.com> References: Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: The update_info_file() function within server-info.c is responsible for moving the info/refs and info/packs files around when updating server info. These updates are staged into a temporary file and then moved into place atomically to avoid race conditions when reading those files. However, the temporary file used to stage these changes is managed outside of the tempfile.h API, and thus survives process death. Manage these files instead with the tempfile.h API so that they are automatically cleaned up upon abnormal process death. Unfortunately, and unlike in the previous step, there isn't a straightforward way to inject a failure into the update-server-info step that causes us to die() rather than take the cleanup path in label 'out', hence the lack of a test here. Signed-off-by: Taylor Blau --- server-info.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/server-info.c b/server-info.c index 6feaa457c5c..37d1085982e 100644 --- a/server-info.c +++ b/server-info.c @@ -13,6 +13,7 @@ #include "object-store-ll.h" #include "server-info.h" #include "strbuf.h" +#include "tempfile.h" struct update_info_ctx { FILE *cur_fp; @@ -75,9 +76,8 @@ static int update_info_file(char *path, int force) { char *tmp = mkpathdup("%s_XXXXXX", path); + struct tempfile *f = NULL; int ret = -1; - int fd = -1; - FILE *to_close; struct update_info_ctx uic = { .cur_fp = NULL, .old_fp = NULL, @@ -86,13 +86,12 @@ static int update_info_file(char *path, }; safe_create_leading_directories(path); - fd = git_mkstemp_mode(tmp, 0666); - if (fd < 0) + f = mks_tempfile_m(tmp, 0666); + if (!f) goto out; - to_close = uic.cur_fp = fdopen(fd, "w"); + uic.cur_fp = fdopen_tempfile(f, "w"); if (!uic.cur_fp) goto out; - fd = -1; /* no problem on ENOENT and old_fp == NULL, it's stale, now */ if (!force) @@ -121,27 +120,22 @@ static int update_info_file(char *path, } uic.cur_fp = NULL; - if (fclose(to_close)) - goto out; if (uic_is_stale(&uic)) { - if (adjust_shared_perm(tmp) < 0) + if (adjust_shared_perm(get_tempfile_path(f)) < 0) goto out; - if (rename(tmp, path) < 0) + if (rename_tempfile(&f, path) < 0) goto out; } else { - unlink(tmp); + delete_tempfile(&f); } ret = 0; out: if (ret) { error_errno("unable to update %s", path); - if (uic.cur_fp) - fclose(uic.cur_fp); - else if (fd >= 0) - close(fd); - unlink(tmp); + if (f) + delete_tempfile(&f); } free(tmp); if (uic.old_fp)