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

quoted

From: John Cai <redacted>

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 [off-list ref]
Signed-off-by: John Cai <redacted>

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

quoted

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

quoted

-	/*
-	 * 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).