Re: [PATCH v2 8/9] refs: implement logic to migrate between ref storage formats

[Patrick Steinhardt <ps@pks.im>]

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

On Fri, May 24, 2024 at 05:32:20PM -0500, Justin Tobler wrote:
> On 24/05/24 12:15PM, Patrick Steinhardt wrote:
[snip]
> > +	/*
> > +	 * 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(buf.buf);
> > +	if (!new_gitdir) {
> > +		strbuf_addf(errbuf, "cannot create migration directory: %s",
> > +			    strerror(errno));
> > +		ret = -1;
> > +		goto done;
> > +	}
> 
> If the repository contains reflogs or has worktrees the migration does
> not proceed. This means that the created tempdir gets left behind with
> no indication and would be left to the user clean it up.
> 
> Instead, we could move tempdir creation to after these checks so it is
> not needlessly created.

True, done.

> > +	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;
> > +	}
> > +
> > +	/*
> > +	 * 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;
> > +	}
> [snip]
> > +	/*
> > +	 * 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.
> > +	 */
> 
> If there a failure after this point, should we provide a hint to user
> that the refernces exist in the tempdir?

Good idea.

Patrick

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