On 04/29/2016 01:40 AM, David Turner wrote: > 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). >> + /* >> + * 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html