Re: [PATCH 01/13] delete_ref(): move declaration to refs.h
From: Stefan Beller <hidden>
Date: 2016-06-15 23:05:12
On Mon, Jun 8, 2015 at 4:45 AM, Michael Haggerty [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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 <redacted> --- 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 <redacted>
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