Re: [PATCH 20/23] reflog_expire(): new function in the reference API
- From
Michael Haggerty <mhagger@alum.mit.edu>
- Date
- Dec 12, 2014, 08:23 UTC
- Message-ID
- <548AA5E9.9090201@alum.mit.edu>
- In-Reply-To
- <20141208233217.GL25562@google.com>
On 12/09/2014 12:32 AM, Stefan Beller wrote:
Show 27 quoted lines
> 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.
Show 9 quoted lines
> +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.
Show 17 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?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.
Show 9 quoted lines
>> + * >> + * 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