From patchwork Tue Feb 25 09:29:04 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Karthik Nayak X-Patchwork-Id: 13989608 Received: from mail-ej1-f53.google.com (mail-ej1-f53.google.com [209.85.218.53]) (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 BE3AB25A2DB for ; Tue, 25 Feb 2025 09:29:30 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.218.53 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1740475772; cv=none; b=qYkPtJus50+laLdzaz+EanmdvpgaIilenA9P0AvK4t4fjxatHisCfgiwgJ69VUaNhRctrQGv4laeFpj9kMT8jWZuSX2j1pWdo5MLFqvwlhjACMWZd4q4q3zMcYeV76I7T3Zy1C6nYPnQdCvl1qyYgmskGoiLx3QA6JMrxzcRcdo= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1740475772; c=relaxed/simple; bh=zmtseKdgAQyOS7jHubzK3mQey7UR7niByDTIziqT9k4=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:References: In-Reply-To:To:Cc; b=pEiYEJWozPmC9iPBRwoVk+AtRJGCeyPbiPtSavR0d03kmr5/Faf7HH7OwYS6mKL/XDxG1W+tfVlMC8IcpjD8uyH7HIYGZNZByslfAcy7ZWXNwsHUZhx+KRUwe913McRa7p01JcrMHrerJpOtVrcRwVf9vici3eBYwibwNrWjun4= 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=iaxek4BZ; arc=none smtp.client-ip=209.85.218.53 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="iaxek4BZ" Received: by mail-ej1-f53.google.com with SMTP id a640c23a62f3a-abb8045c3f3so660572766b.2 for ; Tue, 25 Feb 2025 01:29:30 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1740475769; x=1741080569; 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=b7qH/u1enQSenIflRJo/e9Lu4z7mOyPHg8s9g1x3r6c=; b=iaxek4BZsFBLokcDdqIolC6nnTyH5NUkhAyEk5T5DD1YHYqAD5jfAtM/5sS3eGwPmx MS28Fdw3o7BHzpcnrR+MdV/oNYKehqslyQIKSb/doDiKA2hMNyVEJhw15+BQJn/Mia2W MUmU8xDvxaNDDxYoV8uB98su26jwVRN8ITCmT4LBQJziJXxZuSGoWm/FrH3HtagWjqeN RKosPgkbnopyO0Wr07Z4OwHRTtmXqvPg9ABU/K8zfPQqubMyEyOAkubTv9uw2S2yo0kO AhrKxgWwRjJ+aX/L2it+P1uDxazDldXj+VfHcy75cZj/DI0WUHnR4uhq++PJikiusbca S+Gw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1740475769; x=1741080569; 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=b7qH/u1enQSenIflRJo/e9Lu4z7mOyPHg8s9g1x3r6c=; b=IdYt/Qq2oqZxMKjsEwAQyShYOIZsuwEBuAabgqQVYBjXznI1LZydzBHxYWLk+scTqP 6jerOSo0RGjTiDFVGDm0dV8c0ZmoweV7ZBVKOjiUjfaHnJk9ro9TxPcfk3lxmmMvgk+N NRZoG9Oj0jMg7/65HFH9yLLDyKVR20cj9vdPIbWS8+tBXO8MZm+1ft92gvgTBNxqyJcD ZH4ltTOoz8cMh/vXu0aaa7ygJggDwj29kcuj6Lhwm7TTZmNFI0GBrS9+eceRKZliIPFq eGjo7CmafLzenS3xgqqMETB7lrAwzpDhgi5fLE1DiHwCN0yLdnfuI+IagID9WHi2F28V xXlg== X-Gm-Message-State: AOJu0YyDyquHbi6xssG++hZn8gNFQhrT7+dbvAC7zH3XLxYog4sY2lJW ghgLbTSXrEBiPJJCQS47z3p46D7xW2n3LdLWVCLBAghEVd704SIsK0sdByK5 X-Gm-Gg: ASbGnctROt1CNdJnRkifPbxLKa+8YVEiPE7QRR0ysM8gJWyw1QZEt8JFgRRy/OQeGWU 5mLLwJUB/lmV//ygL8H7bhDD2GRwylzIkNtl6TUGASv7FWnOI9PbQFgu99A3jEsjTQDnJUZb2DL MiaqgW84EUN7JeduU1cxJMWFscGR6F6/3rMFRf+QRvMMgdvkUHyDOtfy/g96/LVXEdEZBb7f7pl 0JkI4rhLy3p8Yj5NAanZKDYqUsKIYGYR/rrYQZLEKbgOrBgcmsaWQuofCz0fNCDBUEwhETkv2cw N0+OtuTSD+AtYHJxJsPz3PZOR1yMk9MQ X-Google-Smtp-Source: AGHT+IGybvhe1hLMvOUJOvfyPCjGct4eg07RpaEGMghrxZeozK6/sGEyF7xsbUYKARayemV/ZRkcYQ== X-Received: by 2002:a17:907:da9:b0:ab6:f4e7:52f9 with SMTP id a640c23a62f3a-abc09a8a264mr1510511466b.25.1740475768723; Tue, 25 Feb 2025 01:29:28 -0800 (PST) Received: from [127.0.0.2] ([2a02:2455:8268:bc00:20c2:4ab6:a193:5b8c]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-abed1cd561esm111944466b.19.2025.02.25.01.29.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 25 Feb 2025 01:29:28 -0800 (PST) From: Karthik Nayak Date: Tue, 25 Feb 2025 10:29:04 +0100 Subject: [PATCH v2 1/7] 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: <20250225-245-partially-atomic-ref-updates-v2-1-cfa3236895d7@gmail.com> References: <20250225-245-partially-atomic-ref-updates-v2-0-cfa3236895d7@gmail.com> In-Reply-To: <20250225-245-partially-atomic-ref-updates-v2-0-cfa3236895d7@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=zmtseKdgAQyOS7jHubzK3mQey7UR7niByDTIziqT9k4=; b=owJ4nAHtARL+kA0DAAoBPtWfJI5GjH8ByyZiAGe9jXOe9fLC9sxjm7ORRpb0PWK8spFX90CZE dtzbMCE+g/DP4kBswQAAQoAHRYhBFfOTH9jdXEPy2XGBj7VnySORox/BQJnvY1zAAoJED7VnySO Rox/3nYL/jARqVVmxZtgtWQERsFKGWnDue4CqWC1KzWuFap0vnyuSFpma25Qpm9FZSQMgJ4q2Cl qjq0lNf+YMGUu+oIEyZoN2pHGSMzx1IL3/EnIWhTu5rHX05a1tlVOOHh1rqpqaBnbWUhkIAo5pb At+oS7YDPZlz5vHWWu/sejiwSgJ27gxW8c5B3l2y7AVs+iyqVkqspAj3owT616Uy3/2lFwa39Xh GzSe/EaCw5OScueUNLiNrVMmAUmfqZGmfQFKcwxfztvDAOfVeWYtlqIw/gt/+672TiPnLl4UuZc bVbE08PneI5FPTxezNkhMZVhIMMKeVEZWbv9FWps/iC3MdIwWKKNf4AKgcXo6YgoN5OqM2P9BIv 081wfB1i1BcN7FXoSuZ52+6qTtzyl9TH3iHQoZ1ZSe6HzC8i6adcuTPV/qHW++y5twl0QLGSQIg /XTOm8qh0LDm5KDiR68N7AsnhLJ1FO2HRqd1nT1BjNocL7BjrntAgeevcKvqABzAFvNkJOq3qi6 Zo= 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 Tue Feb 25 09:29:05 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Karthik Nayak X-Patchwork-Id: 13989611 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 B228125C6EA for ; Tue, 25 Feb 2025 09:29:31 +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=1740475774; cv=none; b=mD2lUU/n10b50rt2nhbddXgbzaB5H2HJD7cSX/PbY+Vitc74hA7AwT4cqvN3r0mf/o3Kt2u2Pq+rqDZA8581Bh0l1PZ75b6/FT4EYSkQiyMBJn4kZs2AUh0PqBJQYGSVj46rU+K/Rx1umUxQySGzZP5Qu47F0lzZ7/Eyx7XDR4A= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1740475774; c=relaxed/simple; bh=jRrrZzrNeSZhCB1rTV+7qqF+po8Gli0leqwAsXygK50=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:References: In-Reply-To:To:Cc; b=VNuE9hUAOud7fXcWZ+TL8B4s0oZ6ByBoHs2S99RnBas8nAMiUr2kHqk+7b4mvXoYp3IrtCoBrSUNXjnnqsCQQGkWZkTlpNP8uNj4SGJ/WHvk6hcdCfrFToDYQIORiSRP5h4HT7i4EOBcKnsqkTd54Ic+GbToKu+/pyBILzM/GRY= 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=Qvtjgzj4; 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="Qvtjgzj4" Received: by mail-ej1-f51.google.com with SMTP id a640c23a62f3a-abb90c20baeso689943366b.1 for ; Tue, 25 Feb 2025 01:29:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1740475770; x=1741080570; 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=MvA6MyXF5/zToRIAf13uF8lOQGWyvrx7I6nc/YLJjDA=; b=Qvtjgzj4GerWEUV4RggZGHviGQWsWa1fClih4UiRYlFbunKMu8swNIHOOykzNruvIv D+/RR3kz2nljA5GI8NWbEPx8m0CA6j+7KQH4RuOvVvntBKU5DV38zhg2n9HjsZXqJae/ bAvdzFlzYnDvQFLEx6A2AviDOL0FHZBuYU0S2umF3wjyViAti857VP5DPkXewZIE/VEe cE5YyJOPBoool3x5qh8abV8rqswearI3N3TuLI1bvPa2n+jfVPqW3oXZJ9S3B+bi1D5o gKKvFdcPbFbmD6Ju6IByChkDbw32jBwTMb4Vmf9ytQMU8XlhGj7we/6o/ceauLZrkwK+ ASjA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1740475770; x=1741080570; 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=MvA6MyXF5/zToRIAf13uF8lOQGWyvrx7I6nc/YLJjDA=; b=gLb3Ugu6pVrbKVNSiH+MjmWzv4KX4vPnjvS/HKWkDr+fPfAy+0O+37zHBSP9jnn6Ua OmvsWFWbG9aMv3CWxzp7//U1PkT5zW0kYDWXOmR2FIasorZCXwfSQsr6l3DvIXs+GSnW 62vRQAIwFIBP2c0l5hYoEbJf3rcORl6djA5zmvgpl8b5QdGGKDjZHxMW1O57GAcBuwiE PwHpDs+RzrvRBxSWgwHKaJ81q1Z210QIlkGlz2JVBQWd17fQGTJff3RsfUpjvn4RC17r eU9Gdz6Uy1ikQHTFY7oFXXuj8Ib8rFzzfPm71WerLWp2vI7+x2b15166EAa40/+2DWTi JpVg== X-Gm-Message-State: AOJu0YwOSDkxDl8nVsX24qrz44Odulfg16sO5B534Z1dm/jWVx2/HyFg qOQr05/oQAQmMhkXH4LVdT7gGkpUVcsSOlpv+/LyPNxnXT3UXZr9 X-Gm-Gg: ASbGncuDb50vK6EtI9mbiaG0jQfTQKVUba7pNfH0BGUXDjvSH5MDkwLrbn15PdQmVNP Q5i2+wbhepqgp2bLqW2V8+0cIzPKUjUxvpwBjUgGHDCTiK3ABcoEKZ2ptvHaCc9KBorhC0HkbEV YvhDq4GP9uChuZH8E3Aen5Sv/rCQpG9NTbVgmYcnSKOQA4RzYq8QpUQqRAc/wf3h6kqsriL+/GL 6qh+F4BqnLBBJ2NOjPf64CqvvdrRWquAGJJ7DtoIcLtQA1vR+jOUTohfZAdtXAfmAEp5JJErkt+ B2fUoSegGbi+C6Gk3WiQNxULZZ1pUh9W X-Google-Smtp-Source: AGHT+IFiMOcZKS96cbzKts/sUywJiF1Gl7Lxq7fOAnwwcIxxNixEcnDtqigGZPpKXu+fXDIeRUQ/IA== X-Received: by 2002:a17:907:7f26:b0:aba:5f40:7f2e with SMTP id a640c23a62f3a-abc0de0de78mr1827534466b.45.1740475769551; Tue, 25 Feb 2025 01:29:29 -0800 (PST) Received: from [127.0.0.2] ([2a02:2455:8268:bc00:20c2:4ab6:a193:5b8c]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-abed1cd561esm111944466b.19.2025.02.25.01.29.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 25 Feb 2025 01:29:29 -0800 (PST) From: Karthik Nayak Date: Tue, 25 Feb 2025 10:29:05 +0100 Subject: [PATCH v2 2/7] 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: <20250225-245-partially-atomic-ref-updates-v2-2-cfa3236895d7@gmail.com> References: <20250225-245-partially-atomic-ref-updates-v2-0-cfa3236895d7@gmail.com> In-Reply-To: <20250225-245-partially-atomic-ref-updates-v2-0-cfa3236895d7@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=jRrrZzrNeSZhCB1rTV+7qqF+po8Gli0leqwAsXygK50=; b=owJ4nAHtARL+kA0DAAoBPtWfJI5GjH8ByyZiAGe9jXRiUX1c9zoynpuc2s00aqT7EzjZwLpmS hADIFZJkX1HjIkBswQAAQoAHRYhBFfOTH9jdXEPy2XGBj7VnySORox/BQJnvY10AAoJED7VnySO Rox/+DkL/i645/SDRoKDETuMvx/1RV/5/H/rIxtBbGbTNhJHel150C/Cb7EjKyeCODid7KGveRP 7rkgc9Qz7AGsOgSqrm3FT9Nejcz6dBWPiGoc+L+bs1FCe0e+Nnpyx9M0OwUsreUymzYugHBqEj6 V4UfYYlNndQY5Iw3VsuVhTpH1xscsY3GLbVZY9ShgPgttLg4u0O+nOOVtDWNYUAxeX5pr/tfzu3 ontVmNTshvEchAtmeonq6jBZWw5oKwmM8nbGhjpUCM0JSU3ZJbMtSDdvY7IttPXnIi5m3GAlwN2 bHTnZ8jKfqDvQT7uAnxppaj5gSohPk4b5hnN9zxd4PC6yIYu6GnFbK4CZMutLLwPuugYsEUY7/U CXRn2ZtI2H7zXnONtm9HE3A/YAyuw4330bNCi7ge7koctmuI2z7yEuVRwL3UjFcYtYpCAKzI5yJ wXHTcPReJwaT8cei1M53PyLTk0x/KtYUQXRX9fhviLfKXl67dEJE+vsqdZ4Qczr6aMDjV47r3a6 aY= 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 71a38acfed..3247871574 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1619,8 +1619,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, @@ -1629,8 +1627,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); @@ -1655,7 +1651,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; /* @@ -1668,34 +1663,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 cadd9808fd..a6e05eaecd 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 Tue Feb 25 09:29:06 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Karthik Nayak X-Patchwork-Id: 13989610 Received: from mail-ej1-f47.google.com (mail-ej1-f47.google.com [209.85.218.47]) (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 6537C1FE465 for ; Tue, 25 Feb 2025 09:29:32 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.218.47 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1740475774; cv=none; b=Wzk5Hi317G3q3Rte7+mg1jcFNNNw9Vq3Bv3AVTeBLDdbFkpd0rBqNmE7OZmWZ5pUR9FBojonC4XBEpTKkzj/t4Nq1oBwpQL3VAlh8HiYLxxXD38+tjqYYhuQKtY4wiWMQ2kFZ3p5v6biCjtisCxtOXGqiu/YeUAncBhhtEVkZbs= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1740475774; c=relaxed/simple; bh=wEYKpiFzoNr39cfnesRh3C7AD4gVkEqAKi5Qllu1OfU=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:References: In-Reply-To:To:Cc; b=iyH+Z5QoteZUWzg9MZKx1CRB2owG5fVrVMNpkhVmIE4F7ZVhQgq193bje2OHYUrsxfL72cjM8KIINSDtLx/7sK0KO2D4NdikE111Et+ICHuaPzPlVsnc8HayI6X4JBUphez6Tm1hrf+GSphPsi5G8ujX8b/jRXPfGOpCIaf9PaY= 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=iHaBIRrA; arc=none smtp.client-ip=209.85.218.47 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="iHaBIRrA" Received: by mail-ej1-f47.google.com with SMTP id a640c23a62f3a-abbd96bef64so871597866b.3 for ; Tue, 25 Feb 2025 01:29:32 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1740475771; x=1741080571; 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=QfSLxZydrSfqW1xMvsv0SqvzxvhQtLB2KzyBUKCQxrI=; b=iHaBIRrA1YhbLY7F2q7bYqtQ9zJaBKcpahsFwUKVJ5em3VhLDIklCADFQZapMa90yu NrWXKzngyOHXqe8bu8ihyYWH0YdLTMPkRmYG1k6uWKrdpHft/eoIJ5+LwJWs6as6rip7 MU13iRdGdbxQnXPS7tKLEQJMQVAWGGrvWEXKi9zUoo5gU7uNwNwgI4byIbF+HyBw/+hh NxIo/6gJEhNZqEU+VCZ+HvC1FNwjWHCLqnBh/JkwVFbmmW+vlhzPM3J84auORycHmQyz rDXw6UbSBq0NFe01VUivyqzlSteSE9bbQCp50Ct56Lo2Qy97NucIU6IsABNGyDA3jAZe GUDA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1740475771; x=1741080571; 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=QfSLxZydrSfqW1xMvsv0SqvzxvhQtLB2KzyBUKCQxrI=; b=OOuS8SayPJlAM+UOayRcx8bozW1n7Wb6nlzTkdGiKroyXaT1EhtRpP1PzsrQgi5VAD a7Yu5KdKm1Kswrtw1AzhSK1XKlaw/c5CXREo37KIFQ7BXANGg/R3RatEVaT4lvZi4PPl 8j1jhtMejvl8ht3VWl8Ern/Na11ae6ObBCqUIGpseUhJmU200caYWc3zJHR9kPT3X97z FUPBGReXojyGnlj5dvMXJTOLbuktXfCKxd6j3diDSiGylbj43PLg2zuZS7ojvTZyQJ6t kD7I+iJyXvDeSyyUupX+Az0NFGQxGM1I/NoM8ms/LEFvU/SrvN7yBRw+O1e13NkObsYx XfMw== X-Gm-Message-State: AOJu0YyG2J4vMjjOtB291MN3VCF4UM6Ite1c3V9d2UDS45oUwrmZk+yN GPH7NXb3osfE8iWJv8GurMPoCaaDnJ17tvRxIPrJaQ9ZcLqvT+TE X-Gm-Gg: ASbGncsJfAQkkIkTG0+p0BVHN++OCb9QihrKe6cDnA6dfN5uh92VywCFJ7L4RR1E+lc UGO/+n20CpdNAevlm44fM6/9csP4Lc3vzL52Gv4V0rctACeAjPnRD95VxwCWAAvRfztCcGMTRS3 QN5pWy/aMeHChNt5XYPrJCofsJKVPI5Gp/5bvMfMMATB360slVd95UXeQyVVlj9YidlWQ5dqI5K wqGX6CcV/HU7noezV5KqjADQp5GbUNO8Lfmt/+yPKZlfypTgkQzHcyCO2YSeObwmwiecNE9z2NS dnUsKy32kl01bqrClaudTDHM3vqeDKAS X-Google-Smtp-Source: AGHT+IFEabihop/mZ53SKaoiVL11c+8QLzNKF6KyaBjbGdN1uQyfAzC/Q+LGqYVR5/vB2RhamrsSpQ== X-Received: by 2002:a17:906:3290:b0:ab7:b30:42ed with SMTP id a640c23a62f3a-abed0959f38mr220924466b.0.1740475770368; Tue, 25 Feb 2025 01:29:30 -0800 (PST) Received: from [127.0.0.2] ([2a02:2455:8268:bc00:20c2:4ab6:a193:5b8c]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-abed1cd561esm111944466b.19.2025.02.25.01.29.29 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 25 Feb 2025 01:29:30 -0800 (PST) From: Karthik Nayak Date: Tue, 25 Feb 2025 10:29:06 +0100 Subject: [PATCH v2 3/7] 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: <20250225-245-partially-atomic-ref-updates-v2-3-cfa3236895d7@gmail.com> References: <20250225-245-partially-atomic-ref-updates-v2-0-cfa3236895d7@gmail.com> In-Reply-To: <20250225-245-partially-atomic-ref-updates-v2-0-cfa3236895d7@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=wEYKpiFzoNr39cfnesRh3C7AD4gVkEqAKi5Qllu1OfU=; b=owJ4nAHtARL+kA0DAAoBPtWfJI5GjH8ByyZiAGe9jXSUZQWqfDPJVWV01p3OeTtRRwQ1sboEz TQZrvc6BHBPqYkBswQAAQoAHRYhBFfOTH9jdXEPy2XGBj7VnySORox/BQJnvY10AAoJED7VnySO Rox/FBIL/3z9fMR8EyUZ7ZGzXJHfWuLBLU/JheB6iLXpexNR3hVPdf3tF5jZA/lo811aPMdOm6h svj8fgh2b5Nu+J6aK4nCoz7TMfV+Me1GDpKrewTg4Xqn8adwKbNbLRVe+vbu/QaqkSFiRQJlj9x EKNJryGk6WjAaZGwZsf4Pfu1CjOOH/OycLNJZi4+gXC6EyB9LT15qZiO9kNCnsr8iRCQ4ESv7ut F70K9Fty6wqEQj5ULok/KxDlq/DcaFhZgu//0JT0+wNTBIL2LeR75OzNW7V28n2J+X6UGqWjC8u wsOUOrB8/qJUO+lvf1vxvbCVxVK9Z3NSZ+7uEDKKgcbGl1MnLYLjFB5jH730nUneAJMNrH54ICw 42PjoGVqb93qHgBdGoHKLC4ldvN3IJ5dZDXnJRcr26rf5s9IkrPQ939s40Av3Q2FTgHZtf8rHqm TJDiBK9ycYhanx97uypUki8gWWrct0RIwxtCSO8hObNy4hbbgTPO26fZAeffsGWQJiIlnCXLYr7 PI= 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 a6e05eaecd..4b7fc8f1ab 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 Tue Feb 25 09:29:07 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Karthik Nayak X-Patchwork-Id: 13989612 Received: from mail-ej1-f46.google.com (mail-ej1-f46.google.com [209.85.218.46]) (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 8D58C260A59 for ; Tue, 25 Feb 2025 09:29:33 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.218.46 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1740475775; cv=none; b=iY5IgLc1BfrUfi4GHzTJbdSzWboRWvBpoUtiG/sB9NAB1N7Pg4vNA/iiGIbhCWOujc+fCzQ6tFsaHx3RscXwdg4dVCiYqucy7mIsYii4QvgD1QsMOaHn31GfXjk7Wk/6dh5QWIa96GPiYvUEDtZMzfYoS/Mw5D9+nUAKe2zIbP0= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1740475775; c=relaxed/simple; bh=DN70+vYSDtjptifCa7oXfyKza6QGPF8ufmZBzATYN8U=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:References: In-Reply-To:To:Cc; b=uU2KQOp6W4HOVUopkurAgV05rLsmj/yKOCDWY3D+55QCSWzUydC6wgdDXKkTeIpfu5rIv05ITZ2tA2i9RQQ1Bv/tAEcJZ6DkvKaW9JDJgEUqAFMWjR1ctdY/aFxD/pl2c7TPTPAl/As5jGrMfJfzwAf6VMVo/eAZN91fGgO04cg= 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=d5KjLCXv; arc=none smtp.client-ip=209.85.218.46 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="d5KjLCXv" Received: by mail-ej1-f46.google.com with SMTP id a640c23a62f3a-aaf900cc7fbso833528966b.3 for ; Tue, 25 Feb 2025 01:29:33 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1740475772; x=1741080572; 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=GoP6RitNuKick9A8qDS3ASkRk+rVaR8PtrVuE6dpStQ=; b=d5KjLCXvE8HbHsS9W7LGqQQyD6QIXNL8cNqS+zZxI115cakweTLSFF6WlR8snbs1sn +b8B5vOifDQKClpSfnpwcCOqXJzhsA4JxQob/OwKvniv/i6RIlQRybQuPxai/bihQMxI 8QBsD8HHcImXrMSCBcWECXXUSxjD8RQAfRghkO4dvGf0q/bU7spDWAuT1cJMFo01UlVS hqPw7wH4Cx/OUSR8Mf0psfUSvLiSq9qtS32Jia+KwGBf0sabaBghvqu9ljROsOT4s/x0 uasFz/nD3pZ+izuKmDUcrEUQmxsfFglpcW3Cx3+rJGf3ByzY7ISVFvvd4OVwGzzShqfX qkcQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1740475772; x=1741080572; 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=GoP6RitNuKick9A8qDS3ASkRk+rVaR8PtrVuE6dpStQ=; b=Bmj9hn1t4W7bVlZOFrljeQRPefc4Q/n6LdxXIQEVRsqqMF//mZ6A+3PffvKqO49hb5 2mFH0Ap7DgZBuNuHwKKhaTrMcIZvOORQuyQr30XgYqroJqAjN79WHelDY7FU6Nk4Xy5a LDVgcuTMb/wYR9KF7eBXF8MRo3zLj4uQ8T+mGyUFibd0Op4F+tjWlz3/r8i5CGmQ2LkZ ihchBGc8hsXkiXeppd19z7aS1J4kRNijvR20p0ZwfdMI3EdQA/yV52g2SqR5qMXvKEzC EsPaph/VppwKvheATea0kVQA7vLfeoOSND8926QSZK4pY2QPRenrCZ9Nu1OpvTSUr6cs 3OXw== X-Gm-Message-State: AOJu0YwkdVLlNNpWL5vdoFpZfRMsB3S2FMOBAF8dpE/Nu3niM42cWvyn y4XKVTbavrtbzWnkrFUpWirMFAlGGL6WSde3/gbKduGFTvSDdi/M X-Gm-Gg: ASbGnctKVhqPQIiCdBdn65/SGgV9IHP0ArNGy7Gi8pw4yBR5z/Pi+pqFSO6zgTtORFP ASj1Grw1BbuR1+IK4c7Cgl0I6aFhL/0IJpSKnIKPXt7HYEbp/UbcuNxFu9ZemIUH1lc4rTYBnbx 4hMgLg74//cccd2Yl4hWqoJJ+ZrYo0vhuEm5IcAJc0OYFCp17GeqOFLPfV7KUoZuE76FhOawHxj 6O5YSXr/QI2SQXqchOg1UZ5uxz+iZSSlPB8FFjzItaqhihNzIyhFIBGHeAaKjZ97sDyjEnZCPut UCL5KWH+xsx5EPFTpdcO8weVtPeEyHl1 X-Google-Smtp-Source: AGHT+IFRT9SIjtFNhRV1gPOf6odNM+mn39s4k2IBfd3lSYFfvIA8jEWnHrsP+BzfTI4Nal+Ix8uSTg== X-Received: by 2002:a17:906:3152:b0:ab7:d87f:665b with SMTP id a640c23a62f3a-abc0de4a2cemr1487877866b.48.1740475771423; Tue, 25 Feb 2025 01:29:31 -0800 (PST) Received: from [127.0.0.2] ([2a02:2455:8268:bc00:20c2:4ab6:a193:5b8c]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-abed1cd561esm111944466b.19.2025.02.25.01.29.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 25 Feb 2025 01:29:30 -0800 (PST) From: Karthik Nayak Date: Tue, 25 Feb 2025 10:29:07 +0100 Subject: [PATCH v2 4/7] 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: <20250225-245-partially-atomic-ref-updates-v2-4-cfa3236895d7@gmail.com> References: <20250225-245-partially-atomic-ref-updates-v2-0-cfa3236895d7@gmail.com> In-Reply-To: <20250225-245-partially-atomic-ref-updates-v2-0-cfa3236895d7@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=DN70+vYSDtjptifCa7oXfyKza6QGPF8ufmZBzATYN8U=; b=owJ4nAHtARL+kA0DAAoBPtWfJI5GjH8ByyZiAGe9jXUdMkoQVh8V757ahCS1bIm9NA1P1r9Hn PvkrN3qJMblbYkBswQAAQoAHRYhBFfOTH9jdXEPy2XGBj7VnySORox/BQJnvY11AAoJED7VnySO Rox/eJUL/jV8Bi0E7VS/zkoM8Vv5+Vej/aAzA7Nz0sIFOVhYtNe/sARMgtnxNuPRaw0vB6gqrMR yuBuvOWY0vZwsWCnn5c2DxfMYsID1opIUXFxQGXrI+5hy7T5Z8fKXKa3aaEqLFVPeT7rxghkZoB CHiKW8zGN9xVuBNtHqJxSvvCNoJ0dZskJF5i/Z7c4sYYufeUl+dkvjJ5p/JPxc9/YlmXlmKDBmF P6s++UTMnLfYmu5WiC+1Wb/hxEfilCm0rUrNyByCoux5/hjjFDkpmxDrTsZjKSVpH4LJ9YevpJD /8uuiGAtQTIARWHpCCDU9OvruhXE9Ew5l9v6VidJ/YSHNXPccYfvmEGXkCyy3lYSZkFL6mb6GPb obeaFV0ST4zmQ8mrHvEwym/lqlKy2dc/4aScZy+r9GYfewGasKJtH4i5020FdsxydPtdWywLR6L yPl/faNbJeYs/bRb9dirw92ByD6LFPJiNftWA1X4XqqW1IJOWHC9IhhOSkySmDH5NBNV5P+6RZ7 08= 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 Tue Feb 25 09:29:08 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Karthik Nayak X-Patchwork-Id: 13989614 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 5E73726159E for ; Tue, 25 Feb 2025 09:29:34 +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=1740475777; cv=none; b=JZzdEDkD0B3VWZuS9F1ANcGFvvD6ZCBHRwklXRUTH2XIFK/pQB4S/s7u5+s53X+/eKBqnn+nwv9hAndKBrg4tajp3HhVPUVJKnR+P94VbDlb5dpzvTmCKvXgpndN70GdR50wSWrSDAo7gs3lrkzJa/23KpqpU0n/uSoFKfzIaZA= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1740475777; c=relaxed/simple; bh=5oYAAacciL00PIoOvUM6oBpdxVgGTcMiKpW//Vw32N8=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:References: In-Reply-To:To:Cc; b=U7lC2SBgA+cBQiyMErvQrV8h4RWLKWyb37yNQqAD0QUIvspZjNfk1w/wyqy/YJnmjbVPeTizHlIj6WBSl5VXBxRivN32LC3Bw5/fgZtfULHKBwzOFzYxFnhU+7BuodswxwE8hK3cypeeGhLbmcjFIDnnwa5J3IhswdYJNXleebI= 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=TSG2EoX1; 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="TSG2EoX1" Received: by mail-ej1-f48.google.com with SMTP id a640c23a62f3a-abb7520028bso714770866b.3 for ; Tue, 25 Feb 2025 01:29:34 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1740475772; x=1741080572; 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=9/caIaIm7HXVyxI7p0shD7HAWJDb9UKbjiK2XbAz+RU=; b=TSG2EoX1dL0A8LVENBB4SvJUgdAdcocuXmlt0dWNkX1Of2vAYwKKLFfT+AvKLOCH2v VGIFCk6cTJwFet40Rhx/UOhEcJG9vucX7fk2iBxrMxJMkqMf4Ui5fVZI26OfaW1KVvDI h2jM21xLHhke7T5BTaSr0zgVwIWVqOsN302FTfffgNWI8oD6nd4Vkzz6F+c/iV2cHacF YjXZuzz4KsdQG2PETt8A3p7pTPGqTovqh+mNmDwBc+P+NzlUc5VZpdNZ5MNRetcBOfn1 YxTPU5hcLkMlrRMHbcQTloGe+hWTvn6U4vTbzOj+EJCSWsUUtwQdWoRU6wykMYNmAHO4 vDqQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1740475772; x=1741080572; 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=9/caIaIm7HXVyxI7p0shD7HAWJDb9UKbjiK2XbAz+RU=; b=n7TQNwg9I4kSfpWKET5pPfeeukN4MQwxnZ+YanLio7atOqXF2A9AKQ4BcvqwRLMHek XUQLaA2T8YqLh8DkbNk+1fHODNyCjIuyD3UXVUTefBcDayO41RuLLAQUihfwO0YXuLeL BKIXO4MvLoqituCYDZ4mluonRNnNjxztjNr6wg+gD6LcCMV+Gz6e/TD8sQsZC/2toKZS C++h0KYSmA6iyFRyogORMv3KjFb6zllnh1F7nVEBBNuP6Y51JLoxNLgAmiFLRfM62llk jLDXkwzIpSPHjG52sY3slbYao96GIwfznDU6Ko7zRoKvZwCkgGb4E+RK8BJCjJvWPGAK A4Eg== X-Gm-Message-State: AOJu0YyPv7be3dKTpQDVuHVpdTGv9kuRc9yODQgouAgrjXgkoTzKUvI3 vr+VaSgzI5QpmjCCpPT7wkJCXqOjfNEI0IO2KxIJszmi4qIddcPamDHmj1K/ X-Gm-Gg: ASbGncsLRy3cpx37WEWdFqMS00IthpWQyrFrsK0tlFxkAQYIMI4s+UU4yII9mofKoM2 bnm6TWCRGshESFfoPApsYOiTEtbhcmVZBYnR9EBN+56wOEB12+im6gS70tzDgdV2uxtuWv/OPrB h88wdjVimVVO0tiCup8n3bKXR7aeBsSl1lxPEhfBOBZBghXakrQ6ZHQA7vJH6V77HgSOwMscLnI 9y26fT/mVopNG0LP68BrrkCu/zPs5sCSQPrCnShDtKa/w4aU2BWnistKlQi55sUL7R04/aCuB8L KBOU8oGWpq+W/ERPH5fOGfPTfQf+1Htk X-Google-Smtp-Source: AGHT+IHAWD/djqzDiN4TmjtFrscLWObcU/uPaBj63zkJuGNZ9LXej5nxl1OTo1Bjgaybun+G/IIVPQ== X-Received: by 2002:a17:907:2dab:b0:ab7:e12a:4846 with SMTP id a640c23a62f3a-abc09a5ee8dmr1706353566b.19.1740475772247; Tue, 25 Feb 2025 01:29:32 -0800 (PST) Received: from [127.0.0.2] ([2a02:2455:8268:bc00:20c2:4ab6:a193:5b8c]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-abed1cd561esm111944466b.19.2025.02.25.01.29.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 25 Feb 2025 01:29:31 -0800 (PST) From: Karthik Nayak Date: Tue, 25 Feb 2025 10:29:08 +0100 Subject: [PATCH v2 5/7] 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: <20250225-245-partially-atomic-ref-updates-v2-5-cfa3236895d7@gmail.com> References: <20250225-245-partially-atomic-ref-updates-v2-0-cfa3236895d7@gmail.com> In-Reply-To: <20250225-245-partially-atomic-ref-updates-v2-0-cfa3236895d7@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=28592; i=karthik.188@gmail.com; h=from:subject:message-id; bh=5oYAAacciL00PIoOvUM6oBpdxVgGTcMiKpW//Vw32N8=; b=owJ4nAHtARL+kA0DAAoBPtWfJI5GjH8ByyZiAGe9jXV7UQEwRWn/LYgxYLTw/TLFujd/yHMTz 6jk7C/HlNYai4kBswQAAQoAHRYhBFfOTH9jdXEPy2XGBj7VnySORox/BQJnvY11AAoJED7VnySO Rox/BtwL/0HoQ7OheHRJX4rsziowxmra9o3FMe6s5Od4rgg2yDpT0DI6WGIhen2e3faM4nzBBR2 GSL4O3aHlVBUaCrTJ5D1mdWh3hOJcXO8LFDFxtQznGIXhuunI/GADCdov/21XgFf8HMMxZIfuLA DsYhziszzsM3nmlixPgm3NdZqSHFXNVrNMwOoEYXdpPuH9Mxg2S508zdgUdq1fdDpkhe9gpem2B 4bgj5LOkovKf7LyjtBxwCzJNO+/eUZ1SDREA1oQe3yx2NFklzA0Twg7N6/4puYkWTcj+F6LRGW9 OGVeOU0rAv7WavZ4ahu1xrCwpQN5ZRMFFvrI+TiAB6OAwxZO8l8PunIq4LtXOqAKNoRTeGEAbYU vB1EiVFJELk/ImEPe/LZSu0DdKgwfGPFziAnHoapQyMItL//VBCJ/CRMsreB4BNKT5Rp+hdyE0y tlPd+vbwlX6rqaTzwhykMSXjbsk0IQsNEfOQuRba7NeXyaGyNjSTSzaOPhNIIj7rz1eYTkYKHj4 0g= X-Developer-Key: i=karthik.188@gmail.com; a=openpgp; fpr=57CE4C7F6375710FCB65C6063ED59F248E468C7F Replace preprocessor-defined transaction errors with a strongly-typed enum `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 --- refs.c | 44 +++++++------ refs.h | 56 ++++++++++------ refs/files-backend.c | 167 ++++++++++++++++++++++++------------------------ refs/packed-backend.c | 21 +++--- refs/refs-internal.h | 5 +- refs/reftable-backend.c | 59 +++++++++-------- 6 files changed, 191 insertions(+), 161 deletions(-) diff --git a/refs.c b/refs.c index 69f385f344..f989a46a5a 100644 --- a/refs.c +++ b/refs.c @@ -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 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 = TRANSACTION_NAME_CONFLICT; /* * For the sake of comments in this function, suppose that @@ -2624,12 +2624,12 @@ 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 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,26 +2817,28 @@ 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 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"); if (!strcmp(referent, update->old_target)) - return 0; + return TRANSACTION_OK; - 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 TRANSACTION_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 TRANSACTION_INCORRECT_OLD_VALUE; } struct migration_data { diff --git a/refs.h b/refs.h index b14ba1f9ff..8e9ead174c 100644 --- a/refs.h +++ b/refs.h @@ -16,6 +16,31 @@ 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 transaction_error represents the following return codes: + * TRANSACTION_OK: success code. + * TRANSACTION_GENERIC_ERROR error_code: default error code. + * TRANSACTION_NAME_CONFLICT error_code: ref name conflict like A vs A/B. + * TRANSACTION_CREATE_EXISTS error_code: ref to be created already exists. + * TRANSACTION_NONEXISTENT_REF error_code: ref expected but doesn't exist. + * TRANSACTION_INCORRECT_OLD_VALUE error_code: provided old_oid or old_target of + * reference doesn't match actual. + * TRANSACTION_INVALID_NEW_VALUE error_code: provided new_oid or new_target is + * invalid. + * TRANSACTION_EXPECTED_SYMREF error_code: expected ref to be symref, but is a + * regular ref. + */ +enum transaction_error { + TRANSACTION_OK = 0, + TRANSACTION_GENERIC_ERROR = -1, + TRANSACTION_NAME_CONFLICT = -2, + TRANSACTION_CREATE_EXISTS = -3, + TRANSACTION_NONEXISTENT_REF = -4, + TRANSACTION_INCORRECT_OLD_VALUE = -5, + TRANSACTION_INVALID_NEW_VALUE = -6, + TRANSACTION_EXPECTED_SYMREF = -7, +}; + /* * Resolve a reference, recursively following symbolic references. * @@ -117,24 +142,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 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 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 +855,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..3b0adf8bb2 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -676,19 +676,19 @@ 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 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 transaction_error ret = TRANSACTION_GENERIC_ERROR; 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,6 +728,7 @@ static int lock_raw_ref(struct files_ref_store *refs, strbuf_reset(err); strbuf_addf(err, "unable to resolve reference '%s'", refname); + ret = TRANSACTION_NONEXISTENT_REF; } else { /* * The error message set by @@ -788,6 +789,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 = TRANSACTION_NONEXISTENT_REF; goto error_return; } else { /* @@ -820,6 +822,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 = TRANSACTION_NONEXISTENT_REF; goto error_return; } else if (remove_dir_recursively(&ref_file, REMOVE_DIR_EMPTY_ONLY)) { @@ -863,7 +866,7 @@ static int lock_raw_ref(struct files_ref_store *refs, string_list_insert(refnames_to_check, refname); } - ret = 0; + ret = TRANSACTION_OK; goto out; error_return: @@ -1517,10 +1520,10 @@ 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 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 +1929,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 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 +1947,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 TRANSACTION_INVALID_NEW_VALUE; } if (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) { strbuf_addf( @@ -1951,7 +1955,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 TRANSACTION_INVALID_NEW_VALUE; } } fd = get_lock_file_fd(&lock->lk); @@ -1962,9 +1966,9 @@ 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 TRANSACTION_GENERIC_ERROR; } - return 0; + return TRANSACTION_OK; } /* @@ -2376,9 +2380,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 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; @@ -2386,10 +2391,10 @@ static int split_head_update(struct ref_update *update, (update->flags & REF_SKIP_CREATE_REFLOG) || (update->flags & REF_IS_PRUNING) || (update->flags & REF_UPDATE_VIA_HEAD)) - return 0; + return TRANSACTION_OK; if (strcmp(update->refname, head_ref)) - return 0; + return TRANSACTION_OK; /* * First make sure that HEAD is not already in the @@ -2419,7 +2424,7 @@ static int split_head_update(struct ref_update *update, if (strcmp(new_update->refname, "HEAD")) BUG("%s unexpectedly not 'HEAD'", new_update->refname); - return 0; + return TRANSACTION_OK; } /* @@ -2430,10 +2435,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 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; @@ -2482,7 +2487,7 @@ static int split_symref_update(struct ref_update *update, update->flags |= REF_LOG_ONLY | REF_NO_DEREF; update->flags &= ~REF_HAVE_OLD; - return 0; + return TRANSACTION_OK; } /* @@ -2491,34 +2496,32 @@ 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 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; + return TRANSACTION_OK; if (is_null_oid(&update->old_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 TRANSACTION_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 TRANSACTION_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 TRANSACTION_INCORRECT_OLD_VALUE; } struct files_transaction_backend_data { @@ -2540,17 +2543,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 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 transaction_error ret = TRANSACTION_OK; struct ref_lock *lock; files_assert_main_repository(refs, "lock_ref_for_update"); @@ -2607,16 +2610,12 @@ static int lock_ref_for_update(struct files_ref_store *refs, } } - 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 { /* @@ -2644,7 +2643,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 = TRANSACTION_EXPECTED_SYMREF; goto out; } else { ret = check_old_oid(update, &lock->old_oid, err); @@ -2693,25 +2692,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)) { diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 3247871574..75e1ebf67d 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1323,10 +1323,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 transaction_error write_with_updates(struct packed_ref_store *refs, + struct string_list *updates, + struct strbuf *err) { + enum transaction_error ret = TRANSACTION_GENERIC_ERROR; struct ref_iterator *iter = NULL; size_t i; int ok; @@ -1350,7 +1351,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 TRANSACTION_GENERIC_ERROR; } strbuf_release(&sb); @@ -1406,6 +1407,7 @@ static int write_with_updates(struct packed_ref_store *refs, strbuf_addf(err, "cannot update ref '%s': " "reference already exists", update->refname); + ret = TRANSACTION_CREATE_EXISTS; goto error; } else if (!oideq(&update->old_oid, iter->oid)) { strbuf_addf(err, "cannot update ref '%s': " @@ -1413,6 +1415,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 = TRANSACTION_INCORRECT_OLD_VALUE; goto error; } } @@ -1449,6 +1452,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 TRANSACTION_NONEXISTENT_REF; goto error; } } @@ -1506,10 +1510,10 @@ static int write_with_updates(struct packed_ref_store *refs, strerror(errno)); strbuf_release(&sb); delete_tempfile(&refs->tempfile); - return -1; + return TRANSACTION_GENERIC_ERROR; } - return 0; + return TRANSACTION_OK; write_error: strbuf_addf(err, "error writing to %s: %s", @@ -1518,7 +1522,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, @@ -1672,7 +1676,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; diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 4b7fc8f1ab..c97045fbed 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -769,8 +769,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 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..e1fd9c2de2 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 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 transaction_error ret = TRANSACTION_OK; 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 TRANSACTION_GENERIC_ERROR; /* 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 TRANSACTION_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 TRANSACTION_INVALID_NEW_VALUE; } } @@ -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 TRANSACTION_GENERIC_ERROR; 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 TRANSACTION_GENERIC_ERROR; } 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 TRANSACTION_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 TRANSACTION_GENERIC_ERROR; } } else { struct ref_update *new_update; @@ -1255,11 +1255,12 @@ 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 TRANSACTION_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)) { @@ -1268,18 +1269,21 @@ static int prepare_single_update(struct reftable_ref_store *refs, ref_update_original_update_refname(u)); return TRANSACTION_CREATE_EXISTS; } - else if (is_null_oid(¤t_oid)) + 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 TRANSACTION_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 TRANSACTION_INCORRECT_OLD_VALUE; + } } /* @@ -1296,10 +1300,10 @@ 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 TRANSACTION_GENERIC_ERROR; - return 0; + return TRANSACTION_OK; } static int reftable_be_transaction_prepare(struct ref_store *ref_store, @@ -1386,7 +1390,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 Tue Feb 25 09:29:09 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Karthik Nayak X-Patchwork-Id: 13989613 Received: from mail-ej1-f43.google.com (mail-ej1-f43.google.com [209.85.218.43]) (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 EBD5B25A326 for ; Tue, 25 Feb 2025 09:29:34 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.218.43 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1740475777; cv=none; b=EuWUPrZOyivwrnkCkHgVuTtVpXOdThacMqfJ6o0BwP7eD9gjYioF0iO35exzsKXBVLwNGtWPBg79B/tyWW3IxCJfYF7bq3lX7B4nqmsugj2Bg2FQRIB5+fpgOjkXaIEeEYg/i9zM/lrviBZo9PW44HcmDmvn4KWkcqYLcSZqDOk= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1740475777; c=relaxed/simple; bh=TGp0iyGaH+OzmddfN5/WALZN5SsP93fAXBRDNfVO+Yk=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:References: In-Reply-To:To:Cc; b=Dw8hLe78KSt9JLPeVGojJa8v2MGQrGrqKaecJwDS61OHhcIbR8TnOrIK+uKuVtQo/jGyqB29LtVcz9m4EgnMWGLCBxMSqcjf5b6ILhcFry2nd8Zw5f26r4NL5ga2HWCwE+QZmVZDM6BbIXgiDtpkMhfWVh8+7+cncxDYTYVvOgE= 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=LMA8A/2O; arc=none smtp.client-ip=209.85.218.43 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="LMA8A/2O" Received: by mail-ej1-f43.google.com with SMTP id a640c23a62f3a-abb7a6ee2deso826627266b.0 for ; Tue, 25 Feb 2025 01:29:34 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1740475773; x=1741080573; 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=CbXt8tJashaNsUx9xxu2Pbh+eLfB21GqvkRR99DUOrA=; b=LMA8A/2Oro+tA6xqiwp2ibsa2qqq1nKCiIHtiFgKl342fsAgKY23ue0TpB+eiWZKjv kshK6B//a8oTIPrqZ6lvDgP6DxBPGr9hTD7n6n7qIYZnUmRNNfCFzWMD244AUH4QFw+g H927+xZmZ8tNaB4K3pndejxMFNBHJb9PUIxWVle3xrlFpOV5lbHP4ukYtJnAVV/qyGUU Hsgnq4CtGLDciH8K0IN1TLjRvDEWtvmifrZ+gXjywPlY+ACZBw8RNf26ixlicIKAhV8d olXqKbaE8ImCOnCPZzJJcS02wQw4kIAu7rt+wVYAxNrUzNO5LA5+Mch+6qxnfyhlYmJz NFlA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1740475773; x=1741080573; 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=CbXt8tJashaNsUx9xxu2Pbh+eLfB21GqvkRR99DUOrA=; b=BIkg98FLq/U7Lbjl8uMiqGPMDaWL0l/9/+e0noV1PUC33x+N4OgZeRb+FxNCxdZ7HF t1UNWs2++mjFYol8VTI/nsmjgannUKt+ORXenfOb29rzdCyRgwy05MKyxakMALV84LvL M164zX+LiBTWuDEoLAi6B6FJd1yWRLsS0xbtjn3QasC3opvMzEuMkd8TxwQgSk083TEY P6KXweVh0NHcizqx1v432PnPmDKBFQpiymJvj9oV1qo6xVwGMcbyScpZEdgM3FdrIOf1 3eTttOBlJDjO1psc0EImkMbF3nFB37WPL+5pa5DzbLEFtA4SHLjj6qPFKsJV0PNX/KVK FoyQ== X-Gm-Message-State: AOJu0YxOQYKetvEVZKEOyVHt+h1pd75lObsxIJP9BcJXPeWV1e582w4w POp656BjW/pIuCE7JNdEIZ/bO6NWO3Ce2f0yBAzpQjHp4ijpkHD3 X-Gm-Gg: ASbGncsXUk/hSVVyvm1Zz16tWCh3/tIp6LucDXlEV3YPg3CdlQV1bPxP1cWC9w3771m 6CLRM9M3gsuWGuJZbPUSbp1m/UjZl/gpeuw/R3MQkFhviAMfltZXfqzsOV5r3x6LaJU0PousCB+ AQlB518H8PdWHSwFWrNVIC/lvlb6cieFZMNC3gXZ9AD52wB9CRF4Bvay+9MnRCsICe+beNE0hr6 BLzf6veRDap45IINfJBA2K+TD9GyFJkJqo3S3Dh3yfdz7BYr8uAcNtSpx0RtjWPTO6Hchmk7iby hS40SBcEduFokdMfrVKpXbLn6lTYE6mE X-Google-Smtp-Source: AGHT+IGY1PUMYDCw8JFKK1pNsXhQzmn/R9fDbqwZC1YyRqbqfbexblDfFFgHisPUPFAZhfklfdEzgA== X-Received: by 2002:a17:907:c22:b0:ab7:b6cb:b633 with SMTP id a640c23a62f3a-abc09b2b7e2mr1682330266b.34.1740475773037; Tue, 25 Feb 2025 01:29:33 -0800 (PST) Received: from [127.0.0.2] ([2a02:2455:8268:bc00:20c2:4ab6:a193:5b8c]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-abed1cd561esm111944466b.19.2025.02.25.01.29.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 25 Feb 2025 01:29:32 -0800 (PST) From: Karthik Nayak Date: Tue, 25 Feb 2025 10:29:09 +0100 Subject: [PATCH v2 6/7] 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: <20250225-245-partially-atomic-ref-updates-v2-6-cfa3236895d7@gmail.com> References: <20250225-245-partially-atomic-ref-updates-v2-0-cfa3236895d7@gmail.com> In-Reply-To: <20250225-245-partially-atomic-ref-updates-v2-0-cfa3236895d7@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=10778; i=karthik.188@gmail.com; h=from:subject:message-id; bh=TGp0iyGaH+OzmddfN5/WALZN5SsP93fAXBRDNfVO+Yk=; b=owJ4nAHtARL+kA0DAAoBPtWfJI5GjH8ByyZiAGe9jXZncw9Srn91aYGKiIDlwBZDv64xvVgS6 8/mt7oFqbfxJYkBswQAAQoAHRYhBFfOTH9jdXEPy2XGBj7VnySORox/BQJnvY12AAoJED7VnySO Rox/TMQL/3VrN5pk51Crah9JkbfdAvy9GX/pJjU/f+hp5U5Ft/N6c7TUS7oZejovS5raoq5q2du o32W1dkRNN/w3IGH359nRyFPkFdfnDOMj5OdwFf4BrCYP3sdaS6Lm8mXBgKkrAse2amsqCLYagh OedefzO4AjP4qzvhnAa8YT/pTXjeRuXNPOuzjqsyNko5jsF6qZr7fx5AwsKHLcrWVO2VOqoy3oI XN6sLQZDk58tP3J2nVAvKGKeREnm6enChTkhBH3hKiLLlT7rlSzhwAoApqBdF5GfTcH2MrPmdvL 1WBTo5R2ZvohsiFZPm33qCJBGJt1c7Rg7USWVKBdwgFgCjN3qHsgUCDvMABAlsi+BzZ9OIOYEyN H68MVuAt3oGUlFgZhMFrbl66DW95C3zoDzJHkjT28jCdSVk6K6TOf3Q+MR3PQZpMh8CGI6w+bh6 +5XzvY6rCzPl705c4JeRG7TmNyG2+16m+64sknQ3UvGewJo6ihgWWPHBzg1Fny0QqKcXicl9f76 ls= 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. Non-system-related errors include issues caused by user-provided input values, whereas system-related errors, such as I/O failures or memory issues, continue to result in a full transaction failure. 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. - 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. The next 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 | 31 +++++++++++++++++++++++++++++++ refs.h | 22 ++++++++++++++++++++++ refs/files-backend.c | 12 +++++++++++- refs/packed-backend.c | 30 ++++++++++++++++++++++++++++-- refs/refs-internal.h | 13 +++++++++++++ refs/reftable-backend.c | 12 +++++++++++- 6 files changed, 116 insertions(+), 4 deletions(-) diff --git a/refs.c b/refs.c index f989a46a5a..243c09c368 100644 --- a/refs.c +++ b/refs.c @@ -1211,6 +1211,15 @@ void ref_transaction_free(struct ref_transaction *transaction) free(transaction); } +void ref_transaction_set_rejected(struct ref_transaction *transaction, + size_t update_idx, + enum transaction_error err) +{ + if (update_idx >= transaction->nr) + BUG("trying to set rejection on invalid update index"); + transaction->updates[update_idx]->rejection_err = err; +} + struct ref_update *ref_transaction_add_update( struct ref_transaction *transaction, const char *refname, unsigned int flags, @@ -1236,6 +1245,7 @@ struct ref_update *ref_transaction_add_update( transaction->updates[transaction->nr++] = update; update->flags = flags; + update->rejection_err = TRANSACTION_OK; update->new_target = xstrdup_or_null(new_target); update->old_target = xstrdup_or_null(old_target); @@ -2726,6 +2736,27 @@ 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->flags & REF_TRANSACTION_ALLOW_PARTIAL)) + return; + + for (size_t i = 0; i < transaction->nr; i++) { + struct ref_update *update = transaction->updates[i]; + + 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 8e9ead174c..e4a6a8218f 100644 --- a/refs.h +++ b/refs.h @@ -675,6 +675,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), }; /* @@ -905,6 +912,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 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 3b0adf8bb2..d0a53c9ace 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2851,8 +2851,18 @@ 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 (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL && + ret != TRANSACTION_GENERIC_ERROR) { + ref_transaction_set_rejected(transaction, i, ret); + + strbuf_setlen(err, 0); + ret = TRANSACTION_OK; + + continue; + } goto cleanup; + } if (update->flags & REF_DELETING && !(update->flags & REF_LOG_ONLY) && diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 75e1ebf67d..0857204213 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1324,10 +1324,11 @@ static int packed_ref_store_remove_on_disk(struct ref_store *ref_store, * remain locked when it is done. */ static enum transaction_error write_with_updates(struct packed_ref_store *refs, - struct string_list *updates, + struct ref_transaction *transaction, struct strbuf *err) { enum transaction_error ret = TRANSACTION_GENERIC_ERROR; + struct string_list *updates = &transaction->refnames; struct ref_iterator *iter = NULL; size_t i; int ok; @@ -1408,6 +1409,14 @@ static enum transaction_error write_with_updates(struct packed_ref_store *refs, "reference already exists", update->refname); ret = TRANSACTION_CREATE_EXISTS; + + if (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL) { + ref_transaction_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': " @@ -1416,6 +1425,14 @@ static enum transaction_error write_with_updates(struct packed_ref_store *refs, oid_to_hex(iter->oid), oid_to_hex(&update->old_oid)); ret = TRANSACTION_INCORRECT_OLD_VALUE; + + if (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL) { + ref_transaction_set_rejected(transaction, i, ret); + strbuf_setlen(err, 0); + ret = 0; + continue; + } + goto error; } } @@ -1453,6 +1470,14 @@ static enum transaction_error write_with_updates(struct packed_ref_store *refs, update->refname, oid_to_hex(&update->old_oid)); return TRANSACTION_NONEXISTENT_REF; + + if (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL) { + ref_transaction_set_rejected(transaction, i, ret); + strbuf_setlen(err, 0); + ret = 0; + continue; + } + goto error; } } @@ -1518,6 +1543,7 @@ static enum transaction_error write_with_updates(struct packed_ref_store *refs, write_error: strbuf_addf(err, "error writing to %s: %s", get_tempfile_path(refs->tempfile), strerror(errno)); + ret = TRANSACTION_GENERIC_ERROR; error: ref_iterator_free(iter); @@ -1676,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 c97045fbed..7196f2d880 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 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,14 @@ 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. To be used in conjuction + * with the `REF_TRANSACTION_ALLOW_PARTIAL` flag to allow partial transactions. + */ +void ref_transaction_set_rejected(struct ref_transaction *transaction, + size_t update_idx, + enum 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 diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index e1fd9c2de2..83cf8d582b 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1374,8 +1374,18 @@ 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 (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL && + ret != TRANSACTION_GENERIC_ERROR) { + ref_transaction_set_rejected(transaction, i, ret); + + strbuf_setlen(err, 0); + ret = TRANSACTION_OK; + + continue; + } goto done; + } } string_list_sort(&refnames_to_check); From patchwork Tue Feb 25 09:29:10 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Karthik Nayak X-Patchwork-Id: 13989615 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 10A8F26138B for ; Tue, 25 Feb 2025 09:29:35 +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=1740475778; cv=none; b=lkqfyohhVCs2POi+iZJGjyOn/uUC7pLdhaHZ/+oAYVk7Ku2hUUGXAy671Xk1uAadSFr7wfW9ETRyxWKfFDhJx0OBpZ1Z3+puHEMM2e3ugoRWm39TEVVNnO5ICnS/OsVaVBI/IQoxszIlZSW3VZ0tckGkx+DfVLfUif1aUweMbzU= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1740475778; c=relaxed/simple; bh=8DbvIz+xlplD0bMqAPtwSi/hN7Ypw5V6eFdyNROASwg=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:References: In-Reply-To:To:Cc; b=EkUAsC9UN91q0nwDNV0rJM5iF4oZPbZuEozwdOsgXk0ozNw26PWV8TcnJpJj2Fg+lZPuk3no1ettu5nLeI/j70L9+JPjqskIURmlf1K8emXhbnCPR46laQXMqJx1if4MF7u5tLjnCPbP17Si5piPs7r4CFahXk/paviUvwfKRzw= 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=j8G1aafR; 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="j8G1aafR" Received: by mail-ej1-f51.google.com with SMTP id a640c23a62f3a-abb90f68f8cso1020620866b.3 for ; Tue, 25 Feb 2025 01:29:35 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1740475774; x=1741080574; 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=DgSrTdaz4gftXbxTgwJhGaBvYrJrXe1Id6S7ComH2B4=; b=j8G1aafRLD3onrAVtNgSPKBNAiu0E0k4g71HW7SoGOEuhV6bw3mTh3JAD8qRE8VAeq TZo1KtBvO6dkEfMyINOzMX3m7MikWBYsNSRyhXWLU5DiKu/emXRLkZWrPY3nazXoPvN/ seNqcryBbLW2d0sk4IIkSuF1nIXyuRBcMvWxpRICv7IGxgf/pHnYttCmSsqwHTWbH1PD boWMGSVcgSk0lpsiOXrAK/TFdU6zJBCJ/d3PdJaDe6p5LD/kFoepwBISqQWJsF+LVZLw R0H0pugpykALEJH8APMsXF7bXFPZ3Nnk5j12s2GoEtYy0MfQ/2Lkrjk00a+4+YGmYFqV He3w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1740475774; x=1741080574; 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=DgSrTdaz4gftXbxTgwJhGaBvYrJrXe1Id6S7ComH2B4=; b=pAGPvtYlRQxUrH3AEqVrjcWCgHWD2xa+g/LwOjPkBiMNALyLh8r8zgPHjjfqg4/drz LYOeUxqyiaGi2vtTYvVjxLU0fGR7grr6Y1d+4zEGIlJrUkZYdRDdlbUdiEpQPT+NgKaf c0Gk2ZOh081M5gayBw9XHCPk8Mn1WkDDIDQjx66J9I46SVWnzD1gEXdVTsRXMAP8Jqvo cCy2DrJwxOYEu+FOn9fGZDdmQK6G9piEjs2vc9Dqb/MR0dhu0a6OdEJv/ZuMdVyXQryR qyUT2Zt67xN4eQT1I76tSdZJmN2G6l5iinyTpzS0FL1UpHUXBeE4+VbgrLC8rljNHl6g Zv4Q== X-Gm-Message-State: AOJu0YyArcaPQHEOQXAFGFfXOxfdnzCC1moBwikqFITdkVr1i4keLDCk cwvC6ZlbTtix1PcMDDCEYDwn3gT3XrqWKZh4vFWvwVD8D8JjpRstrQXNXDVW X-Gm-Gg: ASbGncsruAuEKefNxJ76OC1HDyMowcgWrDWliLKq0qLB5X4h54JPiq9ul0rnsyQw5fu JX9GhE1kmR3lIst11H+lGkT2yoaqRGWCTtBsEiPHKU4VE3auTK6VJ7zb8HiFfpg2Pr2/pvkRnFs CNRT4hXPttrTfsSsSH+TKYq8195p03+CwOgi17eHOAz4krbdpisjkFOFcMMzXal4r2xkOUOMMEP j6EkA4Fet8K9NBtr+GsM9/0vyEt2jICVXacnZzfi1Uj8ZmiojX3wSIF53tQSYRuJyhffC9oEGAg Ppm7J9u+hT7Wa9sm8PcwoHkRvkW4bj6R X-Google-Smtp-Source: AGHT+IF4JWwlg+0HzyEymubYrW0BrokDFl4MUvKwEo8Jf/w6fJ9P2mUUPwZsJGr7ILGLq6nBybLRjg== X-Received: by 2002:a17:907:7fa1:b0:abb:9c8a:fbcd with SMTP id a640c23a62f3a-abed107b0e2mr278945966b.53.1740475773997; Tue, 25 Feb 2025 01:29:33 -0800 (PST) Received: from [127.0.0.2] ([2a02:2455:8268:bc00:20c2:4ab6:a193:5b8c]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-abed1cd561esm111944466b.19.2025.02.25.01.29.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 25 Feb 2025 01:29:33 -0800 (PST) From: Karthik Nayak Date: Tue, 25 Feb 2025 10:29:10 +0100 Subject: [PATCH v2 7/7] 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: <20250225-245-partially-atomic-ref-updates-v2-7-cfa3236895d7@gmail.com> References: <20250225-245-partially-atomic-ref-updates-v2-0-cfa3236895d7@gmail.com> In-Reply-To: <20250225-245-partially-atomic-ref-updates-v2-0-cfa3236895d7@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=16932; i=karthik.188@gmail.com; h=from:subject:message-id; bh=8DbvIz+xlplD0bMqAPtwSi/hN7Ypw5V6eFdyNROASwg=; b=owJ4nAHtARL+kA0DAAoBPtWfJI5GjH8ByyZiAGe9jXamS5RqTqp7E5GhAIlPsn/QU+kvN2t2G 2a1FU+JCrgTo4kBswQAAQoAHRYhBFfOTH9jdXEPy2XGBj7VnySORox/BQJnvY12AAoJED7VnySO Rox/V1AL/ivuCfrO21Vlcag6r5jpbZkc1r9PRRETO1FuyngQ7b4w7pDiAVhQwxjb7MleMBeXObz WDgqyFPYmPlfcb0VET8GPo9cdoaN27UxqbJmONQjd7qGFluupjYyw/v4I9dqPFrZCECJAwa7+tp jDurzyEcHczHDlIDsv8RHlR4JTyMaxWy08fbCQuba0NHA+ovVvyTkYrz4ty4Vs370Wp8CnFVcM2 MzKbY+eve/Y4ryH0xkmmtyTX5UQi4DAFwWJ/bPZ9YlcCJ22QmZ+ww5TO+VeBTcwSLENgQIIBCX7 b3wvuVcqQg5jl2h6antTzuzZd1Vcmz22vnP99P86fN4cQcufC82rtByr0k531tyjYeLISpAZ6hX s2hL0Xcs74o0pmfszOu0NyVbLNme6he+p38l86meWAf0s0w8scFk1AeLLv3JUM2y+A63ZR5voiA RYctJ8Ers+i+YRhGKI4IF5RLqQzLAhmwXKBI/u1HVv363qu9xPr60gUF1yCcpjt9JE53SMmIbb7 fA= 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 or with `-z`: rejected NUL ( | ) NUL ( | ) NUL NUL 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 | 21 +++- builtin/update-ref.c | 74 +++++++++++-- t/t1400-update-ref.sh | 216 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 302 insertions(+), 9 deletions(-) diff --git a/Documentation/git-update-ref.adoc b/Documentation/git-update-ref.adoc index 9e6935d38d..fc73f1d8aa 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 @@ -82,6 +95,10 @@ quoting: In this format, use 40 "0" to specify a zero value, and use the empty string to specify a missing value. +With `-z`, `--allow-partial` will print rejections in the following form: + + rejected NUL ( | ) NUL ( | ) NUL NUL + In either format, values can be specified in any form that Git recognizes as an object name. Commands in any other format or a repeated produce an error. Command meanings are: diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 1d541e13ad..b03b40eacb 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,54 @@ 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 transaction_error err, + void *cb_data UNUSED) +{ + struct strbuf sb = STRBUF_INIT; + char space = ' '; + const char *reason = ""; + + switch (err) { + case TRANSACTION_NAME_CONFLICT: + reason = _("refname conflict"); + break; + case TRANSACTION_CREATE_EXISTS: + reason = _("reference already exists"); + break; + case TRANSACTION_NONEXISTENT_REF: + reason = _("reference does not exist"); + break; + case TRANSACTION_INCORRECT_OLD_VALUE: + reason = _("incorrect old value provided"); + break; + case TRANSACTION_INVALID_NEW_VALUE: + reason = _("invalid new value provided"); + break; + case TRANSACTION_EXPECTED_SYMREF: + reason = _("expected symref but found regular ref"); + break; + default: + reason = _("unkown failure"); + } + + if (!line_termination) + space = line_termination; + + strbuf_addf(&sb, "rejected%c%s%c%s%c%c%s%c%s%c", space, + refname, space, new_oid ? oid_to_hex(new_oid) : new_target, + space, space, old_oid ? oid_to_hex(old_oid) : old_target, + space, reason, line_termination); + + fwrite(sb.buf, sb.len, 1, stdout); + strbuf_release(&sb); + fflush(stdout); +} + static void parse_cmd_commit(struct ref_transaction *transaction, const char *next, const char *end UNUSED) { @@ -573,6 +622,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 +662,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 +670,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 +738,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 +754,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: @@ -726,7 +781,9 @@ int cmd_update_ref(int argc, const char *refname, *oldval; struct object_id oid, oldoid; int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0; - int create_reflog = 0; + int create_reflog = 0, allow_partial = 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 +792,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 +815,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 (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..fb9442982e 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -2066,6 +2066,222 @@ 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 + ) + ' + + # F/D conflicts on the files backend are resolved on an individual + # update level since refs are stored as files. On the reftable backend + # this check is batched to optimize for performance, so failures cannot + # be isolated to a single update. + test_expect_success REFFILES "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 + ) + ' done test_expect_success 'update-ref should also create reflog for HEAD' '