| Message ID | cover.1610362744.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | fetch: implement support for atomic reference updates | expand |
Patrick Steinhardt <ps@pks.im> writes: > 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! Looking good. I left a few comments on remaining or newly introduced issues, but IIRC all of them were rather minor. Thanks, will replace.
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;