| Message ID | 53705281b60285837905137f45fc8607012d2f19.1610107599.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fetch: implement support for atomic reference updates | expand |
Patrick Steinhardt <ps@pks.im> writes: > + /* > + * 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); > + } I do not want to see it done like this; formating what ought to come out identical with two separate mechanisms will lead to bugs under the road. Also what is the deal about "\n" vs "\\n"? Do we already have behaviour differences between two codepaths from the get-go? It would be much more preferrable to see this series go like so: [1/4] create append_fetch_head() that writes out to fetch_head->fp [1.5/4] convert append_fetch_head() to ALWAYS accumulate in fetch_head->buf, and ALWAYS flushes what is accumulated at the end. After these two patches are applied, there shouldn't be any behaviour change or change in the format in generated FETCH_HEAD. [2/4] and [3/4] stay the same [4/4] this step does not touch append_fetch_head() at all. It just changes the way how fetch_head->buf is flushed at the end (namely, under atomic mode and after seeing any failure, the accumulated output gets discarded without being written). I also thought briefly about an alternative approach to rewind and truncate the output to its original length upon seeing a failure under the atomic mode, but rejected it because the code gets hairy. I think "accumulate until we know we want to actually write them out" is a good approach to this issue. Thanks.
On Fri, Jan 08, 2021 at 04:05:53PM -0800, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > + /* > > + * 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); > > + } > > I do not want to see it done like this; formating what ought to come > out identical with two separate mechanisms will lead to bugs under > the road. > > Also what is the deal about "\n" vs "\\n"? Do we already have > behaviour differences between two codepaths from the get-go? Good point. I'll unify those code paths. > It would be much more preferrable to see this series go like so: > > [1/4] create append_fetch_head() that writes out to > fetch_head->fp > > [1.5/4] convert append_fetch_head() to ALWAYS accumulate in > fetch_head->buf, and ALWAYS flushes what is accumulated > at the end. This is a change I'm hesitant to make. The thing is that FETCH_HEAD is quite weird as the current design allows different `git fetch --append` processes to write to FETCH_HEAD at the same time. If we change to always accumulate first and flush once we're done, then we essentially have a change in behaviour as FETCH_HEADs aren't written in an interleaving fashion anymore, but only at the end. Also, if there is any unexpected error in between updating references which causes us to die, then we wouldn't have written the already updated references to FETCH_HEAD now. So I'm all in when it comes to merging formatting directives, even more so as I have missed the "\\n" weirdness. But for now, I'll keep the current flushing semantics in the non-atomic case. Please let me know if you're not convinced and I'll have another look for v4. > After these two patches are applied, there shouldn't be any > behaviour change or change in the format in generated FETCH_HEAD. > > [2/4] and [3/4] stay the same > > [4/4] this step does not touch append_fetch_head() at all. It > just changes the way how fetch_head->buf is flushed at the end > (namely, under atomic mode and after seeing any failure, the > accumulated output gets discarded without being written). > > I also thought briefly about an alternative approach to rewind and > truncate the output to its original length upon seeing a failure > under the atomic mode, but rejected it because the code gets hairy. > I think "accumulate until we know we want to actually write them out" > is a good approach to this issue. > > Thanks. Patrick
Patrick Steinhardt <ps@pks.im> writes: >> It would be much more preferrable to see this series go like so: >> >> [1/4] create append_fetch_head() that writes out to >> fetch_head->fp >> >> [1.5/4] convert append_fetch_head() to ALWAYS accumulate in >> fetch_head->buf, and ALWAYS flushes what is accumulated >> at the end. > > This is a change I'm hesitant to make. The thing is that FETCH_HEAD is > quite weird as the current design allows different `git fetch --append` > processes to write to FETCH_HEAD at the same time. Hmph, do you mean git fetch --append remote-a & git fetch --append remote-b or something else [*1*]? With the current write-out codepath, there is no guarantee that ... > > + 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); ... these stdio operations for a single record would result in a single atomic write() that would not compete with writes by other processes. So I wouldn't call "the current design allows ... at the same time." It has been broken for years and nobody complained. > If we change to > always accumulate first and flush once we're done, then we essentially > have a change in behaviour as FETCH_HEADs aren't written in an > interleaving fashion anymore, but only at the end. I view it a good thing, actually, for another reason [*2*], but that would take a follow-up fix that is much more involved, so let's not go there (yet). At least buffering a single line's worth of output in a buffer and writing it out in a single write() would get us much closer to solving the "mixed up output" from multiple processes problem the current code seems to already have. > Also, if there is any > unexpected error in between updating references which causes us to die, > then we wouldn't have written the already updated references to > FETCH_HEAD now. That one may be a valid concern. Thanks. [Footnote] *1* "git fetch --all/--multiple" was strictly serial operation to avoid such a mixed-up output problem, but perhaps we weren't careful enough when we introduced parallel fetch and broke it? *2* When fetching from a single remote, the code makes sure that a record that is not marked as 'not-for-merge' is listed first by reordering the records, but it does not work well when fetching from multiple remotes. Queuing all in the buffer before showing them would give us an opportunity to sort, but as I said, it takes coordination among the processes---instead of each process writing to FETCH_HEAD on their own, somebody has to collect the records from them, reorder as needed and write them all out. cf. https://lore.kernel.org/git/X8oL190Vl03B0cQ%2F@coredump.intra.peff.net/
On Mon, Jan 11, 2021 at 11:47:08AM -0800, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > >> It would be much more preferrable to see this series go like so: > >> > >> [1/4] create append_fetch_head() that writes out to > >> fetch_head->fp > >> > >> [1.5/4] convert append_fetch_head() to ALWAYS accumulate in > >> fetch_head->buf, and ALWAYS flushes what is accumulated > >> at the end. > > > > This is a change I'm hesitant to make. The thing is that FETCH_HEAD is > > quite weird as the current design allows different `git fetch --append` > > processes to write to FETCH_HEAD at the same time. > > Hmph, do you mean > > git fetch --append remote-a & git fetch --append remote-b > > or something else [*1*]? That's what I mean. > With the current write-out codepath, there is no guarantee that ... > > > > + 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); > > ... these stdio operations for a single record would result in a > single atomic write() that would not compete with writes by other > processes. So I wouldn't call "the current design allows ... at the > same time." It has been broken for years and nobody complained. Fair enough. I somehow blindly assumed that `git fetch --all`, which does invoke `git fetch --append`, did perform the fetch in parallel. If that's not the case: all the better. > > If we change to > > always accumulate first and flush once we're done, then we essentially > > have a change in behaviour as FETCH_HEADs aren't written in an > > interleaving fashion anymore, but only at the end. > > I view it a good thing, actually, for another reason [*2*], but that > would take a follow-up fix that is much more involved, so let's not > go there (yet). At least buffering a single line's worth of output > in a buffer and writing it out in a single write() would get us much > closer to solving the "mixed up output" from multiple processes > problem the current code seems to already have. The buffering of a single line is exactly what we're doing now in the non-atomic case. Previously there had been multiple writes, now we only call `strbuf_write()` once on the buffer for each reference. > > Also, if there is any > > unexpected error in between updating references which causes us to die, > > then we wouldn't have written the already updated references to > > FETCH_HEAD now. > > That one may be a valid concern. > > Thanks. Just to be explicit: are you fine with changes in this patch as-is? They don't solve concurrent writes to FETCH_HEAD, but they should make it easier to solve that if we ever need to. Patrick > > [Footnote] > > *1* "git fetch --all/--multiple" was strictly serial operation to > avoid such a mixed-up output problem, but perhaps we weren't > careful enough when we introduced parallel fetch and broke it? > > *2* When fetching from a single remote, the code makes sure that a > record that is not marked as 'not-for-merge' is listed first by > reordering the records, but it does not work well when fetching > from multiple remotes. Queuing all in the buffer before showing > them would give us an opportunity to sort, but as I said, it > takes coordination among the processes---instead of each process > writing to FETCH_HEAD on their own, somebody has to collect the > records from them, reorder as needed and write them all out. > > cf. https://lore.kernel.org/git/X8oL190Vl03B0cQ%2F@coredump.intra.peff.net/ > >
On Tue, Jan 12, 2021 at 01:22:40PM +0100, Patrick Steinhardt wrote: > On Mon, Jan 11, 2021 at 11:47:08AM -0800, Junio C Hamano wrote: > > Patrick Steinhardt <ps@pks.im> writes: [snip] > > > If we change to > > > always accumulate first and flush once we're done, then we essentially > > > have a change in behaviour as FETCH_HEADs aren't written in an > > > interleaving fashion anymore, but only at the end. > > > > I view it a good thing, actually, for another reason [*2*], but that > > would take a follow-up fix that is much more involved, so let's not > > go there (yet). At least buffering a single line's worth of output > > in a buffer and writing it out in a single write() would get us much > > closer to solving the "mixed up output" from multiple processes > > problem the current code seems to already have. > > The buffering of a single line is exactly what we're doing now in the > non-atomic case. Previously there had been multiple writes, now we only > call `strbuf_write()` once on the buffer for each reference. > > > > Also, if there is any > > > unexpected error in between updating references which causes us to die, > > > then we wouldn't have written the already updated references to > > > FETCH_HEAD now. > > > > That one may be a valid concern. > > > > Thanks. > > Just to be explicit: are you fine with changes in this patch as-is? They > don't solve concurrent writes to FETCH_HEAD, but they should make it > easier to solve that if we ever need to. > > Patrick Never mind, I forgot that I'm still replying on v2 of this patch series. Patrick
Patrick Steinhardt <ps@pks.im> writes: >> >> > > + 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); >> >> ... these stdio operations for a single record would result in a >> single atomic write() that would not compete with writes by other >> processes. So I wouldn't call "the current design allows ... at the >> same time." It has been broken for years and nobody complained. > > Fair enough. I somehow blindly assumed that `git fetch --all`, which > does invoke `git fetch --append`, did perform the fetch in parallel. If > that's not the case: all the better. The "--all" option started as "one after another, one at a time", but these days goes thru fetch_multiple() where we started to use run_processes_parallel() API without giving it much thought what it would do to the writing of FETCH_HEAD; since around d54dea77 (fetch: let --jobs=<n> parallelize --multiple, too, 2019-10-05), this codepath wrt FETCH_HEAD is utterly broken, I would have to say. > The buffering of a single line is exactly what we're doing now in the > non-atomic case. Previously there had been multiple writes, now we only > call `strbuf_write()` once on the buffer for each reference. Exactly.
diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt index 2bf77b46fd..07783deee3 100644 --- a/Documentation/fetch-options.txt +++ b/Documentation/fetch-options.txt @@ -7,6 +7,10 @@ existing contents of `.git/FETCH_HEAD`. Without this option old data in `.git/FETCH_HEAD` will be overwritten. +--atomic:: + Use an atomic transaction to update local refs. Either all refs are + updated, or on error, no refs are updated. + --depth=<depth>:: Limit fetching to the specified number of commits from the tip of each remote branch history. If fetching to a 'shallow' repository diff --git a/builtin/fetch.c b/builtin/fetch.c index 654e0bb520..e3446fc493 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -63,6 +63,7 @@ static int enable_auto_gc = 1; static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen; static int max_jobs = -1, submodule_fetch_jobs_config = -1; static int fetch_parallel_config = 1; +static int atomic_fetch; static enum transport_family family; static const char *depth; static const char *deepen_since; @@ -144,6 +145,8 @@ static struct option builtin_fetch_options[] = { N_("set upstream for git pull/fetch")), OPT_BOOL('a', "append", &append, N_("append to .git/FETCH_HEAD instead of overwriting")), + OPT_BOOL(0, "atomic", &atomic_fetch, + N_("use atomic transaction to update references")), 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), @@ -913,6 +916,7 @@ static int iterate_ref_map(void *cb_data, struct object_id *oid) struct fetch_head { FILE *fp; + struct strbuf buf; }; static int open_fetch_head(struct fetch_head *fetch_head) @@ -922,6 +926,7 @@ 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); @@ -938,19 +943,34 @@ 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); + /* + * 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); + } } static void commit_fetch_head(struct fetch_head *fetch_head) { - /* Nothing to commit yet. */ + if (!write_fetch_head || !atomic_fetch) + return; + strbuf_write(&fetch_head->buf, fetch_head->fp); } static void close_fetch_head(struct fetch_head *fetch_head) @@ -959,6 +979,7 @@ 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[] = @@ -976,7 +997,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, struct fetch_head fetch_head; struct commit *commit; int url_len, i, rc = 0; - struct strbuf note = STRBUF_INIT; + struct strbuf note = STRBUF_INIT, err = STRBUF_INIT; + struct ref_transaction *transaction = NULL; const char *what, *kind; struct ref *rm; char *url; @@ -1002,6 +1024,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, } } + if (atomic_fetch) { + transaction = ref_transaction_begin(&err); + if (!transaction) { + error("%s", err.buf); + goto abort; + } + } + prepare_format_display(ref_map); /* @@ -1089,7 +1119,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, strbuf_reset(¬e); if (ref) { - rc |= update_local_ref(ref, NULL, what, + rc |= update_local_ref(ref, transaction, what, rm, ¬e, summary_width); free(ref); } else if (write_fetch_head || dry_run) { @@ -1115,6 +1145,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, } } + if (!rc && transaction) { + rc = ref_transaction_commit(transaction, &err); + if (rc) { + error("%s", err.buf); + goto abort; + } + } + if (!rc) commit_fetch_head(&fetch_head); @@ -1134,6 +1172,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, abort: strbuf_release(¬e); + strbuf_release(&err); + ref_transaction_free(transaction); free(url); close_fetch_head(&fetch_head); return rc; @@ -1945,6 +1985,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) die(_("--filter can only be used with the remote " "configured in extensions.partialclone")); + if (atomic_fetch) + die(_("--atomic can only be used when fetching " + "from one remote")); + if (stdin_refspecs) die(_("--stdin can only be used when fetching " "from one remote")); diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 2013051a64..109d15be98 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -176,6 +176,174 @@ test_expect_success 'fetch --prune --tags with refspec prunes based on refspec' git rev-parse sometag ' +test_expect_success 'fetch --atomic works with a single branch' ' + test_when_finished "rm -rf \"$D\"/atomic" && + + cd "$D" && + git clone . atomic && + git branch atomic-branch && + oid=$(git rev-parse atomic-branch) && + echo "$oid" >expected && + + git -C atomic fetch --atomic origin && + git -C atomic rev-parse origin/atomic-branch >actual && + test_cmp expected actual && + test $oid = "$(git -C atomic rev-parse --verify FETCH_HEAD)" +' + +test_expect_success 'fetch --atomic works with multiple branches' ' + test_when_finished "rm -rf \"$D\"/atomic" && + + cd "$D" && + git clone . atomic && + git branch atomic-branch-1 && + git branch atomic-branch-2 && + git branch atomic-branch-3 && + git rev-parse refs/heads/atomic-branch-1 refs/heads/atomic-branch-2 refs/heads/atomic-branch-3 >actual && + + git -C atomic fetch --atomic origin && + git -C atomic rev-parse refs/remotes/origin/atomic-branch-1 refs/remotes/origin/atomic-branch-2 refs/remotes/origin/atomic-branch-3 >expected && + test_cmp expected actual +' + +test_expect_success 'fetch --atomic works with mixed branches and tags' ' + test_when_finished "rm -rf \"$D\"/atomic" && + + cd "$D" && + git clone . atomic && + git branch atomic-mixed-branch && + git tag atomic-mixed-tag && + git rev-parse refs/heads/atomic-mixed-branch refs/tags/atomic-mixed-tag >actual && + + git -C atomic fetch --tags --atomic origin && + git -C atomic rev-parse refs/remotes/origin/atomic-mixed-branch refs/tags/atomic-mixed-tag >expected && + test_cmp expected actual +' + +test_expect_success 'fetch --atomic prunes references' ' + test_when_finished "rm -rf \"$D\"/atomic" && + + cd "$D" && + git branch atomic-prune-delete && + git clone . atomic && + git branch --delete atomic-prune-delete && + git branch atomic-prune-create && + git rev-parse refs/heads/atomic-prune-create >actual && + + git -C atomic fetch --prune --atomic origin && + test_must_fail git -C atomic rev-parse refs/remotes/origin/atomic-prune-delete && + git -C atomic rev-parse refs/remotes/origin/atomic-prune-create >expected && + test_cmp expected actual +' + +test_expect_success 'fetch --atomic aborts with non-fast-forward update' ' + test_when_finished "rm -rf \"$D\"/atomic" && + + cd "$D" && + git branch atomic-non-ff && + git clone . atomic && + git rev-parse HEAD >actual && + + git branch atomic-new-branch && + parent_commit=$(git rev-parse atomic-non-ff~) && + git update-ref refs/heads/atomic-non-ff $parent_commit && + + test_must_fail git -C atomic fetch --atomic origin refs/heads/*:refs/remotes/origin/* && + test_must_fail git -C atomic rev-parse refs/remotes/origin/atomic-new-branch && + git -C atomic rev-parse refs/remotes/origin/atomic-non-ff >expected && + test_cmp expected actual && + test_must_be_empty atomic/.git/FETCH_HEAD +' + +test_expect_success 'fetch --atomic executes a single reference transaction only' ' + test_when_finished "rm -rf \"$D\"/atomic" && + + cd "$D" && + git clone . atomic && + git branch atomic-hooks-1 && + git branch atomic-hooks-2 && + head_oid=$(git rev-parse HEAD) && + + cat >expected <<-EOF && + prepared + $ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-1 + $ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-2 + committed + $ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-1 + $ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-2 + EOF + + rm -f atomic/actual && + write_script atomic/.git/hooks/reference-transaction <<-\EOF && + ( echo "$*" && cat ) >>actual + EOF + + git -C atomic fetch --atomic origin && + test_cmp expected atomic/actual +' + +test_expect_success 'fetch --atomic aborts all reference updates if hook aborts' ' + test_when_finished "rm -rf \"$D\"/atomic" && + + cd "$D" && + git clone . atomic && + git branch atomic-hooks-abort-1 && + git branch atomic-hooks-abort-2 && + git branch atomic-hooks-abort-3 && + git tag atomic-hooks-abort && + head_oid=$(git rev-parse HEAD) && + + cat >expected <<-EOF && + prepared + $ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-1 + $ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-2 + $ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-3 + $ZERO_OID $head_oid refs/tags/atomic-hooks-abort + aborted + $ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-1 + $ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-2 + $ZERO_OID $head_oid refs/remotes/origin/atomic-hooks-abort-3 + $ZERO_OID $head_oid refs/tags/atomic-hooks-abort + EOF + + rm -f atomic/actual && + write_script atomic/.git/hooks/reference-transaction <<-\EOF && + ( echo "$*" && cat ) >>actual + exit 1 + EOF + + git -C atomic for-each-ref >expected-refs && + test_must_fail git -C atomic fetch --tags --atomic origin && + git -C atomic for-each-ref >actual-refs && + test_cmp expected-refs actual-refs && + test_must_be_empty atomic/.git/FETCH_HEAD +' + +test_expect_success 'fetch --atomic --append appends to FETCH_HEAD' ' + test_when_finished "rm -rf \"$D\"/atomic" && + + cd "$D" && + git clone . atomic && + oid=$(git rev-parse HEAD) && + + git branch atomic-fetch-head-1 && + git -C atomic fetch --atomic origin atomic-fetch-head-1 && + test_line_count = 1 atomic/.git/FETCH_HEAD && + + git branch atomic-fetch-head-2 && + git -C atomic fetch --atomic --append origin atomic-fetch-head-2 && + test_line_count = 2 atomic/.git/FETCH_HEAD && + cp atomic/.git/FETCH_HEAD expected && + + write_script atomic/.git/hooks/reference-transaction <<-\EOF && + exit 1 + EOF + + git branch atomic-fetch-head-3 && + test_must_fail git -C atomic fetch --atomic --append origin atomic-fetch-head-3 && + test_cmp expected atomic/.git/FETCH_HEAD +' + test_expect_success '--refmap="" ignores configured refspec' ' cd "$TRASH_DIRECTORY" && git clone "$D" remote-refs &&
When executing a fetch, then git will currently allocate one reference transaction per reference update and directly commit it. This means that fetches are non-atomic: even if some of the reference updates fail, others may still succeed and modify local references. This is fine in many scenarios, but this strategy has its downsides. - The view of remote references may be inconsistent and may show a bastardized state of the remote repository. - Batching together updates may improve performance in certain scenarios. While the impact probably isn't as pronounced with loose references, the upcoming reftable backend may benefit as it needs to write less files in case the update is batched. - The reference-update hook is currently being executed twice per updated reference. While this doesn't matter when there is no such hook, we have seen severe performance regressions when doing a git-fetch(1) with reference-transaction hook when the remote repository has hundreds of thousands of references. Similar to `git push --atomic`, this commit thus introduces atomic fetches. Instead of allocating one reference transaction per updated reference, it causes us to only allocate a single transaction and commit it as soon as all updates were received. If locking of any reference fails, then we abort the complete transaction and don't update any reference, which gives us an all-or-nothing fetch. Note that this may not completely fix the first of above downsides, as the consistent view also depends on the server-side. If the server doesn't have a consistent view of its own references during the reference negotiation phase, then the client would get the same inconsistent view the server has. This is a separate problem though and, if it actually exists, can be fixed at a later point. This commit also changes the way we write FETCH_HEAD in case `--atomic` is passed. Instead of writing changes as we go, we need to accumulate all changes first and only commit them at the end when we know that all reference updates succeeded. Ideally, we'd just do so via a temporary file so that we don't need to carry all updates in-memory. This isn't trivially doable though considering the `--append` mode, where we do not truncate the file but simply append to it. And given that we support concurrent processes appending to FETCH_HEAD at the same time without any loss of data, seeding the temporary file with current contents of FETCH_HEAD initially and then doing a rename wouldn't work either. So this commit implements the simple strategy of buffering all changes and appending them to the file on commit. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- Documentation/fetch-options.txt | 4 + builtin/fetch.c | 66 ++++++++++--- t/t5510-fetch.sh | 168 ++++++++++++++++++++++++++++++++ 3 files changed, 227 insertions(+), 11 deletions(-)