From patchwork Wed Mar 5 17:38:56 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Karthik Nayak X-Patchwork-Id: 14003065 Received: from mail-ej1-f48.google.com (mail-ej1-f48.google.com [209.85.218.48]) (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 1F66617B401 for ; Wed, 5 Mar 2025 17:39:16 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.218.48 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741196359; cv=none; b=XWNT09XFD3jjYZPLL0E70YTQt+qwKx1D9CnTzUOpm4D9NfWaarV5yOcVtNP2HoBjAGV9jjsJC4/9MlB9BUKiWzjQ2g7u/adu/HchMWlZnDuEHrPJgUemwq/KXQ8Hzcdj3T6KMrHANSpONUYXonj44qbEFsNy8txx2BDY8aeje+s= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741196359; c=relaxed/simple; bh=ijcews+R3T9WRiFIz5DXUchs6rSe6Or9l3vjvHuB7SA=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:References: In-Reply-To:To:Cc; b=HDKZ1i/oKXrfFus1S1/AUGRiLfjdkxBwSk3SynM8Dx/ghBFkvDkw5RFIMrEw2MQFqGRAS8pvc4NdIrYF003T2c14GZNgx15pT3o8QA+f6vQyrrq+VOIiRDKAPiPUet/NSqJ4wFOYCZjlB15na7Uq+yvipuJ+gI8iZSEsIbB+ASU= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=SA/xvE64; arc=none smtp.client-ip=209.85.218.48 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="SA/xvE64" Received: by mail-ej1-f48.google.com with SMTP id a640c23a62f3a-ac1f5157c90so372805166b.0 for ; Wed, 05 Mar 2025 09:39:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1741196355; x=1741801155; darn=vger.kernel.org; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:from:to:cc:subject:date:message-id :reply-to; bh=zwFj8lgbcCEReiZ0VMtFUt5egxOBI7jMsqXr/ycBxS4=; b=SA/xvE64EvKT4V+NPF95r4QRgbwHhVteHW61PaTyDJHeTewfaQ5RZeCfqgb5aSdVJt GTVrQM9fX5S5+bRI3VvsmKvmxaJQKnlqNtoq4CncaZLfTSL8FbIFfy4lnYJ8de1vUMlG KRNYh2UYowDZia6Xwjvvk1MEAizpi0iM0wpFOyvRlVorPSStrtmrRDA0SrKawVRJmP+M 0D6UTgY9hQ146Ua6HYVV6D8rvjlkupYTtgfc68mvsgGxkBUqZA2cQokKprmDMInZJ8B5 ysShdVbhWCgMqfkEaeskY5m7nGAQTbFoaRkQWvtK0hqSztX9T3YWxvseB4aOqXeVATyY PAtQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1741196355; x=1741801155; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=zwFj8lgbcCEReiZ0VMtFUt5egxOBI7jMsqXr/ycBxS4=; b=GRG3fRY1rv1rU+4oxHwGokLj1eKF+73lEu1jPtbs8h8+Ca8de+fmPXd18fNmuWIclr may/UThIHllmVnLvPUwR2MB5FlJ2y6d62mZiVAOeCLiCpyMljd856ngLRo68YPVtw+A8 Zh1FmY10aWZnj22Uy3mZVLidKcrcSKXukv/m/D76qE9uQ6cMOKcHTcF8Ol1rx+IDnaD0 KktqsqyAC9apQcs0KSWf9S4vp6N/hzVi2V2gSSmVHUYqMCVBgxYD5Qr+naXp9Q3kweXv D0cW/+F9COjT6QdE6AWml8zHm7LRLapuMQBs92cGceMc7/FIL3UCPYx9agdAh8KYtc6s nr9w== X-Gm-Message-State: AOJu0Yz5X+z+hIH8ijCV4VZcRoYP1N9xdw7yDQUrJFbEAuCu+Ginu8Us LaO/VEQJ4VdltI9S8R3yo/2Z4ff5PorM1n2QS4sSwKrtPYRmAOUJHWxzP3EG X-Gm-Gg: ASbGncuRrEQGYok5Pu8UFmyJGo02iEoOp0fGtWYRj/HeYAb5o/8dvKBPUIRC4qrTDnF 690csAq1DTeK48B/V1NcdHIPUtr6DZUFb8cDw1+8cxwjfEISNQ12jYQTebF9dNCp4vkgIPHgl2g j1iXPWCOHYiR8yTwz9PNS3Ojx2ehWkXe6i2r0/uFqfrUbZdMXZ9rJ0C04JtVJ03vt0wf0F1+FXm XHT+8MM+oCpD2c7X8NV6Y8k4AYAzDgwrRy3dAsyBv5RP1clwZt/05UfNdgcpogKaCf9PD0Jn6zK RIqBvG438oLzz6eWJDpGldDmufe+MbUl6qf1sjtClOGmYva9KQ== X-Google-Smtp-Source: AGHT+IG1yI+itF01Rstu97McQMUdpHIgmV1oToWMdrAs723M1RcG+xN7arvVBwVsZ6RxcppNOtFglA== X-Received: by 2002:a17:907:6e92:b0:ac1:f759:f9e7 with SMTP id a640c23a62f3a-ac20da187b5mr353594866b.23.1741196354965; Wed, 05 Mar 2025 09:39:14 -0800 (PST) Received: from [127.0.0.2] ([2a02:2455:8268:bc00:8a90:b290:3a5b:4dd]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-abf3a8e2f53sm946851866b.115.2025.03.05.09.39.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 05 Mar 2025 09:39:14 -0800 (PST) From: Karthik Nayak Date: Wed, 05 Mar 2025 18:38:56 +0100 Subject: [PATCH v3 1/8] refs/files: remove redundant check in split_symref_update() Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Message-Id: <20250305-245-partially-atomic-ref-updates-v3-1-0c64e3052354@gmail.com> References: <20250305-245-partially-atomic-ref-updates-v3-0-0c64e3052354@gmail.com> In-Reply-To: <20250305-245-partially-atomic-ref-updates-v3-0-0c64e3052354@gmail.com> To: git@vger.kernel.org Cc: Karthik Nayak , ps@pks.im, jltobler@gmail.com, phillip.wood123@gmail.com X-Mailer: b4 0.15-dev X-Developer-Signature: v=1; a=openpgp-sha256; l=3232; i=karthik.188@gmail.com; h=from:subject:message-id; bh=ijcews+R3T9WRiFIz5DXUchs6rSe6Or9l3vjvHuB7SA=; b=owJ4nAHtARL+kA0DAAoBPtWfJI5GjH8ByyZiAGfIjEB8VYlcX/44MtlLJQXQFWuhyioSttenz 1QZu7c765c6dYkBswQAAQoAHRYhBFfOTH9jdXEPy2XGBj7VnySORox/BQJnyIxAAAoJED7VnySO Rox/RzUL/RlWbRQ4Cd0LDUJy+I6Jl2GzxPoVfkvLPWhXnS9ISRKGoLpw+aPk33yRUPomCqaMfzk Qtj+aj537OGfW+9MGTZajM3WtYDJNW2symJ8cMi3NjKUsq0boiFSKYHHjInVIzXa/yNtzVA7Yqx JAPfZkyF014ZpGLxN+ntnWCcOlJ1UeptOHxSvOzzw7KpK2EpnsUYjJqSWnOoRYpyvASPd7BSNIR Dau325PlGWOLsjneJP5OaOYgVH3hHzGY14rpmDiTvpITwaOoqpLOVSQ19idGTYf64Ew1PDgcMsW MZkEkmfuKDePhl59mjpvvznlk3qQmGfrh1SlnTd4lsvyfc7iF7LFKVvTCKITs7/rx+7UT3Gt9LF lvC0oNObu21rByqj6sckTrGdpzq+NjWwS39d36hQaF9fCqRpAq6hiq5guHrl+0y8Y8RerAm8SnU CjP0Fh6DZtgwSRjOwtOP76dH8xM+Fzv1Dx0k9/NLP3trBrUkqRFJwGUlIxtqjtEPgPCTdKqMz75 qU= X-Developer-Key: i=karthik.188@gmail.com; a=openpgp; fpr=57CE4C7F6375710FCB65C6063ED59F248E468C7F In `split_symref_update()`, there were two checks for duplicate refnames: - At the start, `string_list_has_string()` ensures the refname is not already in `affected_refnames`, preventing duplicates from being added. - After adding the refname, another check verifies whether the newly inserted item has a `util` value. The second check is unnecessary because the first one guarantees that `string_list_insert()` will never encounter a preexisting entry. Since `item->util` is only used in this context, remove the assignment and simplify the surrounding code. Signed-off-by: Karthik Nayak --- refs/files-backend.c | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 4e1c50fead..6c7df30738 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2382,7 +2382,6 @@ static int split_head_update(struct ref_update *update, struct string_list *affected_refnames, struct strbuf *err) { - struct string_list_item *item; struct ref_update *new_update; if ((update->flags & REF_LOG_ONLY) || @@ -2421,8 +2420,7 @@ static int split_head_update(struct ref_update *update, */ if (strcmp(new_update->refname, "HEAD")) BUG("%s unexpectedly not 'HEAD'", new_update->refname); - item = string_list_insert(affected_refnames, new_update->refname); - item->util = new_update; + string_list_insert(affected_refnames, new_update->refname); return 0; } @@ -2441,7 +2439,6 @@ static int split_symref_update(struct ref_update *update, struct string_list *affected_refnames, struct strbuf *err) { - struct string_list_item *item; struct ref_update *new_update; unsigned int new_flags; @@ -2496,11 +2493,7 @@ static int split_symref_update(struct ref_update *update, * be valid as long as affected_refnames is in use, and NOT * referent, which might soon be freed by our caller. */ - item = string_list_insert(affected_refnames, new_update->refname); - if (item->util) - BUG("%s unexpectedly found in affected_refnames", - new_update->refname); - item->util = new_update; + string_list_insert(affected_refnames, new_update->refname); return 0; } @@ -2834,7 +2827,6 @@ static int files_transaction_prepare(struct ref_store *ref_store, */ for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; - struct string_list_item *item; if ((update->flags & REF_IS_PRUNING) && !(update->flags & REF_NO_DEREF)) @@ -2843,13 +2835,7 @@ static int files_transaction_prepare(struct ref_store *ref_store, if (update->flags & REF_LOG_ONLY) continue; - item = string_list_append(&affected_refnames, update->refname); - /* - * We store a pointer to update in item->util, but at - * the moment we never use the value of this field - * except to check whether it is non-NULL. - */ - item->util = update; + string_list_append(&affected_refnames, update->refname); } string_list_sort(&affected_refnames); if (ref_update_reject_duplicates(&affected_refnames, err)) { From patchwork Wed Mar 5 17:38:57 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Karthik Nayak X-Patchwork-Id: 14003067 Received: from mail-ej1-f51.google.com (mail-ej1-f51.google.com [209.85.218.51]) (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 7D44D1957E4 for ; Wed, 5 Mar 2025 17:39:18 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.218.51 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741196360; cv=none; b=fusrNgw80TP8kZi7y9lJvpcA9ue7a5hC6i+8krixmkMH7zumSASLM23RNqeCGgs1VVKS5D8hichSzKmVEGbnA02Ab6oiSOsijlZ7/j6pkLXtADAJ5TshDV+mCDU5Ln1wHVA8cJhCibGHoENPHrKOemrFv6MZi8UMAR+YDSgDz3o= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741196360; c=relaxed/simple; bh=5bt+/QhQReQpcV1NPz9FwIG1Tj/OgN92R/3ZBHzcVrU=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:References: In-Reply-To:To:Cc; b=EtoylysIf7aXuTSFvwUXlZ+FUtcjx+zQ37lfMK8ba0XgSHzv0I7ZnlUN8nqZktq+SlPJm7nmsx/TURffE6hC9Z4m/HTVzRm/Go6K/tX3FyvmtRYvgXpwQOilWWhGR2SOwEWiiF4yAk1yCDAu4SiqKD2f9+CgAg3kxgiLem6v2EI= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=XFd9Tkgf; arc=none smtp.client-ip=209.85.218.51 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="XFd9Tkgf" Received: by mail-ej1-f51.google.com with SMTP id a640c23a62f3a-ab7430e27b2so1134318866b.3 for ; Wed, 05 Mar 2025 09:39:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1741196356; x=1741801156; darn=vger.kernel.org; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:from:to:cc:subject:date:message-id :reply-to; bh=+SM5DsjCKB+t7zOnRuTK/2XNfabBuFKuKPrBHO1I+Kg=; b=XFd9Tkgf4h0wubhwuT0d1zimPffZuIKMsUXuaEC29YQHsELTm+OdIlPf/Gqqpd+NDW mrGwV0v91s12qU0xXCyuYGlZPEfDSqmfABFUdAIcUQvPFFf/WYYxp6dWEtONabIdE9Hi IIHvJasUA0eXTL0wbctpw0XggIkNgkoXpjbP8jtoP/jRPhXiZHSo3ibw9ruVuOUCYGG8 qogu6l/7t8Pn1H0Hl5ySnTfFR4HfNQrEvrsZI3liFx7JQuoAjkKqLJl/ZtgsSsC33pgx I4Ry/2765Oen6Wekt8t0mK8x3NltX0FxcbbVJD9r8tPEvGb3LrCqivi1HUVWdvijFOHW svoQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1741196356; x=1741801156; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=+SM5DsjCKB+t7zOnRuTK/2XNfabBuFKuKPrBHO1I+Kg=; b=VS2qe1iiXRxBeX02YmO2s4e1JfJu0VRNcCZkXBK4a5QMlM++V63RWgL9JXgQzyOZp/ 08H2DJL7ZLSXi/UD1OwbP0yOCe7LxfwHv5yIfvvO4SkL3Ayc24KJbC+xXdVebwFgWb1M CjR65gMNO0/Thy1yUtPugU5RDSIqm5ognX0kzNy+k+frcipfr3soH4MxCeFY9f0vgNcq JXfZ3JVJjVgxgUDf676TiOtaU6VdNxEZGSZ1k6TYd8acyZAtXtjcm74GnJnRfO/xx+bS UTt+VF4kjXjfSE31qtux+4EyreCXiuvVSlLJUMbLNpZiPI7y8HrAcdhbqRkKC+CqydS5 dtQQ== X-Gm-Message-State: AOJu0YyQ/el9ZmiuJmGKNCY/3853lkrrS0L1i91d2sENwAwqCjaLe+cL 86GoExk/IGrt5cyuU4Al97NXtKPVT3EH/sVq/ME9sL+femQYtc+R1u/i6Aql X-Gm-Gg: ASbGncvYUXvVtxyEfhriUGy5K6TDtX2ge4grS94Z0sU/T8vcs4K9q1guYmKeS+HiQh7 WfDZV2H4M32d5dc374U6g81eqnn9K6MJGv8BsRWbJa5SGptEiMw86M0/XCGEK/FYFTXaIeaoD1f 36gpOeN+L1Qc7ybOfNXKr8DW8xtpQzIzcxP53DsoomR1wUAtqrcY8lGOvYnyAEtlEYWlcQ2bjDb 8xY8udqeodE93vuLe5r1wqokQ7ubCA21fpUOmNoyYrC4Y7JOXbHM2uZ6bng/5mgNNvl1Nws8Wo/ Pu9rNsr0Iz447Irr/KRzyRTvUaHQ32fQeoGIfZxWSKjtnR+58A== X-Google-Smtp-Source: AGHT+IHTXdKuro9LEjM5z5WCY0mOI2g2mxBjxEgO+Vk39hPMxP5Oz0+ZGJIMDpbkkFP2EBlK2DJYcQ== X-Received: by 2002:a17:907:d112:b0:abf:70af:f33a with SMTP id a640c23a62f3a-ac20dac4909mr400973366b.21.1741196355909; Wed, 05 Mar 2025 09:39:15 -0800 (PST) Received: from [127.0.0.2] ([2a02:2455:8268:bc00:8a90:b290:3a5b:4dd]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-abf3a8e2f53sm946851866b.115.2025.03.05.09.39.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 05 Mar 2025 09:39:15 -0800 (PST) From: Karthik Nayak Date: Wed, 05 Mar 2025 18:38:57 +0100 Subject: [PATCH v3 2/8] refs: move duplicate refname update check to generic layer Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Message-Id: <20250305-245-partially-atomic-ref-updates-v3-2-0c64e3052354@gmail.com> References: <20250305-245-partially-atomic-ref-updates-v3-0-0c64e3052354@gmail.com> In-Reply-To: <20250305-245-partially-atomic-ref-updates-v3-0-0c64e3052354@gmail.com> To: git@vger.kernel.org Cc: Karthik Nayak , ps@pks.im, jltobler@gmail.com, phillip.wood123@gmail.com X-Mailer: b4 0.15-dev X-Developer-Signature: v=1; a=openpgp-sha256; l=18724; i=karthik.188@gmail.com; h=from:subject:message-id; bh=5bt+/QhQReQpcV1NPz9FwIG1Tj/OgN92R/3ZBHzcVrU=; b=owJ4nAHtARL+kA0DAAoBPtWfJI5GjH8ByyZiAGfIjEDuctQg2uSCcouvfH9bmMEZmqj1yZtHH C7kxD+BXWdgSYkBswQAAQoAHRYhBFfOTH9jdXEPy2XGBj7VnySORox/BQJnyIxAAAoJED7VnySO Rox/mUML/2/0goFNHYxVa9z9U7tMmWNEoD/TK93rOODPqxahRFFmv2PAT1O29eJDbx4ieMcJgsp wUE0pxZvTTzptRxp1X/GLW3uDxMBLQ5rC+FCxcfyzW4mLYlrcXnD4IdzfUdaZuW+7+C+gsO0IYx HOIDzEQ9Djpiff8EwCoS5vS1oPiJ3zfmtQxxdl6w5DPE1rwVk3rWehGkoh/K7AUfLHM8j4lQxnt VIIa7xXHDTgwPfXJxgmsgOaUe6K2drQi68vg7CW2Rj4N6oxyZAm6msEHw6pygE9piCwVEMMG3t5 hQcNIS+kmSgLamuSMsy5kWIE+9WQzs14iDwGm84ANjMrpn++2newnF3U79zaB6H5fPHR7hxKdAc 9VDi0h1oLDWgQ3b4zKcBO260iSe4qdU15PGfJx4h8+eXe8nxZE0yS5x1haCcJBsBg1Ll2+dZIow k5omid7BqBln8bh+YzKMwLcFBruv+wGNhXDJVRShYsk0x5FU1gYXqBCgs6dwS0+i0BH4Pb2RZi4 48= X-Developer-Key: i=karthik.188@gmail.com; a=openpgp; fpr=57CE4C7F6375710FCB65C6063ED59F248E468C7F Move the tracking of refnames in `affected_refnames` from individual backends into the generic layer in 'refs.c'. This centralizes the duplicate refname detection that was previously handled separately by each backend. Make some changes to accommodate this move: - Add a `string_list` field `refnames` to `ref_transaction` to contain all the references in a transaction. This field is updated whenever a new update is added via `ref_transaction_add_update`, so manual additions in reference backends are dropped. - Modify the backends to use this field internally as needed. The backends need to check if an update for refname already exists when splitting symrefs or adding an update for 'HEAD'. - In the reftable backend, within `reftable_be_transaction_prepare()`, move the `string_list_has_string()` check above `ref_transaction_add_update()`. Since `ref_transaction_add_update()` automatically adds the refname to `transaction->refnames`, performing the check after will always return true, so we perform the check before adding the update. This helps reduce duplication of functionality between the backends and makes it easier to make changes in a more centralized manner. Signed-off-by: Karthik Nayak --- refs.c | 17 +++++++++++++ refs/files-backend.c | 67 +++++++++++-------------------------------------- refs/packed-backend.c | 25 +----------------- refs/refs-internal.h | 2 ++ refs/reftable-backend.c | 54 +++++++++++++-------------------------- 5 files changed, 51 insertions(+), 114 deletions(-) diff --git a/refs.c b/refs.c index 54fd5ce21e..ab69746947 100644 --- a/refs.c +++ b/refs.c @@ -1175,6 +1175,7 @@ struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs, CALLOC_ARRAY(tr, 1); tr->ref_store = refs; tr->flags = flags; + string_list_init_dup(&tr->refnames); return tr; } @@ -1205,6 +1206,7 @@ void ref_transaction_free(struct ref_transaction *transaction) free((char *)transaction->updates[i]->old_target); free(transaction->updates[i]); } + string_list_clear(&transaction->refnames, 0); free(transaction->updates); free(transaction); } @@ -1218,6 +1220,7 @@ struct ref_update *ref_transaction_add_update( const char *committer_info, const char *msg) { + struct string_list_item *item; struct ref_update *update; if (transaction->state != REF_TRANSACTION_OPEN) @@ -1245,6 +1248,16 @@ struct ref_update *ref_transaction_add_update( update->msg = normalize_reflog_message(msg); } + /* + * This list is generally used by the backends to avoid duplicates. + * But we do support multiple log updates for a given refname within + * a single transaction. + */ + if (!(update->flags & REF_LOG_ONLY)) { + item = string_list_append(&transaction->refnames, refname); + item->util = update; + } + return update; } @@ -2405,6 +2418,10 @@ int ref_transaction_prepare(struct ref_transaction *transaction, return -1; } + string_list_sort(&transaction->refnames); + if (ref_update_reject_duplicates(&transaction->refnames, err)) + return TRANSACTION_GENERIC_ERROR; + ret = refs->be->transaction_prepare(refs, transaction, err); if (ret) return ret; diff --git a/refs/files-backend.c b/refs/files-backend.c index 6c7df30738..85ed85ad87 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2378,9 +2378,7 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st */ static int split_head_update(struct ref_update *update, struct ref_transaction *transaction, - const char *head_ref, - struct string_list *affected_refnames, - struct strbuf *err) + const char *head_ref, struct strbuf *err) { struct ref_update *new_update; @@ -2398,7 +2396,7 @@ static int split_head_update(struct ref_update *update, * transaction. This check is O(lg N) in the transaction * size, but it happens at most once per transaction. */ - if (string_list_has_string(affected_refnames, "HEAD")) { + if (string_list_has_string(&transaction->refnames, "HEAD")) { /* An entry already existed */ strbuf_addf(err, "multiple updates for 'HEAD' (including one " @@ -2420,7 +2418,6 @@ static int split_head_update(struct ref_update *update, */ if (strcmp(new_update->refname, "HEAD")) BUG("%s unexpectedly not 'HEAD'", new_update->refname); - string_list_insert(affected_refnames, new_update->refname); return 0; } @@ -2436,7 +2433,6 @@ static int split_head_update(struct ref_update *update, static int split_symref_update(struct ref_update *update, const char *referent, struct ref_transaction *transaction, - struct string_list *affected_refnames, struct strbuf *err) { struct ref_update *new_update; @@ -2448,7 +2444,7 @@ static int split_symref_update(struct ref_update *update, * size, but it happens at most once per symref in a * transaction. */ - if (string_list_has_string(affected_refnames, referent)) { + if (string_list_has_string(&transaction->refnames, referent)) { /* An entry already exists */ strbuf_addf(err, "multiple updates for '%s' (including one " @@ -2486,15 +2482,6 @@ static int split_symref_update(struct ref_update *update, update->flags |= REF_LOG_ONLY | REF_NO_DEREF; update->flags &= ~REF_HAVE_OLD; - /* - * Add the referent. This insertion is O(N) in the transaction - * size, but it happens at most once per symref in a - * transaction. Make sure to add new_update->refname, which will - * be valid as long as affected_refnames is in use, and NOT - * referent, which might soon be freed by our caller. - */ - string_list_insert(affected_refnames, new_update->refname); - return 0; } @@ -2558,7 +2545,6 @@ static int lock_ref_for_update(struct files_ref_store *refs, struct ref_transaction *transaction, const char *head_ref, struct string_list *refnames_to_check, - struct string_list *affected_refnames, struct strbuf *err) { struct strbuf referent = STRBUF_INIT; @@ -2575,8 +2561,7 @@ static int lock_ref_for_update(struct files_ref_store *refs, update->flags |= REF_DELETING; if (head_ref) { - ret = split_head_update(update, transaction, head_ref, - affected_refnames, err); + ret = split_head_update(update, transaction, head_ref, err); if (ret) goto out; } @@ -2586,9 +2571,8 @@ static int lock_ref_for_update(struct files_ref_store *refs, lock->count++; } else { ret = lock_raw_ref(refs, update->refname, mustexist, - refnames_to_check, affected_refnames, - &lock, &referent, - &update->type, err); + refnames_to_check, &transaction->refnames, + &lock, &referent, &update->type, err); if (ret) { char *reason; @@ -2642,9 +2626,8 @@ static int lock_ref_for_update(struct files_ref_store *refs, * of processing the split-off update, so we * don't have to do it here. */ - ret = split_symref_update(update, - referent.buf, transaction, - affected_refnames, err); + ret = split_symref_update(update, referent.buf, + transaction, err); if (ret) goto out; } @@ -2799,7 +2782,6 @@ static int files_transaction_prepare(struct ref_store *ref_store, "ref_transaction_prepare"); size_t i; int ret = 0; - struct string_list affected_refnames = STRING_LIST_INIT_NODUP; struct string_list refnames_to_check = STRING_LIST_INIT_NODUP; char *head_ref = NULL; int head_type; @@ -2818,12 +2800,7 @@ static int files_transaction_prepare(struct ref_store *ref_store, transaction->backend_data = backend_data; /* - * Fail if a refname appears more than once in the - * transaction. (If we end up splitting up any updates using - * split_symref_update() or split_head_update(), those - * functions will check that the new updates don't have the - * same refname as any existing ones.) Also fail if any of the - * updates use REF_IS_PRUNING without REF_NO_DEREF. + * Fail if any of the updates use REF_IS_PRUNING without REF_NO_DEREF. */ for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; @@ -2831,16 +2808,6 @@ static int files_transaction_prepare(struct ref_store *ref_store, if ((update->flags & REF_IS_PRUNING) && !(update->flags & REF_NO_DEREF)) BUG("REF_IS_PRUNING set without REF_NO_DEREF"); - - if (update->flags & REF_LOG_ONLY) - continue; - - string_list_append(&affected_refnames, update->refname); - } - string_list_sort(&affected_refnames); - if (ref_update_reject_duplicates(&affected_refnames, err)) { - ret = TRANSACTION_GENERIC_ERROR; - goto cleanup; } /* @@ -2882,7 +2849,7 @@ static int files_transaction_prepare(struct ref_store *ref_store, ret = lock_ref_for_update(refs, update, transaction, head_ref, &refnames_to_check, - &affected_refnames, err); + err); if (ret) goto cleanup; @@ -2929,7 +2896,7 @@ static int files_transaction_prepare(struct ref_store *ref_store, * So instead, we accept the race for now. */ if (refs_verify_refnames_available(refs->packed_ref_store, &refnames_to_check, - &affected_refnames, NULL, 0, err)) { + &transaction->refnames, NULL, 0, err)) { ret = TRANSACTION_NAME_CONFLICT; goto cleanup; } @@ -2975,7 +2942,6 @@ static int files_transaction_prepare(struct ref_store *ref_store, cleanup: free(head_ref); - string_list_clear(&affected_refnames, 0); string_list_clear(&refnames_to_check, 0); if (ret) @@ -3050,13 +3016,8 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, if (transaction->state != REF_TRANSACTION_PREPARED) BUG("commit called for transaction that is not prepared"); - /* Fail if a refname appears more than once in the transaction: */ - for (i = 0; i < transaction->nr; i++) - if (!(transaction->updates[i]->flags & REF_LOG_ONLY)) - string_list_append(&affected_refnames, - transaction->updates[i]->refname); - string_list_sort(&affected_refnames); - if (ref_update_reject_duplicates(&affected_refnames, err)) { + string_list_sort(&transaction->refnames); + if (ref_update_reject_duplicates(&transaction->refnames, err)) { ret = TRANSACTION_GENERIC_ERROR; goto cleanup; } @@ -3074,7 +3035,7 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, * that we are creating already exists. */ if (refs_for_each_rawref(&refs->base, ref_present, - &affected_refnames)) + &transaction->refnames)) BUG("initial ref transaction called with existing refs"); packed_transaction = ref_store_transaction_begin(refs->packed_ref_store, diff --git a/refs/packed-backend.c b/refs/packed-backend.c index f4c82ba2c7..19220d2e99 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1622,8 +1622,6 @@ int is_packed_transaction_needed(struct ref_store *ref_store, struct packed_transaction_backend_data { /* True iff the transaction owns the packed-refs lock. */ int own_lock; - - struct string_list updates; }; static void packed_transaction_cleanup(struct packed_ref_store *refs, @@ -1632,8 +1630,6 @@ static void packed_transaction_cleanup(struct packed_ref_store *refs, struct packed_transaction_backend_data *data = transaction->backend_data; if (data) { - string_list_clear(&data->updates, 0); - if (is_tempfile_active(refs->tempfile)) delete_tempfile(&refs->tempfile); @@ -1658,7 +1654,6 @@ static int packed_transaction_prepare(struct ref_store *ref_store, REF_STORE_READ | REF_STORE_WRITE | REF_STORE_ODB, "ref_transaction_prepare"); struct packed_transaction_backend_data *data; - size_t i; int ret = TRANSACTION_GENERIC_ERROR; /* @@ -1671,34 +1666,16 @@ static int packed_transaction_prepare(struct ref_store *ref_store, */ CALLOC_ARRAY(data, 1); - string_list_init_nodup(&data->updates); transaction->backend_data = data; - /* - * Stick the updates in a string list by refname so that we - * can sort them: - */ - for (i = 0; i < transaction->nr; i++) { - struct ref_update *update = transaction->updates[i]; - struct string_list_item *item = - string_list_append(&data->updates, update->refname); - - /* Store a pointer to update in item->util: */ - item->util = update; - } - string_list_sort(&data->updates); - - if (ref_update_reject_duplicates(&data->updates, err)) - goto failure; - if (!is_lock_file_locked(&refs->lock)) { if (packed_refs_lock(ref_store, 0, err)) goto failure; data->own_lock = 1; } - if (write_with_updates(refs, &data->updates, err)) + if (write_with_updates(refs, &transaction->refnames, err)) goto failure; transaction->state = REF_TRANSACTION_PREPARED; diff --git a/refs/refs-internal.h b/refs/refs-internal.h index e5862757a7..92db793026 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -3,6 +3,7 @@ #include "refs.h" #include "iterator.h" +#include "string-list.h" struct fsck_options; struct ref_transaction; @@ -198,6 +199,7 @@ enum ref_transaction_state { struct ref_transaction { struct ref_store *ref_store; struct ref_update **updates; + struct string_list refnames; size_t alloc; size_t nr; enum ref_transaction_state state; diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 441b8c69c1..f616d9aabe 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1076,7 +1076,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, struct reftable_ref_store *refs = reftable_be_downcast(ref_store, REF_STORE_WRITE|REF_STORE_MAIN, "ref_transaction_prepare"); struct strbuf referent = STRBUF_INIT, head_referent = STRBUF_INIT; - struct string_list affected_refnames = STRING_LIST_INIT_NODUP; struct string_list refnames_to_check = STRING_LIST_INIT_NODUP; struct reftable_transaction_data *tx_data = NULL; struct reftable_backend *be; @@ -1101,10 +1100,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, transaction->updates[i], err); if (ret) goto done; - - if (!(transaction->updates[i]->flags & REF_LOG_ONLY)) - string_list_append(&affected_refnames, - transaction->updates[i]->refname); } /* @@ -1116,17 +1111,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, tx_data->args[i].updates_alloc = tx_data->args[i].updates_expected; } - /* - * Fail if a refname appears more than once in the transaction. - * This code is taken from the files backend and is a good candidate to - * be moved into the generic layer. - */ - string_list_sort(&affected_refnames); - if (ref_update_reject_duplicates(&affected_refnames, err)) { - ret = TRANSACTION_GENERIC_ERROR; - goto done; - } - /* * TODO: it's dubious whether we should reload the stack that "HEAD" * belongs to or not. In theory, it may happen that we only modify @@ -1194,14 +1178,12 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, !(u->flags & REF_LOG_ONLY) && !(u->flags & REF_UPDATE_VIA_HEAD) && !strcmp(rewritten_ref, head_referent.buf)) { - struct ref_update *new_update; - /* * First make sure that HEAD is not already in the * transaction. This check is O(lg N) in the transaction * size, but it happens at most once per transaction. */ - if (string_list_has_string(&affected_refnames, "HEAD")) { + if (string_list_has_string(&transaction->refnames, "HEAD")) { /* An entry already existed */ strbuf_addf(err, _("multiple updates for 'HEAD' (including one " @@ -1211,12 +1193,11 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, goto done; } - new_update = ref_transaction_add_update( - transaction, "HEAD", - u->flags | REF_LOG_ONLY | REF_NO_DEREF, - &u->new_oid, &u->old_oid, NULL, NULL, NULL, - u->msg); - string_list_insert(&affected_refnames, new_update->refname); + ref_transaction_add_update( + transaction, "HEAD", + u->flags | REF_LOG_ONLY | REF_NO_DEREF, + &u->new_oid, &u->old_oid, NULL, NULL, NULL, + u->msg); } ret = reftable_backend_read_ref(be, rewritten_ref, @@ -1281,6 +1262,15 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, if (!strcmp(rewritten_ref, "HEAD")) new_flags |= REF_UPDATE_VIA_HEAD; + if (string_list_has_string(&transaction->refnames, referent.buf)) { + strbuf_addf(err, + _("multiple updates for '%s' (including one " + "via symref '%s') are not allowed"), + referent.buf, u->refname); + ret = TRANSACTION_NAME_CONFLICT; + goto done; + } + /* * If we are updating a symref (eg. HEAD), we should also * update the branch that the symref points to. @@ -1305,16 +1295,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, */ u->flags |= REF_LOG_ONLY | REF_NO_DEREF; u->flags &= ~REF_HAVE_OLD; - - if (string_list_has_string(&affected_refnames, new_update->refname)) { - strbuf_addf(err, - _("multiple updates for '%s' (including one " - "via symref '%s') are not allowed"), - referent.buf, u->refname); - ret = TRANSACTION_NAME_CONFLICT; - goto done; - } - string_list_insert(&affected_refnames, new_update->refname); } } @@ -1384,7 +1364,8 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, } string_list_sort(&refnames_to_check); - ret = refs_verify_refnames_available(ref_store, &refnames_to_check, &affected_refnames, NULL, + ret = refs_verify_refnames_available(ref_store, &refnames_to_check, + &transaction->refnames, NULL, transaction->flags & REF_TRANSACTION_FLAG_INITIAL, err); if (ret < 0) @@ -1402,7 +1383,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, strbuf_addf(err, _("reftable: transaction prepare: %s"), reftable_error_str(ret)); } - string_list_clear(&affected_refnames, 0); strbuf_release(&referent); strbuf_release(&head_referent); string_list_clear(&refnames_to_check, 0); From patchwork Wed Mar 5 17:38:58 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Karthik Nayak X-Patchwork-Id: 14003066 Received: from mail-ed1-f52.google.com (mail-ed1-f52.google.com [209.85.208.52]) (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 068ED22257F for ; Wed, 5 Mar 2025 17:39:18 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.208.52 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741196360; cv=none; b=Gmxx7FJUvNwknoFBzI7+bZIVyxrbRjajO7E959122K8V0PGGzqLIDX7L4mc9tYFKyqsfBHM+kFRTgQBQghBuPV1c+6rAOcR5OqiroR9hbRbsAWFJvoJ38MCQwESSKHslxsA/5PKFtwETENhzuQss2VNmpUr5cUPVRf54k52uGTM= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741196360; c=relaxed/simple; bh=9YmifEGyq1ueljobaFVEbkIC66lNtY7vX4XH43M3rLI=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:References: In-Reply-To:To:Cc; b=Cto+rgxw1SoSIN13l7m8S5x8CsVaqNlx6Jh8PxlcNAbdl4s1FlvGVilTIFp/E+Nc0Uf9O8VPmfpltB4zOTM0RwFxfYWr9w2pSXEQWVZnG98VZgs+GV1qlcCPZK474qxkI918rTkT/JQH1vPTPlHjxEyn+HLdB7ZJSNJBNv6skyg= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=Y2zthJFY; arc=none smtp.client-ip=209.85.208.52 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="Y2zthJFY" Received: by mail-ed1-f52.google.com with SMTP id 4fb4d7f45d1cf-5e539ea490dso5225808a12.0 for ; Wed, 05 Mar 2025 09:39:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1741196357; x=1741801157; darn=vger.kernel.org; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:from:to:cc:subject:date:message-id :reply-to; bh=HDPXCiVB5QBIs7R6/jJuJGsxfpKTePe6+C/xKixPjNk=; b=Y2zthJFYRRJwbjuiGoNfMZM9gT2jzhI0VLmtS3REO/RzpgY5mgj32zboOdP29VLlsr LCZbbkfj4STa1mVypJHLq2gC2GtKLbFIQV7q9wAhz2PULpnFkFxJDp7OUGXBC4u56pTZ o6cDNiVIn35c5EFniPddzC2iEep2H//yk7uxLpcu2NY5EnWSQe/cStBj9ntriONBpODn mS/putZ/3BumUmkobBc3aO8jmtE6bC2sfhJnR2bEh/xvyOdcE8srfOmuUCu+6qScCcrX 6Z1NiJLdbDcn+OOMVen/XUtOPD1+BCWGDZ5N3/7BXs3+xeLRAdPtCu/cQk3kpZpmMfSf em6g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1741196357; x=1741801157; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=HDPXCiVB5QBIs7R6/jJuJGsxfpKTePe6+C/xKixPjNk=; b=Xcq5I6ZHN4xn6hEbkWL+72A2yx0CBcFgJ25A7KIv+YhLYC7DovmJJcP5+XwkgkDL7w 00HaFbaGKbJyKLvc38HNeVllKRdG5U93DQj4PHk5KinScRZusO5YUb17oa90JvwW00bO ZUL1CkYk0OGTKmZ+g/DG8EkPcpvwc0+oc/hUgHG37f5mDDc0zsGVeMqOP2/Wq8UOz32/ qviJwZJ9AwbjoD6TSH4j08DZOVpOcKNhRIvIoTphqd7yk9aVsg/Zs6rfoPP7ZGO09XcP I9nTp0x9NGrEr8rhNxnAocpoMN3Jg/Wvj5rWUVxyUJ7r3wc7jWNQZZvxiFGeKp+kcX/A A7FA== X-Gm-Message-State: AOJu0YwLdm8NKb/HlgoI1k9BtHqgIVzWbRk41sP0DZ9Ac/9Nxc6ddw5F LpT5cpYXpQjq3sRlOMq5zM4ECsNsGJyi0HPBUrAHdgLj7II7zz2ebHgpKFnZ X-Gm-Gg: ASbGncvmdXTxZXTSbsrgCiNwrYoCyT5JRqybObYXYF4YSUZmyKMCJb/l5fnoyKcosF5 tj7gsNqR+/GlDoxX2k3hAAGDpbkBtFuA1Z19wVJYh3BcMVXHBjjeQKMWLsz/HBxdtf3uhJQ/240 9X4dcnrDubweGKJFdbXztrb732H0WSb7elci3VLFUh97KeB3Mt25DE6lH5HkIx8bP/flvb8xZQ8 xGBo8tE92j4cfihs4ctD/qemGfSVk2QAHL8r85Syk69kRsBz0uTMnnT/deWxMNB8fvQLA4WpYVd a7209sPbEpAWamxzQOH4/cs2t7hnLz9Ca+oM5CTOd20mSvfpVw== X-Google-Smtp-Source: AGHT+IH9nTC3uCEOC4lOHVLPvO8lyxqyk/0cVrU0kIO8WR356kxKtUiSOb06lcRml7Mj8vOTuenNoA== X-Received: by 2002:a05:6402:34cd:b0:5e5:2d5c:4f32 with SMTP id 4fb4d7f45d1cf-5e59f4b69f9mr8869259a12.28.1741196356666; Wed, 05 Mar 2025 09:39:16 -0800 (PST) Received: from [127.0.0.2] ([2a02:2455:8268:bc00:8a90:b290:3a5b:4dd]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-abf3a8e2f53sm946851866b.115.2025.03.05.09.39.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 05 Mar 2025 09:39:16 -0800 (PST) From: Karthik Nayak Date: Wed, 05 Mar 2025 18:38:58 +0100 Subject: [PATCH v3 3/8] refs/files: remove duplicate duplicates check Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Message-Id: <20250305-245-partially-atomic-ref-updates-v3-3-0c64e3052354@gmail.com> References: <20250305-245-partially-atomic-ref-updates-v3-0-0c64e3052354@gmail.com> In-Reply-To: <20250305-245-partially-atomic-ref-updates-v3-0-0c64e3052354@gmail.com> To: git@vger.kernel.org Cc: Karthik Nayak , ps@pks.im, jltobler@gmail.com, phillip.wood123@gmail.com X-Mailer: b4 0.15-dev X-Developer-Signature: v=1; a=openpgp-sha256; l=2998; i=karthik.188@gmail.com; h=from:subject:message-id; bh=9YmifEGyq1ueljobaFVEbkIC66lNtY7vX4XH43M3rLI=; b=owJ4nAHtARL+kA0DAAoBPtWfJI5GjH8ByyZiAGfIjEDFSxV5U+shYinBLFSJWDxOZpkwZ7B1M cQ6aBqeVQgFoYkBswQAAQoAHRYhBFfOTH9jdXEPy2XGBj7VnySORox/BQJnyIxAAAoJED7VnySO Rox/tOMMAJF1y0MpONJ9tDdFr8UB4KnXunWLeE7LjymXogL6TjaQk7ZmcclKKp75eugA24cbdh5 f2FerDr8Q0OJJEs8klUS5t4UzjQi66RVUdMCoF38z9zALlgpF9XVcdYUKw5NSO8Ggnsa3pbljEG glNIsNGcCR+d1UomK6yt1o2f4NRPJlMf2AsluuhofQ+oNF6lVDKwUjVKS6yw0XFW1lLSve24WUF rorlxy5sFNa1/y1bItyO3lHIyzhYpnXK1+aFahY1F9OcvFyXo13Zf3uiK1CJ9yLq8sjPKCeInoV F9PhHMlHQl6MUtS6Sl3DhvPA+8t/L5CGL6iB4npsQ/tPJDDsNM4r9s4+S0P6uu0mMCfPUcBrnte k1yr/DlWAEQPLVJuTKGnaH2BUP9ORanrAjFa5Z79Irv7sK/6agPDIBVVphFeJ07xpOXQzACH9XF IuM+Ks6k2E0Y61fswQfQVGG2Vn0FV85NuHrguIu1BBO5K6wZKGY2Xt9TDyofbpepD9HAPvErlp5 hM= X-Developer-Key: i=karthik.188@gmail.com; a=openpgp; fpr=57CE4C7F6375710FCB65C6063ED59F248E468C7F Within the files reference backend's transaction's 'finish' phase, a verification step is currently performed wherein the refnames list is sorted and examined for multiple updates targeting the same refname. It has been observed that this verification is redundant, as an identical check is already executed during the transaction's 'prepare' stage. Since the refnames list remains unmodified following the 'prepare' stage, this secondary verification can be safely eliminated. The duplicate check has been removed accordingly, and the `ref_update_reject_duplicates()` function has been marked as static, as its usage is now confined to 'refs.c'. Signed-off-by: Karthik Nayak --- refs.c | 9 +++++++-- refs/files-backend.c | 6 ------ refs/refs-internal.h | 8 -------- 3 files changed, 7 insertions(+), 16 deletions(-) diff --git a/refs.c b/refs.c index ab69746947..69f385f344 100644 --- a/refs.c +++ b/refs.c @@ -2303,8 +2303,13 @@ int refs_update_symref_extended(struct ref_store *refs, const char *ref, return ret; } -int ref_update_reject_duplicates(struct string_list *refnames, - struct strbuf *err) +/* + * Write an error to `err` and return a nonzero value iff the same + * refname appears multiple times in `refnames`. `refnames` must be + * sorted on entry to this function. + */ +static int ref_update_reject_duplicates(struct string_list *refnames, + struct strbuf *err) { size_t i, n = refnames->nr; diff --git a/refs/files-backend.c b/refs/files-backend.c index 85ed85ad87..7c6a0b3478 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3016,12 +3016,6 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, if (transaction->state != REF_TRANSACTION_PREPARED) BUG("commit called for transaction that is not prepared"); - string_list_sort(&transaction->refnames); - if (ref_update_reject_duplicates(&transaction->refnames, err)) { - ret = TRANSACTION_GENERIC_ERROR; - goto cleanup; - } - /* * It's really undefined to call this function in an active * repository or when there are existing references: we are diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 92db793026..6d3770d0cc 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -142,14 +142,6 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname, struct object_id *oid, struct strbuf *referent, unsigned int *type, int *failure_errno); -/* - * Write an error to `err` and return a nonzero value iff the same - * refname appears multiple times in `refnames`. `refnames` must be - * sorted on entry to this function. - */ -int ref_update_reject_duplicates(struct string_list *refnames, - struct strbuf *err); - /* * Add a ref_update with the specified properties to transaction, and * return a pointer to the new object. This function does not verify From patchwork Wed Mar 5 17:38:59 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Karthik Nayak X-Patchwork-Id: 14003069 Received: from mail-ej1-f44.google.com (mail-ej1-f44.google.com [209.85.218.44]) (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 D9A4524292A for ; Wed, 5 Mar 2025 17:39:19 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.218.44 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741196362; cv=none; b=bKXxgWi2R/tkLi3xY6JEXnE1Hos0Gtx0bT2QgqTpWEVHFCCpBZZEDFnufOZ6PVLFjCK/fIUVQn1V+XEEmFfq9rs7oG3OFzSFvHNMdLqE+VbyocVbscB4h6Kjp3q3dVjETaYyY3NOzi/vtp7SW1GtsUP6ON9XZQBHLptTC6BzicU= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741196362; c=relaxed/simple; bh=hq8go0Ycc9CHgydnJXhyVX3uRQdNtmvpHOpz8TsjZHw=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:References: In-Reply-To:To:Cc; b=hB8+gZc/W4/vo54Leokqn7VsI6G/c45PYJJLqPOjTrb2X3FDBFJijyFyi1zC316P4CsG44rN15nuTCxv+KYLB1puXgWB4+B0Y9gXqreIJUmMaJ936IQM5EBoxxP24x0I/Rzcuu1Vqht4N09YI1Yv7VZBxsqu4OWlypWl6cF5Vmw= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=DmirU8ER; arc=none smtp.client-ip=209.85.218.44 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="DmirU8ER" Received: by mail-ej1-f44.google.com with SMTP id a640c23a62f3a-ac202264f9cso310023366b.0 for ; Wed, 05 Mar 2025 09:39:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1741196358; x=1741801158; darn=vger.kernel.org; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:from:to:cc:subject:date:message-id :reply-to; bh=4WoqLWnPCw0oE6c9586Fu0OJyx+S8HDzSFUKpGjakzs=; b=DmirU8ER2BVy85lONCVLa9+7Wv1GwN27uza4oWECgB6bjcEUW3xQvtuvPEV+8ph9Hw 4ODlb+vPuhaWS8VCVyUFBFvVPeTu3Ga3TBjCG74smnmtnTfiul8lBHMSY8oBFHBIkGeU odhfVGrZtEarGto9gcuRvgmP1hhuf7Q7ng1xWmqgFB+dKc5tyIPMR3XURtbbIqe6/JN7 y6hkeF6aFjBZFGl7XLcyquG8+3j7oWfvyofNT/C8SvTx0gUGK+MMrFljAhO0/z6wIAmj 44BkuAI7Jxog27soY+5Vd3D1X5XRl2AtGTo4dZ434x41IcBS3HVtsjXx/z7G7x9cUDgT IkiA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1741196358; x=1741801158; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=4WoqLWnPCw0oE6c9586Fu0OJyx+S8HDzSFUKpGjakzs=; b=URJiNVa+OTYw+48tYpN177G/ydTgvrC7mFKdt4bOkGJpPPzTcYx8qimNyK87IpbXvC /40Hj+ULTCZNWNeMdLvxzIOJHH70TrSEX108XLzmCfVHedCcFxxEjKMpjZqZqSCX7KQA /qgk5+JRVzidtDtsh3lXlsWINHa+Tu+EHB2QC1CBVB83PC/Sl/1QvHUeSko+W0zj7HE1 HPAi7zav/HLMLAhWf8DiHFPUmjKpVsFRelOzE1UIszRZQ8tB/gcyDS3dkK/L2lHoBjNi 9OfNNQY8DwTJ5+tNlc32gBPklRcY+fboSa/rYV29lf3I6dWNCuQGJWBxMiRLAJ8jwlJx 4XzQ== X-Gm-Message-State: AOJu0Yxj3v40KDG9gQlXJ9+8id6BqgECCvKlKJPq8aBaYmVtsYF4wdqd iXwpj6PllJwypBmmvr5Tp4FH8Gi4/9d4jcoBFxjWz4nx8azgSIbQ5Ov0Z5Io X-Gm-Gg: ASbGncuWTk02FTjb6NJyGa0hq9EO2rjBT+LZS7ybSzeBh+ioPTKxkWqXPUj/M8IfcDL JtQkCcxO1aD4DGFfJZfjXR3LJn3eB2t/cnFEBER4yh6ihGhLnsl6ck4cddeiTiR1zUlZFvWzcMr vTP4vAIH5nH9j6ot96gDbqTi6Cr3ap3fXQ9ntxWG4tyuFvH/W4hrV/4wwzNhqJ/dYHOYQsqwvSu Xt067Q4/be3H4zkwCZBrLTHrURHyx3H4hIb6bELXkGx7GEevzkF5XFsF0m61DKa4JovXndh2XTN 9H6ZDmRZt1sItUfsp3Vd2gNraOAPUE+zMjG7SdrOPfMKuJRJLQ== X-Google-Smtp-Source: AGHT+IGiX5PwVW/atJrAsgBdwNrAhpkQ1iD3gQvr9KpFLKAyEXENeOyW4yHijk4s9xtn261xS5D8QQ== X-Received: by 2002:a17:907:8b97:b0:ac1:d8a9:f853 with SMTP id a640c23a62f3a-ac20e1db11amr466739066b.48.1741196357429; Wed, 05 Mar 2025 09:39:17 -0800 (PST) Received: from [127.0.0.2] ([2a02:2455:8268:bc00:8a90:b290:3a5b:4dd]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-abf3a8e2f53sm946851866b.115.2025.03.05.09.39.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 05 Mar 2025 09:39:17 -0800 (PST) From: Karthik Nayak Date: Wed, 05 Mar 2025 18:38:59 +0100 Subject: [PATCH v3 4/8] refs/reftable: extract code from the transaction preparation Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Message-Id: <20250305-245-partially-atomic-ref-updates-v3-4-0c64e3052354@gmail.com> References: <20250305-245-partially-atomic-ref-updates-v3-0-0c64e3052354@gmail.com> In-Reply-To: <20250305-245-partially-atomic-ref-updates-v3-0-0c64e3052354@gmail.com> To: git@vger.kernel.org Cc: Karthik Nayak , ps@pks.im, jltobler@gmail.com, phillip.wood123@gmail.com X-Mailer: b4 0.15-dev X-Developer-Signature: v=1; a=openpgp-sha256; l=17407; i=karthik.188@gmail.com; h=from:subject:message-id; bh=hq8go0Ycc9CHgydnJXhyVX3uRQdNtmvpHOpz8TsjZHw=; b=owJ4nAHtARL+kA0DAAoBPtWfJI5GjH8ByyZiAGfIjEDPDdvQTgdPzXTP2UkKIfXKOUHd+Wt8W kekzKpMFTnLaYkBswQAAQoAHRYhBFfOTH9jdXEPy2XGBj7VnySORox/BQJnyIxAAAoJED7VnySO Rox/Y44L/19gTjYvNFX4Q8cD7pAjYDlUJYw4rEHmN3VohT8rJOOV6y1jnRCk6dlJZsNWoh6nPFd 5PgqMwoh0SjOHKqsq2eIafMtH3R5jBtdw2TnEQEYU4xchPeVQOKcB5fphR1VAv3B0OIIggnLcit IeUofqPhx1r2ivk57xZXOvhYfHLRjf5YQxA8cmuLPLoAanU/ErIvpnaYYT3xdQyaKH1RgUviYXk dOkq7qQP1kLlUO7oBXXOwbY16TfiyYVwuI7ro43d7WWV4O1cCewpIrcHOMQfivM8LJ7GKVa3goS s2i9W9W6rN+KHNZ4FNfAcvBvMKLSwBUCejnazA6+/Pc7b+xjhWWJhmyZgYRd/f3V9xdUSRgNGKw B1HrqhXzG4fPrQDoVUZaRectrYCynOxJxeX9ol/QjdBKR8At/JHab1Nfm4+NfEpHeZUS6bl47xM 81sTSFtBzNH4/Pst0ZJYMLP1VUX97px1vKJVPZgCQcJwdOAf0w31vj21oVBzBV82wSnG2v2/lB4 0M= X-Developer-Key: i=karthik.188@gmail.com; a=openpgp; fpr=57CE4C7F6375710FCB65C6063ED59F248E468C7F Extract the core logic for preparing individual reference updates from `reftable_be_transaction_prepare()` into `prepare_single_update()`. This dedicated function now handles all validation and preparation steps for each reference update in the transaction, including object ID verification, HEAD reference handling, and symref processing. The refactoring consolidates all reference update validation into a single logical block, which improves code maintainability and readability. More importantly, this restructuring lays the groundwork for implementing partial transaction support in the reftable backend, which will be introduced in the following commit. No functional changes are included in this commit - it is purely a code reorganization to support future enhancements. Signed-off-by: Karthik Nayak --- refs/reftable-backend.c | 463 +++++++++++++++++++++++++----------------------- 1 file changed, 237 insertions(+), 226 deletions(-) diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index f616d9aabe..2c1e2995de 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1069,6 +1069,239 @@ static int queue_transaction_update(struct reftable_ref_store *refs, return 0; } +static int prepare_single_update(struct reftable_ref_store *refs, + struct reftable_transaction_data *tx_data, + struct ref_transaction *transaction, + struct reftable_backend *be, + struct ref_update *u, + struct string_list *refnames_to_check, + unsigned int head_type, + struct strbuf *head_referent, + struct strbuf *referent, + struct strbuf *err) +{ + struct object_id current_oid = {0}; + const char *rewritten_ref; + int ret = 0; + + /* + * There is no need to reload the respective backends here as + * we have already reloaded them when preparing the transaction + * update. And given that the stacks have been locked there + * shouldn't have been any concurrent modifications of the + * stack. + */ + ret = backend_for(&be, refs, u->refname, &rewritten_ref, 0); + if (ret) + return ret; + + /* Verify that the new object ID is valid. */ + if ((u->flags & REF_HAVE_NEW) && !is_null_oid(&u->new_oid) && + !(u->flags & REF_SKIP_OID_VERIFICATION) && + !(u->flags & REF_LOG_ONLY)) { + struct object *o = parse_object(refs->base.repo, &u->new_oid); + if (!o) { + strbuf_addf(err, + _("trying to write ref '%s' with nonexistent object %s"), + u->refname, oid_to_hex(&u->new_oid)); + return -1; + } + + if (o->type != OBJ_COMMIT && is_branch(u->refname)) { + strbuf_addf(err, _("trying to write non-commit object %s to branch '%s'"), + oid_to_hex(&u->new_oid), u->refname); + return -1; + } + } + + /* + * When we update the reference that HEAD points to we enqueue + * a second log-only update for HEAD so that its reflog is + * updated accordingly. + */ + if (head_type == REF_ISSYMREF && + !(u->flags & REF_LOG_ONLY) && + !(u->flags & REF_UPDATE_VIA_HEAD) && + !strcmp(rewritten_ref, head_referent->buf)) { + /* + * First make sure that HEAD is not already in the + * transaction. This check is O(lg N) in the transaction + * size, but it happens at most once per transaction. + */ + if (string_list_has_string(&transaction->refnames, "HEAD")) { + /* An entry already existed */ + strbuf_addf(err, + _("multiple updates for 'HEAD' (including one " + "via its referent '%s') are not allowed"), + u->refname); + return TRANSACTION_NAME_CONFLICT; + } + + ref_transaction_add_update( + transaction, "HEAD", + u->flags | REF_LOG_ONLY | REF_NO_DEREF, + &u->new_oid, &u->old_oid, NULL, NULL, NULL, + u->msg); + } + + ret = reftable_backend_read_ref(be, rewritten_ref, + ¤t_oid, referent, &u->type); + if (ret < 0) + return ret; + if (ret > 0 && !ref_update_expects_existing_old_ref(u)) { + /* + * The reference does not exist, and we either have no + * old object ID or expect the reference to not exist. + * We can thus skip below safety checks as well as the + * symref splitting. But we do want to verify that + * there is no conflicting reference here so that we + * can output a proper error message instead of failing + * at a later point. + */ + string_list_append(refnames_to_check, u->refname); + + /* + * There is no need to write the reference deletion + * when the reference in question doesn't exist. + */ + if ((u->flags & REF_HAVE_NEW) && !ref_update_has_null_new_value(u)) { + ret = queue_transaction_update(refs, tx_data, u, + ¤t_oid, err); + if (ret) + return ret; + } + + return 0; + } + if (ret > 0) { + /* The reference does not exist, but we expected it to. */ + strbuf_addf(err, _("cannot lock ref '%s': " + + + "unable to resolve reference '%s'"), + ref_update_original_update_refname(u), u->refname); + return -1; + } + + if (u->type & REF_ISSYMREF) { + /* + * The reftable stack is locked at this point already, + * so it is safe to call `refs_resolve_ref_unsafe()` + * here without causing races. + */ + const char *resolved = refs_resolve_ref_unsafe(&refs->base, u->refname, 0, + ¤t_oid, NULL); + + if (u->flags & REF_NO_DEREF) { + if (u->flags & REF_HAVE_OLD && !resolved) { + strbuf_addf(err, _("cannot lock ref '%s': " + "error reading reference"), u->refname); + return -1; + } + } else { + struct ref_update *new_update; + int new_flags; + + new_flags = u->flags; + if (!strcmp(rewritten_ref, "HEAD")) + new_flags |= REF_UPDATE_VIA_HEAD; + + if (string_list_has_string(&transaction->refnames, referent->buf)) { + strbuf_addf(err, + _("multiple updates for '%s' (including one " + "via symref '%s') are not allowed"), + referent->buf, u->refname); + return TRANSACTION_NAME_CONFLICT; + } + + /* + * If we are updating a symref (eg. HEAD), we should also + * update the branch that the symref points to. + * + * This is generic functionality, and would be better + * done in refs.c, but the current implementation is + * intertwined with the locking in files-backend.c. + */ + new_update = ref_transaction_add_update( + transaction, referent->buf, new_flags, + u->new_target ? NULL : &u->new_oid, + u->old_target ? NULL : &u->old_oid, + u->new_target, u->old_target, + u->committer_info, u->msg); + + new_update->parent_update = u; + + /* + * Change the symbolic ref update to log only. Also, it + * doesn't need to check its old OID value, as that will be + * done when new_update is processed. + */ + u->flags |= REF_LOG_ONLY | REF_NO_DEREF; + u->flags &= ~REF_HAVE_OLD; + } + } + + /* + * Verify that the old object matches our expectations. Note + * that the error messages here do not make a lot of sense in + * the context of the reftable backend as we never lock + * individual refs. But the error messages match what the files + * backend returns, which keeps our tests happy. + */ + if (u->old_target) { + if (!(u->type & REF_ISSYMREF)) { + strbuf_addf(err, _("cannot lock ref '%s': " + "expected symref with target '%s': " + "but is a regular ref"), + ref_update_original_update_refname(u), + u->old_target); + return -1; + } + + if (ref_update_check_old_target(referent->buf, u, err)) { + return -1; + } + } else if ((u->flags & REF_HAVE_OLD) && !oideq(¤t_oid, &u->old_oid)) { + if (is_null_oid(&u->old_oid)) { + strbuf_addf(err, _("cannot lock ref '%s': " + "reference already exists"), + ref_update_original_update_refname(u)); + return TRANSACTION_CREATE_EXISTS; + } + else if (is_null_oid(¤t_oid)) + strbuf_addf(err, _("cannot lock ref '%s': " + "reference is missing but expected %s"), + ref_update_original_update_refname(u), + oid_to_hex(&u->old_oid)); + else + strbuf_addf(err, _("cannot lock ref '%s': " + "is at %s but expected %s"), + ref_update_original_update_refname(u), + oid_to_hex(¤t_oid), + oid_to_hex(&u->old_oid)); + return TRANSACTION_NAME_CONFLICT; + } + + /* + * If all of the following conditions are true: + * + * - We're not about to write a symref. + * - We're not about to write a log-only entry. + * - Old and new object ID are different. + * + * Then we're essentially doing a no-op update that can be + * skipped. This is not only for the sake of efficiency, but + * also skips writing unneeded reflog entries. + */ + if ((u->type & REF_ISSYMREF) || + (u->flags & REF_LOG_ONLY) || + (u->flags & REF_HAVE_NEW && !oideq(¤t_oid, &u->new_oid))) + return queue_transaction_update(refs, tx_data, u, + ¤t_oid, err); + + return 0; +} + static int reftable_be_transaction_prepare(struct ref_store *ref_store, struct ref_transaction *transaction, struct strbuf *err) @@ -1133,234 +1366,12 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, ret = 0; for (i = 0; i < transaction->nr; i++) { - struct ref_update *u = transaction->updates[i]; - struct object_id current_oid = {0}; - const char *rewritten_ref; - - /* - * There is no need to reload the respective backends here as - * we have already reloaded them when preparing the transaction - * update. And given that the stacks have been locked there - * shouldn't have been any concurrent modifications of the - * stack. - */ - ret = backend_for(&be, refs, u->refname, &rewritten_ref, 0); + ret = prepare_single_update(refs, tx_data, transaction, be, + transaction->updates[i], + &refnames_to_check, head_type, + &head_referent, &referent, err); if (ret) goto done; - - /* Verify that the new object ID is valid. */ - if ((u->flags & REF_HAVE_NEW) && !is_null_oid(&u->new_oid) && - !(u->flags & REF_SKIP_OID_VERIFICATION) && - !(u->flags & REF_LOG_ONLY)) { - struct object *o = parse_object(refs->base.repo, &u->new_oid); - if (!o) { - strbuf_addf(err, - _("trying to write ref '%s' with nonexistent object %s"), - u->refname, oid_to_hex(&u->new_oid)); - ret = -1; - goto done; - } - - if (o->type != OBJ_COMMIT && is_branch(u->refname)) { - strbuf_addf(err, _("trying to write non-commit object %s to branch '%s'"), - oid_to_hex(&u->new_oid), u->refname); - ret = -1; - goto done; - } - } - - /* - * When we update the reference that HEAD points to we enqueue - * a second log-only update for HEAD so that its reflog is - * updated accordingly. - */ - if (head_type == REF_ISSYMREF && - !(u->flags & REF_LOG_ONLY) && - !(u->flags & REF_UPDATE_VIA_HEAD) && - !strcmp(rewritten_ref, head_referent.buf)) { - /* - * First make sure that HEAD is not already in the - * transaction. This check is O(lg N) in the transaction - * size, but it happens at most once per transaction. - */ - if (string_list_has_string(&transaction->refnames, "HEAD")) { - /* An entry already existed */ - strbuf_addf(err, - _("multiple updates for 'HEAD' (including one " - "via its referent '%s') are not allowed"), - u->refname); - ret = TRANSACTION_NAME_CONFLICT; - goto done; - } - - ref_transaction_add_update( - transaction, "HEAD", - u->flags | REF_LOG_ONLY | REF_NO_DEREF, - &u->new_oid, &u->old_oid, NULL, NULL, NULL, - u->msg); - } - - ret = reftable_backend_read_ref(be, rewritten_ref, - ¤t_oid, &referent, &u->type); - if (ret < 0) - goto done; - if (ret > 0 && !ref_update_expects_existing_old_ref(u)) { - /* - * The reference does not exist, and we either have no - * old object ID or expect the reference to not exist. - * We can thus skip below safety checks as well as the - * symref splitting. But we do want to verify that - * there is no conflicting reference here so that we - * can output a proper error message instead of failing - * at a later point. - */ - string_list_append(&refnames_to_check, u->refname); - - /* - * There is no need to write the reference deletion - * when the reference in question doesn't exist. - */ - if ((u->flags & REF_HAVE_NEW) && !ref_update_has_null_new_value(u)) { - ret = queue_transaction_update(refs, tx_data, u, - ¤t_oid, err); - if (ret) - goto done; - } - - continue; - } - if (ret > 0) { - /* The reference does not exist, but we expected it to. */ - strbuf_addf(err, _("cannot lock ref '%s': " - "unable to resolve reference '%s'"), - ref_update_original_update_refname(u), u->refname); - ret = -1; - goto done; - } - - if (u->type & REF_ISSYMREF) { - /* - * The reftable stack is locked at this point already, - * so it is safe to call `refs_resolve_ref_unsafe()` - * here without causing races. - */ - const char *resolved = refs_resolve_ref_unsafe(&refs->base, u->refname, 0, - ¤t_oid, NULL); - - if (u->flags & REF_NO_DEREF) { - if (u->flags & REF_HAVE_OLD && !resolved) { - strbuf_addf(err, _("cannot lock ref '%s': " - "error reading reference"), u->refname); - ret = -1; - goto done; - } - } else { - struct ref_update *new_update; - int new_flags; - - new_flags = u->flags; - if (!strcmp(rewritten_ref, "HEAD")) - new_flags |= REF_UPDATE_VIA_HEAD; - - if (string_list_has_string(&transaction->refnames, referent.buf)) { - strbuf_addf(err, - _("multiple updates for '%s' (including one " - "via symref '%s') are not allowed"), - referent.buf, u->refname); - ret = TRANSACTION_NAME_CONFLICT; - goto done; - } - - /* - * If we are updating a symref (eg. HEAD), we should also - * update the branch that the symref points to. - * - * This is generic functionality, and would be better - * done in refs.c, but the current implementation is - * intertwined with the locking in files-backend.c. - */ - new_update = ref_transaction_add_update( - transaction, referent.buf, new_flags, - u->new_target ? NULL : &u->new_oid, - u->old_target ? NULL : &u->old_oid, - u->new_target, u->old_target, - u->committer_info, u->msg); - - new_update->parent_update = u; - - /* - * Change the symbolic ref update to log only. Also, it - * doesn't need to check its old OID value, as that will be - * done when new_update is processed. - */ - u->flags |= REF_LOG_ONLY | REF_NO_DEREF; - u->flags &= ~REF_HAVE_OLD; - } - } - - /* - * Verify that the old object matches our expectations. Note - * that the error messages here do not make a lot of sense in - * the context of the reftable backend as we never lock - * individual refs. But the error messages match what the files - * backend returns, which keeps our tests happy. - */ - if (u->old_target) { - if (!(u->type & REF_ISSYMREF)) { - strbuf_addf(err, _("cannot lock ref '%s': " - "expected symref with target '%s': " - "but is a regular ref"), - ref_update_original_update_refname(u), - u->old_target); - ret = -1; - goto done; - } - - if (ref_update_check_old_target(referent.buf, u, err)) { - ret = -1; - goto done; - } - } else if ((u->flags & REF_HAVE_OLD) && !oideq(¤t_oid, &u->old_oid)) { - ret = TRANSACTION_NAME_CONFLICT; - if (is_null_oid(&u->old_oid)) { - strbuf_addf(err, _("cannot lock ref '%s': " - "reference already exists"), - ref_update_original_update_refname(u)); - ret = TRANSACTION_CREATE_EXISTS; - } - else if (is_null_oid(¤t_oid)) - strbuf_addf(err, _("cannot lock ref '%s': " - "reference is missing but expected %s"), - ref_update_original_update_refname(u), - oid_to_hex(&u->old_oid)); - else - strbuf_addf(err, _("cannot lock ref '%s': " - "is at %s but expected %s"), - ref_update_original_update_refname(u), - oid_to_hex(¤t_oid), - oid_to_hex(&u->old_oid)); - goto done; - } - - /* - * If all of the following conditions are true: - * - * - We're not about to write a symref. - * - We're not about to write a log-only entry. - * - Old and new object ID are different. - * - * Then we're essentially doing a no-op update that can be - * skipped. This is not only for the sake of efficiency, but - * also skips writing unneeded reflog entries. - */ - if ((u->type & REF_ISSYMREF) || - (u->flags & REF_LOG_ONLY) || - (u->flags & REF_HAVE_NEW && !oideq(¤t_oid, &u->new_oid))) { - ret = queue_transaction_update(refs, tx_data, u, - ¤t_oid, err); - if (ret) - goto done; - } } string_list_sort(&refnames_to_check); From patchwork Wed Mar 5 17:39:00 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Karthik Nayak X-Patchwork-Id: 14003071 Received: from mail-ej1-f51.google.com (mail-ej1-f51.google.com [209.85.218.51]) (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 C8CD12512D4 for ; Wed, 5 Mar 2025 17:39:20 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.218.51 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741196363; cv=none; b=JjnA/09q7p08c1qgzdrpIexhYS0D/6ScbxQKU5wHT2Zw0SYivZz9c3ls15RftlZ31Rxo+Z2Gqp8zJITIXkpmkdzIay+BiGx4Bb9JmfUmqSPuJ7/KFTO/ivvnWj9xrUG0UzCsn6If+vG87VH9x27LPO/wmlSDdTeiwstHv4BdxWM= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741196363; c=relaxed/simple; bh=U8Peyoymbx7+FOMg1pKxw/cUwNKBN7NEcfJPYBEdfoc=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:References: In-Reply-To:To:Cc; b=KjPiqYZM8KOXokVjNduP0b4Eb0FcaUAz6hHhEtNWcSm4/WdslIYx3xJR2nelJnzYMCf954PkKx3z1rEO/xitiaLkRn9ezsz9MjBKVlW6BmLuVEDcjhvwN1SCtB/TIQlu8r6yH1GCjIiT3+zuGbJGyTVUZCRhmov89chBzSgGbMY= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=jn6dvN2W; arc=none smtp.client-ip=209.85.218.51 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="jn6dvN2W" Received: by mail-ej1-f51.google.com with SMTP id a640c23a62f3a-abf42913e95so794859566b.2 for ; Wed, 05 Mar 2025 09:39:20 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1741196359; x=1741801159; darn=vger.kernel.org; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:from:to:cc:subject:date:message-id :reply-to; bh=L2M0AUq0E5vuNWtsmUZgnwXx0ROzakQ9pEZkPcQ1c8E=; b=jn6dvN2WfYItM1vzhyyr9RJgPH2Vxenu/3m79CQp0/+nj2475UHmNyeTTPOhYCjAk+ Rgl94EmE2d1Au3nItl+TtyGTrpD64Eqg0BHW5MnM8U1+F+V64P+nxNqNCOrptiB3Y4rA yaBB3vKOAvbFQ/t271yVGYvbYvD2/vQr6Ur2GL8Ktl15jywL9h+a7UsGb9ftdHYE2yvn jqbi12RnvCLC9bjCL29Fps1b3YX1noeQ1Orc2SMRZgoBk72uvOQYUxaR20IXOBiifDX2 OB25SuXm+hNu33aiYWp+vtts1m8ptUnE8v1CkGuL6RvCz+dQr1bk04A/QVCOUK71YhOc Blcg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1741196359; x=1741801159; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=L2M0AUq0E5vuNWtsmUZgnwXx0ROzakQ9pEZkPcQ1c8E=; b=SIDCFdP9vTeBR07LmZFNUshwXDXVdjnKd+sjRaO+14TUG0xDEdEvEMEEGP5v4/86eD 9Fu2txKsydYIV0babJfOPwdUhBTCjQg9hob+nfRoEfLCC0MhpiSHMQYqMmW0lSoAtU+8 cHxQ4hTd8tWjuEaaiXQh4FNGyi4dBxrfZ08V8tJUtrgOdGEpdsp+Gb0Uk+WhKGVv2KqX p32Xp5h5j2Wg2QTQ6h+kBav6K+CL1kTvFpDPxPNqpf2WmrlTwZCaWoJElioCifDqQB0U sipu15fUoT9lhvfGwtK+pc1fn/8UGwhU1XMMKIeZTB9UgnWP99Z8LSHXuAPLQeydXdSR 3pww== X-Gm-Message-State: AOJu0YzLg1RZ6feDLGxu/1rRWypvXxByI32fqdkNJpamAfJi5pOsWA0N y5QkC+LeQ+TynF/SaTyVd9M6mL7xyd8npQ9ZHnJ9lQklSf36KY5A21fJGUOO X-Gm-Gg: ASbGncsg5Nb2S05fMt9rrvDRZC38J40MGFr7zttinE+Eoi1H5DnEWK08sMvjDZSzqje fAd/uPMXpIgTstfK+P58K5et6xgTvhUR+J8ZbZLhP74LdcVOGynmaodppBlwHys/fPREN7mACWb kFhQbAwaPdGp6TfhHRSl4iIxKfAw+pgnY8fN+MtHCUdo+FWAWdMYN/RHadyNUXOBJXajbTgvBJ1 HiHwJN6WwkzzpjLlKpcMCHkW3F7qKXALhB/lMYkVkf+9fN7TshJYeJvzOmrGQTUmqfQLvlK4b7R v7eklg54K2TmPK6i8rOMdnrqyiHGa/wpnFr80NSwmyogggn1Qw== X-Google-Smtp-Source: AGHT+IGQ0B8RF3DSIlnwaeGwvRZ6Ur0zz2AqQQQkB5S5gW0Je66KRQZdn/8UR7NqJMqyki3iJ3Utrw== X-Received: by 2002:a17:907:c018:b0:abf:7964:f9e5 with SMTP id a640c23a62f3a-ac20e03b1d0mr445383666b.56.1741196358200; Wed, 05 Mar 2025 09:39:18 -0800 (PST) Received: from [127.0.0.2] ([2a02:2455:8268:bc00:8a90:b290:3a5b:4dd]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-abf3a8e2f53sm946851866b.115.2025.03.05.09.39.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 05 Mar 2025 09:39:17 -0800 (PST) From: Karthik Nayak Date: Wed, 05 Mar 2025 18:39:00 +0100 Subject: [PATCH v3 5/8] refs: introduce enum-based transaction error types Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Message-Id: <20250305-245-partially-atomic-ref-updates-v3-5-0c64e3052354@gmail.com> References: <20250305-245-partially-atomic-ref-updates-v3-0-0c64e3052354@gmail.com> In-Reply-To: <20250305-245-partially-atomic-ref-updates-v3-0-0c64e3052354@gmail.com> To: git@vger.kernel.org Cc: Karthik Nayak , ps@pks.im, jltobler@gmail.com, phillip.wood123@gmail.com X-Mailer: b4 0.15-dev X-Developer-Signature: v=1; a=openpgp-sha256; l=37459; i=karthik.188@gmail.com; h=from:subject:message-id; bh=U8Peyoymbx7+FOMg1pKxw/cUwNKBN7NEcfJPYBEdfoc=; b=owJ4nAHtARL+kA0DAAoBPtWfJI5GjH8ByyZiAGfIjEByVDIhlYQlI2I4dTeH9qH36RQiH2oN/ 7YvU+4PT5Rv0YkBswQAAQoAHRYhBFfOTH9jdXEPy2XGBj7VnySORox/BQJnyIxAAAoJED7VnySO Rox/u7UMAJ0mVktWOsqhClFyPFtFa+c3DVkXA4BsUWjgu4i8NvlU2XedM+CgORyck02Za4EdqBe +Jl3jlTsPy+PGCp4ntgOIYpjdKoqQWDdYQ5vPVXOBUssRA/R0Tm3fTb3e+KfzN63P8hIuOFdjLO WHev5d/P/7g0Xox4v8ClLYhdR0wYQsukqBS0Ui2U8B1ZOjRRAzaneLlezSHuexb9dBwpNPP4EaJ jkXwFWSqfsSI94VjXYx1r7cqz/DODGYwHitUUGVB5j+lm5GCcoKEJlu8TiG7FPP9KQdMWQaH3q9 8zfuBuvHjXkbWm0UEK+Zn5xY/lq5TEONUbONC3iEo3vJCrF66jN4spNVvCfLOxSdGRLm2c2lRLH TVdJVUr9Zhr3/o57QJyAchce3sk3nmRLmA197wJLYaQfPGgdTlkeOktfBJiC1IRl6GdMl/VKY8j WWW4b7TYElCMJtot7m+aKENcJYLPpjEyva4vMg3HxP84ppsw0DIw/+WhPZLmmuHAFQwYNqOsDS4 Wg= X-Developer-Key: i=karthik.188@gmail.com; a=openpgp; fpr=57CE4C7F6375710FCB65C6063ED59F248E468C7F Replace preprocessor-defined transaction errors with a strongly-typed enum `ref_transaction_error`. This change: - Improves type safety and function signature clarity. - Makes error handling more explicit and discoverable. - Maintains existing error cases, while adding new error cases for common scenarios. This refactoring paves the way for more comprehensive error handling which we will utilize in the upcoming commits to add partial transaction support. Signed-off-by: Karthik Nayak --- builtin/fetch.c | 2 +- refs.c | 49 ++++++------ refs.h | 54 ++++++++----- refs/files-backend.c | 202 ++++++++++++++++++++++++------------------------ refs/packed-backend.c | 23 +++--- refs/refs-internal.h | 5 +- refs/reftable-backend.c | 64 +++++++-------- 7 files changed, 213 insertions(+), 186 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 1c740d5aac..52c913d28a 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -687,7 +687,7 @@ static int s_update_ref(const char *action, switch (ref_transaction_commit(our_transaction, &err)) { case 0: break; - case TRANSACTION_NAME_CONFLICT: + case REF_TRANSACTION_ERROR_NAME_CONFLICT: ret = STORE_REF_ERROR_DF_CONFLICT; goto out; default: diff --git a/refs.c b/refs.c index 69f385f344..63b8050ce2 100644 --- a/refs.c +++ b/refs.c @@ -2271,7 +2271,7 @@ int refs_update_symref_extended(struct ref_store *refs, const char *ref, REF_NO_DEREF, logmsg, &err)) goto error_return; prepret = ref_transaction_prepare(transaction, &err); - if (prepret && prepret != TRANSACTION_CREATE_EXISTS) + if (prepret && prepret != REF_TRANSACTION_ERROR_CREATE_EXISTS) goto error_return; } else { if (ref_transaction_update(transaction, ref, NULL, NULL, @@ -2289,7 +2289,7 @@ int refs_update_symref_extended(struct ref_store *refs, const char *ref, } } - if (prepret == TRANSACTION_CREATE_EXISTS) + if (prepret == REF_TRANSACTION_ERROR_CREATE_EXISTS) goto cleanup; if (ref_transaction_commit(transaction, &err)) @@ -2425,7 +2425,7 @@ int ref_transaction_prepare(struct ref_transaction *transaction, string_list_sort(&transaction->refnames); if (ref_update_reject_duplicates(&transaction->refnames, err)) - return TRANSACTION_GENERIC_ERROR; + return REF_TRANSACTION_ERROR_GENERIC; ret = refs->be->transaction_prepare(refs, transaction, err); if (ret) @@ -2497,18 +2497,18 @@ int ref_transaction_commit(struct ref_transaction *transaction, return ret; } -int refs_verify_refnames_available(struct ref_store *refs, - const struct string_list *refnames, - const struct string_list *extras, - const struct string_list *skip, - unsigned int initial_transaction, - struct strbuf *err) +enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs, + const struct string_list *refnames, + const struct string_list *extras, + const struct string_list *skip, + unsigned int initial_transaction, + struct strbuf *err) { struct strbuf dirname = STRBUF_INIT; struct strbuf referent = STRBUF_INIT; struct ref_iterator *iter = NULL; struct strset dirnames; - int ret = -1; + int ret = REF_TRANSACTION_ERROR_NAME_CONFLICT; /* * For the sake of comments in this function, suppose that @@ -2624,12 +2624,13 @@ int refs_verify_refnames_available(struct ref_store *refs, return ret; } -int refs_verify_refname_available(struct ref_store *refs, - const char *refname, - const struct string_list *extras, - const struct string_list *skip, - unsigned int initial_transaction, - struct strbuf *err) +enum ref_transaction_error refs_verify_refname_available( + struct ref_store *refs, + const char *refname, + const struct string_list *extras, + const struct string_list *skip, + unsigned int initial_transaction, + struct strbuf *err) { struct string_list_item item = { .string = (char *) refname }; struct string_list refnames = { @@ -2817,8 +2818,9 @@ int ref_update_has_null_new_value(struct ref_update *update) return !update->new_target && is_null_oid(&update->new_oid); } -int ref_update_check_old_target(const char *referent, struct ref_update *update, - struct strbuf *err) +enum ref_transaction_error ref_update_check_old_target(const char *referent, + struct ref_update *update, + struct strbuf *err) { if (!update->old_target) BUG("called without old_target set"); @@ -2826,17 +2828,18 @@ int ref_update_check_old_target(const char *referent, struct ref_update *update, if (!strcmp(referent, update->old_target)) return 0; - if (!strcmp(referent, "")) + if (!strcmp(referent, "")) { strbuf_addf(err, "verifying symref target: '%s': " "reference is missing but expected %s", ref_update_original_update_refname(update), update->old_target); - else - strbuf_addf(err, "verifying symref target: '%s': " - "is at %s but expected %s", + return REF_TRANSACTION_ERROR_NONEXISTENT_REF; + } + + strbuf_addf(err, "verifying symref target: '%s': is at %s but expected %s", ref_update_original_update_refname(update), referent, update->old_target); - return -1; + return REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE; } struct migration_data { diff --git a/refs.h b/refs.h index b14ba1f9ff..1b9213f9ce 100644 --- a/refs.h +++ b/refs.h @@ -16,6 +16,29 @@ struct worktree; enum ref_storage_format ref_storage_format_by_name(const char *name); const char *ref_storage_format_to_name(enum ref_storage_format ref_storage_format); +/* + * enum ref_transaction_error represents the following return codes: + * REF_TRANSACTION_ERROR_GENERIC error_code: default error code. + * REF_TRANSACTION_ERROR_NAME_CONFLICT error_code: ref name conflict like A vs A/B. + * REF_TRANSACTION_ERROR_CREATE_EXISTS error_code: ref to be created already exists. + * REF_TRANSACTION_ERROR_NONEXISTENT_REF error_code: ref expected but doesn't exist. + * REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE error_code: provided old_oid or old_target of + * reference doesn't match actual. + * REF_TRANSACTION_ERROR_INVALID_NEW_VALUE error_code: provided new_oid or new_target is + * invalid. + * REF_TRANSACTION_ERROR_EXPECTED_SYMREF error_code: expected ref to be symref, but is a + * regular ref. + */ +enum ref_transaction_error { + REF_TRANSACTION_ERROR_GENERIC = -1, + REF_TRANSACTION_ERROR_NAME_CONFLICT = -2, + REF_TRANSACTION_ERROR_CREATE_EXISTS = -3, + REF_TRANSACTION_ERROR_NONEXISTENT_REF = -4, + REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE = -5, + REF_TRANSACTION_ERROR_INVALID_NEW_VALUE = -6, + REF_TRANSACTION_ERROR_EXPECTED_SYMREF = -7, +}; + /* * Resolve a reference, recursively following symbolic references. * @@ -117,24 +140,24 @@ int refs_read_symbolic_ref(struct ref_store *ref_store, const char *refname, * * extras and skip must be sorted. */ -int refs_verify_refname_available(struct ref_store *refs, - const char *refname, - const struct string_list *extras, - const struct string_list *skip, - unsigned int initial_transaction, - struct strbuf *err); +enum ref_transaction_error refs_verify_refname_available(struct ref_store *refs, + const char *refname, + const struct string_list *extras, + const struct string_list *skip, + unsigned int initial_transaction, + struct strbuf *err); /* * Same as `refs_verify_refname_available()`, but checking for a list of * refnames instead of only a single item. This is more efficient in the case * where one needs to check multiple refnames. */ -int refs_verify_refnames_available(struct ref_store *refs, - const struct string_list *refnames, - const struct string_list *extras, - const struct string_list *skip, - unsigned int initial_transaction, - struct strbuf *err); +enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs, + const struct string_list *refnames, + const struct string_list *extras, + const struct string_list *skip, + unsigned int initial_transaction, + struct strbuf *err); int refs_ref_exists(struct ref_store *refs, const char *refname); @@ -830,13 +853,6 @@ int ref_transaction_verify(struct ref_transaction *transaction, unsigned int flags, struct strbuf *err); -/* Naming conflict (for example, the ref names A and A/B conflict). */ -#define TRANSACTION_NAME_CONFLICT -1 -/* When only creation was requested, but the ref already exists. */ -#define TRANSACTION_CREATE_EXISTS -2 -/* All other errors. */ -#define TRANSACTION_GENERIC_ERROR -3 - /* * Perform the preparatory stages of committing `transaction`. Acquire * any needed locks, check preconditions, etc.; basically, do as much diff --git a/refs/files-backend.c b/refs/files-backend.c index 7c6a0b3478..1e1663f44b 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -663,7 +663,7 @@ static void unlock_ref(struct ref_lock *lock) * broken, lock the reference anyway but clear old_oid. * * Return 0 on success. On failure, write an error message to err and - * return TRANSACTION_NAME_CONFLICT or TRANSACTION_GENERIC_ERROR. + * return REF_TRANSACTION_ERROR_NAME_CONFLICT or REF_TRANSACTION_ERROR_GENERIC. * * Implementation note: This function is basically * @@ -676,19 +676,20 @@ static void unlock_ref(struct ref_lock *lock) * avoided, namely if we were successfully able to read the ref * - Generate informative error messages in the case of failure */ -static int lock_raw_ref(struct files_ref_store *refs, - const char *refname, int mustexist, - struct string_list *refnames_to_check, - const struct string_list *extras, - struct ref_lock **lock_p, - struct strbuf *referent, - unsigned int *type, - struct strbuf *err) -{ +static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs, + const char *refname, + int mustexist, + struct string_list *refnames_to_check, + const struct string_list *extras, + struct ref_lock **lock_p, + struct strbuf *referent, + unsigned int *type, + struct strbuf *err) +{ + enum ref_transaction_error ret = REF_TRANSACTION_ERROR_GENERIC; struct ref_lock *lock; struct strbuf ref_file = STRBUF_INIT; int attempts_remaining = 3; - int ret = TRANSACTION_GENERIC_ERROR; int failure_errno; assert(err); @@ -728,13 +729,14 @@ static int lock_raw_ref(struct files_ref_store *refs, strbuf_reset(err); strbuf_addf(err, "unable to resolve reference '%s'", refname); + ret = REF_TRANSACTION_ERROR_NONEXISTENT_REF; } else { /* * The error message set by * refs_verify_refname_available() is * OK. */ - ret = TRANSACTION_NAME_CONFLICT; + ret = REF_TRANSACTION_ERROR_NAME_CONFLICT; } } else { /* @@ -788,6 +790,7 @@ static int lock_raw_ref(struct files_ref_store *refs, /* Garden variety missing reference. */ strbuf_addf(err, "unable to resolve reference '%s'", refname); + ret = REF_TRANSACTION_ERROR_NONEXISTENT_REF; goto error_return; } else { /* @@ -820,6 +823,7 @@ static int lock_raw_ref(struct files_ref_store *refs, /* Garden variety missing reference. */ strbuf_addf(err, "unable to resolve reference '%s'", refname); + ret = REF_TRANSACTION_ERROR_NONEXISTENT_REF; goto error_return; } else if (remove_dir_recursively(&ref_file, REMOVE_DIR_EMPTY_ONLY)) { @@ -830,7 +834,7 @@ static int lock_raw_ref(struct files_ref_store *refs, * The error message set by * verify_refname_available() is OK. */ - ret = TRANSACTION_NAME_CONFLICT; + ret = REF_TRANSACTION_ERROR_NAME_CONFLICT; goto error_return; } else { /* @@ -1517,10 +1521,11 @@ static int rename_tmp_log(struct files_ref_store *refs, const char *newrefname) return ret; } -static int write_ref_to_lockfile(struct files_ref_store *refs, - struct ref_lock *lock, - const struct object_id *oid, - int skip_oid_verification, struct strbuf *err); +static enum ref_transaction_error write_ref_to_lockfile(struct files_ref_store *refs, + struct ref_lock *lock, + const struct object_id *oid, + int skip_oid_verification, + struct strbuf *err); static int commit_ref_update(struct files_ref_store *refs, struct ref_lock *lock, const struct object_id *oid, const char *logmsg, @@ -1926,10 +1931,11 @@ static int files_log_ref_write(struct files_ref_store *refs, * Write oid into the open lockfile, then close the lockfile. On * errors, rollback the lockfile, fill in *err and return -1. */ -static int write_ref_to_lockfile(struct files_ref_store *refs, - struct ref_lock *lock, - const struct object_id *oid, - int skip_oid_verification, struct strbuf *err) +static enum ref_transaction_error write_ref_to_lockfile(struct files_ref_store *refs, + struct ref_lock *lock, + const struct object_id *oid, + int skip_oid_verification, + struct strbuf *err) { static char term = '\n'; struct object *o; @@ -1943,7 +1949,7 @@ static int write_ref_to_lockfile(struct files_ref_store *refs, "trying to write ref '%s' with nonexistent object %s", lock->ref_name, oid_to_hex(oid)); unlock_ref(lock); - return -1; + return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE; } if (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) { strbuf_addf( @@ -1951,7 +1957,7 @@ static int write_ref_to_lockfile(struct files_ref_store *refs, "trying to write non-commit object %s to branch '%s'", oid_to_hex(oid), lock->ref_name); unlock_ref(lock); - return -1; + return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE; } } fd = get_lock_file_fd(&lock->lk); @@ -1962,7 +1968,7 @@ static int write_ref_to_lockfile(struct files_ref_store *refs, strbuf_addf(err, "couldn't write '%s'", get_lock_file_path(&lock->lk)); unlock_ref(lock); - return -1; + return REF_TRANSACTION_ERROR_GENERIC; } return 0; } @@ -2376,9 +2382,10 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st * If update is a direct update of head_ref (the reference pointed to * by HEAD), then add an extra REF_LOG_ONLY update for HEAD. */ -static int split_head_update(struct ref_update *update, - struct ref_transaction *transaction, - const char *head_ref, struct strbuf *err) +static enum ref_transaction_error split_head_update(struct ref_update *update, + struct ref_transaction *transaction, + const char *head_ref, + struct strbuf *err) { struct ref_update *new_update; @@ -2402,7 +2409,7 @@ static int split_head_update(struct ref_update *update, "multiple updates for 'HEAD' (including one " "via its referent '%s') are not allowed", update->refname); - return TRANSACTION_NAME_CONFLICT; + return REF_TRANSACTION_ERROR_NAME_CONFLICT; } new_update = ref_transaction_add_update( @@ -2430,10 +2437,10 @@ static int split_head_update(struct ref_update *update, * Note that the new update will itself be subject to splitting when * the iteration gets to it. */ -static int split_symref_update(struct ref_update *update, - const char *referent, - struct ref_transaction *transaction, - struct strbuf *err) +static enum ref_transaction_error split_symref_update(struct ref_update *update, + const char *referent, + struct ref_transaction *transaction, + struct strbuf *err) { struct ref_update *new_update; unsigned int new_flags; @@ -2450,7 +2457,7 @@ static int split_symref_update(struct ref_update *update, "multiple updates for '%s' (including one " "via symref '%s') are not allowed", referent, update->refname); - return TRANSACTION_NAME_CONFLICT; + return REF_TRANSACTION_ERROR_NAME_CONFLICT; } new_flags = update->flags; @@ -2491,11 +2498,10 @@ static int split_symref_update(struct ref_update *update, * everything is OK, return 0; otherwise, write an error message to * err and return -1. */ -static int check_old_oid(struct ref_update *update, struct object_id *oid, - struct strbuf *err) +static enum ref_transaction_error check_old_oid(struct ref_update *update, + struct object_id *oid, + struct strbuf *err) { - int ret = TRANSACTION_GENERIC_ERROR; - if (!(update->flags & REF_HAVE_OLD) || oideq(oid, &update->old_oid)) return 0; @@ -2504,21 +2510,20 @@ static int check_old_oid(struct ref_update *update, struct object_id *oid, strbuf_addf(err, "cannot lock ref '%s': " "reference already exists", ref_update_original_update_refname(update)); - ret = TRANSACTION_CREATE_EXISTS; - } - else if (is_null_oid(oid)) + return REF_TRANSACTION_ERROR_CREATE_EXISTS; + } else if (is_null_oid(oid)) { strbuf_addf(err, "cannot lock ref '%s': " "reference is missing but expected %s", ref_update_original_update_refname(update), oid_to_hex(&update->old_oid)); - else - strbuf_addf(err, "cannot lock ref '%s': " - "is at %s but expected %s", - ref_update_original_update_refname(update), - oid_to_hex(oid), - oid_to_hex(&update->old_oid)); + return REF_TRANSACTION_ERROR_NONEXISTENT_REF; + } - return ret; + strbuf_addf(err, "cannot lock ref '%s': is at %s but expected %s", + ref_update_original_update_refname(update), oid_to_hex(oid), + oid_to_hex(&update->old_oid)); + + return REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE; } struct files_transaction_backend_data { @@ -2540,17 +2545,17 @@ struct files_transaction_backend_data { * - If it is an update of head_ref, add a corresponding REF_LOG_ONLY * update of HEAD. */ -static int lock_ref_for_update(struct files_ref_store *refs, - struct ref_update *update, - struct ref_transaction *transaction, - const char *head_ref, - struct string_list *refnames_to_check, - struct strbuf *err) +static enum ref_transaction_error lock_ref_for_update(struct files_ref_store *refs, + struct ref_update *update, + struct ref_transaction *transaction, + const char *head_ref, + struct string_list *refnames_to_check, + struct strbuf *err) { struct strbuf referent = STRBUF_INIT; int mustexist = ref_update_expects_existing_old_ref(update); struct files_transaction_backend_data *backend_data; - int ret = 0; + enum ref_transaction_error ret = 0; struct ref_lock *lock; files_assert_main_repository(refs, "lock_ref_for_update"); @@ -2602,22 +2607,17 @@ static int lock_ref_for_update(struct files_ref_store *refs, strbuf_addf(err, "cannot lock ref '%s': " "error reading reference", ref_update_original_update_refname(update)); - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto out; } } - if (update->old_target) { - if (ref_update_check_old_target(referent.buf, update, err)) { - ret = TRANSACTION_GENERIC_ERROR; - goto out; - } - } else { + if (update->old_target) + ret = ref_update_check_old_target(referent.buf, update, err); + else ret = check_old_oid(update, &lock->old_oid, err); - if (ret) { - goto out; - } - } + if (ret) + goto out; } else { /* * Create a new update for the reference this @@ -2644,7 +2644,7 @@ static int lock_ref_for_update(struct files_ref_store *refs, "but is a regular ref"), ref_update_original_update_refname(update), update->old_target); - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_EXPECTED_SYMREF; goto out; } else { ret = check_old_oid(update, &lock->old_oid, err); @@ -2668,14 +2668,14 @@ static int lock_ref_for_update(struct files_ref_store *refs, if (update->new_target && !(update->flags & REF_LOG_ONLY)) { if (create_symref_lock(lock, update->new_target, err)) { - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto out; } if (close_ref_gently(lock)) { strbuf_addf(err, "couldn't close '%s.lock'", update->refname); - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto out; } @@ -2693,25 +2693,27 @@ static int lock_ref_for_update(struct files_ref_store *refs, * The reference already has the desired * value, so we don't need to write it. */ - } else if (write_ref_to_lockfile( - refs, lock, &update->new_oid, - update->flags & REF_SKIP_OID_VERIFICATION, - err)) { - char *write_err = strbuf_detach(err, NULL); - - /* - * The lock was freed upon failure of - * write_ref_to_lockfile(): - */ - update->backend_data = NULL; - strbuf_addf(err, - "cannot update ref '%s': %s", - update->refname, write_err); - free(write_err); - ret = TRANSACTION_GENERIC_ERROR; - goto out; } else { - update->flags |= REF_NEEDS_COMMIT; + ret = write_ref_to_lockfile( + refs, lock, &update->new_oid, + update->flags & REF_SKIP_OID_VERIFICATION, + err); + if (ret) { + char *write_err = strbuf_detach(err, NULL); + + /* + * The lock was freed upon failure of + * write_ref_to_lockfile(): + */ + update->backend_data = NULL; + strbuf_addf(err, + "cannot update ref '%s': %s", + update->refname, write_err); + free(write_err); + goto out; + } else { + update->flags |= REF_NEEDS_COMMIT; + } } } if (!(update->flags & REF_NEEDS_COMMIT)) { @@ -2723,7 +2725,7 @@ static int lock_ref_for_update(struct files_ref_store *refs, if (close_ref_gently(lock)) { strbuf_addf(err, "couldn't close '%s.lock'", update->refname); - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto out; } } @@ -2865,7 +2867,7 @@ static int files_transaction_prepare(struct ref_store *ref_store, refs->packed_ref_store, transaction->flags, err); if (!packed_transaction) { - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto cleanup; } @@ -2897,13 +2899,13 @@ static int files_transaction_prepare(struct ref_store *ref_store, */ if (refs_verify_refnames_available(refs->packed_ref_store, &refnames_to_check, &transaction->refnames, NULL, 0, err)) { - ret = TRANSACTION_NAME_CONFLICT; + ret = REF_TRANSACTION_ERROR_NAME_CONFLICT; goto cleanup; } if (packed_transaction) { if (packed_refs_lock(refs->packed_ref_store, 0, err)) { - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto cleanup; } backend_data->packed_refs_locked = 1; @@ -2934,7 +2936,7 @@ static int files_transaction_prepare(struct ref_store *ref_store, */ backend_data->packed_transaction = NULL; if (ref_transaction_abort(packed_transaction, err)) { - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto cleanup; } } @@ -3035,7 +3037,7 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, packed_transaction = ref_store_transaction_begin(refs->packed_ref_store, transaction->flags, err); if (!packed_transaction) { - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto cleanup; } @@ -3058,7 +3060,7 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, if (!loose_transaction) { loose_transaction = ref_store_transaction_begin(&refs->base, 0, err); if (!loose_transaction) { - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto cleanup; } } @@ -3083,19 +3085,19 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, } if (packed_refs_lock(refs->packed_ref_store, 0, err)) { - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto cleanup; } if (refs_verify_refnames_available(&refs->base, &refnames_to_check, &affected_refnames, NULL, 1, err)) { packed_refs_unlock(refs->packed_ref_store); - ret = TRANSACTION_NAME_CONFLICT; + ret = REF_TRANSACTION_ERROR_NAME_CONFLICT; goto cleanup; } if (ref_transaction_commit(packed_transaction, err)) { - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto cleanup; } packed_refs_unlock(refs->packed_ref_store); @@ -3103,7 +3105,7 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, if (loose_transaction) { if (ref_transaction_prepare(loose_transaction, err) || ref_transaction_commit(loose_transaction, err)) { - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto cleanup; } } @@ -3152,7 +3154,7 @@ static int files_transaction_finish(struct ref_store *ref_store, if (update->flags & REF_NEEDS_COMMIT || update->flags & REF_LOG_ONLY) { if (parse_and_write_reflog(refs, update, lock, err)) { - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto cleanup; } } @@ -3171,7 +3173,7 @@ static int files_transaction_finish(struct ref_store *ref_store, strbuf_addf(err, "couldn't set '%s'", lock->ref_name); unlock_ref(lock); update->backend_data = NULL; - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto cleanup; } } @@ -3227,7 +3229,7 @@ static int files_transaction_finish(struct ref_store *ref_store, strbuf_reset(&sb); files_ref_path(refs, &sb, lock->ref_name); if (unlink_or_msg(sb.buf, err)) { - ret = TRANSACTION_GENERIC_ERROR; + ret = REF_TRANSACTION_ERROR_GENERIC; goto cleanup; } } diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 19220d2e99..5458952624 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1326,10 +1326,11 @@ static int packed_ref_store_remove_on_disk(struct ref_store *ref_store, * The packfile must be locked before calling this function and will * remain locked when it is done. */ -static int write_with_updates(struct packed_ref_store *refs, - struct string_list *updates, - struct strbuf *err) +static enum ref_transaction_error write_with_updates(struct packed_ref_store *refs, + struct string_list *updates, + struct strbuf *err) { + enum ref_transaction_error ret = REF_TRANSACTION_ERROR_GENERIC; struct ref_iterator *iter = NULL; size_t i; int ok; @@ -1353,7 +1354,7 @@ static int write_with_updates(struct packed_ref_store *refs, strbuf_addf(err, "unable to create file %s: %s", sb.buf, strerror(errno)); strbuf_release(&sb); - return -1; + return REF_TRANSACTION_ERROR_GENERIC; } strbuf_release(&sb); @@ -1409,6 +1410,7 @@ static int write_with_updates(struct packed_ref_store *refs, strbuf_addf(err, "cannot update ref '%s': " "reference already exists", update->refname); + ret = REF_TRANSACTION_ERROR_CREATE_EXISTS; goto error; } else if (!oideq(&update->old_oid, iter->oid)) { strbuf_addf(err, "cannot update ref '%s': " @@ -1416,6 +1418,7 @@ static int write_with_updates(struct packed_ref_store *refs, update->refname, oid_to_hex(iter->oid), oid_to_hex(&update->old_oid)); + ret = REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE; goto error; } } @@ -1452,6 +1455,7 @@ static int write_with_updates(struct packed_ref_store *refs, "reference is missing but expected %s", update->refname, oid_to_hex(&update->old_oid)); + return REF_TRANSACTION_ERROR_NONEXISTENT_REF; goto error; } } @@ -1509,7 +1513,7 @@ static int write_with_updates(struct packed_ref_store *refs, strerror(errno)); strbuf_release(&sb); delete_tempfile(&refs->tempfile); - return -1; + return REF_TRANSACTION_ERROR_GENERIC; } return 0; @@ -1521,7 +1525,7 @@ static int write_with_updates(struct packed_ref_store *refs, error: ref_iterator_free(iter); delete_tempfile(&refs->tempfile); - return -1; + return ret; } int is_packed_transaction_needed(struct ref_store *ref_store, @@ -1654,7 +1658,7 @@ static int packed_transaction_prepare(struct ref_store *ref_store, REF_STORE_READ | REF_STORE_WRITE | REF_STORE_ODB, "ref_transaction_prepare"); struct packed_transaction_backend_data *data; - int ret = TRANSACTION_GENERIC_ERROR; + enum ref_transaction_error ret = REF_TRANSACTION_ERROR_GENERIC; /* * Note that we *don't* skip transactions with zero updates, @@ -1675,7 +1679,8 @@ static int packed_transaction_prepare(struct ref_store *ref_store, data->own_lock = 1; } - if (write_with_updates(refs, &transaction->refnames, err)) + ret = write_with_updates(refs, &transaction->refnames, err); + if (ret) goto failure; transaction->state = REF_TRANSACTION_PREPARED; @@ -1707,7 +1712,7 @@ static int packed_transaction_finish(struct ref_store *ref_store, ref_store, REF_STORE_READ | REF_STORE_WRITE | REF_STORE_ODB, "ref_transaction_finish"); - int ret = TRANSACTION_GENERIC_ERROR; + int ret = REF_TRANSACTION_ERROR_GENERIC; char *packed_refs_path; clear_snapshot(refs); diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 6d3770d0cc..3f1d19abd9 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -770,8 +770,9 @@ int ref_update_has_null_new_value(struct ref_update *update); * If everything is OK, return 0; otherwise, write an error message to * err and return -1. */ -int ref_update_check_old_target(const char *referent, struct ref_update *update, - struct strbuf *err); +enum ref_transaction_error ref_update_check_old_target(const char *referent, + struct ref_update *update, + struct strbuf *err); /* * Check if the ref must exist, this means that the old_oid or diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 2c1e2995de..0132b8b06a 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1069,20 +1069,20 @@ static int queue_transaction_update(struct reftable_ref_store *refs, return 0; } -static int prepare_single_update(struct reftable_ref_store *refs, - struct reftable_transaction_data *tx_data, - struct ref_transaction *transaction, - struct reftable_backend *be, - struct ref_update *u, - struct string_list *refnames_to_check, - unsigned int head_type, - struct strbuf *head_referent, - struct strbuf *referent, - struct strbuf *err) +static enum ref_transaction_error prepare_single_update(struct reftable_ref_store *refs, + struct reftable_transaction_data *tx_data, + struct ref_transaction *transaction, + struct reftable_backend *be, + struct ref_update *u, + struct string_list *refnames_to_check, + unsigned int head_type, + struct strbuf *head_referent, + struct strbuf *referent, + struct strbuf *err) { + enum ref_transaction_error ret = 0; struct object_id current_oid = {0}; const char *rewritten_ref; - int ret = 0; /* * There is no need to reload the respective backends here as @@ -1093,7 +1093,7 @@ static int prepare_single_update(struct reftable_ref_store *refs, */ ret = backend_for(&be, refs, u->refname, &rewritten_ref, 0); if (ret) - return ret; + return REF_TRANSACTION_ERROR_GENERIC; /* Verify that the new object ID is valid. */ if ((u->flags & REF_HAVE_NEW) && !is_null_oid(&u->new_oid) && @@ -1104,13 +1104,13 @@ static int prepare_single_update(struct reftable_ref_store *refs, strbuf_addf(err, _("trying to write ref '%s' with nonexistent object %s"), u->refname, oid_to_hex(&u->new_oid)); - return -1; + return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE; } if (o->type != OBJ_COMMIT && is_branch(u->refname)) { strbuf_addf(err, _("trying to write non-commit object %s to branch '%s'"), oid_to_hex(&u->new_oid), u->refname); - return -1; + return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE; } } @@ -1134,7 +1134,7 @@ static int prepare_single_update(struct reftable_ref_store *refs, _("multiple updates for 'HEAD' (including one " "via its referent '%s') are not allowed"), u->refname); - return TRANSACTION_NAME_CONFLICT; + return REF_TRANSACTION_ERROR_NAME_CONFLICT; } ref_transaction_add_update( @@ -1147,7 +1147,7 @@ static int prepare_single_update(struct reftable_ref_store *refs, ret = reftable_backend_read_ref(be, rewritten_ref, ¤t_oid, referent, &u->type); if (ret < 0) - return ret; + return REF_TRANSACTION_ERROR_GENERIC; if (ret > 0 && !ref_update_expects_existing_old_ref(u)) { /* * The reference does not exist, and we either have no @@ -1168,7 +1168,7 @@ static int prepare_single_update(struct reftable_ref_store *refs, ret = queue_transaction_update(refs, tx_data, u, ¤t_oid, err); if (ret) - return ret; + return REF_TRANSACTION_ERROR_GENERIC; } return 0; @@ -1180,7 +1180,7 @@ static int prepare_single_update(struct reftable_ref_store *refs, "unable to resolve reference '%s'"), ref_update_original_update_refname(u), u->refname); - return -1; + return REF_TRANSACTION_ERROR_NONEXISTENT_REF; } if (u->type & REF_ISSYMREF) { @@ -1196,7 +1196,7 @@ static int prepare_single_update(struct reftable_ref_store *refs, if (u->flags & REF_HAVE_OLD && !resolved) { strbuf_addf(err, _("cannot lock ref '%s': " "error reading reference"), u->refname); - return -1; + return REF_TRANSACTION_ERROR_GENERIC; } } else { struct ref_update *new_update; @@ -1211,7 +1211,7 @@ static int prepare_single_update(struct reftable_ref_store *refs, _("multiple updates for '%s' (including one " "via symref '%s') are not allowed"), referent->buf, u->refname); - return TRANSACTION_NAME_CONFLICT; + return REF_TRANSACTION_ERROR_NAME_CONFLICT; } /* @@ -1255,31 +1255,32 @@ static int prepare_single_update(struct reftable_ref_store *refs, "but is a regular ref"), ref_update_original_update_refname(u), u->old_target); - return -1; + return REF_TRANSACTION_ERROR_EXPECTED_SYMREF; } - if (ref_update_check_old_target(referent->buf, u, err)) { - return -1; - } + ret = ref_update_check_old_target(referent->buf, u, err); + if (ret) + return ret; } else if ((u->flags & REF_HAVE_OLD) && !oideq(¤t_oid, &u->old_oid)) { if (is_null_oid(&u->old_oid)) { strbuf_addf(err, _("cannot lock ref '%s': " "reference already exists"), ref_update_original_update_refname(u)); - return TRANSACTION_CREATE_EXISTS; - } - else if (is_null_oid(¤t_oid)) + return REF_TRANSACTION_ERROR_CREATE_EXISTS; + } else if (is_null_oid(¤t_oid)) { strbuf_addf(err, _("cannot lock ref '%s': " "reference is missing but expected %s"), ref_update_original_update_refname(u), oid_to_hex(&u->old_oid)); - else + return REF_TRANSACTION_ERROR_NONEXISTENT_REF; + } else { strbuf_addf(err, _("cannot lock ref '%s': " "is at %s but expected %s"), ref_update_original_update_refname(u), oid_to_hex(¤t_oid), oid_to_hex(&u->old_oid)); - return TRANSACTION_NAME_CONFLICT; + return REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE; + } } /* @@ -1296,8 +1297,8 @@ static int prepare_single_update(struct reftable_ref_store *refs, if ((u->type & REF_ISSYMREF) || (u->flags & REF_LOG_ONLY) || (u->flags & REF_HAVE_NEW && !oideq(¤t_oid, &u->new_oid))) - return queue_transaction_update(refs, tx_data, u, - ¤t_oid, err); + if (queue_transaction_update(refs, tx_data, u, ¤t_oid, err)) + return REF_TRANSACTION_ERROR_GENERIC; return 0; } @@ -1386,7 +1387,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, transaction->state = REF_TRANSACTION_PREPARED; done: - assert(ret != REFTABLE_API_ERROR); if (ret < 0) { free_transaction_data(tx_data); transaction->state = REF_TRANSACTION_CLOSED; From patchwork Wed Mar 5 17:39:01 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Karthik Nayak X-Patchwork-Id: 14003070 Received: from mail-ed1-f44.google.com (mail-ed1-f44.google.com [209.85.208.44]) (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 6F34C251786 for ; Wed, 5 Mar 2025 17:39:21 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.208.44 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741196363; cv=none; b=nc0AnFoXz7UjG0Q/8qs+IoN++yOVG6gHGryWX+RlwycbfQTg3QI9NPvzgEyd2YoeclJnLcXOq9INIIChTjTSaVazgT68SJqh4CfACQ2wZj1Umskigozp5aZdoz4h3ps9IkWq7sqt14b4cmlUe+YcQA5cSa2myzI+7pHDs7+EMHE= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741196363; c=relaxed/simple; bh=xEMGtcZCkzsiOVL2YQegKM1zvYjh6+zLbvI7pT6WNH8=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:References: In-Reply-To:To:Cc; b=tSVhYAVji03t3lyVI+ps/X8jlecvDn/Irce40rIaZEO2dCVqWZArsFRcDisVRtRtLDfs1hGHjkpukqmWmgtKFrrSIqgaExvdc7OdcEyHGMpOdrAtkMMgKFdplWNEvwLupPdBZ7AY6r9bAZF64CXo31nfYmDm5ReArL08iQtYJI0= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=caoP+sfE; arc=none smtp.client-ip=209.85.208.44 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="caoP+sfE" Received: by mail-ed1-f44.google.com with SMTP id 4fb4d7f45d1cf-5e52c1c3599so6310402a12.2 for ; Wed, 05 Mar 2025 09:39:21 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1741196359; x=1741801159; darn=vger.kernel.org; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:from:to:cc:subject:date:message-id :reply-to; bh=1RWgYJRmtIDJ3s5njXrfG/f68DzSe9+cEYdzPpp5ITs=; b=caoP+sfEOGiiX+LP7NvogNUhaAgw9DsWKn/ni0Iz7rRK81XeXx0ceHBc3Ytii7w/TD 667pRMsLSLmZxXEN6QcVO8QAykco4BYsiHUmTFKdscCOVJy77xGMqWbkOY94Xl9gQ9zW 6LHWHvetidG2sVSkEyqSfjP07qos4qZ5yXTdaTQILhX1PyI4jJ/etK26z04aa/RGS1Il V8MkaZdYZT0+yORiBCFLniZGi5SxFDIuY7/dbpTRhV5fVAtTCoxWCU91RtpBkSwgBcZK W2qW8HUZXfBeR1jCKUcUaOYcP45ja09xNnxtKrug/j3q1wkBUyMo3ZT4L2T/Z2ngr/FG sF7g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1741196359; x=1741801159; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=1RWgYJRmtIDJ3s5njXrfG/f68DzSe9+cEYdzPpp5ITs=; b=YvON26CLLgA1fdeTn0SSLUrywpmte1iZE0crvRqQ2qm10+2dJWc6AZ0WlUu/Biw/DG 2blKGZpx4RoQJhD4iNtkhsrVIT18IT1K/pyGMK31ABiCMnSmHKP2/zCkubcr9n41BPnd SQOSz3pFP/Ba/tQr0uBLDWFpN5Hm18OV5+ju6q9g2qWiW2xZOYU11Nj43DTz/mtFaJn2 FO4fxHTe1rAlWFc/ZArVwbQ0zyrogrwCIoMZWlkQ87EWz5sWIfitO4GyJAhdokETKSGg KUlYCsMxtuJH7VFFVr4YjghDib/RWlG3/0QEb25WldjWjdwrxyMYHkZV+lu+GwIEtrEW mWyg== X-Gm-Message-State: AOJu0YwWet+N9NkPqWUpZ0hRVZ+eBc/27Xeb1uDIke4ab88IdrLVp7eL AVConLHwOJRrXUKmJb/zAw1reM6VhFpCGSj+/B+iQp0zctSmFfJXv9lSQ3LL X-Gm-Gg: ASbGncvpA2adorV0WBya2/egP6/qEr8bcFO3mLbvhOpjJKoueHceGJcO4BM05sV9pkr BF04uO2EbWnU/D87YrApgZ2W4bQDT6eS+SNWzSmXz/ToQEhaqxkNpF+e2zaEN2fja9QDgy5wKUP S8/mVed3Q10weJiPnZrcDhAIn52DHo3TYBg5ytrfbPSs9LNR+Sdcx6sQXavOMrPnefeRWFLW+cq ZjAJ0EWHDEDr+O1l4OtWmcWuVvCfekJuStHUFZiijmhAuUwjIJDLgzqZzoeHyNk1BqFPq1nU5an 5ylxUnvYTet9R+g0Rpzp1TrvZLjRauteeH5+/MCkhcTKuCx18A== X-Google-Smtp-Source: AGHT+IEYO0wT2l85XOfHK56ehHdNsS9+gBxwV3YL72wRfWpSY4K2qd2u+hcGbebXDPntXngVwthK/A== X-Received: by 2002:a17:907:868b:b0:abe:fffb:f1ef with SMTP id a640c23a62f3a-ac20d9e816amr393023266b.20.1741196359031; Wed, 05 Mar 2025 09:39:19 -0800 (PST) Received: from [127.0.0.2] ([2a02:2455:8268:bc00:8a90:b290:3a5b:4dd]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-abf3a8e2f53sm946851866b.115.2025.03.05.09.39.18 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 05 Mar 2025 09:39:18 -0800 (PST) From: Karthik Nayak Date: Wed, 05 Mar 2025 18:39:01 +0100 Subject: [PATCH v3 6/8] refs: implement partial reference transaction support Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Message-Id: <20250305-245-partially-atomic-ref-updates-v3-6-0c64e3052354@gmail.com> References: <20250305-245-partially-atomic-ref-updates-v3-0-0c64e3052354@gmail.com> In-Reply-To: <20250305-245-partially-atomic-ref-updates-v3-0-0c64e3052354@gmail.com> To: git@vger.kernel.org Cc: Karthik Nayak , ps@pks.im, jltobler@gmail.com, phillip.wood123@gmail.com X-Mailer: b4 0.15-dev X-Developer-Signature: v=1; a=openpgp-sha256; l=13328; i=karthik.188@gmail.com; h=from:subject:message-id; bh=xEMGtcZCkzsiOVL2YQegKM1zvYjh6+zLbvI7pT6WNH8=; b=owJ4nAHtARL+kA0DAAoBPtWfJI5GjH8ByyZiAGfIjEDbPGtxAz+Y3COAIPKqupFr3s1ECwROM Sk8lp0K/jrGyIkBswQAAQoAHRYhBFfOTH9jdXEPy2XGBj7VnySORox/BQJnyIxAAAoJED7VnySO Rox/iRIMAIWoMU0Yn4lJoyrsjkNULxJrCQ5xJpFS2rTStOoqiVK6sI9nLZmGzxgIVb+S+8soHbG gryHAU9EC9TCZiaM6AkXNzNKfkZwrH73o0cYgzOu7rsyGahvluDw37IcgFmXpLECoSb83zeGKpb REr6REpW7Cpl6ChBMRrPNuO+xfHl+d41GmEMZhb2Ft302zlGBrdB/odjzUMdtfVs4CxLw2xzMlL 7tyuxAAk5VUqp5noYjFbAn0pA2jCuj4yoQAWW0srHAqF5XeiuRlivfulGp6hEg+im6wvTzdZ3je lPEbPq3AOa6ulor3pSCWWVkD6DumOuEbmTB7wNqnZyBxBbKZWXxkvr2zGJMaPlzu/N+k4oUBhh/ 4bxcQxgjyiUtkBc6RnNLh1bmy/BWh2lTCCmjTtEpnITCkuNGhqFjOlbrSe+OjdYH4K5s6Lh9eBa mQa8TWRueDSoRRKfezPcbwwmj7NoTjp3q8Io8Eh4PVlxlVfEJgZKu56He9TxgO1xPCjbAs5ZbV6 bU= X-Developer-Key: i=karthik.188@gmail.com; a=openpgp; fpr=57CE4C7F6375710FCB65C6063ED59F248E468C7F Git's reference transactions are all-or-nothing: either all updates succeed, or none do. While this atomic behavior is generally desirable, it can be suboptimal especially when using the reftable backend, where batching multiple reference updates into a single transaction is more efficient than performing them sequentially. Introduce partial transaction support with a new flag, 'REF_TRANSACTION_ALLOW_PARTIAL'. When enabled, this flag allows individual reference updates that would typically cause the entire transaction to fail due to non-system-related errors to be marked as rejected while permitting other updates to proceed. System errors referred by 'REF_TRANSACTION_ERROR_GENERIC' continue to result in the entire transaction failing. This approach enhances flexibility while preserving transactional integrity where necessary. The implementation introduces several key components: - Add 'rejection_err' field to struct `ref_update` to track failed updates with failure reason. - Add a new struct `ref_transaction_rejections` and a field within `ref_transaction` to this struct to allow quick iteration over rejected updates. - Modify reference backends (files, packed, reftable) to handle partial transactions by using `ref_transaction_set_rejected()` instead of failing the entire transaction when `REF_TRANSACTION_ALLOW_PARTIAL` is set. - Add `ref_transaction_for_each_rejected_update()` to let callers examine which updates were rejected and why. This foundational change enables partial transaction support throughout the reference subsystem. A following commit will expose this capability to users by adding a `--allow-partial` flag to 'git-update-ref(1)', providing both a user-facing feature and a testable implementation. Signed-off-by: Karthik Nayak --- refs.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++ refs.h | 22 ++++++++++++++++++ refs/files-backend.c | 12 +++++++++- refs/packed-backend.c | 27 ++++++++++++++++++++-- refs/refs-internal.h | 25 ++++++++++++++++++++ refs/reftable-backend.c | 12 +++++++++- 6 files changed, 155 insertions(+), 4 deletions(-) diff --git a/refs.c b/refs.c index 63b8050ce2..b735510c3b 100644 --- a/refs.c +++ b/refs.c @@ -1176,6 +1176,10 @@ struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs, tr->ref_store = refs; tr->flags = flags; string_list_init_dup(&tr->refnames); + + if (flags & REF_TRANSACTION_ALLOW_PARTIAL) + CALLOC_ARRAY(tr->rejections, 1); + return tr; } @@ -1206,11 +1210,45 @@ void ref_transaction_free(struct ref_transaction *transaction) free((char *)transaction->updates[i]->old_target); free(transaction->updates[i]); } + + if (transaction->rejections) + free(transaction->rejections->update_indices); + free(transaction->rejections); + string_list_clear(&transaction->refnames, 0); free(transaction->updates); free(transaction); } +int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction, + size_t update_idx, + enum ref_transaction_error err) +{ + if (update_idx >= transaction->nr) + BUG("trying to set rejection on invalid update index"); + + if (!(transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL)) + return 0; + + if (!transaction->rejections) + BUG("transaction not inititalized with partial support"); + + /* + * Don't accept generic errors, since these errors are not user + * input related. + */ + if (err == REF_TRANSACTION_ERROR_GENERIC) + return 0; + + transaction->updates[update_idx]->rejection_err = err; + ALLOC_GROW(transaction->rejections->update_indices, + transaction->rejections->nr + 1, + transaction->rejections->alloc); + transaction->rejections->update_indices[transaction->rejections->nr++] = update_idx; + + return 1; +} + struct ref_update *ref_transaction_add_update( struct ref_transaction *transaction, const char *refname, unsigned int flags, @@ -1236,6 +1274,7 @@ struct ref_update *ref_transaction_add_update( transaction->updates[transaction->nr++] = update; update->flags = flags; + update->rejection_err = 0; update->new_target = xstrdup_or_null(new_target); update->old_target = xstrdup_or_null(old_target); @@ -2727,6 +2766,28 @@ void ref_transaction_for_each_queued_update(struct ref_transaction *transaction, } } +void ref_transaction_for_each_rejected_update(struct ref_transaction *transaction, + ref_transaction_for_each_rejected_update_fn cb, + void *cb_data) +{ + if (!transaction->rejections) + return; + + for (size_t i = 0; i < transaction->rejections->nr; i++) { + size_t update_index = transaction->rejections->update_indices[i]; + struct ref_update *update = transaction->updates[update_index]; + + if (!update->rejection_err) + continue; + + cb(update->refname, + (update->flags & REF_HAVE_OLD) ? &update->old_oid : NULL, + (update->flags & REF_HAVE_NEW) ? &update->new_oid : NULL, + update->old_target, update->new_target, + update->rejection_err, cb_data); + } +} + int refs_delete_refs(struct ref_store *refs, const char *logmsg, struct string_list *refnames, unsigned int flags) { diff --git a/refs.h b/refs.h index 1b9213f9ce..5e5ff9e57d 100644 --- a/refs.h +++ b/refs.h @@ -673,6 +673,13 @@ enum ref_transaction_flag { * either be absent or null_oid. */ REF_TRANSACTION_FLAG_INITIAL = (1 << 0), + + /* + * The transaction mechanism by default fails all updates if any conflict + * is detected. This flag allows transactions to partially apply updates + * while rejecting updates which do not match the expected state. + */ + REF_TRANSACTION_ALLOW_PARTIAL = (1 << 1), }; /* @@ -903,6 +910,21 @@ void ref_transaction_for_each_queued_update(struct ref_transaction *transaction, ref_transaction_for_each_queued_update_fn cb, void *cb_data); +/* + * Execute the given callback function for each of the reference updates which + * have been rejected in the given transaction. + */ +typedef void ref_transaction_for_each_rejected_update_fn(const char *refname, + const struct object_id *old_oid, + const struct object_id *new_oid, + const char *old_target, + const char *new_target, + enum ref_transaction_error err, + void *cb_data); +void ref_transaction_for_each_rejected_update(struct ref_transaction *transaction, + ref_transaction_for_each_rejected_update_fn cb, + void *cb_data); + /* * Free `*transaction` and all associated data. */ diff --git a/refs/files-backend.c b/refs/files-backend.c index 1e1663f44b..c2fdee6013 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2852,8 +2852,15 @@ static int files_transaction_prepare(struct ref_store *ref_store, ret = lock_ref_for_update(refs, update, transaction, head_ref, &refnames_to_check, err); - if (ret) + if (ret) { + if (ref_transaction_maybe_set_rejected(transaction, i, ret)) { + strbuf_setlen(err, 0); + ret = 0; + + continue; + } goto cleanup; + } if (update->flags & REF_DELETING && !(update->flags & REF_LOG_ONLY) && @@ -3151,6 +3158,9 @@ static int files_transaction_finish(struct ref_store *ref_store, struct ref_update *update = transaction->updates[i]; struct ref_lock *lock = update->backend_data; + if (update->rejection_err) + continue; + if (update->flags & REF_NEEDS_COMMIT || update->flags & REF_LOG_ONLY) { if (parse_and_write_reflog(refs, update, lock, err)) { diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 5458952624..bfc6135743 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1327,10 +1327,11 @@ static int packed_ref_store_remove_on_disk(struct ref_store *ref_store, * remain locked when it is done. */ static enum ref_transaction_error write_with_updates(struct packed_ref_store *refs, - struct string_list *updates, + struct ref_transaction *transaction, struct strbuf *err) { enum ref_transaction_error ret = REF_TRANSACTION_ERROR_GENERIC; + struct string_list *updates = &transaction->refnames; struct ref_iterator *iter = NULL; size_t i; int ok; @@ -1411,6 +1412,13 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re "reference already exists", update->refname); ret = REF_TRANSACTION_ERROR_CREATE_EXISTS; + + if (ref_transaction_maybe_set_rejected(transaction, i, ret)) { + strbuf_setlen(err, 0); + ret = 0; + continue; + } + goto error; } else if (!oideq(&update->old_oid, iter->oid)) { strbuf_addf(err, "cannot update ref '%s': " @@ -1419,6 +1427,13 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re oid_to_hex(iter->oid), oid_to_hex(&update->old_oid)); ret = REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE; + + if (ref_transaction_maybe_set_rejected(transaction, i, ret)) { + strbuf_setlen(err, 0); + ret = 0; + continue; + } + goto error; } } @@ -1456,6 +1471,13 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re update->refname, oid_to_hex(&update->old_oid)); return REF_TRANSACTION_ERROR_NONEXISTENT_REF; + + if (ref_transaction_maybe_set_rejected(transaction, i, ret)) { + strbuf_setlen(err, 0); + ret = 0; + continue; + } + goto error; } } @@ -1521,6 +1543,7 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re write_error: strbuf_addf(err, "error writing to %s: %s", get_tempfile_path(refs->tempfile), strerror(errno)); + ret = REF_TRANSACTION_ERROR_GENERIC; error: ref_iterator_free(iter); @@ -1679,7 +1702,7 @@ static int packed_transaction_prepare(struct ref_store *ref_store, data->own_lock = 1; } - ret = write_with_updates(refs, &transaction->refnames, err); + ret = write_with_updates(refs, transaction, err); if (ret) goto failure; diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 3f1d19abd9..c417aec217 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -123,6 +123,11 @@ struct ref_update { */ uint64_t index; + /* + * Used in partial transactions to mark if a given update was rejected. + */ + enum ref_transaction_error rejection_err; + /* * If this ref_update was split off of a symref update via * split_symref_update(), then this member points at that @@ -142,6 +147,13 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname, struct object_id *oid, struct strbuf *referent, unsigned int *type, int *failure_errno); +/* + * Mark a given update as rejected with a given reason. + */ +int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction, + size_t update_idx, + enum ref_transaction_error err); + /* * Add a ref_update with the specified properties to transaction, and * return a pointer to the new object. This function does not verify @@ -183,6 +195,18 @@ enum ref_transaction_state { REF_TRANSACTION_CLOSED = 2 }; +/* + * Data structure to hold indices of updates which were rejected, when + * partial transactions where enabled. While the updates themselves hold + * the rejection error, this structure allows a transaction to iterate + * only over the rejected updates. + */ +struct ref_transaction_rejections { + size_t *update_indices; + size_t alloc; + size_t nr; +}; + /* * Data structure for holding a reference transaction, which can * consist of checks and updates to multiple references, carried out @@ -195,6 +219,7 @@ struct ref_transaction { size_t alloc; size_t nr; enum ref_transaction_state state; + struct ref_transaction_rejections *rejections; void *backend_data; unsigned int flags; uint64_t max_index; diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 0132b8b06a..dd9912d637 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1371,8 +1371,15 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, transaction->updates[i], &refnames_to_check, head_type, &head_referent, &referent, err); - if (ret) + if (ret) { + if (ref_transaction_maybe_set_rejected(transaction, i, ret)) { + strbuf_setlen(err, 0); + ret = 0; + + continue; + } goto done; + } } string_list_sort(&refnames_to_check); @@ -1455,6 +1462,9 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data struct reftable_transaction_update *tx_update = &arg->updates[i]; struct ref_update *u = tx_update->update; + if (u->rejection_err) + continue; + /* * Write a reflog entry when updating a ref to point to * something new in either of the following cases: From patchwork Wed Mar 5 17:39:02 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Karthik Nayak X-Patchwork-Id: 14003072 Received: from mail-ed1-f51.google.com (mail-ed1-f51.google.com [209.85.208.51]) (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 E29C6252901 for ; Wed, 5 Mar 2025 17:39:21 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.208.51 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741196365; cv=none; b=mMFm+WdGeimC6pzjEhtJNwpoUqx30wwytFpf1BHYgFFkZ9hD9rdwIaH63tYlq4fWSDITRTmmKNSGnxEufsTIy3Sx9xjTsk5GFMlPDlN9R5J/X3j+tCVGQJ0H+ks7vHfFwr7jrAjQXaBFJWAzQI8kHRYQTVkRwcovhjZjuJReQjk= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741196365; c=relaxed/simple; bh=1uVRVC5AK0r3l5poScKiYx9wQtnJFVTfYadriahjgZY=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:References: In-Reply-To:To:Cc; b=qVjqQ/nRN720NfhR/D8IMGxHr8LGA6bIzR6u2Ds3F2qcR7y0DsVD1osrlQuhqbDdg2fGLzCs0eUKDGNeJXMIRdPK15kEZgfrrqUPKNRMkbRqXEoJ4/OPZ2lvdMtVyYPd4p0Ps8v+AnG0sgCxsRUBQ6VquxqjB1gThVq/vQCkVZg= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=WOpsa7sc; arc=none smtp.client-ip=209.85.208.51 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="WOpsa7sc" Received: by mail-ed1-f51.google.com with SMTP id 4fb4d7f45d1cf-5e52c1c3599so6310413a12.2 for ; Wed, 05 Mar 2025 09:39:21 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1741196360; x=1741801160; darn=vger.kernel.org; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:from:to:cc:subject:date:message-id :reply-to; bh=qsnuHRtcTbMPDXn4rBYnYEt0A372DK6jcrXME9NQVFI=; b=WOpsa7scnKQuVrrTRHPwQhCPnfOXJ9K6bsOgns1bO+QAdaHt25BQIRUmB1C7KG3l0F /fE81iuutJ4h+a8udjm4rw0gIm8K5YHaqa02G/BOiQGkq+ZufjSl2a8FSIK4Bko76SUH PZhtazdqe9KST6/2tJhvovY2uwVzR+IDljY/Xu/jo9FTrPkb1tSfNfVvLDZ+oOolOShE Dqoy/KH2ZvK+4p1dxEjmrOYAD6RP9VHg3DFVpQymmWsSlZnrLZXTeu9aQGmMUnWLDWiS K4WGBgUwn2axsYt9XWl5sc1karApH2HwDHA8HrOqLdCpZPpb4QGt8kelNYD2SpX4mz+W Lk2w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1741196360; x=1741801160; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=qsnuHRtcTbMPDXn4rBYnYEt0A372DK6jcrXME9NQVFI=; b=VNQgDkJbg6ctfW/9wPk+BmNZNvfSkQ3LAV0vwOOUFDwsw7+twfDCgH8Ld29P1y1UR9 RAlsCYX543Jc32sgYjq/yRWFddqwGBQCvmg7ugg6ymlx7IqCx++Su+ALhPRdFe9wQvTJ 6iB/B+zEdkSEpdVMWvamcUaN3bd6II13xM2EPwTYg298vPp2fcRgoHVDx1Nu834kT+bT Gw+UjaH8a7G0pl37e3cvh/jEAJvRxXawK5rkS3GfsrDWkQbxBQ53BIaeAZBnUDQNdN1M PbZN+P7kMJSgvKCJKet8FUkEXHIckJhpPQgelqdPjTvGQVVs2m9U6TdzTO17JeYka/o6 esjg== X-Gm-Message-State: AOJu0Yye3ILsLbRn8K7OHPUe8FtLU2fkm1novSPUOYzkRnAtsMx9KAl7 ObWW04asaU4FGGRNUkNQDcWalZjA+e0RMKzXBN9/qkHZhpgPwbq9ueheG+er X-Gm-Gg: ASbGncuk+UUkWaoEvXFJqAKWR3oSWg7D7R9tkJB/C3kPIlh2ZK+Ay7hNDi5ugfNs0Y9 pEClDyedYxUySekb4UAqMM5zFgY1Rjh2kRN3JqXbuQ5UL+2vEq9bbWp1QesuTuFDBMsxZaYIc5D P8+UVaWI5a+VI0O9bUfYRd6HnVeH8RrMa4T+5L5r5EUv+ByWMFt9P3OG/FDz/8liJv6EXNchVtB iADevcG99TCGdTfnbNbTsDjn2heVgy49rUP2VQ8MVvHb15Zl5hp8vJ99LTkI+4MvIWgkg7s2Zce IvfOBSPPO+5Tl+BVE4oYddtq/DJB44AYDMqCvA9PzZLanjDaLQ== X-Google-Smtp-Source: AGHT+IFTsRKPgLlUibFxcsBOYH6SuSca0iUjNpp4AuQClVI4bHsNp0iaVz4BGIcb+cKDMguiuk1R6A== X-Received: by 2002:a17:907:d92:b0:abf:4bde:51b1 with SMTP id a640c23a62f3a-ac20d9e8f1cmr364404166b.21.1741196359838; Wed, 05 Mar 2025 09:39:19 -0800 (PST) Received: from [127.0.0.2] ([2a02:2455:8268:bc00:8a90:b290:3a5b:4dd]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-abf3a8e2f53sm946851866b.115.2025.03.05.09.39.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 05 Mar 2025 09:39:19 -0800 (PST) From: Karthik Nayak Date: Wed, 05 Mar 2025 18:39:02 +0100 Subject: [PATCH v3 7/8] refs: support partial update rejections during F/D checks Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Message-Id: <20250305-245-partially-atomic-ref-updates-v3-7-0c64e3052354@gmail.com> References: <20250305-245-partially-atomic-ref-updates-v3-0-0c64e3052354@gmail.com> In-Reply-To: <20250305-245-partially-atomic-ref-updates-v3-0-0c64e3052354@gmail.com> To: git@vger.kernel.org Cc: Karthik Nayak , ps@pks.im, jltobler@gmail.com, phillip.wood123@gmail.com X-Mailer: b4 0.15-dev X-Developer-Signature: v=1; a=openpgp-sha256; l=12730; i=karthik.188@gmail.com; h=from:subject:message-id; bh=1uVRVC5AK0r3l5poScKiYx9wQtnJFVTfYadriahjgZY=; b=owJ4nAHtARL+kA0DAAoBPtWfJI5GjH8ByyZiAGfIjEBEjo0fW14z4o5PUOKZyEUfrm5n3HeHY iCpU7uiYbW8tIkBswQAAQoAHRYhBFfOTH9jdXEPy2XGBj7VnySORox/BQJnyIxAAAoJED7VnySO Rox/iogL/3JJrH3/u+H3XZYDjzdlu1IDdfwLVWiIby3eDe8zQtG57kixbGxlIKC41zgDCZtKyrz QWE1MzGeXX7OwI+uF5NW6/UEX5DNZA3nGpXAFkOXoCUk2Z1vLmljnXm3U4AKbwTu2mZPsfAFZtT TRaV3pj8/o8DPxpXxpjioC+2Kg30apRITi9C3hcFVNPkoNyUB7EKrOteFLowv6dm0AVweUE1gc1 FOi/D25m6da9yOpVKDQxDoCulHQQ4Di5O4B5pFzUKKTxvPQIBiAXov7VSEfah5bxUqUGWINZUAe eFhm1EP+zrhBvrZLa7ZuS1bsRMLdNLF9nyVhU4Hv8d4ALSFqwkDU6ZSSN0BXTOEpqV6i7DS50I7 yBIGl4EGqfsYcvGvqmQX8UAg2e7o0MO4NmseV3oWB2oLMOckyLBHI7kMEQ6Is3D/lGsZsLS0C17 8EOQNk3bhGTE6qfm/CFIXcbie2GRYUnoQndQ96QDwza08y38WMJ4DxvRXNsZeEUJKNEm9X1WIn5 t8= X-Developer-Key: i=karthik.188@gmail.com; a=openpgp; fpr=57CE4C7F6375710FCB65C6063ED59F248E468C7F The `refs_verify_refnames_available()` is used to batch check refnames for F/D conflicts. While this is the more performant alternative than its individual version, it does not provide rejection capabilities on a single update level. For partial transactions, this would mean a rejection of the entire transaction whenever one reference has a F/D conflict. Modify the function to call `ref_transaction_maybe_set_rejected()` to check if a single update can be rejected. Since this function is only internally used within 'refs/' and we want to pass in a `struct ref_transaction *` as a variable. We also move and mark `refs_verify_refnames_available()` to 'refs-internal.h' to be an internal function. Signed-off-by: Karthik Nayak --- refs.c | 28 +++++++++++++++++++++++++++- refs.h | 12 ------------ refs/files-backend.c | 27 ++++++++++++++++++--------- refs/refs-internal.h | 17 +++++++++++++++++ refs/reftable-backend.c | 11 ++++++++--- 5 files changed, 70 insertions(+), 25 deletions(-) diff --git a/refs.c b/refs.c index b735510c3b..c4dccf9d8b 100644 --- a/refs.c +++ b/refs.c @@ -2540,6 +2540,7 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs const struct string_list *refnames, const struct string_list *extras, const struct string_list *skip, + struct ref_transaction *transaction, unsigned int initial_transaction, struct strbuf *err) { @@ -2559,6 +2560,7 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs strset_init(&dirnames); for (size_t i = 0; i < refnames->nr; i++) { + const size_t *update_idx = (size_t *)refnames->items[i].util; const char *refname = refnames->items[i].string; const char *extra_refname; struct object_id oid; @@ -2598,12 +2600,26 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs if (!initial_transaction && !refs_read_raw_ref(refs, dirname.buf, &oid, &referent, &type, &ignore_errno)) { + if (transaction && ref_transaction_maybe_set_rejected( + transaction, *update_idx, + REF_TRANSACTION_ERROR_NAME_CONFLICT)) { + strset_remove(&dirnames, dirname.buf); + continue; + } + strbuf_addf(err, _("'%s' exists; cannot create '%s'"), dirname.buf, refname); goto cleanup; } if (extras && string_list_has_string(extras, dirname.buf)) { + if (transaction && ref_transaction_maybe_set_rejected( + transaction, *update_idx, + REF_TRANSACTION_ERROR_NAME_CONFLICT)) { + strset_remove(&dirnames, dirname.buf); + continue; + } + strbuf_addf(err, _("cannot process '%s' and '%s' at the same time"), refname, dirname.buf); goto cleanup; @@ -2636,6 +2652,11 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs string_list_has_string(skip, iter->refname)) continue; + if (transaction && ref_transaction_maybe_set_rejected( + transaction, *update_idx, + REF_TRANSACTION_ERROR_NAME_CONFLICT)) + continue; + strbuf_addf(err, _("'%s' exists; cannot create '%s'"), iter->refname, refname); goto cleanup; @@ -2647,6 +2668,11 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs extra_refname = find_descendant_ref(dirname.buf, extras, skip); if (extra_refname) { + if (transaction && ref_transaction_maybe_set_rejected( + transaction, *update_idx, + REF_TRANSACTION_ERROR_NAME_CONFLICT)) + continue; + strbuf_addf(err, _("cannot process '%s' and '%s' at the same time"), refname, extra_refname); goto cleanup; @@ -2678,7 +2704,7 @@ enum ref_transaction_error refs_verify_refname_available( }; return refs_verify_refnames_available(refs, &refnames, extras, skip, - initial_transaction, err); + NULL, initial_transaction, err); } struct do_for_each_reflog_help { diff --git a/refs.h b/refs.h index 5e5ff9e57d..938420bec4 100644 --- a/refs.h +++ b/refs.h @@ -147,18 +147,6 @@ enum ref_transaction_error refs_verify_refname_available(struct ref_store *refs, unsigned int initial_transaction, struct strbuf *err); -/* - * Same as `refs_verify_refname_available()`, but checking for a list of - * refnames instead of only a single item. This is more efficient in the case - * where one needs to check multiple refnames. - */ -enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs, - const struct string_list *refnames, - const struct string_list *extras, - const struct string_list *skip, - unsigned int initial_transaction, - struct strbuf *err); - int refs_ref_exists(struct ref_store *refs, const char *refname); int should_autocreate_reflog(enum log_refs_config log_all_ref_updates, diff --git a/refs/files-backend.c b/refs/files-backend.c index c2fdee6013..7525bf75ab 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -677,16 +677,18 @@ static void unlock_ref(struct ref_lock *lock) * - Generate informative error messages in the case of failure */ static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs, - const char *refname, + struct ref_update *update, + size_t update_idx, int mustexist, struct string_list *refnames_to_check, const struct string_list *extras, struct ref_lock **lock_p, struct strbuf *referent, - unsigned int *type, struct strbuf *err) { enum ref_transaction_error ret = REF_TRANSACTION_ERROR_GENERIC; + const char *refname = update->refname; + unsigned int *type = &update->type; struct ref_lock *lock; struct strbuf ref_file = STRBUF_INIT; int attempts_remaining = 3; @@ -785,6 +787,8 @@ static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs, if (files_read_raw_ref(&refs->base, refname, &lock->old_oid, referent, type, &failure_errno)) { + struct string_list_item *item; + if (failure_errno == ENOENT) { if (mustexist) { /* Garden variety missing reference. */ @@ -864,7 +868,9 @@ static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs, * make sure there is no existing packed ref that conflicts * with refname. This check is deferred so that we can batch it. */ - string_list_insert(refnames_to_check, refname); + item = string_list_insert(refnames_to_check, refname); + item->util = xmalloc(sizeof(update_idx)); + memcpy(item->util, &update_idx, sizeof(update_idx)); } ret = 0; @@ -2547,6 +2553,7 @@ struct files_transaction_backend_data { */ static enum ref_transaction_error lock_ref_for_update(struct files_ref_store *refs, struct ref_update *update, + size_t update_idx, struct ref_transaction *transaction, const char *head_ref, struct string_list *refnames_to_check, @@ -2575,9 +2582,9 @@ static enum ref_transaction_error lock_ref_for_update(struct files_ref_store *re if (lock) { lock->count++; } else { - ret = lock_raw_ref(refs, update->refname, mustexist, + ret = lock_raw_ref(refs, update, update_idx, mustexist, refnames_to_check, &transaction->refnames, - &lock, &referent, &update->type, err); + &lock, &referent, err); if (ret) { char *reason; @@ -2849,7 +2856,7 @@ static int files_transaction_prepare(struct ref_store *ref_store, for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; - ret = lock_ref_for_update(refs, update, transaction, + ret = lock_ref_for_update(refs, update, i, transaction, head_ref, &refnames_to_check, err); if (ret) { @@ -2905,7 +2912,8 @@ static int files_transaction_prepare(struct ref_store *ref_store, * So instead, we accept the race for now. */ if (refs_verify_refnames_available(refs->packed_ref_store, &refnames_to_check, - &transaction->refnames, NULL, 0, err)) { + &transaction->refnames, NULL, transaction, + 0, err)) { ret = REF_TRANSACTION_ERROR_NAME_CONFLICT; goto cleanup; } @@ -2951,7 +2959,7 @@ static int files_transaction_prepare(struct ref_store *ref_store, cleanup: free(head_ref); - string_list_clear(&refnames_to_check, 0); + string_list_clear(&refnames_to_check, 1); if (ret) files_transaction_cleanup(refs, transaction); @@ -3097,7 +3105,8 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, } if (refs_verify_refnames_available(&refs->base, &refnames_to_check, - &affected_refnames, NULL, 1, err)) { + &affected_refnames, NULL, transaction, + 1, err)) { packed_refs_unlock(refs->packed_ref_store); ret = REF_TRANSACTION_ERROR_NAME_CONFLICT; goto cleanup; diff --git a/refs/refs-internal.h b/refs/refs-internal.h index c417aec217..f0e958dc83 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -805,4 +805,21 @@ enum ref_transaction_error ref_update_check_old_target(const char *referent, */ int ref_update_expects_existing_old_ref(struct ref_update *update); +/* + * Same as `refs_verify_refname_available()`, but checking for a list of + * refnames instead of only a single item. This is more efficient in the case + * where one needs to check multiple refnames. + * + * If a transaction is provided with partial support, then individual updates + * are marked rejected, reference backends are then in charge of not committing + * those updates. + */ +enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs, + const struct string_list *refnames, + const struct string_list *extras, + const struct string_list *skip, + struct ref_transaction *transaction, + unsigned int initial_transaction, + struct strbuf *err); + #endif /* REFS_REFS_INTERNAL_H */ diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index dd9912d637..a50e004d96 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1074,6 +1074,7 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor struct ref_transaction *transaction, struct reftable_backend *be, struct ref_update *u, + size_t update_idx, struct string_list *refnames_to_check, unsigned int head_type, struct strbuf *head_referent, @@ -1149,6 +1150,7 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor if (ret < 0) return REF_TRANSACTION_ERROR_GENERIC; if (ret > 0 && !ref_update_expects_existing_old_ref(u)) { + struct string_list_item *item; /* * The reference does not exist, and we either have no * old object ID or expect the reference to not exist. @@ -1158,7 +1160,9 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor * can output a proper error message instead of failing * at a later point. */ - string_list_append(refnames_to_check, u->refname); + item = string_list_append(refnames_to_check, u->refname); + item->util = xmalloc(sizeof(update_idx)); + memcpy(item->util, &update_idx, sizeof(update_idx)); /* * There is no need to write the reference deletion @@ -1368,7 +1372,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, for (i = 0; i < transaction->nr; i++) { ret = prepare_single_update(refs, tx_data, transaction, be, - transaction->updates[i], + transaction->updates[i], i, &refnames_to_check, head_type, &head_referent, &referent, err); if (ret) { @@ -1385,6 +1389,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, string_list_sort(&refnames_to_check); ret = refs_verify_refnames_available(ref_store, &refnames_to_check, &transaction->refnames, NULL, + transaction, transaction->flags & REF_TRANSACTION_FLAG_INITIAL, err); if (ret < 0) @@ -1403,7 +1408,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, } strbuf_release(&referent); strbuf_release(&head_referent); - string_list_clear(&refnames_to_check, 0); + string_list_clear(&refnames_to_check, 1); return ret; } From patchwork Wed Mar 5 17:39:03 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Karthik Nayak X-Patchwork-Id: 14003073 Received: from mail-ed1-f51.google.com (mail-ed1-f51.google.com [209.85.208.51]) (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 E2A27252903 for ; Wed, 5 Mar 2025 17:39:22 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.208.51 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741196365; cv=none; b=gEpEjnrVr08wrcI5mbdU+m4/Wg6/Ll9Rqx12On9krfD6lrR7MgAnKNpqhVEZvWXJHqOgTqVbrD/FffYVmCmOw7PhVothMQ0Nt+jl+d+ubpRocQmQv555JxkIiNsiMLM/cRCOzjNb1w3/u2rYs+c6g5xWO1/gSTmUv/ljYdSFSE0= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741196365; c=relaxed/simple; bh=3ZwXA3U9aBbIgH+h2/BZZyfOW9CNUXCKFrjKBtSDF/U=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:References: In-Reply-To:To:Cc; b=IhvBXPSvb6Wd8o4ZfeXze5B7bOmPm6lbHnX1+fWl7cY63i05rm/mquyQH8do2dmfN4+p7KT41e1H89C1p0cFg98TLXeV2eQx3rsxUY095h8p6Gc0/kOzMYImO1C/XD9/y2WscLD2MWxnZrqXH94SRQhsXS2ZFDCSqlEVlrU/by0= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=CFyiaoRh; arc=none smtp.client-ip=209.85.208.51 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="CFyiaoRh" Received: by mail-ed1-f51.google.com with SMTP id 4fb4d7f45d1cf-5e57c81d5d0so3728962a12.2 for ; Wed, 05 Mar 2025 09:39:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1741196361; x=1741801161; darn=vger.kernel.org; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:from:to:cc:subject:date:message-id :reply-to; bh=PWGEkBMpYgm5pb4UtFBa+nVSflMdDrKFEKrx9XdC/dI=; b=CFyiaoRhx0a/T4ev3AzYddrbITtXZiFHQuXHM3VSZ2b49xp2jMEnyPZSI4E7lPKT+8 zZr5t3Y7ZwZkVQWBwWwfP6mFnp0bl3+91WEBqgdOErJNupDJTa6qPvfrhgHZkf78xMtW /Kmmef+eKqFjm+HDVDlnyy8G3Ui42gelQafPuopEoMc3PI7TRElFDxJc/fI7xaHMwP2j eVX41qQ+CbFNFumlCQFZPkR+cRGbotgZpBK6v6JuwWtK6J2GdHLRyA6c1vNA+IDuLxer 010rAu+dytq4F3BKK+mx2mEcs+LazuBknl61nkTu0p6XjEe0RdKMi0LMo1HnLqrIb6jC bZpw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1741196361; x=1741801161; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=PWGEkBMpYgm5pb4UtFBa+nVSflMdDrKFEKrx9XdC/dI=; b=wNCGLLMt1uOEssn0dKvbPP0jr3KO9pwRqm5sarEXfarb6pQXr6NZmZfC+u44JgqvLB UB3dYoa272HRiaMVCeyzROhlCWJjF93k970yI9ELp6jlYQREn85inWprk4EKUv4oFN/v TvJKnleSwCywFGwFhef3lgbLPFPmsqDNyFgdi144LXKz3eIBPZI/oC6yLPH0N5BBALz4 Gzc7sVUwgZTaTMnaWHYcoMC1T37prtSZ+zC9XEFPOVcU14wQ3x8NOiM9JyX3AGH4znn7 lT/RRUah/pvSmQboxsylkDJ0AZ0hYqg1vF2pXE4ypIwSNGt/YP/5hIWF/cB4djAHIzDH piIw== X-Gm-Message-State: AOJu0YyspSDZ7cYFT9P6svhvWSwsYipU/0gjIgSXyLPkyfo8kLRkSohb hjs9vEQH0gUpS2rLP10TmRMHHGmz9PPrAiw9XIk9ZRhosTlIJINjI+Xaj+U2 X-Gm-Gg: ASbGncvE06a6ESJTwaRdWvVFa7KcUN6Qbg1eYja/5fL4r4UJiSPzxjIx0mnur1Nrz+u ncOtz7styHh6eZd0JoFe44xthIAM8gRG6WbHXD1KxI+Db48dyPq2Xn/tnHbG5C4TIw93s8GG2Q/ W3/BVH9RW/Vx4tnQbNWNhXLhX6Ehjr1hu4eN9f9CkxrS/E3/SJna51GZ10Pc0TVn+b1Q1unNxdH uHv7RXLf9SCgPE6Q3AEw6gbWRqSwBGM0s5Hi4iDtxM6QVQLbCu5IPo0La7oy/84cdUUgu9XRF02 UAbSX7qIdxq6edFTMQjOQNgh1dBHZGz3BL78H6l9j+5IdWhYzA== X-Google-Smtp-Source: AGHT+IEMjtVsWDRVALK/zZWGYkSQRiBbb548M/qTwEj0gI/Y0kcFUmloU/C5IlOK42/N6YYgR0bX/w== X-Received: by 2002:a05:6402:34cd:b0:5e5:2d5c:4f32 with SMTP id 4fb4d7f45d1cf-5e59f4b69f9mr8869791a12.28.1741196360598; Wed, 05 Mar 2025 09:39:20 -0800 (PST) Received: from [127.0.0.2] ([2a02:2455:8268:bc00:8a90:b290:3a5b:4dd]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-abf3a8e2f53sm946851866b.115.2025.03.05.09.39.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 05 Mar 2025 09:39:20 -0800 (PST) From: Karthik Nayak Date: Wed, 05 Mar 2025 18:39:03 +0100 Subject: [PATCH v3 8/8] update-ref: add --allow-partial flag for stdin mode Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Message-Id: <20250305-245-partially-atomic-ref-updates-v3-8-0c64e3052354@gmail.com> References: <20250305-245-partially-atomic-ref-updates-v3-0-0c64e3052354@gmail.com> In-Reply-To: <20250305-245-partially-atomic-ref-updates-v3-0-0c64e3052354@gmail.com> To: git@vger.kernel.org Cc: Karthik Nayak , ps@pks.im, jltobler@gmail.com, phillip.wood123@gmail.com X-Mailer: b4 0.15-dev X-Developer-Signature: v=1; a=openpgp-sha256; l=16579; i=karthik.188@gmail.com; h=from:subject:message-id; bh=3ZwXA3U9aBbIgH+h2/BZZyfOW9CNUXCKFrjKBtSDF/U=; b=owJ4nAHtARL+kA0DAAoBPtWfJI5GjH8ByyZiAGfIjEFrb+DJWjWe+LyxqOAaeGDBRuSJjDph6 CXntIe2ivbB0IkBswQAAQoAHRYhBFfOTH9jdXEPy2XGBj7VnySORox/BQJnyIxBAAoJED7VnySO Rox/fF8L+wbTREE9V4LVg3edEIJsmlaQNKpVroGRpDJoiAnF3qrLhbdSJ+NNA2Nkc4o0SzGj/VK Xp8WrzZst+3sti19LBgzQmHIzMRZkWknqOs0e2cuicKlaio2UGpW9Zq1ppaK4z07koMfWcfGjeD yEHuN/hwMnKyOgDJan3LWQp9JSzRUoCW27KoKCqRsqrNJx1XlV8odm+ahmHUthtlCDYAKDLLZNe UcEg6bJLn4CNjymDVgZqJpiCzwYS5npqpQ+V37uA9Bsha3H3EOFkYuHSAS5R8jm11Hpk1l87RI4 +AISWLqD2dAsduD3j8lQYnuoHYKMtbUFdpNyfE/ebTayYY7H0kN4JS456A26AoqG7LSaVMlDWTI t4LmjQJ3vFfVohqAzwHHh+mUZQ7stHlVPHfvO7thrKpYPMTG0anEwUS3LVpGfgfJNXKF2eZgRzu VmBdlouUdbNmOKaCP6A1LIFKPBfsI45mSTJP81VMBVNWTEY7yZmUGeBYftNtbYpERJJfrprgHp5 wA= X-Developer-Key: i=karthik.188@gmail.com; a=openpgp; fpr=57CE4C7F6375710FCB65C6063ED59F248E468C7F When updating multiple references through stdin, Git's update-ref command normally aborts the entire transaction if any single update fails. While this atomic behavior prevents partial updates by default, there are cases where applying successful updates while reporting failures is desirable. Add a new `--allow-partial` flag that allows the transaction to continue even when individual reference updates fail. This flag can only be used in `--stdin` mode and builds upon the partial transaction support added to the refs subsystem. When enabled, failed updates are reported in the following format: rejected SP ( | ) SP ( | ) SP LF Update the documentation to reflect this change and also tests to cover different scenarios where an update could be rejected. Signed-off-by: Karthik Nayak --- Documentation/git-update-ref.adoc | 17 ++- builtin/update-ref.c | 67 ++++++++++- t/t1400-update-ref.sh | 233 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 309 insertions(+), 8 deletions(-) diff --git a/Documentation/git-update-ref.adoc b/Documentation/git-update-ref.adoc index 9e6935d38d..bcf38850a4 100644 --- a/Documentation/git-update-ref.adoc +++ b/Documentation/git-update-ref.adoc @@ -7,8 +7,10 @@ git-update-ref - Update the object name stored in a ref safely SYNOPSIS -------- -[verse] -'git update-ref' [-m ] [--no-deref] (-d [] | [--create-reflog] [] | --stdin [-z]) +[synopsis] +git update-ref [-m ] [--no-deref] -d [] + [-m ] [--no-deref] [--create-reflog] [] + [-m ] [--no-deref] --stdin [-z] [--allow-partial] DESCRIPTION ----------- @@ -57,6 +59,17 @@ performs all modifications together. Specify commands of the form: With `--create-reflog`, update-ref will create a reflog for each ref even if one would not ordinarily be created. +With `--allow-partial`, update-ref continues executing the transaction even if +some updates fail due to invalid or incorrect user input, applying only the +successful updates. Errors resulting from user-provided input are treated as +non-system-related and do not cause the entire transaction to be aborted. +However, system-related errors—such as I/O failures or memory issues—will still +result in a full failure. Additionally, errors like F/D conflicts are batched +for performance optimization and will also cause a full failure. Any failed +updates will be reported in the following format: + + rejected SP ( | ) SP ( | ) SP LF + Quote fields containing whitespace as if they were strings in C source code; i.e., surrounded by double-quotes and with backslash escapes. Use 40 "0" characters or the empty string to specify a zero value. To diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 1d541e13ad..66bd3cb44f 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -5,6 +5,7 @@ #include "config.h" #include "gettext.h" #include "hash.h" +#include "hex.h" #include "refs.h" #include "object-name.h" #include "parse-options.h" @@ -13,7 +14,7 @@ static const char * const git_update_ref_usage[] = { N_("git update-ref [] -d []"), N_("git update-ref [] []"), - N_("git update-ref [] --stdin [-z]"), + N_("git update-ref [] --stdin [-z] [--allow-partial]"), NULL }; @@ -565,6 +566,49 @@ static void parse_cmd_abort(struct ref_transaction *transaction, report_ok("abort"); } +static void print_rejected_refs(const char *refname, + const struct object_id *old_oid, + const struct object_id *new_oid, + const char *old_target, + const char *new_target, + enum ref_transaction_error err, + void *cb_data UNUSED) +{ + struct strbuf sb = STRBUF_INIT; + const char *reason = ""; + + switch (err) { + case REF_TRANSACTION_ERROR_NAME_CONFLICT: + reason = "refname conflict"; + break; + case REF_TRANSACTION_ERROR_CREATE_EXISTS: + reason = "reference already exists"; + break; + case REF_TRANSACTION_ERROR_NONEXISTENT_REF: + reason = "reference does not exist"; + break; + case REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE: + reason = "incorrect old value provided"; + break; + case REF_TRANSACTION_ERROR_INVALID_NEW_VALUE: + reason = "invalid new value provided"; + break; + case REF_TRANSACTION_ERROR_EXPECTED_SYMREF: + reason = "expected symref but found regular ref"; + break; + default: + reason = "unkown failure"; + } + + strbuf_addf(&sb, "rejected %s %s %s %s\n", refname, + new_oid ? oid_to_hex(new_oid) : new_target, + old_oid ? oid_to_hex(old_oid) : old_target, + reason); + + fwrite(sb.buf, sb.len, 1, stdout); + strbuf_release(&sb); +} + static void parse_cmd_commit(struct ref_transaction *transaction, const char *next, const char *end UNUSED) { @@ -573,6 +617,10 @@ static void parse_cmd_commit(struct ref_transaction *transaction, die("commit: extra input: %s", next); if (ref_transaction_commit(transaction, &error)) die("commit: %s", error.buf); + + ref_transaction_for_each_rejected_update(transaction, + print_rejected_refs, NULL); + report_ok("commit"); ref_transaction_free(transaction); } @@ -609,7 +657,7 @@ static const struct parse_cmd { { "commit", parse_cmd_commit, 0, UPDATE_REFS_CLOSED }, }; -static void update_refs_stdin(void) +static void update_refs_stdin(unsigned int flags) { struct strbuf input = STRBUF_INIT, err = STRBUF_INIT; enum update_refs_state state = UPDATE_REFS_OPEN; @@ -617,7 +665,7 @@ static void update_refs_stdin(void) int i, j; transaction = ref_store_transaction_begin(get_main_ref_store(the_repository), - 0, &err); + flags, &err); if (!transaction) die("%s", err.buf); @@ -685,7 +733,7 @@ static void update_refs_stdin(void) */ state = cmd->state; transaction = ref_store_transaction_begin(get_main_ref_store(the_repository), - 0, &err); + flags, &err); if (!transaction) die("%s", err.buf); @@ -701,6 +749,8 @@ static void update_refs_stdin(void) /* Commit by default if no transaction was requested. */ if (ref_transaction_commit(transaction, &err)) die("%s", err.buf); + ref_transaction_for_each_rejected_update(transaction, + print_rejected_refs, NULL); ref_transaction_free(transaction); break; case UPDATE_REFS_STARTED: @@ -727,6 +777,8 @@ int cmd_update_ref(int argc, struct object_id oid, oldoid; int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0; int create_reflog = 0; + unsigned int flags = 0; + struct option options[] = { OPT_STRING( 'm', NULL, &msg, N_("reason"), N_("reason of the update")), OPT_BOOL('d', NULL, &delete, N_("delete the reference")), @@ -735,6 +787,8 @@ int cmd_update_ref(int argc, OPT_BOOL('z', NULL, &end_null, N_("stdin has NUL-terminated arguments")), OPT_BOOL( 0 , "stdin", &read_stdin, N_("read updates from stdin")), OPT_BOOL( 0 , "create-reflog", &create_reflog, N_("create a reflog")), + OPT_BIT('0', "allow-partial", &flags, N_("allow partial transactions"), + REF_TRANSACTION_ALLOW_PARTIAL), OPT_END(), }; @@ -756,9 +810,10 @@ int cmd_update_ref(int argc, usage_with_options(git_update_ref_usage, options); if (end_null) line_termination = '\0'; - update_refs_stdin(); + update_refs_stdin(flags); return 0; - } + } else if (flags & REF_TRANSACTION_ALLOW_PARTIAL) + die("--allow-partial can only be used with --stdin"); if (end_null) usage_with_options(git_update_ref_usage, options); diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 29045aad43..62a82f4af6 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -2066,6 +2066,239 @@ do grep "$(git rev-parse $a) $(git rev-parse $a)" actual ' + test_expect_success "stdin $type allow-partial" ' + git init repo && + test_when_finished "rm -fr repo" && + ( + cd repo && + test_commit commit && + head=$(git rev-parse HEAD) && + + format_command $type "update refs/heads/ref1" "$head" "$Z" >stdin && + format_command $type "update refs/heads/ref2" "$head" "$Z" >>stdin && + git update-ref $type --stdin --allow-partial expect && + git rev-parse refs/heads/ref1 >actual && + test_cmp expect actual && + git rev-parse refs/heads/ref2 >actual && + test_cmp expect actual + ) + ' + + test_expect_success "stdin $type allow-partial with invalid new_oid" ' + git init repo && + test_when_finished "rm -fr repo" && + ( + cd repo && + test_commit one && + old_head=$(git rev-parse HEAD) && + test_commit two && + head=$(git rev-parse HEAD) && + git update-ref refs/heads/ref1 $head && + git update-ref refs/heads/ref2 $head && + + format_command $type "update refs/heads/ref1" "$old_head" "$head" >stdin && + format_command $type "update refs/heads/ref2" "$(test_oid 001)" "$head" >>stdin && + git update-ref $type --stdin --allow-partial stdout && + echo $old_head >expect && + git rev-parse refs/heads/ref1 >actual && + test_cmp expect actual && + echo $head >expect && + git rev-parse refs/heads/ref2 >actual && + test_cmp expect actual && + test_grep -q "invalid new value provided" stdout + ) + ' + + test_expect_success "stdin $type allow-partial with non-commit new_oid" ' + git init repo && + test_when_finished "rm -fr repo" && + ( + cd repo && + test_commit one && + old_head=$(git rev-parse HEAD) && + test_commit two && + head=$(git rev-parse HEAD) && + head_tree=$(git rev-parse HEAD^{tree}) && + git update-ref refs/heads/ref1 $head && + git update-ref refs/heads/ref2 $head && + + format_command $type "update refs/heads/ref1" "$old_head" "$head" >stdin && + format_command $type "update refs/heads/ref2" "$head_tree" "$head" >>stdin && + git update-ref $type --stdin --allow-partial stdout && + echo $old_head >expect && + git rev-parse refs/heads/ref1 >actual && + test_cmp expect actual && + echo $head >expect && + git rev-parse refs/heads/ref2 >actual && + test_cmp expect actual && + test_grep -q "invalid new value provided" stdout + ) + ' + + test_expect_success "stdin $type allow-partial with non-existent ref" ' + git init repo && + test_when_finished "rm -fr repo" && + ( + cd repo && + test_commit one && + old_head=$(git rev-parse HEAD) && + test_commit two && + head=$(git rev-parse HEAD) && + git update-ref refs/heads/ref1 $head && + + format_command $type "update refs/heads/ref1" "$old_head" "$head" >stdin && + format_command $type "update refs/heads/ref2" "$old_head" "$head" >>stdin && + git update-ref $type --stdin --allow-partial stdout && + echo $old_head >expect && + git rev-parse refs/heads/ref1 >actual && + test_cmp expect actual && + test_must_fail git rev-parse refs/heads/ref2 && + test_grep -q "reference does not exist" stdout + ) + ' + + test_expect_success "stdin $type allow-partial with dangling symref" ' + git init repo && + test_when_finished "rm -fr repo" && + ( + cd repo && + test_commit one && + old_head=$(git rev-parse HEAD) && + test_commit two && + head=$(git rev-parse HEAD) && + git update-ref refs/heads/ref1 $head && + git symbolic-ref refs/heads/ref2 refs/heads/nonexistent && + + format_command $type "update refs/heads/ref1" "$old_head" "$head" >stdin && + format_command $type "update refs/heads/ref2" "$old_head" "$head" >>stdin && + git update-ref $type --no-deref --stdin --allow-partial stdout && + echo $old_head >expect && + git rev-parse refs/heads/ref1 >actual && + test_cmp expect actual && + echo $head >expect && + test_must_fail git rev-parse refs/heads/ref2 && + test_grep -q "reference does not exist" stdout + ) + ' + + test_expect_success "stdin $type allow-partial with regular ref as symref" ' + git init repo && + test_when_finished "rm -fr repo" && + ( + cd repo && + test_commit one && + old_head=$(git rev-parse HEAD) && + test_commit two && + head=$(git rev-parse HEAD) && + git update-ref refs/heads/ref1 $head && + git update-ref refs/heads/ref2 $head && + + format_command $type "update refs/heads/ref1" "$old_head" "$head" >stdin && + format_command $type "symref-update refs/heads/ref2" "$old_head" "ref" "refs/heads/nonexistent" >>stdin && + git update-ref $type --no-deref --stdin --allow-partial stdout && + echo $old_head >expect && + git rev-parse refs/heads/ref1 >actual && + test_cmp expect actual && + echo $head >expect && + echo $head >expect && + git rev-parse refs/heads/ref2 >actual && + test_cmp expect actual && + test_grep -q "expected symref but found regular ref" stdout + ) + ' + + test_expect_success "stdin $type allow-partial with invalid old_oid" ' + git init repo && + test_when_finished "rm -fr repo" && + ( + cd repo && + test_commit one && + old_head=$(git rev-parse HEAD) && + test_commit two && + head=$(git rev-parse HEAD) && + git update-ref refs/heads/ref1 $head && + git update-ref refs/heads/ref2 $head && + + format_command $type "update refs/heads/ref1" "$old_head" "$head" >stdin && + format_command $type "update refs/heads/ref2" "$old_head" "$Z" >>stdin && + git update-ref $type --stdin --allow-partial stdout && + echo $old_head >expect && + git rev-parse refs/heads/ref1 >actual && + test_cmp expect actual && + echo $head >expect && + git rev-parse refs/heads/ref2 >actual && + test_cmp expect actual && + test_grep -q "reference already exists" stdout + ) + ' + + test_expect_success "stdin $type allow-partial with incorrect old oid" ' + git init repo && + test_when_finished "rm -fr repo" && + ( + cd repo && + test_commit one && + old_head=$(git rev-parse HEAD) && + test_commit two && + head=$(git rev-parse HEAD) && + git update-ref refs/heads/ref1 $head && + git update-ref refs/heads/ref2 $head && + + format_command $type "update refs/heads/ref1" "$old_head" "$head" >stdin && + format_command $type "update refs/heads/ref2" "$head" "$old_head" >>stdin && + git update-ref $type --stdin --allow-partial stdout && + echo $old_head >expect && + git rev-parse refs/heads/ref1 >actual && + test_cmp expect actual && + echo $head >expect && + git rev-parse refs/heads/ref2 >actual && + test_cmp expect actual && + test_grep -q "incorrect old value provided" stdout + ) + ' + + test_expect_success "stdin $type allow-partial refname conflict" ' + git init repo && + test_when_finished "rm -fr repo" && + ( + cd repo && + test_commit one && + old_head=$(git rev-parse HEAD) && + test_commit two && + head=$(git rev-parse HEAD) && + git update-ref refs/heads/ref/foo $head && + + format_command $type "update refs/heads/ref/foo" "$old_head" "$head" >stdin && + format_command $type "update refs/heads/ref" "$old_head" "" >>stdin && + git update-ref $type --stdin --allow-partial stdout && + echo $old_head >expect && + git rev-parse refs/heads/ref/foo >actual && + test_cmp expect actual && + test_grep -q "refname conflict" stdout + ) + ' + + test_expect_success "stdin $type allow-partial refname conflict new ref" ' + git init repo && + test_when_finished "rm -fr repo" && + ( + cd repo && + test_commit one && + old_head=$(git rev-parse HEAD) && + test_commit two && + head=$(git rev-parse HEAD) && + git update-ref refs/heads/ref/foo $head && + + format_command $type "update refs/heads/foo" "$old_head" "" >stdin && + format_command $type "update refs/heads/ref" "$old_head" "" >>stdin && + git update-ref $type --stdin --allow-partial stdout && + echo $old_head >expect && + git rev-parse refs/heads/foo >actual && + test_cmp expect actual && + test_grep -q "refname conflict" stdout + ) + ' done test_expect_success 'update-ref should also create reflog for HEAD' '