diff mbox series

[v6,7/7] refs: remove `create_symref` and associated dead code

Message ID 20240503124115.252413-8-knayak@gitlab.com (mailing list archive)
State Superseded
Headers show
Series refs: add support for transactional symref updates | expand

Commit Message

Karthik Nayak May 3, 2024, 12:41 p.m. UTC 
From: Karthik Nayak <karthik.188@gmail.com>

In the previous commits, we converted `refs_create_symref()` to utilize
transactions to perform symref updates. Earlier `refs_create_symref()`
used `create_symref()` to do the same.

We can now remove `create_symref()` and any code associated with it
which is no longer used. We remove `create_symref()` code from all the
reference backends and also remove it entirely from the `ref_storage_be`
struct.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs/debug.c            | 13 -------
 refs/files-backend.c    | 67 --------------------------------
 refs/packed-backend.c   |  1 -
 refs/refs-internal.h    |  5 ---
 refs/reftable-backend.c | 86 -----------------------------------------
 5 files changed, 172 deletions(-)

Comments

Junio C Hamano May 3, 2024, 11:09 p.m. UTC | #1 
Karthik Nayak <karthik.188@gmail.com> writes:

> From: Karthik Nayak <karthik.188@gmail.com>
>
> In the previous commits, we converted `refs_create_symref()` to utilize
> transactions to perform symref updates. Earlier `refs_create_symref()`
> used `create_symref()` to do the same.
>
> We can now remove `create_symref()` and any code associated with it
> which is no longer used. We remove `create_symref()` code from all the
> reference backends and also remove it entirely from the `ref_storage_be`
> struct.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---

This has serious semantic conflicts with in-flight topics.  I think
I resolved them all correctly while merging it in 'seen', but please
double check the result after I push it out.

This comment equally applies to the "force all callers to use
get_main_ref_store() on the_repository and remove functions that
implicitly used the main ref store of the_repository" topic by
Patrick, but we really should devise a bit more smoother way to cope
with out of tree and in-flight topics.  For example, as the new
refs_update_symref() function works exactly like the existing
refs_create_symref() function, after renaming all the in-base (i.e.,
in-tree at the point this topic forks from) users to call the new
function, instead of just removing the original one, it would have
been nice to guide authors of other in-flight topics by (1) causing
build failure and at the same time (2) telling them what they need
to do to adjust to the new world order.  This patch does only (1)
but does a poor job for (2).  We may want to establish a better
convention than just outright removing and breaking others' topics.

Thanks.
Karthik Nayak May 4, 2024, 9:30 a.m. UTC | #2 
Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> In the previous commits, we converted `refs_create_symref()` to utilize
>> transactions to perform symref updates. Earlier `refs_create_symref()`
>> used `create_symref()` to do the same.
>>
>> We can now remove `create_symref()` and any code associated with it
>> which is no longer used. We remove `create_symref()` code from all the
>> reference backends and also remove it entirely from the `ref_storage_be`
>> struct.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>
> This has serious semantic conflicts with in-flight topics.  I think
> I resolved them all correctly while merging it in 'seen', but please
> double check the result after I push it out.
>

I had a look at the commit pushed to 'seen', looked good to me. Thanks.

> This comment equally applies to the "force all callers to use
> get_main_ref_store() on the_repository and remove functions that
> implicitly used the main ref store of the_repository" topic by
> Patrick, but we really should devise a bit more smoother way to cope
> with out of tree and in-flight topics.  For example, as the new
> refs_update_symref() function works exactly like the existing
> refs_create_symref() function, after renaming all the in-base (i.e.,
> in-tree at the point this topic forks from) users to call the new
> function, instead of just removing the original one, it would have
> been nice to guide authors of other in-flight topics by (1) causing
> build failure and at the same time (2) telling them what they need
> to do to adjust to the new world order.  This patch does only (1)
> but does a poor job for (2).  We may want to establish a better
> convention than just outright removing and breaking others' topics.
>
> Thanks.

Just so I can do better next time, what is your suggestion for doing (2)
better?
diff mbox series

Patch

diff --git a/refs/debug.c b/refs/debug.c
index c7531b17f0..8be316bb67 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -131,18 +131,6 @@  static int debug_pack_refs(struct ref_store *ref_store, struct pack_refs_opts *o
 	return res;
 }
 
