Re: [PATCH 01/13] delete_ref(): move declaration to refs.h

From: Stefan Beller <sbeller@google.com>
Date: Jun 8, 2015, 16:43 UTC
Message-ID: <CAGZ79kYy3zUezRzAG3Uo7bDbLgJ7srj3F1k-ama1C44ZdbHr=Q@mail.gmail.com>
In-Reply-To: <2eea274d60e355e4944ca9ed400e6c2132b4ed32.1433763494.git.mhagger@alum.mit.edu>

On Mon, Jun 8, 2015 at 4:45 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:

Show 62 quoted lines

> 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@alum.mit.edu>
> ---
>  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@google.com>

Show 12 quoted lines

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