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

[Michael Haggerty <mhagger@alum.mit.edu>]

On 12/09/2014 12:32 AM, Stefan Beller wrote:
> On Fri, Dec 05, 2014 at 12:08:32AM +0100, Michael Haggerty wrote:
>> 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.
> 
>> ---
>>  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.

Will fix.

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

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.

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

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