diff mbox series

[v4,4/8] refs: extract out refname verification in transactions

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

Commit Message

Karthik Nayak Dec. 16, 2024, 4:44 p.m. UTC
Unless the `REF_SKIP_REFNAME_VERIFICATION` flag is set for an update,
the refname of the update is verified for:

  - Ensuring it is not a pseudoref.
  - Checking the refname format.

These checks will also be needed in a following commit where the
function to add reflog updates to the transaction is introduced. Extract
the code out into a new static function.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 refs.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

Comments

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

> Unless the `REF_SKIP_REFNAME_VERIFICATION` flag is set for an update,
> the refname of the update is verified for:
>
>   - Ensuring it is not a pseudoref.
>   - Checking the refname format.
>
> These checks will also be needed in a following commit where the
> function to add reflog updates to the transaction is introduced. Extract
> the code out into a new static function.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  refs.c | 37 +++++++++++++++++++++++--------------
>  1 file changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index f003e51c6bf5229bfbce8ce61ffad7cdba0572e0..9c9f4940c60d3cdd34ce8f1e668d17b9da3cd801 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1196,6 +1196,28 @@ struct ref_update *ref_transaction_add_update(
>  	return update;
>  }
>  
> +static int transaction_refname_valid(const char *refname,
> +				     const struct object_id *new_oid,
> +				     unsigned int flags, struct strbuf *err)
> +{
> +	if (flags & REF_SKIP_REFNAME_VERIFICATION)
> +		return 1;
> +
> +	if (is_pseudo_ref(refname)) {
> +		strbuf_addf(err, _("refusing to update pseudoref '%s'"),
> +			    refname);
> +		return 0;

With this early return you don't need the `else` below? Why did you add
it?

> +	} else if ((new_oid && !is_null_oid(new_oid)) ?
> +		 check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :
> +		 !refname_is_safe(refname)) {
> +		strbuf_addf(err, _("refusing to update ref with bad name '%s'"),
> +			    refname);
> +		return 0;
> +	}

I see you've swapped order of checking whether it's a pseudoref with
checking whether the format is okay. I think this shouldn't make a big
difference, but it will give a different error message when attempting
to update an illformatted pseudoref. For me it makes more sense how
you've done it now. But because you mention both checks as bullet points
in the commit message, do you think it would make sense to say something
about them being swapped?
Karthik Nayak Dec. 20, 2024, 10:30 a.m. UTC | #2
Toon Claes <toon@iotcl.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>> +static int transaction_refname_valid(const char *refname,
>> +				     const struct object_id *new_oid,
>> +				     unsigned int flags, struct strbuf *err)
>> +{
>> +	if (flags & REF_SKIP_REFNAME_VERIFICATION)
>> +		return 1;
>> +
>> +	if (is_pseudo_ref(refname)) {
>> +		strbuf_addf(err, _("refusing to update pseudoref '%s'"),
>> +			    refname);
>> +		return 0;
>
> With this early return you don't need the `else` below? Why did you add
> it?
>

You mean we could simply have

  if { ... check for pseudoref ... }

  if { ... check for bad refname ... }

  return -1;

then, you're right, that would work too. No specific reason that I added
an else.

>> +	} else if ((new_oid && !is_null_oid(new_oid)) ?
>> +		 check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :
>> +		 !refname_is_safe(refname)) {
>> +		strbuf_addf(err, _("refusing to update ref with bad name '%s'"),
>> +			    refname);
>> +		return 0;
>> +	}
>
> I see you've swapped order of checking whether it's a pseudoref with
> checking whether the format is okay. I think this shouldn't make a big
> difference, but it will give a different error message when attempting
> to update an illformatted pseudoref. For me it makes more sense how
> you've done it now. But because you mention both checks as bullet points
> in the commit message, do you think it would make sense to say something
> about them being swapped?
>

I actually didn't notice that I did swap them. It doesn't change the
logic. However, for creation of a pseudoref, in the old flow, we'd check
if the refname is safe and then go to the section where we check for
pseudorefs. Now we simply skip to the pseudoref check. I'll add more
details in the commit message locally for now and will include it if I
do re-roll!

> --
> Toon
diff mbox series

Patch

diff --git a/refs.c b/refs.c
index f003e51c6bf5229bfbce8ce61ffad7cdba0572e0..9c9f4940c60d3cdd34ce8f1e668d17b9da3cd801 100644
--- a/refs.c
+++ b/refs.c
@@ -1196,6 +1196,28 @@  struct ref_update *ref_transaction_add_update(
 	return update;
 }
 
+static int transaction_refname_valid(const char *refname,
+				     const struct object_id *new_oid,
+				     unsigned int flags, struct strbuf *err)
+{
+	if (flags & REF_SKIP_REFNAME_VERIFICATION)
+		return 1;
+
+	if (is_pseudo_ref(refname)) {
+		strbuf_addf(err, _("refusing to update pseudoref '%s'"),
+			    refname);
+		return 0;
+	} else if ((new_oid && !is_null_oid(new_oid)) ?
+		 check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :
+		 !refname_is_safe(refname)) {
+		strbuf_addf(err, _("refusing to update ref with bad name '%s'"),
+			    refname);
+		return 0;
+	}
+
+	return 1;
+}
+
 int ref_transaction_update(struct ref_transaction *transaction,
 			   const char *refname,
 			   const struct object_id *new_oid,
@@ -1213,21 +1235,8 @@  int ref_transaction_update(struct ref_transaction *transaction,
 		return -1;
 	}
 
-	if (!(flags & REF_SKIP_REFNAME_VERIFICATION) &&
-	    ((new_oid && !is_null_oid(new_oid)) ?
-		     check_refname_format(refname, REFNAME_ALLOW_ONELEVEL) :
-			   !refname_is_safe(refname))) {
-		strbuf_addf(err, _("refusing to update ref with bad name '%s'"),
-			    refname);
+	if (!transaction_refname_valid(refname, new_oid, flags, err))
 		return -1;
-	}
-
-	if (!(flags & REF_SKIP_REFNAME_VERIFICATION) &&
-	    is_pseudo_ref(refname)) {
-		strbuf_addf(err, _("refusing to update pseudoref '%s'"),
-			    refname);
-		return -1;
-	}
 
 	if (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)
 		BUG("illegal flags 0x%x passed to ref_transaction_update()", flags);