From patchwork Mon Jan 11 11:05:11 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Patrick Steinhardt X-Patchwork-Id: 12010243 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6CE6CC433E6 for ; Mon, 11 Jan 2021 11:06:27 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 2E7842250F for ; Mon, 11 Jan 2021 11:06:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729632AbhAKLGY (ORCPT ); Mon, 11 Jan 2021 06:06:24 -0500 Received: from wout4-smtp.messagingengine.com ([64.147.123.20]:34379 "EHLO wout4-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728413AbhAKLGY (ORCPT ); Mon, 11 Jan 2021 06:06:24 -0500 Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailout.west.internal (Postfix) with ESMTP id 0FD7E290A; Mon, 11 Jan 2021 06:05:18 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute5.internal (MEProxy); Mon, 11 Jan 2021 06:05:18 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=fm3; bh=TdirxcV1wcdigcumbFj3bx6RCZA EhizEsGjFLLGEawo=; b=o2tBQipvTduVSjRFMJp0o1EunBgu16TyTAa7ROjJWpt amxGQwdN1zUPnAakwZ9V2GQgcKN2AjSv383lP4oPzwM2gNyHEMvwds2D7RiiS93a 6o+jWHpTEvZ9JNk/QrGfm6SmuxmJcD97fhFu3OK7muer1WXSOMgVcJ0MD6ZtPRhy dvjB1HgYV+Kjmw5Eo6lJTZCmW6YhTEwWP1K1IM9u792bOx0XTdGtdfuNoekfk4CB c7Jzd4UUIdzCOPMcHtljsW9C1wtlx6lUj63PyzC96UVKM8TlNWZOC43ztDea3XSN sJxBVv8O4b/21rqBATlyMZJeFLlHRebeCTrnt6RKhXQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; bh=Tdirxc V1wcdigcumbFj3bx6RCZAEhizEsGjFLLGEawo=; b=rhdySS4TOmFC/w44j0Zz/o QtVTEMS1zPkAa3/xahiRkSYwVl69V5F/fdQGZxkjCY31lG/AwAqx8+QGjxUQ6r1K HetD4WCqVPcHfo8Vs6L/PXUdy1XKACpt1EINnAmgnxK/dPsDS+Tl2DLDd28Ypjax WS0gnTjMEzGgjrqvFQ02oOmEtWDvH/oeja338Yolk/brYNavXY4JFFNZH7WlG5eh 7Ld1/sg25e8XTpwDiMcXbU/ObNguzPEUNpWtRyvD38VHkM/mr410A3UQofFds6zL LTLtg2Qq55nqagIWZ3p77O8SiftPQc9zXQ+GIhCdv1XQXcsZxqzPbxDV4exKtg+g == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedrvdehuddgvdehucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepfffhvffukfhfgggtuggjsehgtderredttddvnecuhfhrohhmpefrrghtrhhi tghkucfuthgvihhnhhgrrhguthcuoehpshesphhkshdrihhmqeenucggtffrrghtthgvrh hnpeehgefhtdefueffheekgfffudelffejtdfhvdejkedthfehvdelgfetgfdvtedthfen ucfkphepkeelrddugedrgeehrddujedtnecuvehluhhsthgvrhfuihiivgeptdenucfrrg hrrghmpehmrghilhhfrhhomhepphhssehpkhhsrdhimh X-ME-Proxy: Received: from vm-mail.pks.im (dynamic-089-014-045-170.89.14.pool.telefonica.de [89.14.45.170]) by mail.messagingengine.com (Postfix) with ESMTPA id 5BD86108005C; Mon, 11 Jan 2021 06:05:16 -0500 (EST) Received: from localhost (ncase [10.192.0.11]) by vm-mail.pks.im (OpenSMTPD) with ESMTPSA id 88eabf28 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Mon, 11 Jan 2021 11:05:12 +0000 (UTC) Date: Mon, 11 Jan 2021 12:05:11 +0100 From: Patrick Steinhardt To: git@vger.kernel.org Cc: Junio C Hamano , Christian Couder Subject: [PATCH v3 0/5] fetch: implement support for atomic reference updates Message-ID: References: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Hi, this is the third version of my patch series to implement support for atomic reference updates for git-fetch(1). It's similar to `git push --atomic`, only that it applies to the local side. That is the fetch will either succeed and update all remote references or it will fail and update none. Changes compared to v2: - `append_fetch_head` now takes a `struct object_id *old_oid` instead of a `char *old_oid`. - Handling of the merge status marker was moved into `append_fetch_head`. - I've unified code paths to format the line we're about to write to FETCH_HEAD in atomic and non-atomic cases, which is the new 2/5 patch. - We now always initialize `fetch_head->fp` and use it to derive whether anything should be written at all instead of using the global `write_fetch_head` variable. I think this should address all of Junio's feedback. Thanks for your input! Patrick Patrick Steinhardt (5): fetch: extract writing to FETCH_HEAD fetch: use strbuf to format FETCH_HEAD updates fetch: refactor `s_update_ref` to use common exit path fetch: allow passing a transaction to `s_update_ref()` fetch: implement support for atomic reference updates Documentation/fetch-options.txt | 4 + builtin/fetch.c | 228 +++++++++++++++++++++++--------- remote.h | 2 +- t/t5510-fetch.sh | 168 +++++++++++++++++++++++ 4 files changed, 342 insertions(+), 60 deletions(-) Range-diff against v2: 1: d80dbc5a9c ! 1: 61dc19a1ca fetch: extract writing to FETCH_HEAD @@ builtin/fetch.c: static int iterate_ref_map(void *cb_data, struct object_id *oid +{ + const char *filename = git_path_fetch_head(the_repository); + -+ if (!write_fetch_head) -+ return 0; -+ -+ fetch_head->fp = fopen(filename, "a"); -+ if (!fetch_head->fp) -+ return error_errno(_("cannot open %s"), filename); ++ if (write_fetch_head) { ++ fetch_head->fp = fopen(filename, "a"); ++ if (!fetch_head->fp) ++ return error_errno(_("cannot open %s"), filename); ++ } else { ++ fetch_head->fp = NULL; ++ } + + return 0; +} + -+static void append_fetch_head(struct fetch_head *fetch_head, const char *old_oid, -+ const char *merge_status_marker, const char *note, ++static void append_fetch_head(struct fetch_head *fetch_head, ++ const struct object_id *old_oid, ++ enum fetch_head_status fetch_head_status, ++ const char *note, + const char *url, size_t url_len) +{ ++ char old_oid_hex[GIT_MAX_HEXSZ + 1]; ++ const char *merge_status_marker; + size_t i; + -+ if (!write_fetch_head) ++ if (!fetch_head->fp) + return; + ++ switch (fetch_head_status) { ++ case FETCH_HEAD_NOT_FOR_MERGE: ++ merge_status_marker = "not-for-merge"; ++ break; ++ case FETCH_HEAD_MERGE: ++ merge_status_marker = ""; ++ break; ++ default: ++ /* do not write anything to FETCH_HEAD */ ++ return; ++ } ++ + fprintf(fetch_head->fp, "%s\t%s\t%s", -+ old_oid, merge_status_marker, note); ++ oid_to_hex_r(old_oid_hex, old_oid), merge_status_marker, note); + for (i = 0; i < url_len; ++i) + if ('\n' == url[i]) + fputs("\\n", fetch_head->fp); @@ builtin/fetch.c: static int iterate_ref_map(void *cb_data, struct object_id *oid + +static void close_fetch_head(struct fetch_head *fetch_head) +{ -+ if (!write_fetch_head) ++ if (!fetch_head->fp) + return; + + fclose(fetch_head->fp); @@ builtin/fetch.c: N_("It took %.2f seconds to check forced updates. You can use\n if (raw_url) url = transport_anonymize_url(raw_url); @@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *remote_name, - merge_status_marker = "not-for-merge"; - /* fall-through */ - case FETCH_HEAD_MERGE: + want_status++) { + for (rm = ref_map; rm; rm = rm->next) { + struct ref *ref = NULL; +- const char *merge_status_marker = ""; + + if (rm->status == REF_STATUS_REJECT_SHALLOW) { + if (want_status == FETCH_HEAD_MERGE) +@@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *remote_name, + strbuf_addf(¬e, "%s ", kind); + strbuf_addf(¬e, "'%s' of ", what); + } +- switch (rm->fetch_head_status) { +- case FETCH_HEAD_NOT_FOR_MERGE: +- merge_status_marker = "not-for-merge"; +- /* fall-through */ +- case FETCH_HEAD_MERGE: - fprintf(fp, "%s\t%s\t%s", - oid_to_hex(&rm->old_oid), - merge_status_marker, @@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char * - else - fputc(url[i], fp); - fputc('\n', fp); -+ append_fetch_head(&fetch_head, -+ oid_to_hex(&rm->old_oid), -+ merge_status_marker, -+ note.buf, url, url_len); - break; - default: - /* do not write anything to FETCH_HEAD */ +- break; +- default: +- /* do not write anything to FETCH_HEAD */ +- break; +- } ++ ++ append_fetch_head(&fetch_head, &rm->old_oid, ++ rm->fetch_head_status, ++ note.buf, url, url_len); + + strbuf_reset(¬e); + if (ref) { @@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *remote_name, } } @@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char * return rc; } + + ## remote.h ## +@@ remote.h: struct ref { + * should be 0, so that xcalloc'd structures get it + * by default. + */ +- enum { ++ enum fetch_head_status { + FETCH_HEAD_MERGE = -1, + FETCH_HEAD_NOT_FOR_MERGE = 0, + FETCH_HEAD_IGNORE = 1 -: ---------- > 2: a19762690e fetch: use strbuf to format FETCH_HEAD updates 2: 718a8bf5d7 ! 3: c411f30e09 fetch: refactor `s_update_ref` to use common exit path @@ builtin/fetch.c: static int s_update_ref(const char *action, - check_old ? &ref->old_oid : NULL, - 0, msg, &err)) - goto fail; +- +- ret = ref_transaction_commit(transaction, &err); +- if (ret) { +- df_conflict = (ret == TRANSACTION_NAME_CONFLICT); +- goto fail; + if (!transaction) { + ret = STORE_REF_ERROR_OTHER; + goto out; -+ } -+ + } + + ret = ref_transaction_update(transaction, ref->name, &ref->new_oid, + check_old ? &ref->old_oid : NULL, + 0, msg, &err); @@ builtin/fetch.c: static int s_update_ref(const char *action, + ret = STORE_REF_ERROR_OTHER; + goto out; + } - - ret = ref_transaction_commit(transaction, &err); - if (ret) { -- df_conflict = (ret == TRANSACTION_NAME_CONFLICT); -- goto fail; -+ ret = (ret == TRANSACTION_NAME_CONFLICT) ? STORE_REF_ERROR_DF_CONFLICT -+ : STORE_REF_ERROR_OTHER; ++ ++ switch (ref_transaction_commit(transaction, &err)) { ++ case 0: ++ break; ++ case TRANSACTION_NAME_CONFLICT: ++ ret = STORE_REF_ERROR_DF_CONFLICT; + goto out; - } - ++ default: ++ ret = STORE_REF_ERROR_OTHER; ++ goto out; ++ } ++ +out: ref_transaction_free(transaction); + if (ret) 3: 4162d10fcb ! 4: 865d357ba7 fetch: allow passing a transaction to `s_update_ref()` @@ builtin/fetch.c: static int s_update_ref(const char *action, goto out; } -- ret = ref_transaction_commit(transaction, &err); -- if (ret) { -- ret = (ret == TRANSACTION_NAME_CONFLICT) ? STORE_REF_ERROR_DF_CONFLICT -- : STORE_REF_ERROR_OTHER; +- switch (ref_transaction_commit(transaction, &err)) { +- case 0: +- break; +- case TRANSACTION_NAME_CONFLICT: +- ret = STORE_REF_ERROR_DF_CONFLICT; +- goto out; +- default: +- ret = STORE_REF_ERROR_OTHER; - goto out; + if (our_transaction) { -+ ret = ref_transaction_commit(our_transaction, &err); -+ if (ret) { -+ ret = (ret == TRANSACTION_NAME_CONFLICT) ? STORE_REF_ERROR_DF_CONFLICT -+ : STORE_REF_ERROR_OTHER; ++ switch (ref_transaction_commit(our_transaction, &err)) { ++ case 0: ++ break; ++ case TRANSACTION_NAME_CONFLICT: ++ ret = STORE_REF_ERROR_DF_CONFLICT; ++ goto out; ++ default: ++ ret = STORE_REF_ERROR_OTHER; + goto out; + } } 4: 53705281b6 ! 5: 6a79e7adcc fetch: implement support for atomic reference updates @@ builtin/fetch.c: static struct option builtin_fetch_options[] = { OPT_STRING(0, "upload-pack", &upload_pack, N_("path"), N_("path to upload pack on remote end")), OPT__FORCE(&force, N_("force overwrite of local reference"), 0), -@@ builtin/fetch.c: static int iterate_ref_map(void *cb_data, struct object_id *oid) +@@ builtin/fetch.c: static void append_fetch_head(struct fetch_head *fetch_head, + strbuf_addch(&fetch_head->buf, url[i]); + strbuf_addch(&fetch_head->buf, '\n'); - struct fetch_head { - FILE *fp; -+ struct strbuf buf; - }; - - static int open_fetch_head(struct fetch_head *fetch_head) -@@ builtin/fetch.c: static int open_fetch_head(struct fetch_head *fetch_head) - if (!write_fetch_head) - return 0; - -+ strbuf_init(&fetch_head->buf, 0); - fetch_head->fp = fopen(filename, "a"); - if (!fetch_head->fp) - return error_errno(_("cannot open %s"), filename); -@@ builtin/fetch.c: static void append_fetch_head(struct fetch_head *fetch_head, const char *old_oid - if (!write_fetch_head) - return; - -- fprintf(fetch_head->fp, "%s\t%s\t%s", -- old_oid, merge_status_marker, note); -- for (i = 0; i < url_len; ++i) -- if ('\n' == url[i]) -- fputs("\\n", fetch_head->fp); -- else -- fputc(url[i], fetch_head->fp); -- fputc('\n', fetch_head->fp); +- strbuf_write(&fetch_head->buf, fetch_head->fp); +- strbuf_reset(&fetch_head->buf); + /* + * When using an atomic fetch, we do not want to update FETCH_HEAD if + * any of the reference updates fails. We thus have to write all + * updates to a buffer first and only commit it as soon as all + * references have been successfully updated. + */ -+ if (atomic_fetch) { -+ strbuf_addf(&fetch_head->buf, "%s\t%s\t%s", -+ old_oid, merge_status_marker, note); -+ strbuf_add(&fetch_head->buf, url, url_len); -+ strbuf_addch(&fetch_head->buf, '\n'); -+ } else { -+ fprintf(fetch_head->fp, "%s\t%s\t%s", -+ old_oid, merge_status_marker, note); -+ for (i = 0; i < url_len; ++i) -+ if ('\n' == url[i]) -+ fputs("\\n", fetch_head->fp); -+ else -+ fputc(url[i], fetch_head->fp); -+ fputc('\n', fetch_head->fp); ++ if (!atomic_fetch) { ++ strbuf_write(&fetch_head->buf, fetch_head->fp); ++ strbuf_reset(&fetch_head->buf); + } } static void commit_fetch_head(struct fetch_head *fetch_head) { - /* Nothing to commit yet. */ -+ if (!write_fetch_head || !atomic_fetch) ++ if (!fetch_head->fp || !atomic_fetch) + return; + strbuf_write(&fetch_head->buf, fetch_head->fp); } static void close_fetch_head(struct fetch_head *fetch_head) -@@ builtin/fetch.c: static void close_fetch_head(struct fetch_head *fetch_head) - return; - - fclose(fetch_head->fp); -+ strbuf_release(&fetch_head->buf); - } - - static const char warn_show_forced_updates[] = @@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *remote_name, struct fetch_head fetch_head; struct commit *commit;