Web lists-archives.com

Re: [PATCH v2 05/24] refs: update ref transactions to use struct object_id




brian m. carlson wrote:

> Update the ref transaction code to use struct object_id.  Remove one
> NULL pointer check which was previously inserted around a dereference;
> since we now pass a pointer to struct object_id directly through, the
> code we're calling handles this for us.
>
> Signed-off-by: brian m. carlson <sandals@xxxxxxxxxxxxxxxxxxxx>
> ---
>  branch.c               |  2 +-
>  builtin/clone.c        |  2 +-
>  builtin/commit.c       |  4 ++--
>  builtin/fetch.c        |  4 ++--
>  builtin/receive-pack.c |  4 ++--
>  builtin/replace.c      |  2 +-
>  builtin/tag.c          |  2 +-
>  builtin/update-ref.c   |  8 ++++----
>  fast-import.c          |  4 ++--
>  refs.c                 | 44 +++++++++++++++++++++-----------------------
>  refs.h                 | 24 ++++++++++++------------
>  refs/files-backend.c   | 12 ++++++------
>  refs/refs-internal.h   |  4 ++--
>  sequencer.c            |  2 +-
>  walker.c               |  2 +-
>  15 files changed, 59 insertions(+), 61 deletions(-)

Makes sense.

[...]
> +++ b/refs.c
[...]
>  int ref_transaction_create(struct ref_transaction *transaction,
>  			   const char *refname,
> -			   const unsigned char *new_sha1,
> +			   const struct object_id *new_oid,
>  			   unsigned int flags, const char *msg,
>  			   struct strbuf *err)
>  {
> -	if (!new_sha1 || is_null_sha1(new_sha1))
> +	if (!new_oid || is_null_oid(new_oid))
>  		die("BUG: create called without valid new_sha1");

The error message still refers to new_sha1.

[...]
>  int ref_transaction_delete(struct ref_transaction *transaction,
>  			   const char *refname,
> -			   const unsigned char *old_sha1,
> +			   const struct object_id *old_oid,
>  			   unsigned int flags, const char *msg,
>  			   struct strbuf *err)
>  {
> -	if (old_sha1 && is_null_sha1(old_sha1))
> +	if (old_oid && is_null_oid(old_oid))
>  		die("BUG: delete called with old_sha1 set to zeros");

Likewise.

[...]
>  int ref_transaction_verify(struct ref_transaction *transaction,
>  			   const char *refname,
> -			   const unsigned char *old_sha1,
> +			   const struct object_id *old_oid,
>  			   unsigned int flags,
>  			   struct strbuf *err)
>  {
> -	if (!old_sha1)
> +	if (!old_oid)
>  		die("BUG: verify called with old_sha1 set to NULL");

Likewise.

[...]
> +++ b/refs.h
> @@ -519,15 +519,15 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err);
[...]
>  /*
> - * Add a reference creation to transaction. new_sha1 is the value that
> + * Add a reference creation to transaction. new_oid is the value that
>   * the reference should have after the update; it must not be
> - * null_sha1. It is verified that the reference does not exist
> + * null_oid. It is verified that the reference does not exist
>   * already.
>   *
>   * See the above comment "Reference transaction updates" for more
> @@ -535,35 +535,35 @@ int ref_transaction_update(struct ref_transaction *transaction,
>   */

(Possible diff generation bug: there's exactly one line represented in
that @@ field.  I would have expected the diff generator to combine
the hunks.)

I think this is fine to go in without the error messages being fixed.
A grep for 'sha1' will find them later so that they can be addressed
in a separate patch.

Reviewed-by: Jonathan Nieder <jrnieder@xxxxxxxxx>