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
