Re: [PATCH 25/29] refs: resolve symbolic refs first

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Apr 29, 2016, 09:51 UTC
Message-ID: <57232EAB.3080406@alum.mit.edu>
In-Reply-To: <1461886803.4123.14.camel@twopensource.com>

On 04/29/2016 01:40 AM, David Turner wrote:

Show 7 quoted lines

> On Wed, 2016-04-27 at 18:57 +0200, Michael Haggerty wrote:
> +retry:
> ...
>> +		if (--attempts_remaining > 0)
>> +			goto retry;
> 
> could this be a loop instead of using gotos?

It certainly could. The goto-vs-loop question was debated on the mailing list when I first added very similar code elsewhere (unfortunately I'm unable to find a link to that conversation). I was persuaded to change my loop into gotos, the argument being that the "retry" case is exceptional and shouldn't be such a dominant part of the function structure. Plus the goto code is briefer and feels less awkward to me in this case (that's subjective, of course).

Show 17 quoted lines

>> +			/*
>> +			 * There is a directory in the way,
>> +			 * but we	 don't know of any references
>> +			 * that it should contain. This might
>> +			 * be a directory that used to contain
>> +			 * references but is now empty. Try to
>> +			 * remove it; otherwise it might cause
>> +			 * trouble when we try to rename the
>> +			 * lockfile into place.
>> +			 */
>> +			strbuf_addf(err, "there is a non-empty directory '%s' "
>> +				          "blocking reference '%s'",
>> +					  ref_file.buf,refname);
>> +			goto error_return;
> 
> We don't actually try to remove anything here, so that comment seems
> wrong?

Thanks, will fix.

Michael