git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Patrick Steinhardt <ps@pks.im>
To: git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>,
	Christian Couder <christian.couder@gmail.com>
Subject: [PATCH v3 0/5] fetch: implement support for atomic reference updates
Date: Mon, 11 Jan 2021 12:05:11 +0100	[thread overview]
Message-ID: <cover.1610362744.git.ps@pks.im> (raw)
In-Reply-To: <cover.1610027375.git.ps@pks.im>

[-- Attachment #1: Type: text/plain, Size: 12479 bytes --]

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(&note, "%s ", kind);
    + 				strbuf_addf(&note, "'%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(&note);
    + 			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;
-- 
2.30.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  parent reply	other threads:[~2021-01-11 11:08 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-07 13:51 [PATCH 0/2] fetch: implement support for atomic reference updates Patrick Steinhardt
2021-01-07 13:51 ` [PATCH 1/2] fetch: allow passing a transaction to `s_update_ref()` Patrick Steinhardt
2021-01-07 22:59   ` Junio C Hamano
2021-01-08  0:45     ` Christian Couder
2021-01-08  7:18       ` Junio C Hamano
2021-01-07 13:51 ` [PATCH 2/2] fetch: implement support for atomic reference updates Patrick Steinhardt
2021-01-08  0:19   ` Junio C Hamano
2021-01-08 12:11 ` [PATCH v2 0/4] " Patrick Steinhardt
2021-01-08 12:11   ` [PATCH v2 1/4] fetch: extract writing to FETCH_HEAD Patrick Steinhardt
2021-01-08 23:40     ` Junio C Hamano
2021-01-11 10:26       ` Patrick Steinhardt
2021-01-11 19:24         ` Junio C Hamano
2021-01-08 12:11   ` [PATCH v2 2/4] fetch: refactor `s_update_ref` to use common exit path Patrick Steinhardt
2021-01-08 23:50     ` Junio C Hamano
2021-01-11 10:28       ` Patrick Steinhardt
2021-01-08 12:11   ` [PATCH v2 3/4] fetch: allow passing a transaction to `s_update_ref()` Patrick Steinhardt
2021-01-08 23:53     ` Junio C Hamano
2021-01-08 12:11   ` [PATCH v2 4/4] fetch: implement support for atomic reference updates Patrick Steinhardt
2021-01-09  0:05     ` Junio C Hamano
2021-01-11 10:42       ` Patrick Steinhardt
2021-01-11 19:47         ` Junio C Hamano
2021-01-12 12:22           ` Patrick Steinhardt
2021-01-12 12:29             ` Patrick Steinhardt
2021-01-12 19:19             ` Junio C Hamano
2021-01-11 11:05 ` Patrick Steinhardt [this message]
2021-01-11 11:05   ` [PATCH v3 1/5] fetch: extract writing to FETCH_HEAD Patrick Steinhardt
2021-01-11 23:16     ` Junio C Hamano
2021-01-11 11:05   ` [PATCH v3 2/5] fetch: use strbuf to format FETCH_HEAD updates Patrick Steinhardt
2021-01-11 11:10     ` Patrick Steinhardt
2021-01-11 11:17     ` Christian Couder
2021-01-11 23:21     ` Junio C Hamano
2021-01-11 11:05   ` [PATCH v3 3/5] fetch: refactor `s_update_ref` to use common exit path Patrick Steinhardt
2021-01-11 11:05   ` [PATCH v3 4/5] fetch: allow passing a transaction to `s_update_ref()` Patrick Steinhardt
2021-01-11 11:05   ` [PATCH v3 5/5] fetch: implement support for atomic reference updates Patrick Steinhardt
2021-01-12  0:04   ` [PATCH v3 0/5] " Junio C Hamano
2021-01-12 12:27 ` [PATCH v4 " Patrick Steinhardt
2021-01-12 12:27   ` [PATCH v4 1/5] fetch: extract writing to FETCH_HEAD Patrick Steinhardt
2021-01-12 12:27   ` [PATCH v4 2/5] fetch: use strbuf to format FETCH_HEAD updates Patrick Steinhardt
2021-01-12 12:27   ` [PATCH v4 3/5] fetch: refactor `s_update_ref` to use common exit path Patrick Steinhardt
2021-01-12 12:27   ` [PATCH v4 4/5] fetch: allow passing a transaction to `s_update_ref()` Patrick Steinhardt
2021-01-12 12:27   ` [PATCH v4 5/5] fetch: implement support for atomic reference updates Patrick Steinhardt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cover.1610362744.git.ps@pks.im \
    --to=ps@pks.im \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).