Re: [PATCH 1/3] reflog: libify delete reflog function and helpers

[Taylor Blau <me@ttaylorr.com>]

On Fri, Feb 18, 2022 at 08:10:07PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > +int reflog_delete(const char *rev, int flags, int verbose)
> > +{
> > +	struct cmd_reflog_expire_cb cmd = { 0 };
> > +	int status = 0;
> > +	reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
> > +	const char *spec = strstr(rev, "@{");
> > +	char *ep, *ref;
> > +	int recno;
> > +	struct expire_reflog_policy_cb cb = {
> > +		.dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
> > +	};
> > +
> > +	if (verbose)
> > +		should_prune_fn = should_expire_reflog_ent_verbose;
> > +
> > +	if (!spec) {
> > +		status |= error(_("not a reflog: %s"), rev);
> > +	}
> > +
> > +	if (!dwim_log(rev, spec - rev, NULL, &ref)) {
> > +		status |= error(_("no reflog for '%s'"), rev);
> > +	}
>
> For these let's follow our usual style of not having braces for
> single-line if's.
>
> Buuuut in this case doing so will make the diff move detection less
> useful for 1..2.
>
> So probably best to leave it, or do some post-cleanup at the end maybe.

Hmm. I don't think the diff detection mechanism would have an
opportunity to kick in here, since the code is added in one patch and
then removed in another. I think I may be missing what you're trying to
say here ;).

In any case, I don't think it's a huge deal if we can't accurately
colorize this with `--color-moved`, so I'd probably just as soon clean
up the style nits in this patch.

> > +int reflog_delete(const char*, int, int);
> > +void reflog_expiry_cleanup(void *);
> > +void reflog_expiry_prepare(const char*, const struct object_id*,
> > +			   void *);
> > +int should_expire_reflog_ent(struct object_id *, struct object_id*,
> > +				    const char *, timestamp_t, int,
> > +				    const char *, void *);
> > +int count_reflog_ent(struct object_id *ooid, struct object_id *noid,
> > +		const char *email, timestamp_t timestamp, int tz,
> > +		const char *message, void *cb_data);
> > +int should_expire_reflog_ent_verbose(struct object_id *,
> > +				     struct object_id *,
> > +				     const char *,
> > +				     timestamp_t, int,
> > +				     const char *, void *);
> > +#endif /* REFLOG_H */
>
> Just a style preference, but I for one prefer the style where we retain
> the parameter names, it helps to read these, especially when we add API
> docs here.
>
> Some of these are mis-indented. We align with the opening "(" with "\t"
> = 8 chars, so e.g. 2x \t + 5 SP for the count_reflog_ent() arguments
> etc.

Thanks for noting on both.

Thanks,
Taylor