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

[Junio C Hamano <gitster@pobox.com>]

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

I do not quite get this step.  Yes, eventually cmd_reflog_delete()
and cmd_stash() can both call the new reflog_delete() function, but
at this step, cmd_reflog_delete() should ALREADY be able to call it,
but I see duplicated code.

Why?

> @@ -726,6 +320,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
>  	int i, status = 0;
>  	unsigned int flags = 0;
>  	int verbose = 0;
> +
>  	reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
>  	const struct option options[] = {
>  		OPT_BIT(0, "dry-run", &flags, N_("do not actually prune any entries"),

In other words, why the only change this step makes to
cmd_reflog_delete() is a style change here, instead of replacing the
logic inside its "while loop" with a call to the new helper
function, which is introduced ...

> diff --git a/reflog.c b/reflog.c
> new file mode 100644
> index 00000000000..227ed83b3da
> --- /dev/null
> +++ b/reflog.c
> ...
> +int reflog_delete(const char *rev, int flags, int verbose)
> +{

... here?

> +	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 this were "we moved one iteration of an existing loop to a
separate function, while trying to keep what the existing code
looked like as pristine as possible", then it is fine, but it does
not look like that is what is going on (and I do not think it is
feasible to do so, as the original being an interation in a loop,
it has "continue" here, instead you'd need to arrange to return an
error from here to allow the caller to work in a similar way).

So, if the patch is adding an adjusted implementation by removing
"continue;" that is not suitable in the context of this helper, it
should remove the now-unnecessary {braces} around the single-statement
block.

But I think you should keep {braces} around, because the "if spec is
NULL" case should not be a single-statement block.  You'd need to
return or this code is buggy.

> +	if (!dwim_log(rev, spec - rev, NULL, &ref)) {
> +		status |= error(_("no reflog for '%s'"), rev);
> +	}

I already hinted that the previous "if spec is NULL" code is buggy
because it does not return to allow the original caller to keep
working as before.  Because you lack an early return there, you'll
end up calling dwim_log with spec==NULL here, which does not end
well.