Re: [PATCH 20/23] reflog_expire(): new function in the reference API

From: Stefan Beller <sbeller@google.com>
Date: Dec 8, 2014, 23:32 UTC
Message-ID: <20141208233217.GL25562@google.com>
In-Reply-To: <1417734515-11812-21-git-send-email-mhagger@alum.mit.edu>

On Fri, Dec 05, 2014 at 12:08:32AM +0100, Michael Haggerty wrote:

Show 6 quoted lines

> Move expire_reflog() into refs.c and rename it to reflog_expire().
> Turn the three policy functions into function pointers that are passed
> into reflog_expire(). Add function prototypes and documentation to
> refs.h.
> 
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

With or without the nits fixed
Reviewed-by: Stefan Beller <sbeller@google.com>
as the nits are not degrading functionality.

Show 6 quoted lines

> ---
>  builtin/reflog.c | 133 +++++++------------------------------------------------
>  refs.c           | 114 +++++++++++++++++++++++++++++++++++++++++++++++
>  refs.h           |  45 +++++++++++++++++++
>  3 files changed, 174 insertions(+), 118 deletions(-)
>

> +static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
> +		const char *email, unsigned long timestamp, int tz,
> +		const char *message, void *cb_data)

Nit: According to our Codingguidelines we want to indent it further, so it aligns with
the arguments from the first line.

+static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
+                             const char *email, unsigned long timestamp, int tz,
+                             const char *message, void *cb_data)

> +	}
> +	return 0;

Why do we need the return value for expire_reflog_ent? The "return 0:" at the very end of the function is the only return I see here.

Show 6 quoted lines

> +enum expire_reflog_flags {
> +	EXPIRE_REFLOGS_DRY_RUN = 1 << 0,
> +	EXPIRE_REFLOGS_UPDATE_REF = 1 << 1,
> +	EXPIRE_REFLOGS_VERBOSE = 1 << 2,
> +	EXPIRE_REFLOGS_REWRITE = 1 << 3
> +};

Sometimes we align the assigned numbers and sometimes we don't in git, so an alternative would be

enum expire_reflog_flags {
     EXPIRE_REFLOGS_DRY_RUN    = 1 << 0,
     EXPIRE_REFLOGS_UPDATE_REF = 1 << 1,
     EXPIRE_REFLOGS_VERBOSE    = 1 << 2,
     EXPIRE_REFLOGS_REWRITE    = 1 << 3
}

Do we have a preference in the coding style on this one?

> + *
> + * reflog_expiry_select_fn -- Called once for each entry in the
> + *     existing reflog. It should return true iff that entry should be
> + *     pruned.

Also I know how we got here, I wonder if we should inverse the logic here
(in a later patch). "select" sounds to me as if the line is selected to keep it.
However the opposite is true. To actually select (keep) the line we need to return
0. Would it make sense to rename this to reflog_expiry_should_prune_fn ?