Re: [PATCH 7/8] refs: stop unsetting REF_HAVE_OLD for log-only updates

[Karthik Nayak <karthik.188@gmail.com>]

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

Patrick Steinhardt <ps@pks.im> writes:

> diff --git a/refs.c b/refs.c
> index 64544300dc3..c78d5be6e20 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1384,11 +1384,6 @@ int ref_transaction_update_reflog(struct ref_transaction *transaction,
>  	update = ref_transaction_add_update(transaction, refname, flags,
>  					    new_oid, old_oid, NULL, NULL,
>  					    committer_info, msg);
> -	/*
> -	 * While we do set the old_oid value, we unset the flag to skip
> -	 * old_oid verification which only makes sense for refs.
> -	 */
> -	update->flags &= ~REF_HAVE_OLD;
>  	update->index = index;
>
>  	/*

So we no longer unset the flag, this will ensure that the provided
old_oid is propagated correctly.

> @@ -3310,7 +3305,7 @@ int repo_migrate_ref_storage_format(struct repository *repo,
>
>  int ref_update_expects_existing_old_ref(struct ref_update *update)

Nit: Wonder if we should update the comment for the function to reflect
how this works with reflog only entries.

>  {
> -	return (update->flags & REF_HAVE_OLD) &&
> +	return (update->flags & (REF_HAVE_OLD | REF_LOG_ONLY)) == REF_HAVE_OLD &&
>  		(!is_null_oid(&update->old_oid) || update->old_target);
>  }
>

Okay this is check now says, if this is a reflog only entry, we don't
expect the reference to exist.

Nit: I wonder if can make this a bit more readable, perhaps:

  int ref_update_expects_existing_old_ref(struct ref_update *update)
  {
      /* reflog only entries may not match on-disk status of a reference */
      if (update->flags & REF_LOG_ONLY)
          return 0;

      return (update->flags & REF_HAVE_OLD &&
          (!is_null_oid(&update->old_oid) || update->old_target);
  }

I'm okay with the current version too.

> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 89ae4517a97..d519bb615fa 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2493,7 +2493,6 @@ static enum ref_transaction_error split_symref_update(struct ref_update *update,
>  	 * done when new_update is processed.
>  	 */
>  	update->flags |= REF_LOG_ONLY | REF_NO_DEREF;
> -	update->flags &= ~REF_HAVE_OLD;
>
>  	return 0;
>  }
> @@ -2508,8 +2507,9 @@ static enum ref_transaction_error check_old_oid(struct ref_update *update,
>  						struct object_id *oid,
>  						struct strbuf *err)
>  {
> -	if (!(update->flags & REF_HAVE_OLD) ||
> -		   oideq(oid, &update->old_oid))
> +	if (update->flags & REF_LOG_ONLY ||
> +	    !(update->flags & REF_HAVE_OLD) ||
> +	    oideq(oid, &update->old_oid))
>  		return 0;
>
>  	if (is_null_oid(&update->old_oid)) {
> @@ -3061,7 +3061,8 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,
>  	for (i = 0; i < transaction->nr; i++) {
>  		struct ref_update *update = transaction->updates[i];
>
> -		if ((update->flags & REF_HAVE_OLD) &&
> +		if (!(update->flags & REF_LOG_ONLY) &&
> +		    (update->flags & REF_HAVE_OLD) &&
>  		    !is_null_oid(&update->old_oid))
>  			BUG("initial ref transaction with old_sha1 set");
>
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 4c3817f4ec1..44af58ac50b 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -1180,8 +1180,6 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor
>  	if (ret > 0) {
>  		/* The reference does not exist, but we expected it to. */
>  		strbuf_addf(err, _("cannot lock ref '%s': "
> -
> -

Huh. I'm the author of this misshap. Thanks for the cleanup :D

>  				   "unable to resolve reference '%s'"),
>  			    ref_update_original_update_refname(u), u->refname);
>  		return REF_TRANSACTION_ERROR_NONEXISTENT_REF;
> @@ -1235,13 +1233,8 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor
>
>  			new_update->parent_update = u;
>
> -			/*
> -			 * Change the symbolic ref update to log only. Also, it
> -			 * doesn't need to check its old OID value, as that will be
> -			 * done when new_update is processed.
> -			 */
> +			/* Change the symbolic ref update to log only. */
>  			u->flags |= REF_LOG_ONLY | REF_NO_DEREF;
> -			u->flags &= ~REF_HAVE_OLD;
>  		}
>  	}
>
> @@ -1265,7 +1258,8 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor
>  		ret = ref_update_check_old_target(referent->buf, u, err);
>  		if (ret)
>  			return ret;
> -	} else if ((u->flags & REF_HAVE_OLD) && !oideq(&current_oid, &u->old_oid)) {
> +	} else if ((u->flags & (REF_LOG_ONLY | REF_HAVE_OLD)) == REF_HAVE_OLD &&
> +		   !oideq(&current_oid, &u->old_oid)) {
>  		if (is_null_oid(&u->old_oid)) {
>  			strbuf_addf(err, _("cannot lock ref '%s': "
>  					   "reference already exists"),
>
> --
> 2.50.1.465.gcb3da1c9e6.dirty

Looks good overall. Thanks

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