Re: [PATCH v2 4/7] struct ref_lock: delete the force_write member
- From
Michael Haggerty <mhagger@alum.mit.edu>
- Date
- Mar 3, 2015, 10:50 UTC
- Message-ID
- <54F59212.5040105@alum.mit.edu>
- In-Reply-To
- <xmqqk2yz9ir9.fsf@gitster.dls.corp.google.com>
On 03/02/2015 10:44 PM, Junio C Hamano wrote:
Show 34 quoted lines
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> Instead, compute the value when it is needed.
>
>> @@ -2318,8 +2317,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>> lock->ref_name = xstrdup(refname);
>> lock->orig_ref_name = xstrdup(orig_refname);
>> ref_file = git_path("%s", refname);
>> - if ((flags & REF_NODEREF) && (type & REF_ISSYMREF))
>> - lock->force_write = 1;
>>
>> retry:
>> switch (safe_create_leading_directories(ref_file)) {
>> @@ -3787,8 +3784,13 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>> struct ref_update *update = updates[i];
>>
>> if (!is_null_sha1(update->new_sha1)) {
>> - if (!update->lock->force_write &&
>> - !hashcmp(update->lock->old_sha1, update->new_sha1)) {
>> + if (!((update->type & REF_ISSYMREF)
>> + && (update->flags & REF_NODEREF))
>> + && !hashcmp(update->lock->old_sha1, update->new_sha1)) {
>> + /*
>> + * The reference already has the desired
>> + * value, so we don't need to write it.
>> + */
>> unlock_ref(update->lock);
>> update->lock = NULL;
>> } else if (write_ref_sha1(update->lock, update->new_sha1,
>
> The code before and after the change are equivalent.
>
> It shouldn't be the case, but somehow I find the original slightly
> easier to understand. [...]I had the same feeling; thanks for confirming it. How about I introduce a temporary variable `overwriting_symref` as an aid to the reader? I think this makes it pretty clear:
Show 15 quoted lines
> if (!is_null_sha1(update->new_sha1)) {
> - if (!update->lock->force_write &&
> - !hashcmp(update->lock->old_sha1, update->new_sha1)) {
> + int overwriting_symref = ((update->type & REF_ISSYMREF) &&
> + (update->flags & REF_NODEREF));
> +
> + if (!overwriting_symref
> + && !hashcmp(update->lock->old_sha1, update->new_sha1)) {
> + /*
> + * The reference already has the desired
> + * value, so we don't need to write it.
> + */
> unlock_ref(update->lock);
> update->lock = NULL;
> } else if (write_ref_sha1(update->lock, update->new_sha1,Michael
-- Michael Haggerty mhagger@alum.mit.edu