Subject: Re: [PATCH v13 01/13] refs.h: clarify reflog iteration order

From: Junio C Hamano <[email-masked]>
Date: Mon, 18 May 2020 16:31:36 -0700

"Han-Wen Nienhuys via GitGitGadget" <[email-masked]> writes:

> From: Han-Wen Nienhuys <[email-masked]>
>
> Signed-off-by: Han-Wen Nienhuys <[email-masked]>
> ---
>  refs.h | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)

I think the split of the topics was a good first move; now we need
to make sure that we need no more work on the early part.

> diff --git a/refs.h b/refs.h
> index a92d2c74c83..99ba9e331e5 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -432,19 +432,31 @@ int delete_refs(const char *msg, struct string_list *refnames,
>  int refs_delete_reflog(struct ref_store *refs, const char *refname);
>  int delete_reflog(const char *refname);
>  
> -/* iterate over reflog entries */
> +/* Callback to process a reflog entry found by the iteration functions (see
> + * below) */

/*
 * A multi-line comment in our codebase
 * look like this, with the opening slash-asterisk
 * and closing asterisk-slash occupying their own
 * lines.
 */
>  typedef int each_reflog_ent_fn(
>  		struct object_id *old_oid, struct object_id *new_oid,
>  		const char *committer, timestamp_t timestamp,
>  		int tz, const char *msg, void *cb_data);
>  
> +/* Iterate in over reflog entries in the log for `refname`. */

"in over"???  Should this just be "iterate over reflog entries in ..."?

> +
> +/* oldest entry first */
>  int refs_for_each_reflog_ent(struct ref_store *refs, const char *refname,
>  			     each_reflog_ent_fn fn, void *cb_data);
> +
> +/* youngest entry first */
>  int refs_for_each_reflog_ent_reverse(struct ref_store *refs,
>  				     const char *refname,
>  				     each_reflog_ent_fn fn,
>  				     void *cb_data);
> +
> +/* Call a function for each reflog entry in the log for `refname`. */
> +
> +/* oldest entry first */
>  int for_each_reflog_ent(const char *refname, each_reflog_ent_fn fn, void *cb_data);
> +
> +/* youngest entry first */
>  int for_each_reflog_ent_reverse(const char *refname, each_reflog_ent_fn fn, void *cb_data);

The only difference betwen refs_for_each_reflog_ent() and
for_each_reflog_ent() is that the former takes an arbitrary
ref-store while the latter works on the default ref-store, no?  They
should otherwise be identical, so describing one as "Iterate over
reflog entries" while describing the other as "Call a function for
each" feel misleading.