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

[Michael Haggerty <mhagger@alum.mit.edu>]

On 04/15/2013 06:51 PM, Junio C Hamano wrote:
> 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?

I find either way of writing it hard to read quickly.  I find that the
construct

    if (!(situation in which the function should continue))
        return

makes it easier to pick out the "situation in which the function should
continue".  But granted, the nesting of multiple parentheses across
lines compromises the clarity.

In projects where I can choose the coding standard, I like to use extra
whitespace to help the eye pick out the range of parentheses, like

        if (!(
                (flags & DO_FOR_EACH_INCLUDE_BROKEN) ||
                ref_resolves_to_object(entry)
                ))
                return 0;

but I've never seen this style in the Git project so I haven't used it here.

Since you prefer the other version, I will change it.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/