On 10/09/2017 03:11 AM, brian m. carlson wrote:
> Convert update_ref, refs_update_ref, and write_pseudoref to use struct
> object_id. Update the existing callers as well. Remove update_ref_oid,
> as it is no longer needed.
>
> Signed-off-by: brian m. carlson <sandals@xxxxxxxxxxxxxxxxxxxx>
> ---
> bisect.c | 5 +++--
> builtin/am.c | 14 +++++++-------
> builtin/checkout.c | 3 +--
> builtin/clone.c | 14 +++++++-------
> builtin/merge.c | 13 ++++++-------
> builtin/notes.c | 10 +++++-----
> builtin/pull.c | 2 +-
> builtin/reset.c | 4 ++--
> builtin/update-ref.c | 2 +-
> notes-cache.c | 2 +-
> notes-utils.c | 2 +-
> refs.c | 40 ++++++++++++++++------------------------
> refs.h | 5 +----
> sequencer.c | 9 +++------
> t/helper/test-ref-store.c | 10 +++++-----
> transport-helper.c | 3 ++-
> transport.c | 4 ++--
> 17 files changed, 64 insertions(+), 78 deletions(-)
>
> [...]
> diff --git a/refs.c b/refs.c
> index 0a5b68d6fb..51942df7b3 100644
> --- a/refs.c
> +++ b/refs.c
> [...]
> @@ -1003,12 +995,12 @@ int refs_update_ref(struct ref_store *refs, const char *msg,
> int ret = 0;
>
> if (ref_type(refname) == REF_TYPE_PSEUDOREF) {
> - assert(refs == get_main_ref_store());
Was the deletion of the line above intentional?
> - ret = write_pseudoref(refname, new_sha1, old_sha1, &err);
> + ret = write_pseudoref(refname, new_oid, old_oid, &err);
This is not new to your code, but I just noticed a problem here.
`refs_update_ref()` is documented, via its reference to
`ref_transaction_update()`, to allow `new_sha1` (i.e., now `new_oid`) to
be NULL. (NULL signifies that the value of the reference shouldn't be
changed.)
But `write_pseudoref()` dereferences its `oid` argument unconditionally,
so this call would fail if `new_oid` is NULL.
This has all been the case since `write_pseudoref()` was introduced in
74ec19d4be (pseudorefs: create and use pseudoref update and delete
functions, 2015-07-31).
In my opinion, `write_pseudoref()` is broken. It should accept NULL as
its `oid` argument.
> } else {
> t = ref_store_transaction_begin(refs, &err);
> if (!t ||
> - ref_transaction_update(t, refname, new_sha1, old_sha1,
> + ref_transaction_update(t, refname, new_oid ? new_oid->hash : NULL,
> + old_oid ? old_oid->hash : NULL,
> flags, msg, &err) ||
> ref_transaction_commit(t, &err)) {
> ret = 1;
> [...]
Michael
