diff mbox series

[v4,10/12] refs: implement removal of ref storages

Message ID f9d9420cf93025595e00aa4287bc0cc1a1c067ab.1717402363.git.ps@pks.im (mailing list archive)
State Superseded
Commit 9b708939859b0229ca1b9d24e668fd10e9fb220c
Headers show
Series refs: ref storage migrations | expand

Commit Message

Patrick Steinhardt June 3, 2024, 9:30 a.m. UTC
We're about to introduce logic to migrate ref storages. One part of the
migration will be to delete the files that are part of the old ref
storage format. We don't yet have a way to delete such data generically
across ref backends though.

Implement a new `delete` callback and expose it via a new
`ref_storage_delete()` function.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.c                  |  5 ++++
 refs.h                  |  5 ++++
 refs/files-backend.c    | 63 +++++++++++++++++++++++++++++++++++++++++
 refs/packed-backend.c   | 15 ++++++++++
 refs/refs-internal.h    |  7 +++++
 refs/reftable-backend.c | 52 ++++++++++++++++++++++++++++++++++
 6 files changed, 147 insertions(+)

Comments

karthik nayak June 4, 2024, 11:17 a.m. UTC | #1
Patrick Steinhardt <ps@pks.im> writes:

[snip]

> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index bffed9257f..e555be4671 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -1,6 +1,7 @@
>  #include "../git-compat-util.h"
>  #include "../abspath.h"
>  #include "../chdir-notify.h"
> +#include "../dir.h"
>  #include "../environment.h"
>  #include "../gettext.h"
>  #include "../hash.h"
> @@ -343,6 +344,56 @@ static int reftable_be_create_on_disk(struct ref_store *ref_store,
>  	return 0;
>  }
>
> +static int reftable_be_remove_on_disk(struct ref_store *ref_store,
> +				      struct strbuf *err)
> +{
> +	struct reftable_ref_store *refs =
> +		reftable_be_downcast(ref_store, REF_STORE_WRITE, "remove");
> +	struct strbuf sb = STRBUF_INIT;
> +	int ret = 0;
> +
> +	/*
> +	 * Release the ref store such that all stacks are closed. This is
> +	 * required so that the "tables.list" file is not open anymore, which
> +	 * would otherwise make it impossible to remove the file on Windows.
> +	 */
> +	reftable_be_release(ref_store);
> +
> +	strbuf_addf(&sb, "%s/reftable", refs->base.gitdir);
> +	if (remove_dir_recursively(&sb, 0) < 0) {
> +		strbuf_addf(err, "could not delete reftables: %s",
> +			    strerror(errno));
> +		ret = -1;
> +	}
> +	strbuf_reset(&sb);
> +
> +	strbuf_addf(&sb, "%s/HEAD", refs->base.gitdir);
> +	if (unlink(sb.buf) < 0) {
> +		strbuf_addf(err, "could not delete stub HEAD: %s",
> +			    strerror(errno));
> +		ret = -1;
> +	}
> +	strbuf_reset(&sb);
> +
> +	strbuf_addf(&sb, "%s/refs/heads", refs->base.gitdir);
> +	if (unlink(sb.buf) < 0) {
> +		strbuf_addf(err, "could not delete stub heads: %s",
> +			    strerror(errno));
> +		ret = -1;
> +	}
> +	strbuf_reset(&sb);
> +
> +	strbuf_addf(&sb, "%s/refs", refs->base.gitdir);
> +	if (rmdir(sb.buf) < 0) {
> +		strbuf_addf(err, "could not delete stub heads: %s",

Nit: Wouldn't it be nicer to be able to differentiate this from the
previous case? Both have the same error message.

Otherwise this patch looks good, since we use unlink(2), that handles
symrefs which are symbolic links too.
Jeff King June 5, 2024, 10:12 a.m. UTC | #2
On Mon, Jun 03, 2024 at 11:30:55AM +0200, Patrick Steinhardt wrote:

> +static int files_ref_store_remove_on_disk(struct ref_store *ref_store,
> +					  struct strbuf *err)
> +{
> [...]
> +	strbuf_addf(&sb, "%s/refs", refs->base.gitdir);
> +	if (remove_dir_recursively(&sb, 0) < 0) {
> +		strbuf_addf(err, "could not delete refs: %s",
> +			    strerror(errno));
> +		ret = -1;
> +	}
> +	strbuf_reset(&sb);
> +
> +	strbuf_addf(&sb, "%s/logs", refs->base.gitdir);
> +	if (remove_dir_recursively(&sb, 0) < 0) {
> +		strbuf_addf(err, "could not delete logs: %s",
> +			    strerror(errno));
> +		ret = -1;
> +	}
> +	strbuf_reset(&sb);

If removing either of the directories fails, we set ret to "-1". Make
sense. But...

> +	ret = for_each_root_ref(refs, remove_one_root_ref, &data);
> +	if (ret < 0)
> +		ret = -1;

...then we unconditionally overwrite it, forgetting the earlier error.
Either we should jump to the end on the first failure, or if the goal is
to do as much as possible, should we |= the result? I'm not clear why we
assign "ret" and then immediately check it to assign "-1" again.

Is that a mistake, or are we normalizing other negative values? Maybe
just:

  if (for_each_root_ref(refs, remove_one_root_ref, &data) < 0)
	ret = -1;

would work?

-Peff
Junio C Hamano June 5, 2024, 4:54 p.m. UTC | #3
Jeff King <peff@peff.net> writes:

> If removing either of the directories fails, we set ret to "-1". Make
> sense. But...
>
>> +	ret = for_each_root_ref(refs, remove_one_root_ref, &data);
>> +	if (ret < 0)
>> +		ret = -1;
>
> ...then we unconditionally overwrite it, forgetting the earlier error.

Ouch.

> Is that a mistake, or are we normalizing other negative values? Maybe
> just:
>
>   if (for_each_root_ref(refs, remove_one_root_ref, &data) < 0)
> 	ret = -1;
>
> would work?

Sounds sensible.

Thanks for carefully reading.
Patrick Steinhardt June 6, 2024, 4:51 a.m. UTC | #4
On Wed, Jun 05, 2024 at 06:12:00AM -0400, Jeff King wrote:
> On Mon, Jun 03, 2024 at 11:30:55AM +0200, Patrick Steinhardt wrote:
> 
> > +static int files_ref_store_remove_on_disk(struct ref_store *ref_store,
> > +					  struct strbuf *err)
> > +{
> > [...]
> > +	strbuf_addf(&sb, "%s/refs", refs->base.gitdir);
> > +	if (remove_dir_recursively(&sb, 0) < 0) {
> > +		strbuf_addf(err, "could not delete refs: %s",
> > +			    strerror(errno));
> > +		ret = -1;
> > +	}
> > +	strbuf_reset(&sb);
> > +
> > +	strbuf_addf(&sb, "%s/logs", refs->base.gitdir);
> > +	if (remove_dir_recursively(&sb, 0) < 0) {
> > +		strbuf_addf(err, "could not delete logs: %s",
> > +			    strerror(errno));
> > +		ret = -1;
> > +	}
> > +	strbuf_reset(&sb);
> 
> If removing either of the directories fails, we set ret to "-1". Make
> sense. But...
> 
> > +	ret = for_each_root_ref(refs, remove_one_root_ref, &data);
> > +	if (ret < 0)
> > +		ret = -1;
> 
> ...then we unconditionally overwrite it, forgetting the earlier error.
> Either we should jump to the end on the first failure, or if the goal is
> to do as much as possible, should we |= the result? I'm not clear why we
> assign "ret" and then immediately check it to assign "-1" again.
> 
> Is that a mistake, or are we normalizing other negative values? Maybe
> just:
> 
>   if (for_each_root_ref(refs, remove_one_root_ref, &data) < 0)
> 	ret = -1;
> 
> would work?

Yup, that would work, good catch.

Patrick
diff mbox series

Patch

diff --git a/refs.c b/refs.c
index 66e9585767..9b112b0527 100644
--- a/refs.c
+++ b/refs.c
@@ -1861,6 +1861,11 @@  int ref_store_create_on_disk(struct ref_store *refs, int flags, struct strbuf *e
 	return refs->be->create_on_disk(refs, flags, err);
 }
 
+int ref_store_remove_on_disk(struct ref_store *refs, struct strbuf *err)
+{
+	return refs->be->remove_on_disk(refs, err);
+}
+
 int repo_resolve_gitlink_ref(struct repository *r,
 			     const char *submodule, const char *refname,
 			     struct object_id *oid)
diff --git a/refs.h b/refs.h
index 50a2b3ab09..61ee7b7a15 100644
--- a/refs.h
+++ b/refs.h
@@ -129,6 +129,11 @@  int ref_store_create_on_disk(struct ref_store *refs, int flags, struct strbuf *e
  */
 void ref_store_release(struct ref_store *ref_store);
 
+/*
+ * Remove the ref store from disk. This deletes all associated data.
+ */
+int ref_store_remove_on_disk(struct ref_store *refs, struct strbuf *err);
+
 /*
  * Return the peeled value of the oid currently being iterated via
  * for_each_ref(), etc. This is equivalent to calling:
diff --git a/refs/files-backend.c b/refs/files-backend.c
index b7268b26c8..cb752d32b6 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3340,11 +3340,74 @@  static int files_ref_store_create_on_disk(struct ref_store *ref_store,
 	return 0;
 }
 
+struct remove_one_root_ref_data {
+	const char *gitdir;
+	struct strbuf *err;
+};
+
+static int remove_one_root_ref(const char *refname,
+			       void *cb_data)
+{
+	struct remove_one_root_ref_data *data = cb_data;
+	struct strbuf buf = STRBUF_INIT;
+	int ret = 0;
+
+	strbuf_addf(&buf, "%s/%s", data->gitdir, refname);
+
+	ret = unlink(buf.buf);
+	if (ret < 0)
+		strbuf_addf(data->err, "could not delete %s: %s\n",
+			    refname, strerror(errno));
+
+	strbuf_release(&buf);
+	return ret;
+}
+
+static int files_ref_store_remove_on_disk(struct ref_store *ref_store,
+					  struct strbuf *err)
+{
+	struct files_ref_store *refs =
+		files_downcast(ref_store, REF_STORE_WRITE, "remove");
+	struct remove_one_root_ref_data data = {
+		.gitdir = refs->base.gitdir,
+		.err = err,
+	};
+	struct strbuf sb = STRBUF_INIT;
+	int ret = 0;
+
+	strbuf_addf(&sb, "%s/refs", refs->base.gitdir);
+	if (remove_dir_recursively(&sb, 0) < 0) {
+		strbuf_addf(err, "could not delete refs: %s",
+			    strerror(errno));
+		ret = -1;
+	}
+	strbuf_reset(&sb);
+
+	strbuf_addf(&sb, "%s/logs", refs->base.gitdir);
+	if (remove_dir_recursively(&sb, 0) < 0) {
+		strbuf_addf(err, "could not delete logs: %s",
+			    strerror(errno));
+		ret = -1;
+	}
+	strbuf_reset(&sb);
+
+	ret = for_each_root_ref(refs, remove_one_root_ref, &data);
+	if (ret < 0)
+		ret = -1;
+
+	if (ref_store_remove_on_disk(refs->packed_ref_store, err) < 0)
+		ret = -1;
+
+	strbuf_release(&sb);
+	return ret;
+}
+
 struct ref_storage_be refs_be_files = {
 	.name = "files",
 	.init = files_ref_store_init,
 	.release = files_ref_store_release,
 	.create_on_disk = files_ref_store_create_on_disk,
+	.remove_on_disk = files_ref_store_remove_on_disk,
 
 	.transaction_prepare = files_transaction_prepare,
 	.transaction_finish = files_transaction_finish,
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 2789fd92f5..c4c1e36aa2 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1,5 +1,6 @@ 
 #include "../git-compat-util.h"
 #include "../config.h"
+#include "../dir.h"
 #include "../gettext.h"
 #include "../hash.h"
 #include "../hex.h"
@@ -1266,6 +1267,19 @@  static int packed_ref_store_create_on_disk(struct ref_store *ref_store UNUSED,
 	return 0;
 }
 
+static int packed_ref_store_remove_on_disk(struct ref_store *ref_store,
+					   struct strbuf *err)
+{
+	struct packed_ref_store *refs = packed_downcast(ref_store, 0, "remove");
+
+	if (remove_path(refs->path) < 0) {
+		strbuf_addstr(err, "could not delete packed-refs");
+		return -1;
+	}
+
+	return 0;
+}
+
 /*
  * Write the packed refs from the current snapshot to the packed-refs
  * tempfile, incorporating any changes from `updates`. `updates` must
@@ -1724,6 +1738,7 @@  struct ref_storage_be refs_be_packed = {
 	.init = packed_ref_store_init,
 	.release = packed_ref_store_release,
 	.create_on_disk = packed_ref_store_create_on_disk,
+	.remove_on_disk = packed_ref_store_remove_on_disk,
 
 	.transaction_prepare = packed_transaction_prepare,
 	.transaction_finish = packed_transaction_finish,
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 33749fbd83..cbcb6f9c36 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -517,6 +517,12 @@  typedef int ref_store_create_on_disk_fn(struct ref_store *refs,
 					int flags,
 					struct strbuf *err);
 
+/*
+ * Remove the reference store from disk.
+ */
+typedef int ref_store_remove_on_disk_fn(struct ref_store *refs,
+					struct strbuf *err);
+
 typedef int ref_transaction_prepare_fn(struct ref_store *refs,
 				       struct ref_transaction *transaction,
 				       struct strbuf *err);
@@ -649,6 +655,7 @@  struct ref_storage_be {
 	ref_store_init_fn *init;
 	ref_store_release_fn *release;
 	ref_store_create_on_disk_fn *create_on_disk;
+	ref_store_remove_on_disk_fn *remove_on_disk;
 
 	ref_transaction_prepare_fn *transaction_prepare;
 	ref_transaction_finish_fn *transaction_finish;
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index bffed9257f..e555be4671 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1,6 +1,7 @@ 
 #include "../git-compat-util.h"
 #include "../abspath.h"
 #include "../chdir-notify.h"
+#include "../dir.h"
 #include "../environment.h"
 #include "../gettext.h"
 #include "../hash.h"
@@ -343,6 +344,56 @@  static int reftable_be_create_on_disk(struct ref_store *ref_store,
 	return 0;
 }
 
+static int reftable_be_remove_on_disk(struct ref_store *ref_store,
+				      struct strbuf *err)
+{
+	struct reftable_ref_store *refs =
+		reftable_be_downcast(ref_store, REF_STORE_WRITE, "remove");
+	struct strbuf sb = STRBUF_INIT;
+	int ret = 0;
+
+	/*
+	 * Release the ref store such that all stacks are closed. This is
+	 * required so that the "tables.list" file is not open anymore, which
+	 * would otherwise make it impossible to remove the file on Windows.
+	 */
+	reftable_be_release(ref_store);
+
+	strbuf_addf(&sb, "%s/reftable", refs->base.gitdir);
+	if (remove_dir_recursively(&sb, 0) < 0) {
+		strbuf_addf(err, "could not delete reftables: %s",
+			    strerror(errno));
+		ret = -1;
+	}
+	strbuf_reset(&sb);
+
+	strbuf_addf(&sb, "%s/HEAD", refs->base.gitdir);
+	if (unlink(sb.buf) < 0) {
+		strbuf_addf(err, "could not delete stub HEAD: %s",
+			    strerror(errno));
+		ret = -1;
+	}
+	strbuf_reset(&sb);
+
+	strbuf_addf(&sb, "%s/refs/heads", refs->base.gitdir);
+	if (unlink(sb.buf) < 0) {
+		strbuf_addf(err, "could not delete stub heads: %s",
+			    strerror(errno));
+		ret = -1;
+	}
+	strbuf_reset(&sb);
+
+	strbuf_addf(&sb, "%s/refs", refs->base.gitdir);
+	if (rmdir(sb.buf) < 0) {
+		strbuf_addf(err, "could not delete stub heads: %s",
+			    strerror(errno));
+		ret = -1;
+	}
+
+	strbuf_release(&sb);
+	return ret;
+}
+
 struct reftable_ref_iterator {
 	struct ref_iterator base;
 	struct reftable_ref_store *refs;
@@ -2196,6 +2247,7 @@  struct ref_storage_be refs_be_reftable = {
 	.init = reftable_be_init,
 	.release = reftable_be_release,
 	.create_on_disk = reftable_be_create_on_disk,
+	.remove_on_disk = reftable_be_remove_on_disk,
 
 	.transaction_prepare = reftable_be_transaction_prepare,
 	.transaction_finish = reftable_be_transaction_finish,