On Fri, Feb 18 2022, John Cai via GitGitGadget wrote:
> From: John Cai <johncai86@xxxxxxxxx>
>
> Now that cmd_reflog_delete has been libified an exported it into a new
> reflog.c library so we can call it directly from builtin/stash.c. This
> not only gives us a performance gain since we don't need to create a
> subprocess, but it also allows us to use the ref transactions api in the
> future.
>
> Helped-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
> Signed-off-by: John Cai <johncai86@xxxxxxxxx>
Very nicely done, and nice that despite the ~500 lines added/removed in
the diffstat that the "actual" changes in this series are so small.q
> @@ -635,18 +636,9 @@ static int reflog_is_empty(const char *refname)
> static int do_drop_stash(struct stash_info *info, int quiet)
> {
> int ret;
> - struct child_process cp_reflog = CHILD_PROCESS_INIT;
> -
Nit: We usually separate variables decls with a \n\n, as is done in the
pre-image, but you end up dropping that.
> - /*
> - * reflog does not provide a simple function for deleting refs. One will
> - * need to be added to avoid implementing too much reflog code here
> - */
> -
> - cp_reflog.git_cmd = 1;
> - strvec_pushl(&cp_reflog.args, "reflog", "delete", "--updateref",
> - "--rewrite", NULL);
> - strvec_push(&cp_reflog.args, info->revision.buf);
> - ret = run_command(&cp_reflog);
> + ret = reflog_delete(info->revision.buf,
> + EXPIRE_REFLOGS_REWRITE | EXPIRE_REFLOGS_REWRITE,
> + 0);
> if (!ret) {
> if (!quiet)
> printf_ln(_("Dropped %s (%s)"), info->revision.buf,
I think per the above squashing this in would be nice, i.e. you get rid
of the long line & it'sclear that "ret" is not used for anything now:
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.
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.
So adding a prep commit to this series where we either drop it, or add
the missing test would be a very nice addition.
See my quite recent 5ac15ad2509 (reflog tests: add --updateref tests,
2021-10-16) for adding such tests, but in that case I just covered "git
reflog" itself, not "git stash". Maybe we can just add something to that
for-loop in t1417 (or similar for a new stash test).
Also, a s/unsigned int flags/enum expire_reflog_flags/ while we're at it
would be very nice here, but that could be done as another small prep
commit. I.e. it's *not* a new issue since cmd_reflog_delete() had it
before, but when converting this to a documented API it would be very
nice to have it reflect the actual type we end up using.
