diff mbox series

[v4,2/8] refs: add `index` field to `struct ref_udpate`

Message ID 20241216-320-git-refs-migrate-reflogs-v4-2-d7cd3f197453@gmail.com (mailing list archive)
State Accepted
Commit a3582e2eacfae4328146b5142a0950fffcc05198
Headers show
Series refs: add reflog support to `git refs migrate` | expand

Commit Message

Karthik Nayak Dec. 16, 2024, 4:44 p.m. UTC
The reftable backend, sorts its updates by refname before applying them,
this ensures that the references are stored sorted. When migrating
reflogs from one backend to another, the order of the reflogs must be
maintained. Add a new `index` field to the `ref_update` struct to
facilitate this.

This field is used in the reftable backend's sort comparison function
`transaction_update_cmp`, to ensure that indexed fields maintain their
order.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs/refs-internal.h    |  7 +++++++
 refs/reftable-backend.c | 13 +++++++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

Comments

Toon Claes Dec. 19, 2024, 7:28 p.m. UTC | #1
Karthik Nayak <karthik.188@gmail.com> writes:

> The reftable backend, sorts its updates by refname before applying them,
> this ensures that the references are stored sorted. When migrating
> reflogs from one backend to another, the order of the reflogs must be
> maintained. Add a new `index` field to the `ref_update` struct to
> facilitate this.
>
> This field is used in the reftable backend's sort comparison function
> `transaction_update_cmp`, to ensure that indexed fields maintain their
> order.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  refs/refs-internal.h    |  7 +++++++
>  refs/reftable-backend.c | 13 +++++++++++--
>  2 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index 0fd95cdacd99e4a728c22f5286f6b3f0f360c110..f5c733d099f0c6f1076a25f4f77d9d5eb345ec87 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -115,6 +115,13 @@ struct ref_update {
>  	char *msg;
>  	char *committer_info;
>  
> +	/*
> +	 * The index overrides the default sort algorithm. This is needed
> +	 * when migrating reflogs and we want to ensure we carry over the
> +	 * same order.
> +	 */
> +	unsigned int index;
> +
>  	/*
>  	 * If this ref_update was split off of a symref update via
>  	 * split_symref_update(), then this member points at that
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index e882602487c66261d586a94101bb1b4e9a2ed60e..c008f20be719fec3af6a8f81c821cb9c263764d7 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -1279,8 +1279,17 @@ static int reftable_be_transaction_abort(struct ref_store *ref_store UNUSED,
>  
>  static int transaction_update_cmp(const void *a, const void *b)
>  {
> -	return strcmp(((struct reftable_transaction_update *)a)->update->refname,
> -		      ((struct reftable_transaction_update *)b)->update->refname);
> +	struct reftable_transaction_update *update_a = (struct reftable_transaction_update *)a;
> +	struct reftable_transaction_update *update_b = (struct reftable_transaction_update *)b;
> +
> +	/*
> +	 * If there is an index set, it should take preference (default is 0).
> +	 * This ensures that updates with indexes are sorted amongst themselves.
> +	 */
> +	if (update_a->update->index || update_b->update->index)

What if one of both simply isn't set, and the other one is? Then we
compare an unset with one that is set? Or am I being too paranoid?

--
Toon
Karthik Nayak Dec. 20, 2024, 10:09 a.m. UTC | #2
Toon Claes <toon@iotcl.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
>> index e882602487c66261d586a94101bb1b4e9a2ed60e..c008f20be719fec3af6a8f81c821cb9c263764d7 100644
>> --- a/refs/reftable-backend.c
>> +++ b/refs/reftable-backend.c
>> @@ -1279,8 +1279,17 @@ static int reftable_be_transaction_abort(struct ref_store *ref_store UNUSED,
>>
>>  static int transaction_update_cmp(const void *a, const void *b)
>>  {
>> -	return strcmp(((struct reftable_transaction_update *)a)->update->refname,
>> -		      ((struct reftable_transaction_update *)b)->update->refname);
>> +	struct reftable_transaction_update *update_a = (struct reftable_transaction_update *)a;
>> +	struct reftable_transaction_update *update_b = (struct reftable_transaction_update *)b;
>> +
>> +	/*
>> +	 * If there is an index set, it should take preference (default is 0).
>> +	 * This ensures that updates with indexes are sorted amongst themselves.
>> +	 */
>> +	if (update_a->update->index || update_b->update->index)
>
> What if one of both simply isn't set, and the other one is? Then we
> compare an unset with one that is set? Or am I being too paranoid?
>
> --
> Toon

Those are expected scenarios, if one of them contains an index value,
then it'll be sorted before the other. At the end, we need:
1. Values with index to be sorted amongst themselves by index value.
2. Values without index to be sorted amongst themselves by the refname.

Karthik
diff mbox series

Patch

diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 0fd95cdacd99e4a728c22f5286f6b3f0f360c110..f5c733d099f0c6f1076a25f4f77d9d5eb345ec87 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -115,6 +115,13 @@  struct ref_update {
 	char *msg;
 	char *committer_info;
 
+	/*
+	 * The index overrides the default sort algorithm. This is needed
+	 * when migrating reflogs and we want to ensure we carry over the
+	 * same order.
+	 */
+	unsigned int index;
+
 	/*
 	 * If this ref_update was split off of a symref update via
 	 * split_symref_update(), then this member points at that
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index e882602487c66261d586a94101bb1b4e9a2ed60e..c008f20be719fec3af6a8f81c821cb9c263764d7 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1279,8 +1279,17 @@  static int reftable_be_transaction_abort(struct ref_store *ref_store UNUSED,
 
 static int transaction_update_cmp(const void *a, const void *b)
 {
-	return strcmp(((struct reftable_transaction_update *)a)->update->refname,
-		      ((struct reftable_transaction_update *)b)->update->refname);
+	struct reftable_transaction_update *update_a = (struct reftable_transaction_update *)a;
+	struct reftable_transaction_update *update_b = (struct reftable_transaction_update *)b;
+
+	/*
+	 * If there is an index set, it should take preference (default is 0).
+	 * This ensures that updates with indexes are sorted amongst themselves.
+	 */
+	if (update_a->update->index || update_b->update->index)
+		return update_a->update->index - update_b->update->index;
+
+	return strcmp(update_a->update->refname, update_b->update->refname);
 }
 
 static int write_transaction_table(struct reftable_writer *writer, void *cb_data)