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

[Michael Haggerty]

On 10/09/2017 03:11 AM, 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 
> ---
>  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(-)
> 
> [...]
> diff --git a/refs.h b/refs.h
> index 369614d392..543dcc5956 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -519,15 +519,15 @@ struct ref_transaction *ref_transaction_begin(struct 
> strbuf *err);
>   */
>  int ref_transaction_update(struct ref_transaction *transaction,

The docstring for this function needs to be updated.

>  const char *refname,
> -const unsigned char *new_sha1,
> -const unsigned char *old_sha1,
> +const struct object_id *new_oid,
> +const struct object_id *old_oid,
>  unsigned int flags, const char *msg,
>  struct strbuf *err);
>  
> [...]

Michael

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

[Jonathan Nieder]

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 
> ---
>  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 

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

[brian m. carlson]

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 
---
 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(-)

diff --git a/branch.c b/branch.c
index 4377ce2fb1..45029ea142 100644
--- a/branch.c
+++ b/branch.c
@@ -305,7 +305,7 @@ void create_branch(const char *name, const char *start_name,
transaction = ref_transaction_begin();
if (!transaction ||
ref_transaction_update(transaction, ref.buf,
-  oid.hash, forcing ? NULL : null_sha1,
+  , forcing ? NULL : _oid,
   0, msg, ) ||
ref_transaction_commit(transaction, ))
die("%s", err.buf);
diff --git a/builtin/clone.c b/builtin/clone.c
index 4135621aa3..665a0e2673 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -588,7 +588,7 @@ static void write_remote_refs(const struct ref *local_refs)
for (r = local_refs; r; r = r->next) {
if (!r->peer_ref)
continue;
-   if (ref_transaction_create(t, r->peer_ref->name, 
r->old_oid.hash,
+   if (ref_transaction_create(t, r->peer_ref->name, >old_oid,
   0, NULL, ))
die("%s", err.buf);
}
diff --git a/builtin/commit.c b/builtin/commit.c
index 0f8ddb6866..d5fbf404f4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1785,9 +1785,9 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
 
transaction = ref_transaction_begin();
if (!transaction ||
-   ref_transaction_update(transaction, "HEAD", oid.hash,
+   ref_transaction_update(transaction, "HEAD", ,
   current_head
-  ? current_head->object.oid.hash : null_sha1,
+  ? _head->object.oid : _oid,
   0, sb.buf, ) ||
ref_transaction_commit(transaction, )) {
rollback_index_files();
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 225c734924..859be91d6c 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -457,8 +457,8 @@ static int s_update_ref(const char *action,
transaction = ref_transaction_begin();
if (!transaction ||
ref_transaction_update(transaction, ref->name,
-  ref->new_oid.hash,
-  check_old ? ref->old_oid.hash : NULL,
+  >new_oid,
+  check_old ? >old_oid : NULL,
   0, msg, ))
goto fail;
 
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 29a0f3b75f..39defd4e3c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1139,7 +1139,7 @@ static const char *update(struct command *cmd, struct 
shallow_info *si)
}
if (ref_transaction_delete(transaction,
   namespaced_name,
-  old_oid ? old_oid->hash : NULL,
+  old_oid,
   0, "push", )) {
rp_error("%s", err.buf);
strbuf_release();
@@ -1156,7 +1156,7 @@ static const char *update(struct command *cmd, struct 
shallow_info *si)
 
if (ref_transaction_update(transaction,
   namespaced_name,
-  new_oid->hash, old_oid->hash,
+  new_oid, old_oid,
   0, "push",
   )) {
rp_error("%s", err.buf);
diff --git a/builtin/replace.c b/builtin/replace.c
index 2854eaa0f3..3099e55307 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -175,7 +175,7 @@ static int replace_object_oid(const char *object_ref,
 
transaction = ref_transaction_begin();
if