On Mon, Jun 8, 2015 at 4:45 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote:
> Also
>
> * Add a docstring
>
> * Rename the second parameter to "old_sha1", to be consistent with the
> convention used elsewhere in the refs module
>
> Signed-off-by: Michael Haggerty <mhagger@xxxxxxxxxxxx>
> ---
> cache.h | 2 --
> refs.c | 5 +++--
> refs.h | 9 +++++++++
> 3 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 571c98f..be92121 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -585,8 +585,6 @@ extern void update_index_if_able(struct index_state *, struct lock_file *);
> extern int hold_locked_index(struct lock_file *, int);
> extern void set_alternate_index_output(const char *);
>
> -extern int delete_ref(const char *, const unsigned char *sha1, unsigned int flags);
> -
> /* Environment bits from configuration mechanism */
> extern int trust_executable_bit;
> extern int trust_ctime;
> diff --git a/refs.c b/refs.c
> index a742d79..b575bb8 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2789,7 +2789,8 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
> return 0;
> }
>
> -int delete_ref(const char *refname, const unsigned char *sha1, unsigned int flags)
> +int delete_ref(const char *refname, const unsigned char *old_sha1,
> + unsigned int flags)
> {
> struct ref_transaction *transaction;
> struct strbuf err = STRBUF_INIT;
> @@ -2797,7 +2798,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, unsigned int flag
> transaction = ref_transaction_begin(&err);
> if (!transaction ||
> ref_transaction_delete(transaction, refname,
> - (sha1 && !is_null_sha1(sha1)) ? sha1 : NULL,
> + (old_sha1 && !is_null_sha1(old_sha1)) ? old_sha1 : NULL,
> flags, NULL, &err) ||
> ref_transaction_commit(transaction, &err)) {
> error("%s", err.buf);
> diff --git a/refs.h b/refs.h
> index 8c3d433..8785bca 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -202,6 +202,15 @@ extern int read_ref_at(const char *refname, unsigned int flags,
> /** Check if a particular reflog exists */
> extern int reflog_exists(const char *refname);
>
> +/*
> + * Delete the specified reference. If old_sha1 is non-NULL and not
> + * NULL_SHA1, then verify that the current value of the reference is
> + * old_sha1 before deleting it.
And here I wondered what the distinction between NULL and non-NULL,
but NULL_SHA1
is and digging into the code, there is none. (As you can also see in
this patch above with
(old_sha1 && !is_null_sha1(old_sha1)) ? old_sha1 : NULL,
but when digging deeper, the !is_null_sha1(old_sha1) is an arbitrary
limitation (i.e.
ref_transaction_delete and ref_transaction_update don't differ between
those two cases.)
The distinction comes in at lock_ref_sha1_basic, where I think we may
want to get rid of
the is_null_sha1 check and depend on NULL/non-NULL as the difference
for valid and invalid
sha1's?
That said, do we want to add another sentence to the doc here saying
non-NULL and not
NULL_SHA1 are treated the same or is it clear enough?
With or without this question addressed:
Reviewed-by: Stefan Beller <sbeller@xxxxxxxxxx>
> flags is passed to
> + * ref_transaction_delete().
> + */
> +extern int delete_ref(const char *refname, const unsigned char *old_sha1,
> + unsigned int flags);
> +
> /** Delete a reflog */
> extern int delete_reflog(const char *refname);
>
> --
> 2.1.4
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html