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

On 12/09/2014 12:32 AM, Stefan Beller wrote:

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

quoted

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 <redacted>

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

quoted

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

quoted

+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.

Will fix.

expire_reflog_ent() is passed to for_each_reflog_ent() and therefore
must be an each_reflog_ent_fn. If it returns a nonzero value, the
iteration is ended prematurely and the value is returned to the caller
of for_each_reflog_ent(). We don't ever want to end the iteration
prematurely here, so we always return 0.

quoted

+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?

Both styles are used in our codebase, and I don't think the style guide
says anything about it. My practice in such cases is:

* If I'm modifying existing code, preserve the existing style (to avoid
unnecessary churn)
* If most of our code uses one style, then use that style
* If our code uses both styles frequently, just use whatever style looks
better to me

If and when somebody cares enough to build a consensus for one policy or
the other and to submit a patch to the CodingGuidelines I will be happy
to follow it.

quoted

+ *
+ * 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 ?

Yes, that would be clearer. I will make the change.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu