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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Feb 18 2022, John Cai via GitGitGadget wrote:

> 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 .. :)

> +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.

> +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.