Re: [PATCH 10/33] refs: extract a function ref_resolves_to_object()

> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> It is a nice unit of work and soon will be needed from multiple
>> locations.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>>  refs.c | 28 ++++++++++++++++++++--------
>>  1 file changed, 20 insertions(+), 8 deletions(-)
>>
>> diff --git a/refs.c b/refs.c
>> index c523978..dfc8600 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -529,6 +529,22 @@ static void sort_ref_dir(struct ref_dir *dir)
>>  #define DO_FOR_EACH_INCLUDE_BROKEN 0x01
>>  
>>  /*
>> + * Return true iff the reference described by entry can be resolved to
>> + * an object in the database.  Emit a warning if the referred-to
>> + * object does not exist.
>> + */
>> +static int ref_resolves_to_object(struct ref_entry *entry)
>> +{
>> +	if (entry->flag & REF_ISBROKEN)
>> +		return 0;
>> +	if (!has_sha1_file(entry->u.value.sha1)) {
>> +		error("%s does not point to a valid object!", entry->name);
>> +		return 0;
>> +	}
>> +	return 1;
>> +}
>> +
>> +/*
>>   * current_ref is a performance hack: when iterating over references
>>   * using the for_each_ref*() functions, current_ref is set to the
>>   * current reference's entry before calling the callback function.  If
>> @@ -549,14 +565,10 @@ static int do_one_ref(const char *base, each_ref_fn fn, int trim,
>>  	if (prefixcmp(entry->name, base))
>>  		return 0;
>>  
>> -	if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN)) {
>> -		if (entry->flag & REF_ISBROKEN)
>> -			return 0; /* ignore broken refs e.g. dangling symref */
>> -		if (!has_sha1_file(entry->u.value.sha1)) {
>> -			error("%s does not point to a valid object!", entry->name);
>> -			return 0;
>> -		}
>> -	}
>> +	if (!((flags & DO_FOR_EACH_INCLUDE_BROKEN) ||
>> +	      ref_resolves_to_object(entry)))
>> +		return 0;
>> +
> 
> The original says "Unless flags tell us to include broken ones,
> return 0 for the broken ones and the ones that point at invalid
> objects".
> 
> The updated says "Unless flags tell us to include broken ones or the
> ref is a good one, return 0".
> 
> Are they the same?  If we have a good one, and if we are not told to
> include broken one, the original just passes the control down to the
> remainder of the function.  The updated one will return 0.
> 
> Oh, wait, that is not the case.  The OR is inside !( A || B ), so
> what it really wants to say is:
> 
> 	if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN) &&
>             !ref_resolves_to_object(entry))
> 		return 0;
> 
> that is, "When we are told to exclude broken ones and the one we are
> looking at is broken, return 0".
> 
> Am I the only one who was confused with this, or was the way the
> patch wrote this logic unnecessarily convoluted?