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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 18, 2022, 20:21 UTC
Message-ID: <xmqqk0dsylgj.fsf@gitster.g>
In-Reply-To: <9e17ece8d8956c7fd41b7be2f5c0475b1f9af6ec.1645209647.git.gitgitgadget@gmail.com>

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

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

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?

Show 8 quoted lines

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

Show 8 quoted lines

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

Show 16 quoted lines

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