On Tue, Jun 04, 2024 at 03:28:07PM +0000, Karthik Nayak wrote:
> Patrick Steinhardt <ps@xxxxxx> 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
Attachment:
signature.asc
Description: PGP signature
