Re: [PATCH 1/3] reflog: libify delete reflog function and helpers
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Feb 18, 2022, 19:10 UTC
- Message-ID
- <220218.86wnhsgf5n.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <9e17ece8d8956c7fd41b7be2f5c0475b1f9af6ec.1645209647.git.gitgitgadget@gmail.com>
On Fri, Feb 18 2022, John Cai via GitGitGadget wrote:
Show 14 quoted lines
> From: John Cai <johncai86@gmail.com> > > Currently stash shells out to reflog in order to delete refs. In an > effort to reduce how much we shell out to a subprocess, libify the > functionality that stash needs into reflog.c. > > Add a reflog_delete function that is pretty much the logic in the while > loop in builtin/reflog.c cmd_reflog_delete(). This is a function that > builtin/reflog.c and builtin/stash.c can both call. > > Also move functions needed by reflog_delete and export them. > > Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > Signed-off-by: John Cai <johncai86@gmail.com>
Yay! This looks great. Even though I'll need to deal with some conflicts locally .. :)
Show 22 quoted lines
> +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.
Show 16 quoted lines
> +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.