Re: [PATCH v2 25/25] refs: document the lifetime of the args passed to each_ref_fn

From: Junio C Hamano <gitster@pobox.com>
Date: May 29, 2013, 16:54 UTC
Message-ID: <7vd2s9via5.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1369472904-12875-26-git-send-email-mhagger@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

Show 6 quoted lines

> The commits leading up to this have (hopefully) fixed all of the
> callers of the for_each_ref()-like functions.  This commit does the
> last step: documents what each_ref_fn callbacks can assume about
> object lifetimes.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

I looked at all the patches in the series and they all looked sensible.

I have a few comments (sent as individual review) but all of the suggestions in them are "by the way I noticed this issue that is not new to this series while I was reading the code, and we may want to fix it elsewhere", not "this is broken in the patch, we need to fix it before queuing".

Thanks for a pleasant read.

Show 38 quoted lines

> ---
>  refs.h | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/refs.h b/refs.h
> index a35eafc..122ec03 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -15,13 +15,23 @@ struct ref_lock {
>  #define REF_ISBROKEN 0x04
>  
>  /*
> - * Calls the specified function for each ref file until it returns
> - * nonzero, and returns the value.  Please note that it is not safe to
> - * modify references while an iteration is in progress, unless the
> - * same callback function invocation that modifies the reference also
> - * returns a nonzero value to immediately stop the iteration.
> + * The signature for the callback function for the for_each_*()
> + * functions below.  The memory pointed to by the refname and sha1
> + * arguments is only guaranteed to be valid for the duration of a
> + * single callback invocation.
> + */
> +typedef int each_ref_fn(const char *refname,
> +			const unsigned char *sha1, int flags, void *cb_data);
> +
> +/*
> + * The following functions invoke the specified callback function for
> + * each reference indicated.  If the function ever returns a nonzero
> + * value, stop the iteration and return that value.  Please note that
> + * it is not safe to modify references while an iteration is in
> + * progress, unless the same callback function invocation that
> + * modifies the reference also returns a nonzero value to immediately
> + * stop the iteration.
>   */
> -typedef int each_ref_fn(const char *refname, const unsigned char *sha1, int flags, void *cb_data);
>  extern int head_ref(each_ref_fn, void *);
>  extern int for_each_ref(each_ref_fn, void *);
>  extern int for_each_ref_in(const char *, each_ref_fn, void *);