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@xxxxxxxxxxxx>
>
> With or without the nits fixed
> Reviewed-by: Stefan Beller <sbeller@xxxxxxxxxx>
> 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@xxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html