Re: [PATCH 1/5] Introduces for_each_revision() helper

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 28, 2007, 02:46 UTC
Message-ID: <Pine.LNX.4.64.0704280446180.12006@racer.site>
In-Reply-To: <1177693212202-git-send-email-lcapitulino@mandriva.com.br>

Hi,

On Fri, 27 Apr 2007, Luiz Fernando N. Capitulino wrote:

Show 20 quoted lines

> diff --git a/revision.h b/revision.h
> index cdf94ad..7be3fc7 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -133,4 +133,15 @@ extern void add_object(struct object *obj,
>  extern void add_pending_object(struct rev_info *revs, struct object *obj, const char *name);
>  extern void add_pending_object_with_mode(struct rev_info *revs, struct object *obj, const char *name, unsigned mode);
>  
> +/* helpers */
> +
> +/**
> + * for_each_revision	- iterate over revisions
> + * @commit:	pointer to a commit object returned for each iteration
> + * @rev:	revision pointer
> + */
> +#define for_each_revision(commit, rev) \
> +	for (prepare_revision_walk(rev); \
> +		  (commit = get_revision(rev)) != NULL; )
> +
>  #endif

I object to this, additionally to the magic argument that I agree to, on the grounds that it is actually wrong. The first iteration will work on an _uninitialized_ "commit" variable.

Furthermore, it is not like it was a huge piece of code that is being replaced by a shortcut. There are better places to do some libification than this.

Ciao, Dscho