| Message ID | 1f26051eff8b7c18bb7114803454611272f84e19.1717402363.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | 8603dc702083a250f68939fa854f69f74c7d4ff4 |
| Headers | show |
| Series | refs: ref storage migrations | expand |
Patrick Steinhardt <ps@pks.im> writes: [snip] > diff --git a/refs.c b/refs.c > index 9b112b0527..f7c7765d23 100644 > --- a/refs.c > +++ b/refs.c > @@ -2570,3 +2570,308 @@ int ref_update_check_old_target(const char *referent, struct ref_update *update, > referent, update->old_target); > return -1; > } > + > +struct migration_data { > + struct ref_store *old_refs; > + struct ref_transaction *transaction; > + struct strbuf *errbuf; > +}; > + > +static int migrate_one_ref(const char *refname, const struct object_id *oid, > + int flags, void *cb_data) > +{ > + struct migration_data *data = cb_data; > + struct strbuf symref_target = STRBUF_INIT; > + int ret; > + > + if (flags & REF_ISSYMREF) { > + ret = refs_read_symbolic_ref(data->old_refs, refname, &symref_target); > + if (ret < 0) > + goto done; > + > + ret = ref_transaction_update(data->transaction, refname, NULL, null_oid(), > + symref_target.buf, NULL, > + REF_SKIP_CREATE_REFLOG | REF_NO_DEREF, NULL, data->errbuf); > + if (ret < 0) > + goto done; > + } else { > + ret = ref_transaction_create(data->transaction, refname, oid, > + REF_SKIP_CREATE_REFLOG | REF_SKIP_OID_VERIFICATION, > + NULL, data->errbuf); > + if (ret < 0) > + goto done; > + } I was a little perplexed about the first scenario being `ref_transaction_update` and the second being `ref_transaction_create`, I then realized that this is because the latter doesn't support creating symrefs yet (changes in my series kn/update-ref-symref), makes sense to do it this way. [snip] > +int repo_migrate_ref_storage_format(struct repository *repo, > + enum ref_storage_format format, > + unsigned int flags, > + struct strbuf *errbuf) > +{ > + struct ref_store *old_refs = NULL, *new_refs = NULL; > + struct ref_transaction *transaction = NULL; > + struct strbuf buf = STRBUF_INIT; > + struct migration_data data; > + size_t reflog_count = 0; > + char *new_gitdir = NULL; > + int did_migrate_refs = 0; > + int ret; > + > + old_refs = get_main_ref_store(repo); Should we add a check to ensure the `old_refs->repo->ref_storage_format` and `format` are different? > + > + /* > + * We do not have any interfaces that would allow us to write many > + * reflog entries. Once we have them we can remove this restriction. > + */ > + if (refs_for_each_reflog(old_refs, count_reflogs, &reflog_count) < 0) { > + strbuf_addstr(errbuf, "cannot count reflogs"); > + ret = -1; > + goto done; > + } > + if (reflog_count) { > + strbuf_addstr(errbuf, "migrating reflogs is not supported yet"); > + ret = -1; > + goto done; > + } Isn't this restrictive? It would be nice to perhaps say "git refs migrate --ignore-reflogs", which could make it possible to not care about reflogs. But maybe that can be part of a follow up. > + /* > + * Worktrees complicate the migration because every worktree has a > + * separate ref storage. While it should be feasible to implement, this > + * is pushed out to a future iteration. > + * > + * TODO: we should really be passing the caller-provided repository to > + * `has_worktrees()`, but our worktree subsystem doesn't yet support > + * that. > + */ > + if (has_worktrees()) { > + strbuf_addstr(errbuf, "migrating repositories with worktrees is not supported yet"); > + ret = -1; > + goto done; > + } > + Same as above. > + /* > + * The overall logic looks like this: > + * > + * 1. Set up a new temporary directory and initialize it with the new > + * format. This is where all refs will be migrated into. > + * > + * 2. Enumerate all refs and write them into the new ref storage. > + * This operation is safe as we do not yet modify the main > + * repository. > + * > + * 3. If we're in dry-run mode then we are done and can hand over the > + * directory to the caller for inspection. If not, we now start > + * with the destructive part. > + * > + * 4. Delete the old ref storage from disk. As we have a copy of refs > + * in the new ref storage it's okay(ish) if we now get interrupted > + * as there is an equivalent copy of all refs available. > + * > + * 5. Move the new ref storage files into place. > + * > + * 6. Change the repository format to the new ref format. > + */ > + strbuf_addf(&buf, "%s/%s", old_refs->gitdir, "ref_migration.XXXXXX"); > + new_gitdir = mkdtemp(xstrdup(buf.buf)); > + if (!new_gitdir) { > + strbuf_addf(errbuf, "cannot create migration directory: %s", > + strerror(errno)); > + ret = -1; > + goto done; > + } > + > + new_refs = ref_store_init(repo, format, new_gitdir, > + REF_STORE_ALL_CAPS); > + ret = ref_store_create_on_disk(new_refs, 0, errbuf); > + if (ret < 0) > + goto done; > + > + transaction = ref_store_transaction_begin(new_refs, errbuf); > + if (!transaction) > + goto done; > + > + data.old_refs = old_refs; > + data.transaction = transaction; > + data.errbuf = errbuf; > + > + /* > + * We need to use the internal `do_for_each_ref()` here so that we can > + * also include broken refs and symrefs. These would otherwise be > + * skipped silently. > + * > + * Ideally, we would do this call while locking the old ref storage > + * such that there cannot be any concurrent modifications. We do not > + * have the infra for that though, and the "files" backend does not > + * allow for a central lock due to its design. It's thus on the user to > + * ensure that there are no concurrent writes. > + */ > + ret = do_for_each_ref(old_refs, "", NULL, migrate_one_ref, 0, > + DO_FOR_EACH_INCLUDE_ROOT_REFS | DO_FOR_EACH_INCLUDE_BROKEN, > + &data); > + if (ret < 0) > + goto done; > + > + /* > + * TODO: we might want to migrate to `initial_ref_transaction_commit()` > + * here, which is more efficient for the files backend because it would > + * write new refs into the packed-refs file directly. At this point, > + * the files backend doesn't handle pseudo-refs and symrefs correctly > + * though, so this requires some more work. > + */ > + ret = ref_transaction_commit(transaction, errbuf); > + if (ret < 0) > + goto done; > + did_migrate_refs = 1; > + > + if (flags & REPO_MIGRATE_REF_STORAGE_FORMAT_DRYRUN) { > + printf(_("Finished dry-run migration of refs, " > + "the result can be found at '%s'\n"), new_gitdir); > + ret = 0; > + goto done; > + } > + > + /* > + * Until now we were in the non-destructive phase, where we only > + * populated the new ref store. From hereon though we are about > + * to get hands by deleting the old ref store and then moving > + * the new one into place. > + * > + * Assuming that there were no concurrent writes, the new ref > + * store should have all information. So if we fail from hereon > + * we may be in an in-between state, but it would still be able > + * to recover by manually moving remaining files from the > + * temporary migration directory into place. > + */ This also means that the recovery would only be possible into the new format. Makes sense. [snip]
On Tue, Jun 04, 2024 at 03:28:07PM +0000, Karthik Nayak wrote: > Patrick Steinhardt <ps@pks.im> writes: > > +int repo_migrate_ref_storage_format(struct repository *repo, > > + enum ref_storage_format format, > > + unsigned int flags, > > + struct strbuf *errbuf) > > +{ > > + struct ref_store *old_refs = NULL, *new_refs = NULL; > > + struct ref_transaction *transaction = NULL; > > + struct strbuf buf = STRBUF_INIT; > > + struct migration_data data; > > + size_t reflog_count = 0; > > + char *new_gitdir = NULL; > > + int did_migrate_refs = 0; > > + int ret; > > + > > + old_refs = get_main_ref_store(repo); > > Should we add a check to ensure the `old_refs->repo->ref_storage_format` > and `format` are different? Hm, yeah. We do have that check in git-refs(1), but having it here as well wouldn't hurt. As the patch series has been merged to `next`, I'll leave this for a future iteration though. Probably the one where I implement support for migrating reflogs. > > + > > + /* > > + * We do not have any interfaces that would allow us to write many > > + * reflog entries. Once we have them we can remove this restriction. > > + */ > > + if (refs_for_each_reflog(old_refs, count_reflogs, &reflog_count) < 0) { > > + strbuf_addstr(errbuf, "cannot count reflogs"); > > + ret = -1; > > + goto done; > > + } > > + if (reflog_count) { > > + strbuf_addstr(errbuf, "migrating reflogs is not supported yet"); > > + ret = -1; > > + goto done; > > + } > > Isn't this restrictive? It would be nice to perhaps say "git refs > migrate --ignore-reflogs", which could make it possible to not care > about reflogs. But maybe that can be part of a follow up. Oh yeah, it is. In this case it would be possible to add a flag to override this check, because the result would be that we simply discard all reflogs altogether. But I don't think adding such a flag makes sense, because I'd much rather want to remove the underlying restriction itself and start handling the migration of reflogs. > > + /* > > + * Worktrees complicate the migration because every worktree has a > > + * separate ref storage. While it should be feasible to implement, this > > + * is pushed out to a future iteration. > > + * > > + * TODO: we should really be passing the caller-provided repository to > > + * `has_worktrees()`, but our worktree subsystem doesn't yet support > > + * that. > > + */ > > + if (has_worktrees()) { > > + strbuf_addstr(errbuf, "migrating repositories with worktrees is not supported yet"); > > + ret = -1; > > + goto done; > > + } > > + > > Same as above. Allowing users to override this would leave them with broken worktree refdbs, so I don't think we should add such a flag, either. Patrick
On Mon, Jun 03, 2024 at 11:31:00AM +0200, Patrick Steinhardt wrote: > +int repo_migrate_ref_storage_format(struct repository *repo, > + enum ref_storage_format format, > + unsigned int flags, > + struct strbuf *errbuf) > +{ > [...] > + new_gitdir = mkdtemp(xstrdup(buf.buf)); > + if (!new_gitdir) { > + strbuf_addf(errbuf, "cannot create migration directory: %s", > + strerror(errno)); > + ret = -1; > + goto done; > + } Coverity complains here of a leak of the xstrdup(). The return from mkdtemp() should generally point to the same buffer we passed in, but if it sees an error it will return NULL and the new heap buffer will be lost. Probably unlikely, but since you are on a leak-checking kick, I thought I'd mention it. ;) Since you have a writable strbuf already, maybe: new_gitdir = mkdtemp(buf.buf); if (!new_gitdir) ... new_gitdir = strbuf_detach(&buf, NULL); /* same pointer, but now we own it */ Or since "buf" is not used for anything else, we could just leave it attached to the strbuf. And probably give it a better name. Maybe: diff --git a/refs.c b/refs.c index 166b6f269e..9a6655abee 100644 --- a/refs.c +++ b/refs.c @@ -2726,10 +2726,9 @@ int repo_migrate_ref_storage_format(struct repository *repo, { struct ref_store *old_refs = NULL, *new_refs = NULL; struct ref_transaction *transaction = NULL; - struct strbuf buf = STRBUF_INIT; + struct strbuf new_gitdir = STRBUF_INIT; struct migration_data data; size_t reflog_count = 0; - char *new_gitdir = NULL; int did_migrate_refs = 0; int ret; @@ -2787,16 +2786,15 @@ int repo_migrate_ref_storage_format(struct repository *repo, * * 6. Change the repository format to the new ref format. */ - strbuf_addf(&buf, "%s/%s", old_refs->gitdir, "ref_migration.XXXXXX"); - new_gitdir = mkdtemp(xstrdup(buf.buf)); - if (!new_gitdir) { + strbuf_addf(&new_gitdir, "%s/%s", old_refs->gitdir, "ref_migration.XXXXXX"); + if (!mkdtemp(new_gitdir.buf)) { strbuf_addf(errbuf, "cannot create migration directory: %s", strerror(errno)); ret = -1; goto done; } - new_refs = ref_store_init(repo, format, new_gitdir, + new_refs = ref_store_init(repo, format, new_gitdir.buf, REF_STORE_ALL_CAPS); ret = ref_store_create_on_disk(new_refs, 0, errbuf); if (ret < 0) @@ -2841,7 +2839,7 @@ int repo_migrate_ref_storage_format(struct repository *repo, if (flags & REPO_MIGRATE_REF_STORAGE_FORMAT_DRYRUN) { printf(_("Finished dry-run migration of refs, " - "the result can be found at '%s'\n"), new_gitdir); + "the result can be found at '%s'\n"), new_gitdir.buf); ret = 0; goto done; } @@ -2862,13 +2860,13 @@ int repo_migrate_ref_storage_format(struct repository *repo, if (ret < 0) goto done; - ret = move_files(new_gitdir, old_refs->gitdir, errbuf); + ret = move_files(new_gitdir.buf, old_refs->gitdir, errbuf); if (ret < 0) goto done; - if (rmdir(new_gitdir) < 0) + if (rmdir(new_gitdir.buf) < 0) warning_errno(_("could not remove temporary migration directory '%s'"), - new_gitdir); + new_gitdir.buf); /* * We have migrated the repository, so we now need to adjust the @@ -2888,13 +2886,12 @@ int repo_migrate_ref_storage_format(struct repository *repo, if (ret && did_migrate_refs) { strbuf_complete(errbuf, '\n'); strbuf_addf(errbuf, _("migrated refs can be found at '%s'"), - new_gitdir); + new_gitdir.buf); } if (ret && new_refs) ref_store_release(new_refs); ref_transaction_free(transaction); - strbuf_release(&buf); - free(new_gitdir); + strbuf_release(&new_gitdir); return ret; }
Jeff King <peff@peff.net> writes: > Coverity complains here of a leak of the xstrdup(). The return from > mkdtemp() should generally point to the same buffer we passed in, but if > it sees an error it will return NULL and the new heap buffer will be > lost. > > Probably unlikely, but since you are on a leak-checking kick, I thought > I'd mention it. ;) > > Since you have a writable strbuf already, maybe: > > new_gitdir = mkdtemp(buf.buf); > if (!new_gitdir) > ... > new_gitdir = strbuf_detach(&buf, NULL); /* same pointer, but now we own it */ > > Or since "buf" is not used for anything else, we could just leave it > attached to the strbuf. And probably give it a better name. Maybe: > ... Hmph, I think this is the second one we want to amend on the topic and it seems that I merged it a bit too prematurely. I do not mind reverting the topic out of 'next' and actually would prefer replacing it with a corrected version, which would allow us to merge the clean copy to the next release. Thanks.
On Wed, Jun 05, 2024 at 06:03:18AM -0400, Jeff King wrote: > On Mon, Jun 03, 2024 at 11:31:00AM +0200, Patrick Steinhardt wrote: > > > +int repo_migrate_ref_storage_format(struct repository *repo, > > + enum ref_storage_format format, > > + unsigned int flags, > > + struct strbuf *errbuf) > > +{ > > [...] > > + new_gitdir = mkdtemp(xstrdup(buf.buf)); > > + if (!new_gitdir) { > > + strbuf_addf(errbuf, "cannot create migration directory: %s", > > + strerror(errno)); > > + ret = -1; > > + goto done; > > + } > > Coverity complains here of a leak of the xstrdup(). The return from > mkdtemp() should generally point to the same buffer we passed in, but if > it sees an error it will return NULL and the new heap buffer will be > lost. > > Probably unlikely, but since you are on a leak-checking kick, I thought > I'd mention it. ;) > > Since you have a writable strbuf already, maybe: > > new_gitdir = mkdtemp(buf.buf); > if (!new_gitdir) > ... > new_gitdir = strbuf_detach(&buf, NULL); /* same pointer, but now we own it */ > > Or since "buf" is not used for anything else, we could just leave it > attached to the strbuf. And probably give it a better name. Maybe: I like that version, thanks! Patrick
On Wed, Jun 05, 2024 at 09:59:14AM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > Coverity complains here of a leak of the xstrdup(). The return from > > mkdtemp() should generally point to the same buffer we passed in, but if > > it sees an error it will return NULL and the new heap buffer will be > > lost. > > > > Probably unlikely, but since you are on a leak-checking kick, I thought > > I'd mention it. ;) > > > > Since you have a writable strbuf already, maybe: > > > > new_gitdir = mkdtemp(buf.buf); > > if (!new_gitdir) > > ... > > new_gitdir = strbuf_detach(&buf, NULL); /* same pointer, but now we own it */ > > > > Or since "buf" is not used for anything else, we could just leave it > > attached to the strbuf. And probably give it a better name. Maybe: > > ... > > Hmph, I think this is the second one we want to amend on the topic > and it seems that I merged it a bit too prematurely. > > I do not mind reverting the topic out of 'next' and actually would > prefer replacing it with a corrected version, which would allow us > to merge the clean copy to the next release. I wouldn't exactly say prematurely, given that it likely wouldn't have gotten a review without the merge because it was spurred by Coverity :) I really wish that the Coverity tooling was available to run at will and locally in our pipelines so that we can stop reacting to it, but instead address whatever it flags _before_ the code hits the target branch. But, well, that's not how Coverity works. Anyway, I'll send a revised version in a bit. Thanks for your extra review, Peff! Patrick
On Thu, Jun 06, 2024 at 06:51:15AM +0200, Patrick Steinhardt wrote: > > I do not mind reverting the topic out of 'next' and actually would > > prefer replacing it with a corrected version, which would allow us > > to merge the clean copy to the next release. > > I wouldn't exactly say prematurely, given that it likely wouldn't have > gotten a review without the merge because it was spurred by Coverity :) > I really wish that the Coverity tooling was available to run at will and > locally in our pipelines so that we can stop reacting to it, but instead > address whatever it flags _before_ the code hits the target branch. But, > well, that's not how Coverity works. Yeah, I'd agree with the analysis here. While somebody _could_ have found these by inspection, in practice it was the merge to next that led me to them. It may imply that we should be running Coverity earlier, though. In my fork I trigger Coverity runs based on my personal integration branch, which is based on next plus a list of non-garbage topics I'm working on. So I get to see (and fix) my own bugs before anybody else does. But I don't see other people's bugs until they're in next. I could try running against "seen", but it's a minor hassle. I don't otherwise touch that branch at all, and I certainly don't want my daily driver built off of it. Plus it sometimes has test failures or other hiccups, and I already get enough false positive noise from Coverity (so even if I ran it, I'd be unlikely to spend much time digging into failures). So what I'd suggest is that you try setting up the Coverity workflow yourself. There are rough instructions in the GitHub workflow file, and I imagine you'd be able to port it to GitLab. Coverity does do SSO login with GitHub, but I don't think it's relevant once you've got an account there. The opaque token they give you is all you need to upload a build. -Peff
Jeff King <peff@peff.net> writes: > In my fork I trigger Coverity runs based on my personal integration > branch, which is based on next plus a list of non-garbage topics I'm > working on. So I get to see (and fix) my own bugs before anybody else > does. But I don't see other people's bugs until they're in next. I am on a mostly same boat but doing a bit better ;-) in that my daily driver is a point marked as 'jch', somewhere between 'next' and 'seen', that appears on "git log --first-parent --oneline master..seen", and this serves as a very small way [*] to see breakages by others before they hit 'next'. Side note: This does not work as well as I should, because my use cases are too narrow to prevent all breakage from getting into 'next'. > I could try running against "seen", but it's a minor hassle. I don't > otherwise touch that branch at all, and I certainly don't want my daily > driver built off of it. Plus it sometimes has test failures or other > hiccups, and I already get enough false positive noise from Coverity (so > even if I ran it, I'd be unlikely to spend much time digging into > failures). I'd recommend against anybody using "seen" as their daily driver. Being in 'seen' merely is "I happened to have seen it floating on the list", and the only guarantee I can give them is that I at least have read sections of their code that happened to conflict with other topics more carefully than just giving a casual reading over them. If CI is broken for more than a few days for 'seen', I may look at them a bit more carefully, only to see which one is causing the breakage. But that is not necessarily to fix the breakage myself but to just eject it out of 'seen' ;-).
On Thu, Jun 06, 2024 at 08:41:10AM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > In my fork I trigger Coverity runs based on my personal integration > > branch, which is based on next plus a list of non-garbage topics I'm > > working on. So I get to see (and fix) my own bugs before anybody else > > does. But I don't see other people's bugs until they're in next. > > I am on a mostly same boat but doing a bit better ;-) in that my > daily driver is a point marked as 'jch', somewhere between 'next' > and 'seen', that appears on "git log --first-parent --oneline > master..seen", and this serves as a very small way [*] to see > breakages by others before they hit 'next'. > > Side note: This does not work as well as I should, because my > use cases are too narrow to prevent all breakage from getting > into 'next'. Possibly I should base my daily driver branch on "jch". Like you, there are many parts of the code I won't exercise day to day. But it would mean I'd do more testing (and CI) on those topics. The big question is whether that would introduce a bunch of noise from not-quite-ready topics being merged to jch. It depends how careful / conservative you are. :) -Peff
Jeff King <peff@peff.net> writes: > Possibly I should base my daily driver branch on "jch". Like you, there > are many parts of the code I won't exercise day to day. But it would > mean I'd do more testing (and CI) on those topics. The big question is > whether that would introduce a bunch of noise from not-quite-ready > topics being merged to jch. It depends how careful / conservative you > are. :) I am not all that conservative. Especially with the parts of the system that I do not exercise myself.
diff --git a/refs.c b/refs.c index 9b112b0527..f7c7765d23 100644 --- a/refs.c +++ b/refs.c @@ -2570,3 +2570,308 @@ int ref_update_check_old_target(const char *referent, struct ref_update *update, referent, update->old_target); return -1; } + +struct migration_data { + struct ref_store *old_refs; + struct ref_transaction *transaction; + struct strbuf *errbuf; +}; + +static int migrate_one_ref(const char *refname, const struct object_id *oid, + int flags, void *cb_data) +{ + struct migration_data *data = cb_data; + struct strbuf symref_target = STRBUF_INIT; + int ret; + + if (flags & REF_ISSYMREF) { + ret = refs_read_symbolic_ref(data->old_refs, refname, &symref_target); + if (ret < 0) + goto done; + + ret = ref_transaction_update(data->transaction, refname, NULL, null_oid(), + symref_target.buf, NULL, + REF_SKIP_CREATE_REFLOG | REF_NO_DEREF, NULL, data->errbuf); + if (ret < 0) + goto done; + } else { + ret = ref_transaction_create(data->transaction, refname, oid, + REF_SKIP_CREATE_REFLOG | REF_SKIP_OID_VERIFICATION, + NULL, data->errbuf); + if (ret < 0) + goto done; + } + +done: + strbuf_release(&symref_target); + return ret; +} + +static int move_files(const char *from_path, const char *to_path, struct strbuf *errbuf) +{ + struct strbuf from_buf = STRBUF_INIT, to_buf = STRBUF_INIT; + size_t from_len, to_len; + DIR *from_dir; + int ret; + + from_dir = opendir(from_path); + if (!from_dir) { + strbuf_addf(errbuf, "could not open source directory '%s': %s", + from_path, strerror(errno)); + ret = -1; + goto done; + } + + strbuf_addstr(&from_buf, from_path); + strbuf_complete(&from_buf, '/'); + from_len = from_buf.len; + + strbuf_addstr(&to_buf, to_path); + strbuf_complete(&to_buf, '/'); + to_len = to_buf.len; + + while (1) { + struct dirent *ent; + + errno = 0; + ent = readdir(from_dir); + if (!ent) + break; + + if (!strcmp(ent->d_name, ".") || + !strcmp(ent->d_name, "..")) + continue; + + strbuf_setlen(&from_buf, from_len); + strbuf_addstr(&from_buf, ent->d_name); + + strbuf_setlen(&to_buf, to_len); + strbuf_addstr(&to_buf, ent->d_name); + + ret = rename(from_buf.buf, to_buf.buf); + if (ret < 0) { + strbuf_addf(errbuf, "could not link file '%s' to '%s': %s", + from_buf.buf, to_buf.buf, strerror(errno)); + goto done; + } + } + + if (errno) { + strbuf_addf(errbuf, "could not read entry from directory '%s': %s", + from_path, strerror(errno)); + ret = -1; + goto done; + } + + ret = 0; + +done: + strbuf_release(&from_buf); + strbuf_release(&to_buf); + if (from_dir) + closedir(from_dir); + return ret; +} + +static int count_reflogs(const char *reflog UNUSED, void *payload) +{ + size_t *reflog_count = payload; + (*reflog_count)++; + return 0; +} + +static int has_worktrees(void) +{ + struct worktree **worktrees = get_worktrees(); + int ret = 0; + size_t i; + + for (i = 0; worktrees[i]; i++) { + if (is_main_worktree(worktrees[i])) + continue; + ret = 1; + } + + free_worktrees(worktrees); + return ret; +} + +int repo_migrate_ref_storage_format(struct repository *repo, + enum ref_storage_format format, + unsigned int flags, + struct strbuf *errbuf) +{ + struct ref_store *old_refs = NULL, *new_refs = NULL; + struct ref_transaction *transaction = NULL; + struct strbuf buf = STRBUF_INIT; + struct migration_data data; + size_t reflog_count = 0; + char *new_gitdir = NULL; + int did_migrate_refs = 0; + int ret; + + old_refs = get_main_ref_store(repo); + + /* + * We do not have any interfaces that would allow us to write many + * reflog entries. Once we have them we can remove this restriction. + */ + if (refs_for_each_reflog(old_refs, count_reflogs, &reflog_count) < 0) { + strbuf_addstr(errbuf, "cannot count reflogs"); + ret = -1; + goto done; + } + if (reflog_count) { + strbuf_addstr(errbuf, "migrating reflogs is not supported yet"); + ret = -1; + goto done; + } + + /* + * Worktrees complicate the migration because every worktree has a + * separate ref storage. While it should be feasible to implement, this + * is pushed out to a future iteration. + * + * TODO: we should really be passing the caller-provided repository to + * `has_worktrees()`, but our worktree subsystem doesn't yet support + * that. + */ + if (has_worktrees()) { + strbuf_addstr(errbuf, "migrating repositories with worktrees is not supported yet"); + ret = -1; + goto done; + } + + /* + * The overall logic looks like this: + * + * 1. Set up a new temporary directory and initialize it with the new + * format. This is where all refs will be migrated into. + * + * 2. Enumerate all refs and write them into the new ref storage. + * This operation is safe as we do not yet modify the main + * repository. + * + * 3. If we're in dry-run mode then we are done and can hand over the + * directory to the caller for inspection. If not, we now start + * with the destructive part. + * + * 4. Delete the old ref storage from disk. As we have a copy of refs + * in the new ref storage it's okay(ish) if we now get interrupted + * as there is an equivalent copy of all refs available. + * + * 5. Move the new ref storage files into place. + * + * 6. Change the repository format to the new ref format. + */ + strbuf_addf(&buf, "%s/%s", old_refs->gitdir, "ref_migration.XXXXXX"); + new_gitdir = mkdtemp(xstrdup(buf.buf)); + if (!new_gitdir) { + strbuf_addf(errbuf, "cannot create migration directory: %s", + strerror(errno)); + ret = -1; + goto done; + } + + new_refs = ref_store_init(repo, format, new_gitdir, + REF_STORE_ALL_CAPS); + ret = ref_store_create_on_disk(new_refs, 0, errbuf); + if (ret < 0) + goto done; + + transaction = ref_store_transaction_begin(new_refs, errbuf); + if (!transaction) + goto done; + + data.old_refs = old_refs; + data.transaction = transaction; + data.errbuf = errbuf; + + /* + * We need to use the internal `do_for_each_ref()` here so that we can + * also include broken refs and symrefs. These would otherwise be + * skipped silently. + * + * Ideally, we would do this call while locking the old ref storage + * such that there cannot be any concurrent modifications. We do not + * have the infra for that though, and the "files" backend does not + * allow for a central lock due to its design. It's thus on the user to + * ensure that there are no concurrent writes. + */ + ret = do_for_each_ref(old_refs, "", NULL, migrate_one_ref, 0, + DO_FOR_EACH_INCLUDE_ROOT_REFS | DO_FOR_EACH_INCLUDE_BROKEN, + &data); + if (ret < 0) + goto done; + + /* + * TODO: we might want to migrate to `initial_ref_transaction_commit()` + * here, which is more efficient for the files backend because it would + * write new refs into the packed-refs file directly. At this point, + * the files backend doesn't handle pseudo-refs and symrefs correctly + * though, so this requires some more work. + */ + ret = ref_transaction_commit(transaction, errbuf); + if (ret < 0) + goto done; + did_migrate_refs = 1; + + if (flags & REPO_MIGRATE_REF_STORAGE_FORMAT_DRYRUN) { + printf(_("Finished dry-run migration of refs, " + "the result can be found at '%s'\n"), new_gitdir); + ret = 0; + goto done; + } + + /* + * Until now we were in the non-destructive phase, where we only + * populated the new ref store. From hereon though we are about + * to get hands by deleting the old ref store and then moving + * the new one into place. + * + * Assuming that there were no concurrent writes, the new ref + * store should have all information. So if we fail from hereon + * we may be in an in-between state, but it would still be able + * to recover by manually moving remaining files from the + * temporary migration directory into place. + */ + ret = ref_store_remove_on_disk(old_refs, errbuf); + if (ret < 0) + goto done; + + ret = move_files(new_gitdir, old_refs->gitdir, errbuf); + if (ret < 0) + goto done; + + if (rmdir(new_gitdir) < 0) + warning_errno(_("could not remove temporary migration directory '%s'"), + new_gitdir); + + /* + * We have migrated the repository, so we now need to adjust the + * repository format so that clients will use the new ref store. + * We also need to swap out the repository's main ref store. + */ + initialize_repository_version(hash_algo_by_ptr(repo->hash_algo), format, 1); + + free(new_refs->gitdir); + new_refs->gitdir = xstrdup(old_refs->gitdir); + repo->refs_private = new_refs; + ref_store_release(old_refs); + + ret = 0; + +done: + if (ret && did_migrate_refs) { + strbuf_complete(errbuf, '\n'); + strbuf_addf(errbuf, _("migrated refs can be found at '%s'"), + new_gitdir); + } + + if (ret && new_refs) + ref_store_release(new_refs); + ref_transaction_free(transaction); + strbuf_release(&buf); + free(new_gitdir); + return ret; +} diff --git a/refs.h b/refs.h index 61ee7b7a15..76d25df4de 100644 --- a/refs.h +++ b/refs.h @@ -1070,6 +1070,24 @@ int is_root_ref(const char *refname); */ int is_pseudo_ref(const char *refname); +/* + * The following flags can be passed to `repo_migrate_ref_storage_format()`: + * + * - REPO_MIGRATE_REF_STORAGE_FORMAT_DRYRUN: perform a dry-run migration + * without touching the main repository. The result will be written into a + * temporary ref storage directory. + */ +#define REPO_MIGRATE_REF_STORAGE_FORMAT_DRYRUN (1 << 0) + +/* + * Migrate the ref storage format used by the repository to the + * specified one. + */ +int repo_migrate_ref_storage_format(struct repository *repo, + enum ref_storage_format format, + unsigned int flags, + struct strbuf *err); + /* * The following functions have been removed in Git v2.45 in favor of functions * that receive a `ref_store` as parameter. The intent of this section is
With the introduction of the new "reftable" backend, users may want to migrate repositories between the backends without having to recreate the whole repository. Add the logic to do so. The implementation is generic and works with arbitrary ref storage formats so that a backend does not need to implement any migration logic. It does have a few limitations though: - We do not migrate repositories with worktrees, because worktrees have separate ref storages. It makes the overall affair more complex if we have to migrate multiple storages at once. - We do not migrate reflogs, because we have no interfaces to write many reflog entries. - We do not lock the repository for concurrent access, and thus concurrent writes may end up with weird in-between states. There is no way to fully lock the "files" backend for writes due to its format, and thus we punt on this topic altogether and defer to the user to avoid those from happening. In other words, this version is a minimum viable product for migrating a repository's ref storage format. It works alright for bare repos, which often have neither worktrees nor reflogs. But it will not work for many other repositories without some preparations. These limitations are not set into stone though, and ideally we will eventually address them over time. The logic is not yet used by anything, and thus there are no tests for it. Those will be added in the next commit. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- refs.c | 305 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ refs.h | 18 ++++ 2 files changed, 323 insertions(+)