Web lists-archives.com

Re: [RFC PATCH 2/6] refs/refs-internal.h: avoid forward declaration of an enum




On Sun, Jul 08, 2018 at 04:43:38PM +0200, Beat Bolli wrote:

> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index dd834314bd..a78b5cb803 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -1,6 +1,8 @@
>  #ifndef REFS_REFS_INTERNAL_H
>  #define REFS_REFS_INTERNAL_H
>  
> +#include "iterator.h"   /* for enum iterator_selection */

IMHO this kind of comment does more harm than good, because it is so
prone to going stale (nobody is going to bother updating it when they
add new dependencies on iterator.h). Anybody who is interested in the
original reason can use "git blame" to dig up your commit message. And
anybody who is thinking about deleting that line would need to dig into
whether anything had been added in the meantime that also requires the
include.

So at best it's redundant, and at worst it's slightly misleading. :)

Not worth a re-roll by itself, but it looked like you had a few other
bits in the other patches to address.

Other than this minor quibble, the whole series looks good to me, modulo
the existing review.

-Peff