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

[Taylor Blau <me@xxxxxxxxxxxx>]

On Fri, Feb 18, 2022 at 08:20:13PM +0100, Ævar Arnfjörð Bjarmason wrote:
> diff --git a/builtin/stash.c b/builtin/stash.c
> index d0967b3d3c3..7b939576720 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -635,11 +635,9 @@ static int reflog_is_empty(const char *refname)
>
>  static int do_drop_stash(struct stash_info *info, int quiet)
>  {
> -	int ret;
> -	ret = reflog_delete(info->revision.buf,
> -			    EXPIRE_REFLOGS_REWRITE | EXPIRE_REFLOGS_REWRITE,
> -			    0);
> -	if (!ret) {
> +	unsigned int flags = EXPIRE_REFLOGS_REWRITE | EXPIRE_REFLOGS_REWRITE;
> +
> +	if (!reflog_delete(info->revision.buf, flags, 0)) {
>  		if (!quiet)
>  			printf_ln(_("Dropped %s (%s)"), info->revision.buf,
>  				  oid_to_hex(&info->w_commit));
>
> But, having written that I notice that we have *_REWRITE twice there, so
> I almost just carried forward a new bug in 3/3 when composing this :)
>
> So one should be EXPIRE_REFLOGS_UPDATE_REF, presumably.

Thanks for pointing it out. Just thinking aloud here, the old code ran:

    git reflog delete --updateref --rewrite

where `--updateref` sets the `EXPIRE_REFLOGS_UPDATE_REF` bit and
`-rewrite` sets the `EXPIRE_REFLOGS_REWRITE` bit.

So yep, you and I have the same conclusion there, and one of these flags
should be the UPDATE_REF variant.

> And perhaps it's a big pain, but that suggests that the code isn't
> either used at all, or that we're missing a test for it.

I think this is much more in the "lacking test coverage" category than
the "unused features we should remove" one.

Thanks,
Taylor