-static int debug_create_symref(struct ref_store *ref_store,
-			       const char *ref_name, const char *target,
-			       const char *logmsg)
-{
-	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
-	int res = drefs->refs->be->create_symref(drefs->refs, ref_name, target,
-						 logmsg);
-	trace_printf_key(&trace_refs, "create_symref: %s -> %s \"%s\": %d\n", ref_name,
-		target, logmsg, res);
-	return res;
-}
-
 static int debug_rename_ref(struct ref_store *ref_store, const char *oldref,
 			    const char *newref, const char *logmsg)
 {
@@ -441,7 +429,6 @@  struct ref_storage_be refs_be_debug = {
 	.initial_transaction_commit = debug_initial_transaction_commit,
 
 	.pack_refs = debug_pack_refs,
-	.create_symref = debug_create_symref,
 	.rename_ref = debug_rename_ref,
 	.copy_ref = debug_copy_ref,
 
diff --git a/refs/files-backend.c b/refs/files-backend.c
index e1f0ca74c0..3ce260d07d 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1903,23 +1903,6 @@  static int create_ref_symlink(struct ref_lock *lock, const char *target)
 	return ret;
 }
 
-static void update_symref_reflog(struct files_ref_store *refs,
-				 struct ref_lock *lock, const char *refname,
-				 const char *target, const char *logmsg)
-{
-	struct strbuf err = STRBUF_INIT;
-	struct object_id new_oid;
-
-	if (logmsg &&
-	    refs_resolve_ref_unsafe(&refs->base, target,
-				    RESOLVE_REF_READING, &new_oid, NULL) &&
-	    files_log_ref_write(refs, refname, &lock->old_oid,
-				&new_oid, logmsg, 0, &err)) {
-		error("%s", err.buf);
-		strbuf_release(&err);
-	}
-}
-
 static int create_symref_lock(struct files_ref_store *refs,
 			      struct ref_lock *lock, const char *refname,
 			      const char *target, struct strbuf *err)
@@ -1939,55 +1922,6 @@  static int create_symref_lock(struct files_ref_store *refs,
 	return 0;
 }
 
-static int create_and_commit_symref(struct files_ref_store *refs,
-				    struct ref_lock *lock, const char *refname,
-				    const char *target, const char *logmsg)
-{
-	struct strbuf err = STRBUF_INIT;
-	int ret;
-
-	if (prefer_symlink_refs && !create_ref_symlink(lock, target)) {
-		update_symref_reflog(refs, lock, refname, target, logmsg);
-		return 0;
-	}
-
-	ret = create_symref_lock(refs, lock, refname, target, &err);
-	if (!ret) {
-		update_symref_reflog(refs, lock, refname, target, logmsg);
-
-		if (commit_ref(lock) < 0)
-			return error("unable to write symref for %s: %s", refname,
-				     strerror(errno));
-	} else {
-		return error("%s", err.buf);
-	}
-
-	return ret;
-}
-
-static int files_create_symref(struct ref_store *ref_store,
-			       const char *refname, const char *target,
-			       const char *logmsg)
-{
-	struct files_ref_store *refs =
-		files_downcast(ref_store, REF_STORE_WRITE, "create_symref");
-	struct strbuf err = STRBUF_INIT;
-	struct ref_lock *lock;
-	int ret;
-
-	lock = lock_ref_oid_basic(refs, refname, &err);
-	if (!lock) {
-		error("%s", err.buf);
-		strbuf_release(&err);
-		return -1;
-	}
-
-	ret = create_and_commit_symref(refs, lock, refname, target, logmsg);
-
-	unlock_ref(lock);
-	return ret;
-}
-
 static int files_reflog_exists(struct ref_store *ref_store,
 			       const char *refname)
 {
@@ -3413,7 +3347,6 @@  struct ref_storage_be refs_be_files = {
 	.initial_transaction_commit = files_initial_transaction_commit,
 
 	.pack_refs = files_pack_refs,
-	.create_symref = files_create_symref,
 	.rename_ref = files_rename_ref,
 	.copy_ref = files_copy_ref,
 
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 4e826c05ff..a937e7dbfc 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1714,7 +1714,6 @@  struct ref_storage_be refs_be_packed = {
 	.initial_transaction_commit = packed_initial_transaction_commit,
 
 	.pack_refs = packed_pack_refs,
-	.create_symref = NULL,
 	.rename_ref = NULL,
 	.copy_ref = NULL,
 
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 03eda70ea4..0c620d4bce 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -566,10 +566,6 @@  typedef int ref_transaction_commit_fn(struct ref_store *refs,
 
 typedef int pack_refs_fn(struct ref_store *ref_store,
 			 struct pack_refs_opts *opts);
-typedef int create_symref_fn(struct ref_store *ref_store,
-			     const char *ref_target,
-			     const char *refs_heads_master,
-			     const char *logmsg);
 typedef int rename_ref_fn(struct ref_store *ref_store,
 			  const char *oldref, const char *newref,
 			  const char *logmsg);
@@ -690,7 +686,6 @@  struct ref_storage_be {
 	ref_transaction_commit_fn *initial_transaction_commit;
 
 	pack_refs_fn *pack_refs;
-	create_symref_fn *create_symref;
 	rename_ref_fn *rename_ref;
 	copy_ref_fn *copy_ref;
 
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 54c4d8b771..999b5bca21 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1275,91 +1275,6 @@  struct write_create_symref_arg {
 	const char *logmsg;
 };
 
-static int write_create_symref_table(struct reftable_writer *writer, void *cb_data)
-{
-	struct write_create_symref_arg *create = cb_data;
-	uint64_t ts = reftable_stack_next_update_index(create->stack);
-	struct reftable_ref_record ref = {
-		.refname = (char *)create->refname,
-		.value_type = REFTABLE_REF_SYMREF,
-		.value.symref = (char *)create->target,
-		.update_index = ts,
-	};
-	struct reftable_log_record log = {0};
-	struct object_id new_oid;
-	struct object_id old_oid;
-	int ret;
-
-	reftable_writer_set_limits(writer, ts, ts);
-
-	ret = reftable_writer_add_ref(writer, &ref);
-	if (ret)
-		return ret;
-
-	/*
-	 * Note that it is important to try and resolve the reference before we
-	 * write the log entry. This is because `should_write_log()` will munge
-	 * `core.logAllRefUpdates`, which is undesirable when we create a new
-	 * repository because it would be written into the config. As HEAD will
-	 * not resolve for new repositories this ordering will ensure that this
-	 * never happens.
-	 */
-	if (!create->logmsg ||
-	    !refs_resolve_ref_unsafe(&create->refs->base, create->target,
-				     RESOLVE_REF_READING, &new_oid, NULL) ||
-	    !should_write_log(&create->refs->base, create->refname))
-		return 0;
-
-	fill_reftable_log_record(&log);
-	log.refname = xstrdup(create->refname);
-	log.update_index = ts;
-	log.value.update.message = xstrndup(create->logmsg,
-					    create->refs->write_options.block_size / 2);
-	memcpy(log.value.update.new_hash, new_oid.hash, GIT_MAX_RAWSZ);
-	if (refs_resolve_ref_unsafe(&create->refs->base, create->refname,
-				    RESOLVE_REF_READING, &old_oid, NULL))
-		memcpy(log.value.update.old_hash, old_oid.hash, GIT_MAX_RAWSZ);
-
-	ret = reftable_writer_add_log(writer, &log);
-	reftable_log_record_release(&log);
-	return ret;
-}
-
-static int reftable_be_create_symref(struct ref_store *ref_store,
-				     const char *refname,
-				     const char *target,
-				     const char *logmsg)
-{
-	struct reftable_ref_store *refs =
-		reftable_be_downcast(ref_store, REF_STORE_WRITE, "create_symref");
-	struct reftable_stack *stack = stack_for(refs, refname, &refname);
-	struct write_create_symref_arg arg = {
-		.refs = refs,
-		.stack = stack,
-		.refname = refname,
-		.target = target,
-		.logmsg = logmsg,
-	};
-	int ret;
-
-	ret = refs->err;
-	if (ret < 0)
-		goto done;
-
-	ret = reftable_stack_reload(stack);
-	if (ret)
-		goto done;
-
-	ret = reftable_stack_add(stack, &write_create_symref_table, &arg);
-
-done:
-	assert(ret != REFTABLE_API_ERROR);
-	if (ret)
-		error("unable to write symref for %s: %s", refname,
-		      reftable_error_str(ret));
-	return ret;
-}
-
 struct write_copy_arg {
 	struct reftable_ref_store *refs;
 	struct reftable_stack *stack;
@@ -2267,7 +2182,6 @@  struct ref_storage_be refs_be_reftable = {
 	.initial_transaction_commit = reftable_be_initial_transaction_commit,
 
 	.pack_refs = reftable_be_pack_refs,
-	.create_symref = reftable_be_create_symref,
 	.rename_ref = reftable_be_rename_ref,
 	.copy_ref = reftable_be_copy_ref,