Re: [PATCH 3/3] stash: call reflog_delete from reflog.c
From: Taylor Blau <hidden>
Date: 2022-02-19 00:21:53
On Fri, Feb 18, 2022 at 08:20:13PM +0100, Ævar Arnfjörð Bjarmason wrote:
quoted hunk ↗ jump to hunk
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