Re: [PATCH 2/3] reflog: call reflog_delete from reflog.c

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

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

> From: John Cai <johncai86@gmail.com>
>
> Now that reflog is libified into reflog.c, we can call reflog_delete
> from the reflog.c library.
>
> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
>  builtin/reflog.c | 42 ++----------------------------------------
>  1 file changed, 2 insertions(+), 40 deletions(-)
>
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 65198320cd2..03d347e5832 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -316,12 +316,10 @@ static const char * reflog_delete_usage[] = {
>  
>  static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
>  {
> -	struct cmd_reflog_expire_cb cmd = { 0 };
>  	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"),
>  			EXPIRE_REFLOGS_DRY_RUN),
> @@ -337,48 +335,12 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
>  
>  	argc = parse_options(argc, argv, prefix, options, reflog_delete_usage, 0);
>  
> -	if (verbose)
> -		should_prune_fn = should_expire_reflog_ent_verbose;
> -
>  	if (argc < 1)
>  		return error(_("no reflog specified to delete"));
>  
> -	for (i = 0; i < argc; i++) {
> -		const char *spec = strstr(argv[i], "@{");
> -		char *ep, *ref;
> -		int recno;
> -		struct expire_reflog_policy_cb cb = {
> -			.dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
> -		};
> -
> -		if (!spec) {
> -			status |= error(_("not a reflog: %s"), argv[i]);
> -			continue;
> -		}
> -
> -		if (!dwim_log(argv[i], spec - argv[i], NULL, &ref)) {
> -			status |= error(_("no reflog for '%s'"), argv[i]);
> -			continue;
> -		}
> -
> -		recno = strtoul(spec + 2, &ep, 10);
> -		if (*ep == '}') {
> -			cmd.recno = -recno;
> -			for_each_reflog_ent(ref, count_reflog_ent, &cmd);
> -		} else {
> -			cmd.expire_total = approxidate(spec + 2);
> -			for_each_reflog_ent(ref, count_reflog_ent, &cmd);
> -			cmd.expire_total = 0;
> -		}
> +	for (i = 0; i < argc; i++)
> +		status |= reflog_delete(argv[i], flags, verbose);
>  
> -		cb.cmd = cmd;
> -		status |= reflog_expire(ref, flags,
> -					reflog_expiry_prepare,
> -					should_prune_fn,
> -					reflog_expiry_cleanup,
> -					&cb);
> -		free(ref);
> -	}
>  	return status;
>  }


Maybe others will disagree, but per my comment on 1/2 I found reviewing
this locally much easier with this squashed into 1/2 (without the {}
changes I suggested).

I.e. the diff move/rename detection eats up this change & shows that the
combinatino of 1/3 and 2/3 is almost entirely just moving around
existing code (good!).

But without this squashed 1/3 has a reflog_delete() "addition", that we
later can see is mostly just moving things around.

I'll leave it to you to decide what you want to do there, just
suggestion on an otherwise very trivial-to-review change :)