Re: [PATCH 3/3] stash: 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 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@gmail.com>
> Signed-off-by: John Cai <johncai86@gmail.com>

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.