libify reflog

From: John Cai <[email protected]>

Currently stash shells out to reflog in order to delete refs. In an effort to reduce how much we shell out to a subprocess, libify the functionality that stash needs into reflog.c.

Add a reflog_delete function that is pretty much the logic in the while loop in builtin/reflog.c cmd_reflog_delete(). This is a function that builtin/reflog.c and builtin/stash.c can both call.

Also move functions needed by reflog_delete and export them.

Helped-by Ævar Arnfjörð Bjarmason <[email protected]>

Signed-off-by John Cai <[email protected]>

reflog.c new file +12 -0

+int reflog_delete(const char *rev, int flags, int verbose)
+{
+	struct cmd_reflog_expire_cb cmd = { 0 };
+	int status = 0;
+	reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
+	const char *spec = strstr(rev, "@{");
+	char *ep, *ref;
+	int recno;
+	struct expire_reflog_policy_cb cb = {
+		.dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
+	};
+

reflog.h new file +6 -0

+int reflog_delete(const char*, int, int);
+void reflog_expiry_cleanup(void *);
+void reflog_expiry_prepare(const char*, const struct object_id*,
+			   void *);
+int should_expire_reflog_ent(struct object_id *, struct object_id*,
+				    const char *, timestamp_t, int,

From: John Cai <[email protected]>

Now that reflog is libified into reflog.c, we can call reflog_delete from the reflog.c library.

Helped-by Ævar Arnfjörð Bjarmason <[email protected]>

Signed-off-by John Cai <[email protected]>

---
 builtin/reflog.c | 42 ++----------------------------------------
 1 file changed, 2 insertions(+), 40 deletions(-)

builtin/reflog.c +2 -32

index 65198320cd2..03d347e5832 100644
@@ -316,12 +316,10 @@ static const char * reflog_delete_usage[] = {
 
 static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 {
-	struct cmd_reflog_expire_cb cmd = { 0 };
 	int i, status = 0;
 	unsigned int flags = 0;
 	int verbose = 0;
 
-	reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
 	const struct option options[] = {
 		OPT_BIT(0, "dry-run", &flags, N_("do not actually prune any entries"),
 			EXPIRE_REFLOGS_DRY_RUN),

@@ -337,48 +335,12 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, prefix, options, reflog_delete_usage, 0);
 
-	if (verbose)
-		should_prune_fn = should_expire_reflog_ent_verbose;
-
 	if (argc < 1)
 		return error(_("no reflog specified to delete"));
 
-	for (i = 0; i < argc; i++) {
-		const char *spec = strstr(argv[i], "@{");
-		char *ep, *ref;
-		int recno;
-		struct expire_reflog_policy_cb cb = {
-			.dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
-		};
-
-		if (!spec) {
-			status |= error(_("not a reflog: %s"), argv[i]);
-			continue;
-		}
-
-		if (!dwim_log(argv[i], spec - argv[i], NULL, &ref)) {
-			status |= error(_("no reflog for '%s'"), argv[i]);
-			continue;
-		}
-
-		recno = strtoul(spec + 2, &ep, 10);
-		if (*ep == '}') {
-			cmd.recno = -recno;
-			for_each_reflog_ent(ref, count_reflog_ent, &cmd);
-		} else {
-			cmd.expire_total = approxidate(spec + 2);
-			for_each_reflog_ent(ref, count_reflog_ent, &cmd);
-			cmd.expire_total = 0;
-		}
+	for (i = 0; i < argc; i++)
+		status |= reflog_delete(argv[i], flags, verbose);
 

From: John Cai <[email protected]>

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 <[email protected]>

Signed-off-by John Cai <[email protected]>

(quoted diff) +0 -6

-	/*
-	 * 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;

In [1], there was a discussion around a bug report of stash not recovering in the middle of the process. It turned out to not be a bug we need to fix. However, out of that discussion came the idea of libifying reflog. This can stand alone as a code improvement.

stash.c currently shells out to call reflog to delete reflogs. Libify reflog delete and call it from both builtin/reflog.c and builtin/stash.c.

This patch has three parts:

* libify reflog's delete functionality and move some of the helpers into a reflog.c library and export them
* call reflog_delete from builtin/reflog.c
* call reflog_delete from builtin/stash.c

1. https://lore.kernel.org/git/[email protected]/

From: John Cai <[email protected]>

Currently stash shells out to reflog in order to delete refs. In an effort to reduce how much we shell out to a subprocess, libify the functionality that stash needs into reflog.c.

Add a reflog_delete function that is pretty much the logic in the while loop in builtin/reflog.c cmd_reflog_delete(). This is a function that builtin/reflog.c and builtin/stash.c can both call.

Quoted 12 lines

reflog.c new file +12 -0

+int reflog_delete(const char *rev, int flags, int verbose)
+{
+	struct cmd_reflog_expire_cb cmd = { 0 };
+	int status = 0;
+	reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
+	const char *spec = strstr(rev, "@{");
+	char *ep, *ref;
+	int recno;
+	struct expire_reflog_policy_cb cb = {
+		.dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
+	};
+

reflog.c line 408

Quoted 10 lines

reflog.c new file +10 -0

+	if (verbose)
+		should_prune_fn = should_expire_reflog_ent_verbose;
+
+	if (!spec) {
+		status |= error(_("not a reflog: %s"), rev);
+	}
+
+	if (!dwim_log(rev, spec - rev, NULL, &ref)) {
+		status |= error(_("no reflog for '%s'"), rev);
+	}

For these let's follow our usual style of not having braces for single-line if's.

Buuuut in this case doing so will make the diff move detection less useful for 1..2.

So probably best to leave it, or do some post-cleanup at the end maybe.

Quoted 6 lines

reflog.h new file +6 -0

+int reflog_delete(const char*, int, int);
+void reflog_expiry_cleanup(void *);
+void reflog_expiry_prepare(const char*, const struct object_id*,
+			   void *);
+int should_expire_reflog_ent(struct object_id *, struct object_id*,
+				    const char *, timestamp_t, int,

reflog.h line 49

Quoted 10 lines

reflog.h new file +10 -0

+				    const char *, void *);
+int count_reflog_ent(struct object_id *ooid, struct object_id *noid,
+		const char *email, timestamp_t timestamp, int tz,
+		const char *message, void *cb_data);
+int should_expire_reflog_ent_verbose(struct object_id *,
+				     struct object_id *,
+				     const char *,
+				     timestamp_t, int,
+				     const char *, void *);
+#endif /* REFLOG_H */

Just a style preference, but I for one prefer the style where we retain the parameter names, it helps to read these, especially when we add API docs here.

Some of these are mis-indented. We align with the opening "(" with "\t" = 8 chars, so e.g. 2x \t + 5 SP for the count_reflog_ent() arguments etc.

Quoted 26 lines · Ævar Arnfjörð Bjarmason

Quoted 12 lines

reflog.c new file +12 -0

+int reflog_delete(const char *rev, int flags, int verbose)
+{
+	struct cmd_reflog_expire_cb cmd = { 0 };
+	int status = 0;
+	reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
+	const char *spec = strstr(rev, "@{");
+	char *ep, *ref;
+	int recno;
+	struct expire_reflog_policy_cb cb = {
+		.dry_run = !!(flags & EXPIRE_REFLOGS_DRY_RUN),
+	};
+

reflog.c line 408

Quoted 10 lines

reflog.c new file +10 -0

+	if (verbose)
+		should_prune_fn = should_expire_reflog_ent_verbose;
+
+	if (!spec) {
+		status |= error(_("not a reflog: %s"), rev);
+	}
+
+	if (!dwim_log(rev, spec - rev, NULL, &ref)) {
+		status |= error(_("no reflog for '%s'"), rev);
+	}

For these let's follow our usual style of not having braces for single-line if's.

Quoted 4 lines

Buuuut in this case doing so will make the diff move detection less useful for 1..2.

So probably best to leave it, or do some post-cleanup at the end maybe.

Hmm. I don't think the diff detection mechanism would have an opportunity to kick in here, since the code is added in one patch and then removed in another. I think I may be missing what you're trying to say here ;).

In any case, I don't think it's a huge deal if we can't accurately colorize this with --color-moved, so I'd probably just as soon clean up the style nits in this patch.

reflog.h new file +0 -0

new file mode 100644
index 00000000000..e4a8a104f45

reflog.h new file +38 -0

+struct cmd_reflog_expire_cb {
+	int stalefix;
+	int explicit_expiry;
+	timestamp_t expire_total;
+	timestamp_t expire_unreachable;
+	int recno;
+};
+
+struct expire_reflog_policy_cb {
+	enum {
+		UE_NORMAL,
+		UE_ALWAYS,
+		UE_HEAD
+	} unreachable_expire_kind;
+	struct commit_list *mark_list;
+	unsigned long mark_limit;
+	struct cmd_reflog_expire_cb cmd;
+	struct commit *tip_commit;
+	struct commit_list *tips;
+	unsigned int dry_run:1;
+};
+
+int reflog_delete(const char*, int, int);
+void reflog_expiry_cleanup(void *);
+void reflog_expiry_prepare(const char*, const struct object_id*,
+			   void *);
+int should_expire_reflog_ent(struct object_id *, struct object_id*,
+				    const char *, timestamp_t, int,
+				    const char *, void *);
+int count_reflog_ent(struct object_id *ooid, struct object_id *noid,
+		const char *email, timestamp_t timestamp, int tz,
+		const char *message, void *cb_data);
+int should_expire_reflog_ent_verbose(struct object_id *,
+				     struct object_id *,
+				     const char *,
+				     timestamp_t, int,
+				     const char *, void *);
+#endif /* REFLOG_H */

From: John Cai <[email protected]>

Currently stash shells out to reflog in order to delete refs. In an effort to reduce how much we shell out to a subprocess, libify the functionality that stash needs into reflog.c.

Add a reflog_delete function that is pretty much the logic in the while loop in builtin/reflog.c cmd_reflog_delete(). This is a function that builtin/reflog.c and builtin/stash.c can both call.

reflog.c new file +6 -0

+	struct cmd_reflog_expire_cb cmd = { 0 };
+	int status = 0;
+	reflog_expiry_should_prune_fn *should_prune_fn = should_expire_reflog_ent;
+	const char *spec = strstr(rev, "@{");
+	char *ep, *ref;
+	int recno;

Maybe others will disagree, but per my comment on 1/2 I found reviewing this locally much easier with this squashed into 1/2 (without the {} changes I suggested).

builtin/stash.c +3 -5

index d0967b3d3c3..7b939576720 100644
@@ -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.

Quoted 6 lines · John Cai via GitGitGadget

reflog.h new file +0 -0

new file mode 100644
index 00000000000..e4a8a104f45

reflog.h line 10

Quoted 10 lines

reflog.h new file +10 -0

@@ -0,0 +1,49 @@
+#ifndef REFLOG_H
+#define REFLOG_H
+
+#include "cache.h"
+#include "commit.h"
+
+/* Remember to update object flag allocation in object.h */
+#define INCOMPLETE	(1u<<10)
+#define STUDYING	(1u<<11)
+#define REACHABLE	(1u<<12)

These names were unique enough back when they were part of internal implementation detail inside builtin/reflog.c but it no longer is in the scope it is visible. builtin/stash.c (and whatever other things that may want to deal with reflogs in the future) is about stash entries and wants to have a way to differentiate these symbols from others and hint that these are about reflog operation.

Perhaps prefix them with REFLOG_ or something?

Or, do we even NEED to expose these bits outside the implementation of reflog.c? If these three constants are used ONLY by reflog.c and not by builtin/reflog.c (or builtin/stash.c), then moving them to where they are used, i.e. in reflog.c, without exposing them to others in reflog.h, would be a far better thing to do. That way, they can stay with their original name, without having to add a differentiating prefix.

Quoted 28 lines · Junio C Hamano

Quoted 6 lines · John Cai via GitGitGadget

reflog.h new file +0 -0

new file mode 100644
index 00000000000..e4a8a104f45

reflog.h line 10

Quoted 10 lines

reflog.h new file +10 -0

@@ -0,0 +1,49 @@
+#ifndef REFLOG_H
+#define REFLOG_H
+
+#include "cache.h"
+#include "commit.h"
+
+/* Remember to update object flag allocation in object.h */
+#define INCOMPLETE	(1u<<10)
+#define STUDYING	(1u<<11)
+#define REACHABLE	(1u<<12)

These names were unique enough back when they were part of internal implementation detail inside builtin/reflog.c but it no longer is in the scope it is visible. builtin/stash.c (and whatever other things that may want to deal with reflogs in the future) is about stash entries and wants to have a way to differentiate these symbols from others and hint that these are about reflog operation.

Perhaps prefix them with REFLOG_ or something?

Quoted 7 lines

Or, do we even NEED to expose these bits outside the implementation of reflog.c? If these three constants are used ONLY by reflog.c and not by builtin/reflog.c (or builtin/stash.c), then moving them to where they are used, i.e. in reflog.c, without exposing them to others in reflog.h, would be a far better thing to do. That way, they can stay with their original name, without having to add a differentiating prefix.

No objection to this, but FWIW the general pattern for these object.h flags is to use these un-prefixed names:

git grep -A20 'Remember to update object flag allocation' | grep define

To be fair the only one that's really comparable is the revision.h ones, but those are very non-namespace-y, with names like SEEN, ADDED, SHOWN etc.

I'd be fine with just leaving these as-is, especially with compilers warning about macro re-definitions.

Quoted 9 lines

Perhaps prefix them with REFLOG_ or something?

Or, do we even NEED to expose these bits outside the implementation of reflog.c? If these three constants are used ONLY by reflog.c and not by builtin/reflog.c (or builtin/stash.c), then moving them to where they are used, i.e. in reflog.c, without exposing them to others in reflog.h, would be a far better thing to do. That way, they can stay with their original name, without having to add a differentiating prefix.

No objection to this, but FWIW the general pattern for these object.h flags is to use these un-prefixed names:

git grep -A20 'Remember to update object flag allocation' | grep define

To be fair the only one that's really comparable is the revision.h ones, but those are very non-namespace-y, with names like SEEN, ADDED, SHOWN etc.

Quoted 5 lines · John Cai via GitGitGadget

From: John Cai <[email protected]>

Currently stash shells out to reflog in order to delete refs. In an effort to reduce how much we shell out to a subprocess, libify the functionality that stash needs into reflog.c.

Sounds great. For other reviewers, looking at this with --color-moved if you have the patches applied locally makes it much easier to see what is going on here, which is basically:

- All of the implementation that used to be in builtin/reflog.c moved to reflog.c.

- The function signatures and structure declarations moved to reflog.h.

Quoted 3 lines

Add a reflog_delete function that is pretty much the logic in the while loop in builtin/reflog.c cmd_reflog_delete(). This is a function that builtin/reflog.c and builtin/stash.c can both call.

As you mentioned, the reflog_delete() implementation is indeed new. It looks like Ævar reviewed most of it already, so I'll take a look at his message before I end up repeating everything he already said ;).

It's worth noting that the subsequent clean-up to rewrite cmd_reflog_delete() in terms of your new reflog_delete() happens in the subsequent commit. If you end up rerolling this series, mentioning that here may be worthwhile.

One question that I had which I don't see answered already is what the plan is for existing reflog-related functions that live in refs.h. Should or will those functions be moved to the new reflog-specific header, too?

Thanks,
Taylor

Quoted 4 lines

One question that I had which I don't see answered already is what the plan is for existing reflog-related functions that live in refs.h. Should or will those functions be moved to the new reflog-specific header, too?

Thanks for bringing ths up! Maybe this cleanup can be included in a separate patch series since this one is mainly about getting rid of the shell-out in stash.c. What do you think?

Quoted 10 lines · John Cai via GitGitGadget

From: John Cai <[email protected]>

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 <[email protected]>

Signed-off-by John Cai <[email protected]>

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

Review static int reflog_is_empty(const char *refname) · line 639

Quoted 6 lines

(quoted diff) +0 -2

@@ -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 6 lines

(quoted diff) +0 -6

-	/*
-	 * 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;

Review static int reflog_is_empty(const char *refname) · line 644

Quoted 10 lines

(quoted diff) +3 -4

-	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:

builtin/stash.c +3 -5

index d0967b3d3c3..7b939576720 100644
@@ -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.

Hi Ævar,

Quoted 77 lines · Ævar Arnfjörð Bjarmason

Quoted 10 lines · John Cai via GitGitGadget

From: John Cai <[email protected]>

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 <[email protected]>

Signed-off-by John Cai <[email protected]>

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

Review static int reflog_is_empty(const char *refname) · line 639

Quoted 6 lines

(quoted diff) +0 -2

@@ -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 6 lines

(quoted diff) +0 -6

-	/*
-	 * 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;

Review static int reflog_is_empty(const char *refname) · line 644

Quoted 10 lines

(quoted diff) +3 -4

-	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:

builtin/stash.c +3 -5

index d0967b3d3c3..7b939576720 100644
@@ -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.

Quoted 4 lines

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

So I was trying to write a test to exercise the reflog --updateref and --rewrite cases. --updateref is pretty straightforward, but with --rewrite I noticed that it rewrites the old sha in the .git/log/refs/stash file. But, I was having trouble finding somewhere that read this value. The test could reach into this file and check the literal contents, but wondering if there is a better way. Any help appreciated!

Quoted 6 lines

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.

From: John Cai <[email protected]>

There is missing test coverage to ensure that the resulting reflogs after a git stash drop has had its old oid rewritten if applicable, and if the refs/stash has been updated if applicable.

Helped-by Ævar Arnfjörð Bjarmason <[email protected]>

Signed-off-by John Cai <[email protected]>

---
 t/t3903-stash.sh | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

t/t3903-stash.sh +0 -0

index b149e2af441..ec9cc5646d6 100755
@@ -185,10 +185,33 @@ test_expect_success 'drop middle stash by index' '
 	test 1 = $(git show HEAD:file)
 '

From: John Cai <[email protected]>

Currently stash shells out to reflog in order to delete refs. In an effort to reduce how much we shell out to a subprocess, libify the functionality that stash needs into reflog.c.

Add a reflog_delete function that is pretty much the logic in the while loop in builtin/reflog.c cmd_reflog_delete(). This is a function that builtin/reflog.c and builtin/stash.c can both call.

Also move functions needed by reflog_delete and export them.

Helped-by Ævar Arnfjörð Bjarmason <[email protected]>

Signed-off-by John Cai <[email protected]>

---
 Makefile         |   1 +
 builtin/reflog.c | 451 +----------------------------------------------
 object.h         |   2 +-
 reflog.c         | 435 +++++++++++++++++++++++++++++++++++++++++++++
 reflog.h         |  49 +++++
 5 files changed, 490 insertions(+), 448 deletions(-)
 create mode 100644 reflog.c
 create mode 100644 reflog.h

Makefile +1 -0

index 6f0b4b775fe..876d4dfd6cb 100644
@@ -989,6 +989,7 @@ LIB_OBJS += rebase-interactive.o
 LIB_OBJS += rebase.o
 LIB_OBJS += ref-filter.o
 LIB_OBJS += reflog-walk.o
+LIB_OBJS += reflog.o
 LIB_OBJS += refs.o
 LIB_OBJS += refs/debug.o
 LIB_OBJS += refs/files-backend.o

builtin/reflog.c +1 -4

index 85b838720c3..03d347e5832 100644
@@ -1,16 +1,13 @@
 #include "builtin.h"
 #include "config.h"
 #include "lockfile.h"
-#include "object-store.h"
 #include "repository.h"
-#include "commit.h"
-#include "refs.h"
 #include "dir.h"
-#include "tree-walk.h"
 #include "diff.h"
 #include "revision.h"
 #include "reachable.h"
 #include "worktree.h"
+#include "reflog.h"
[...]

reflog.c new file +0 -0

new file mode 100644
index 00000000000..8d57dc43503

Quoted 66 lines · John Cai via GitGitGadget

From: John Cai <[email protected]>

Currently stash shells out to reflog in order to delete refs. In an effort to reduce how much we shell out to a subprocess, libify the functionality that stash needs into reflog.c.

Add a reflog_delete function that is pretty much the logic in the while loop in builtin/reflog.c cmd_reflog_delete(). This is a function that builtin/reflog.c and builtin/stash.c can both call.

Also move functions needed by reflog_delete and export them.

Helped-by Ævar Arnfjörð Bjarmason <[email protected]>

Signed-off-by John Cai <[email protected]>

---
 Makefile         |   1 +
 builtin/reflog.c | 451 +----------------------------------------------
 object.h         |   2 +-
 reflog.c         | 435 +++++++++++++++++++++++++++++++++++++++++++++
 reflog.h         |  49 +++++
 5 files changed, 490 insertions(+), 448 deletions(-)
 create mode 100644 reflog.c
 create mode 100644 reflog.h

Makefile +1 -0

index 6f0b4b775fe..876d4dfd6cb 100644
@@ -989,6 +989,7 @@ LIB_OBJS += rebase-interactive.o
 LIB_OBJS += rebase.o
 LIB_OBJS += ref-filter.o
 LIB_OBJS += reflog-walk.o
+LIB_OBJS += reflog.o
 LIB_OBJS += refs.o
 LIB_OBJS += refs/debug.o
 LIB_OBJS += refs/files-backend.o

builtin/reflog.c +1 -4

index 85b838720c3..03d347e5832 100644
@@ -1,16 +1,13 @@
 #include "builtin.h"
 #include "config.h"
 #include "lockfile.h"
-#include "object-store.h"
 #include "repository.h"
-#include "commit.h"
-#include "refs.h"
 #include "dir.h"
-#include "tree-walk.h"
 #include "diff.h"
 #include "revision.h"
 #include "reachable.h"
 #include "worktree.h"
+#include "reflog.h"
[...]

reflog.c new file +4 -0

new file mode 100644
index 00000000000..8d57dc43503
@@ -0,0 +1,435 @@
+#include "cache.h"
+#include "commit.h"
+#include "object-store.h"
+#include "reachable.h"

reflog.c line 9

Quoted 5 lines

reflog.c new file +5 -0

+#include "reflog.h"
+#include "refs.h"
+#include "revision.h"
+#include "tree-walk.h"
+#include "worktree.h"

I think you missed some now-redundant headers, and copied over others we didn't need. This compiles for me with this on top:

builtin/reflog.c +0 -4

index 03d347e5832..940db196f62 100644
@@ -1,9 +1,5 @@
 #include "builtin.h"
 #include "config.h"
-#include "lockfile.h"
-#include "repository.h"
-#include "dir.h"
-#include "diff.h"
 #include "revision.h"
 #include "reachable.h"
 #include "worktree.h"

reflog.c +0 -3

index 8d57dc43503..333fd8708fe 100644
@@ -1,11 +1,8 @@
 #include "cache.h"
-#include "commit.h"
 #include "object-store.h"
-#include "reachable.h"
 #include "reflog.h"
 #include "refs.h"
 #include "revision.h"
-#include "tree-walk.h"
 #include "worktree.h"
 

But perhaps some of those are really "needed" but brought in implicitly?

reflog.h line 4

Quoted 11 lines

[...]

reflog.h new file +4 -0

new file mode 100644
index 00000000000..3427021cdc2
@@ -0,0 +1,49 @@
+#ifndef REFLOG_H
+#define REFLOG_H
+
+#include "refs.h"

Just a nit but I think the reflog_delete() should be wrapped (ends up at 80 cols), and the usual style in this project is to not whitespace-pad so much, i.e. this on top:

reflog.h +2 -8

index 3427021cdc2..d2906fb9f8d 100644
@@ -1,6 +1,5 @@
 #ifndef REFLOG_H
 #define REFLOG_H
-
 #include "refs.h"
 
 struct cmd_reflog_expire_cb {

@@ -25,25 +24,20 @@ struct expire_reflog_policy_cb {
 	unsigned int dry_run:1;
 };
 
-int reflog_delete(const char *rev, enum expire_reflog_flags flags, int verbose);
-
+int reflog_delete(const char *rev, enum expire_reflog_flags flags,
+		  int verbose);
 void reflog_expiry_cleanup(void *cb_data);
-
 void reflog_expiry_prepare(const char *refname, const struct object_id *oid,
 			   void *cb_data);
-
 int should_expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
 			     const char *email, timestamp_t timestamp, int tz,
 			     const char *message, void *cb_data);
-
 int count_reflog_ent(struct object_id *ooid, struct object_id *noid,
 		     const char *email, timestamp_t timestamp, int tz,
 		     const char *message, void *cb_data);
-
 int should_expire_reflog_ent_verbose(struct object_id *ooid,
 				     struct object_id *noid,
 				     const char *email,
 				     timestamp_t timestamp, int tz,
 				     const char *message, void *cb_data);
-
 #endif /* REFLOG_H */

Other than all that I really can't find anything at all to comment on, and I see that all the points raised in previous rounds by others were addressed.

Quoted 8 lines · John Cai

Quoted 4 lines

One question that I had which I don't see answered already is what the plan is for existing reflog-related functions that live in refs.h. Should or will those functions be moved to the new reflog-specific header, too?

Thanks for bringing ths up! Maybe this cleanup can be included in a separate patch series since this one is mainly about getting rid of the shell-out in stash.c. What do you think?

Yeah; I think that's fine. We don't need to get all reflog-related functions moved into reflog.h in the first series, so I think it's fine to move things as you discover them.

I was just wondering whether it was something you planned to do at some point (as part of this series, or a future one).

Thanks,
Taylor

This test was already a bit broken in needing the preceding tests, but it will break now if REFFILES isn't true, which you can reproduce e.g. with:

    ./t3903-stash.sh --run=1-16,18-50 -vixd

Perhaps the least sucky solution to that is:

t/t3903-stash.sh +0 -0

index ec9cc5646d6..1d11c9bda20 100755

t/t3903-stash.sh +6 -1

 test_expect_success 'stash pop' '
 	git reset --hard &&
 	git stash pop &&
-	test 9 = $(cat file) &&
+	if test -e dropped-stash
+	then
+		test 9 = $(cat file)
+	else
+		test 3 = $(cat file)
+	fi &&
 	test 1 = $(git show :file) &&
 	test 1 = $(git show HEAD:file) &&
 	test 0 = $(git stash list | wc -l)

From: John Cai <[email protected]>

Currently stash shells out to reflog in order to delete refs. In an effort to reduce how much we shell out to a subprocess, libify the functionality that stash needs into reflog.c.

Add a reflog_delete function that is pretty much the logic in the while loop in builtin/reflog.c cmd_reflog_delete(). This is a function that builtin/reflog.c and builtin/stash.c can both call.

Also move functions needed by reflog_delete and export them.

Helped-by Ævar Arnfjörð Bjarmason <[email protected]>

Signed-off-by John Cai <[email protected]>

---
 Makefile         |   1 +
 builtin/reflog.c | 451 +----------------------------------------------
 object.h         |   2 +-
 reflog.c         | 435 +++++++++++++++++++++++++++++++++++++++++++++
 reflog.h         |  49 +++++
 5 files changed, 490 insertions(+), 448 deletions(-)
 create mode 100644 reflog.c
 create mode 100644 reflog.h

Quoted 13 lines · Ævar Arnfjörð Bjarmason

This test was already a bit broken in needing the preceding tests, but it will break now if REFFILES isn't true, which you can reproduce e.g. with:

    ./t3903-stash.sh --run=1-16,18-50 -vixd

Perhaps the least sucky solution to that is:

t/t3903-stash.sh +0 -0

index ec9cc5646d6..1d11c9bda20 100755

t/t3903-stash.sh test_expect_success 'drop stash reflog updates refs/stash with rewrite' ' · line 210

Quoted 7 lines

t/t3903-stash.sh +2 -1

@@ -205,13 +205,19 @@ test_expect_success 'drop stash reflog updates refs/stash with rewrite' '
 	cat >expect <<-EOF &&
 	$(test_oid zero) $oid
 	EOF
-	test_cmp expect actual
+	test_cmp expect actual &&
+	>dropped-stash
 '

If "git stash drop", invoked in earlier part of this test before the precontext, fails, then test_cmp would fail and we leave dropped-stash untouched, even though we did run "git stash drop" already.

Why does the next test need to depend on what has happened earlier?

Quoted 14 lines

t/t3903-stash.sh +6 -1

 test_expect_success 'stash pop' '
 	git reset --hard &&
 	git stash pop &&
-	test 9 = $(cat file) &&
+	if test -e dropped-stash
+	then
+		test 9 = $(cat file)
+	else
+		test 3 = $(cat file)
+	fi &&
 	test 1 = $(git show :file) &&
 	test 1 = $(git show HEAD:file) &&
 	test 0 = $(git stash list | wc -l)

Quoted 17 lines · Ævar Arnfjörð Bjarmason

This test was already a bit broken in needing the preceding tests, but it will break now if REFFILES isn't true, which you can reproduce e.g. with:

    ./t3903-stash.sh --run=1-16,18-50 -vixd

Perhaps the least sucky solution to that is:

t/t3903-stash.sh +0 -1

index ec9cc5646d6..1d11c9bda20 100755
@@ -205,13 +205,19 @@ test_expect_success 'drop stash reflog updates refs/stash with rewrite' '
 	cat >expect <<-EOF &&
 	$(test_oid zero) $oid
 	EOF
-	test_cmp expect actual

t/t3903-stash.sh test_expect_success 'drop stash reflog updates refs/stash with rewrite' ' · line 210

Quoted 3 lines

t/t3903-stash.sh +2 -0

+	test_cmp expect actual &&
+	>dropped-stash
 '

If "git stash drop", invoked in earlier part of this test before the precontext, fails, then test_cmp would fail and we leave dropped-stash untouched, even though we did run "git stash drop" already.

Why does the next test need to depend on what has happened earlier?

Quoted 13 lines

t/t3903-stash.sh +6 -1

 test_expect_success 'stash pop' '
 	git reset --hard &&
 	git stash pop &&
-	test 9 = $(cat file) &&
+	if test -e dropped-stash
+	then
+		test 9 = $(cat file)
+	else
+		test 3 = $(cat file)
+	fi &&
 	test 1 = $(git show :file) &&
 	test 1 = $(git show HEAD:file) &&
 	test 0 = $(git stash list | wc -l)

Review

Quoted 9 lines

(quoted diff) +9 -0

+test_expect_success 'drop stash reflog updates refs/stash' '
+	git reset --hard &&
+	git rev-parse refs/stash >expect &&
+	echo 9 >file &&
+	git stash &&
+	git stash drop stash@{0} &&
+	git rev-parse refs/stash >actual &&
+	test_cmp expect actual
+'

This one will be portable to the reftable backend.

Quoted 1 line

+test_expect_success 'drop stash reflog updates refs/stash with rewrite' '

But as I noted in <[email protected]> (but it was easy to miss) this test will need to depend on REFFILES. So just changing this line to:

test_expect_success REFFILES 'drop stash[...]'

Review

Quoted 11 lines

(quoted diff) +11 -0

+	git reset --hard &&
+	echo 9 >file &&
+	git stash &&
+	oid="$(git rev-parse stash@{0})" &&
+	git stash drop stash@{1} &&
+	cut -d" " -f1-2 .git/logs/refs/stash >actual &&
+	cat >expect <<-EOF &&
+	$(test_oid zero) $oid
+	EOF
+	test_cmp expect actual
+'

Quoted 20 lines · Ævar Arnfjörð Bjarmason

Review

Quoted 9 lines

(quoted diff) +9 -0

+test_expect_success 'drop stash reflog updates refs/stash' '
+	git reset --hard &&
+	git rev-parse refs/stash >expect &&
+	echo 9 >file &&
+	git stash &&
+	git stash drop stash@{0} &&
+	git rev-parse refs/stash >actual &&
+	test_cmp expect actual
+'

This one will be portable to the reftable backend.

Quoted 1 line

+test_expect_success 'drop stash reflog updates refs/stash with rewrite' '

But as I noted in <[email protected]> (but it was easy to miss) this test will need to depend on REFFILES. So just changing this line to:

test_expect_success REFFILES 'drop stash[...]'

Quoted 11 lines

Review

Quoted 11 lines

(quoted diff) +11 -0

+	git reset --hard &&
+	echo 9 >file &&
+	git stash &&
+	oid="$(git rev-parse stash@{0})" &&
+	git stash drop stash@{1} &&
+	cut -d" " -f1-2 .git/logs/refs/stash >actual &&
+	cat >expect <<-EOF &&
+	$(test_oid zero) $oid
+	EOF
+	test_cmp expect actual
+'

Why should this be tested with "cut" in the first place, though?

If we start from

    stash@{0} = A
    stash@{1} = B
    stash@{2} = C

and after saying "drop stash@{1}", what we need to check is that

    stash@{0} = A
    stash@{1} = C

now holds, which can be done with "git rev-parse", and the fact that the ref-files backend happens to record both before-and-after object IDs is an irrelevant implementation detail, no?

I am still puzzled.

Yes, this is true but that doesn't seem to test the --rewrite functionality. I could be missing something, but it seems that the reflog --rewrite option will write the LHS old oid value in the .git/logs/refs/stash file. When --rewrite isn't used, the reflog delete still does the right thing to the RHS entry.

I couldn't find any way to check this LFS value other than reaching into the actual file. If there is a way that would be preferable.

Hi Junio,

Quoted 38 lines · Junio C Hamano

Quoted 20 lines · Ævar Arnfjörð Bjarmason

Review

Quoted 9 lines

(quoted diff) +9 -0

+test_expect_success 'drop stash reflog updates refs/stash' '
+	git reset --hard &&
+	git rev-parse refs/stash >expect &&
+	echo 9 >file &&
+	git stash &&
+	git stash drop stash@{0} &&
+	git rev-parse refs/stash >actual &&
+	test_cmp expect actual
+'

This one will be portable to the reftable backend.

Quoted 1 line

+test_expect_success 'drop stash reflog updates refs/stash with rewrite' '

But as I noted in <[email protected]> (but it was easy to miss) this test will need to depend on REFFILES. So just changing this line to:

test_expect_success REFFILES 'drop stash[...]'

Quoted 11 lines

Review

Quoted 11 lines

(quoted diff) +11 -0

+	git reset --hard &&
+	echo 9 >file &&
+	git stash &&
+	oid="$(git rev-parse stash@{0})" &&
+	git stash drop stash@{1} &&
+	cut -d" " -f1-2 .git/logs/refs/stash >actual &&
+	cat >expect <<-EOF &&
+	$(test_oid zero) $oid
+	EOF
+	test_cmp expect actual
+'

Why should this be tested with "cut" in the first place, though?

If we start from

Quoted 8 lines

    stash@{0} = A
    stash@{1} = B
    stash@{2} = C

and after saying "drop stash@{1}", what we need to check is that

    stash@{0} = A
    stash@{1} = C

Yes, this is true but that doesn't seem to test the --rewrite functionality. I could be missing something, but it seems that the reflog --rewrite option will write the LHS old oid value in the .git/logs/refs/stash file. When --rewrite isn't used, the reflog delete still does the right thing to the RHS entry.

I couldn't find any way to check this LFS value other than reaching into the actual file. If there is a way that would be preferable.

Quoted 8 lines · John Cai

Yes, this is true but that doesn't seem to test the --rewrite functionality. I could be missing something, but it seems that the reflog --rewrite option will write the LHS old oid value in the .git/logs/refs/stash file. When --rewrite isn't used, the reflog delete still does the right thing to the RHS entry.

I couldn't find any way to check this LFS value other than reaching into the actual file. If there is a way that would be preferable.

Ah, that one.

As 2b81fab2 (git-reflog: add option --rewrite to update reflog entries while expiring, 2008-02-22) says, the redundant half of the reflog entry only matters to "certain sanity checks" and would not be even visible to normal ref API users. I wonder why we need to even say "--rewrite" in the first place. Perhaps we should implicitly set it always and eventually deprecate that option.

Quoted 19 lines · Junio C Hamano

Quoted 17 lines · Ævar Arnfjörð Bjarmason

This test was already a bit broken in needing the preceding tests, but it will break now if REFFILES isn't true, which you can reproduce e.g. with:

    ./t3903-stash.sh --run=1-16,18-50 -vixd

Perhaps the least sucky solution to that is:

t/t3903-stash.sh +0 -1

index ec9cc5646d6..1d11c9bda20 100755
@@ -205,13 +205,19 @@ test_expect_success 'drop stash reflog updates refs/stash with rewrite' '
 	cat >expect <<-EOF &&
 	$(test_oid zero) $oid
 	EOF
-	test_cmp expect actual

Quoted 8 lines

t/t3903-stash.sh test_expect_success 'drop stash reflog updates refs/stash with rewrite' ' · line 210

Quoted 3 lines

t/t3903-stash.sh +2 -0

+	test_cmp expect actual &&
+	>dropped-stash
 '

If "git stash drop", invoked in earlier part of this test before the precontext, fails, then test_cmp would fail and we leave dropped-stash untouched, even though we did run "git stash drop" already.

Yes, that's an edge case that's exposed here, but which I thought wasn't worth bothering with. I.e. if you get such a failure on test N getting N+1 failing as well isn't that big of a deal.

The big deal is rather that we know we're adding a REFFILES dependency to this, which won't run this at all, which will make the "stash pop" below fail.

Quoted 1 line

Why does the next test need to depend on what has happened earlier?

They don't need to, and ideally wouldn't, but most of our test suite has this issue already. Try e.g. running it with:

    prove t[0-9]*.sh :: --run=10-20 --immediate

And for this particular file running e.g. this on master:

    ./t3903-stash.sh --run=1-10,30-40

Will fail 7 tests in the 30-40 range.

So while it's ideal that we can combine tests with arbitrary --run parameters, i.e. all tests would tear down fully, not depend on earlier tests etc. we're really far from that being the case in practice.

So insisting on some general refactoring of this file as part of this series seems a bit overzelous, which is why I'm suggesting the bare minimum to expect and work around the inevitable REFFILES failure, as Han-Wen is actively working in that area.

Quoted 14 lines

Quoted 14 lines

t/t3903-stash.sh +6 -1

 test_expect_success 'stash pop' '
 	git reset --hard &&
 	git stash pop &&
-	test 9 = $(cat file) &&
+	if test -e dropped-stash
+	then
+		test 9 = $(cat file)
+	else
+		test 3 = $(cat file)
+	fi &&
 	test 1 = $(git show :file) &&
 	test 1 = $(git show HEAD:file) &&
 	test 0 = $(git stash list | wc -l)

Hi Ævar,

Quoted 55 lines · Ævar Arnfjörð Bjarmason

Quoted 19 lines · Junio C Hamano

Quoted 17 lines · Ævar Arnfjörð Bjarmason

This test was already a bit broken in needing the preceding tests, but it will break now if REFFILES isn't true, which you can reproduce e.g. with:

    ./t3903-stash.sh --run=1-16,18-50 -vixd

Perhaps the least sucky solution to that is:

t/t3903-stash.sh +0 -1

index ec9cc5646d6..1d11c9bda20 100755
@@ -205,13 +205,19 @@ test_expect_success 'drop stash reflog updates refs/stash with rewrite' '
 	cat >expect <<-EOF &&
 	$(test_oid zero) $oid
 	EOF
-	test_cmp expect actual

Quoted 8 lines

t/t3903-stash.sh test_expect_success 'drop stash reflog updates refs/stash with rewrite' ' · line 210

Quoted 3 lines

t/t3903-stash.sh +2 -0

+	test_cmp expect actual &&
+	>dropped-stash
 '

If "git stash drop", invoked in earlier part of this test before the precontext, fails, then test_cmp would fail and we leave dropped-stash untouched, even though we did run "git stash drop" already.

Yes, that's an edge case that's exposed here, but which I thought wasn't worth bothering with. I.e. if you get such a failure on test N getting N+1 failing as well isn't that big of a deal.

The big deal is rather that we know we're adding a REFFILES dependency to this, which won't run this at all, which will make the "stash pop" below fail.

Quoted 1 line

Why does the next test need to depend on what has happened earlier?

They don't need to, and ideally wouldn't, but most of our test suite has this issue already. Try e.g. running it with:

    prove t[0-9]*.sh :: --run=10-20 --immediate

And for this particular file running e.g. this on master:

    ./t3903-stash.sh --run=1-10,30-40

Will fail 7 tests in the 30-40 range.

So while it's ideal that we can combine tests with arbitrary --run parameters, i.e. all tests would tear down fully, not depend on earlier tests etc. we're really far from that being the case in practice.

Quoted 4 lines

So insisting on some general refactoring of this file as part of this series seems a bit overzelous, which is why I'm suggesting the bare minimum to expect and work around the inevitable REFFILES failure, as Han-Wen is actively working in that area.

Curious what your thoughts are on an effort to isolate these tests from each other. I like your approach in t/t1417 in creating a test repo and copying it over each time. Something like this?

t/t3903-stash.sh +3 -1

index ac345eced8cb..40254f8dc99c 100755
@@ -41,7 +41,9 @@ diff_cmp () {
        rm -f "$1.compare" "$2.compare"
 }
 
-test_expect_success 'stash some dirty working directory' '
+test_expect_success 'setup' '
+       git init repo &&
+       cd repo &&
        echo 1 >file &&
        git add file &&
        echo unrelated >other-file &&

t/t3903-stash.sh +14 -9

+       diff --git a/file b/file
+       index 0cfbf08..00750ed 100644
+       --- a/file
+       +++ b/file
+       @@ -1 +1 @@
+       -2
+       +3
+       EOF
+       cd ../
 '
 
-cat >expect <<EOF
-diff --git a/file b/file
-index 0cfbf08..00750ed 100644
---- a/file
-+++ b/file
-@@ -1 +1 @@
--2
-+3
-EOF
+test_stash () {
+       cp -R repo copy &&
+       cd copy &&
+       test_expect_success "$@" &&
+       cd ../ &&

Not sure if it's worth it though?

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

t/t3903-stash.sh +6 -1

 test_expect_success 'stash pop' '
 	git reset --hard &&
 	git stash pop &&
-	test 9 = $(cat file) &&
+	if test -e dropped-stash
+	then
+		test 9 = $(cat file)
+	else
+		test 3 = $(cat file)
+	fi &&
 	test 1 = $(git show :file) &&
 	test 1 = $(git show HEAD:file) &&
 	test 0 = $(git stash list | wc -l)

Quoted 3 lines

Curious what your thoughts are on an effort to isolate these tests from each other. I like your approach in t/t1417 in creating a test repo and copying it over each time. Something like this?

That looks good to me if you're willing to do that legwork, probably better in a preceding cleanup commit.

Quoted 8 lines

t/t3903-stash.sh +0 -0

index ac345eced8cb..40254f8dc99c 100755
@@ -41,7 +41,9 @@ diff_cmp () {
        rm -f "$1.compare" "$2.compare"
 }
 

t/t3903-stash.sh diff_cmp () { · line 46

Quoted 4 lines

t/t3903-stash.sh +3 -1

-test_expect_success 'stash some dirty working directory' '
+test_expect_success 'setup' '
+       git init repo &&
+       cd repo &&

Quoted 8 lines

t/t3903-stash.sh +0 -8

-cat >expect <<EOF
-diff --git a/file b/file
-index 0cfbf08..00750ed 100644
---- a/file
-+++ b/file
-@@ -1 +1 @@
--2
-+3

t/t3903-stash.sh test_expect_success 'stash some dirty working directory' ' · line 78

Quoted 8 lines

t/t3903-stash.sh +7 -1

-EOF
+test_stash () {
+       cp -R repo copy &&
+       cd copy &&
+       test_expect_success "$@" &&
+       cd ../ &&
+       rm -rf copy
+}

t/t3903-stash.sh test_expect_success 'stash some dirty working directory' ' · line 84

Quoted 6 lines

t/t3903-stash.sh +1 -1

-test_expect_success 'parents of stash' '
+test_stash 'parents of stash' '
        test $(git rev-parse stash^) = $(git rev-parse HEAD) &&
        git diff stash^2..stash >output &&
        diff_cmp expect output
 '

For this sort of thing I think it's usually better to override "test_expect_success" as a last resort, i.e. to have that "test_setup_stash_copy" just be a "setup_stash" or whatever function called from within your test_expect_success.

And instead of the "rm -rf" later, just do:

    test_when_finished "rm -rf copy" &&
    cp -R repo copy &&
    [...]

The test still needs to deal with the sub-repo, but it could cd or use "-C".

John Cai (3):
stash: add tests to ensure reflog --rewrite --updatref behavior
reflog: libify delete reflog function and helpers
stash: call reflog_delete() in reflog.c

 Makefile         |   1 +
 builtin/reflog.c | 455 +----------------------------------------------
 builtin/stash.c  |  18 +-
 object.h         |   2 +-
 reflog.c         | 432 ++++++++++++++++++++++++++++++++++++++++++++
 reflog.h         |  43 +++++
 t/t3903-stash.sh |  65 +++++--
 7 files changed, 539 insertions(+), 477 deletions(-)
 create mode 100644 reflog.c
 create mode 100644 reflog.h

Quoted 5 lines · John Cai via GitGitGadget

John Cai (3):
stash: add tests to ensure reflog --rewrite --updatref behavior
reflog: libify delete reflog function and helpers
stash: call reflog_delete() in reflog.c

Quoted 10 lines

 Makefile         |   1 +
 builtin/reflog.c | 455 +----------------------------------------------
 builtin/stash.c  |  18 +-
 object.h         |   2 +-
 reflog.c         | 432 ++++++++++++++++++++++++++++++++++++++++++++
 reflog.h         |  43 +++++
 t/t3903-stash.sh |  65 +++++--
 7 files changed, 539 insertions(+), 477 deletions(-)
 create mode 100644 reflog.c
 create mode 100644 reflog.h

Thanks; I glossed over the discussion about tests (since it looks like you and Ævar already have a good handle on how things are going there).

The rest of this version of the series (which I looked at more closely) looks good to me.

Thanks
Taylor

From: John Cai <[email protected]>

There is missing test coverage to ensure that the resulting reflogs after a git stash drop has had its old oid rewritten if applicable, and if the refs/stash has been updated if applicable.

Add two tests that verify both of these happen.

Helped-by Ævar Arnfjörð Bjarmason <[email protected]>

Signed-off-by John Cai <[email protected]>

---
 t/t3903-stash.sh | 65 ++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 54 insertions(+), 11 deletions(-)

t/t3903-stash.sh +47 -11

index b149e2af441..73785cf862f 100755
@@ -41,20 +41,29 @@ diff_cmp () {
 	rm -f "$1.compare" "$2.compare"
 }
 
-test_expect_success 'stash some dirty working directory' '
-	echo 1 >file &&
-	git add file &&
-	echo unrelated >other-file &&
-	git add other-file &&
+setup_stash() {
+	repo_dir=$1
+	if test -z $repo_dir; then
+		repo_dir="."
+	fi
+
+	echo 1 >$repo_dir/file &&
+	git -C $repo_dir add file &&
+	echo unrelated >$repo_dir/other-file &&
+	git -C $repo_dir add other-file &&
 	test_tick &&
-	git commit -m initial &&
-	echo 2 >file &&
+	git -C $repo_dir commit -m initial &&
+	echo 2 >$repo_dir/file &&
 	git add file &&
-	echo 3 >file &&
+	echo 3 >$repo_dir/file &&
 	test_tick &&
-	git stash &&
-	git diff-files --quiet &&
-	git diff-index --cached --quiet HEAD
+	git -C $repo_dir stash &&
+	git -C $repo_dir diff-files --quiet &&
+	git -C $repo_dir diff-index --cached --quiet HEAD
+}
+
+test_expect_success 'stash some dirty working directory' '
+	setup_stash
 '
 
 cat >expect <<EOF

@@ -185,6 +194,40 @@ test_expect_success 'drop middle stash by index' '
 	test 1 = $(git show HEAD:file)
 '
 
+test_expect_success 'drop stash reflog updates refs/stash' '
+	git reset --hard &&
+	git rev-parse refs/stash >expect &&
+	echo 9 >file &&
+	git stash &&
+	git stash drop stash@{0} &&
+	git rev-parse refs/stash >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success REFFILES 'drop stash reflog updates refs/stash with rewrite' '
+	git init repo &&
+	setup_stash repo &&
+	echo 9 >repo/file &&
+
+	old_oid="$(git -C repo rev-parse stash@{0})" &&
+	git -C repo stash &&
+	new_oid="$(git -C repo rev-parse stash@{0})" &&
+
+	cat >expect <<-EOF &&
+	$(test_oid zero) $old_oid
+	$old_oid $new_oid
+	EOF
+	cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
+	test_cmp expect actual &&
+
+	git -C repo stash drop stash@{1} &&

Just wanted to bump this thread. It'd be good to get another ack on these last set of changes.

This patch has three parts:

* add missing test coverage for git stash delete

* libify reflog's delete functionality and move some of the helpers into a reflog.c library and call reflog_delete from builtin/reflog.c

* call reflog_delete from builtin/stash.c

From: John Cai <[email protected]>

There is missing test coverage to ensure that the resulting reflogs after a git stash drop has had its old oid rewritten if applicable, and if the refs/stash has been updated if applicable.

Add two tests that verify both of these happen.

Helped-by Ævar Arnfjörð Bjarmason <[email protected]>

Signed-off-by John Cai <[email protected]>

---
 t/t3903-stash.sh | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

t/t3903-stash.sh +0 -0

index b149e2af441..a2f8d0c52e7 100755

t/t3903-stash.sh +6 -0

+test_expect_success 'drop stash reflog updates refs/stash' '
+	git reset --hard &&
+	git rev-parse refs/stash >expect &&
+	echo 9 >file &&
+	git stash &&
+	git stash drop stash@{0} &&

t/t3903-stash.sh +21 -0

+	echo 9 >repo/file &&
+
+	old_oid="$(git -C repo rev-parse stash@{0})" &&
+	git -C repo stash &&
+	new_oid="$(git -C repo rev-parse stash@{0})" &&
+
+	cat >expect <<-EOF &&
+	$(test_oid zero) $old_oid
+	$old_oid $new_oid
+	EOF
+	cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
+	test_cmp expect actual &&
+
+	git -C repo stash drop stash@{1} &&
+	cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
+	cat >expect <<-EOF &&
+	$(test_oid zero) $new_oid
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'stash pop' '
 	git reset --hard &&
 	git stash pop &&

Quoted 19 lines · John Cai via GitGitGadget

From: John Cai <[email protected]>

There is missing test coverage to ensure that the resulting reflogs after a git stash drop has had its old oid rewritten if applicable, and if the refs/stash has been updated if applicable.

Add two tests that verify both of these happen.

Helped-by Ævar Arnfjörð Bjarmason <[email protected]>

Signed-off-by John Cai <[email protected]>

---
 t/t3903-stash.sh | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

t/t3903-stash.sh +0 -0

index b149e2af441..a2f8d0c52e7 100755

t/t3903-stash.sh diff_cmp () { · line 44

Quoted 5 lines

t/t3903-stash.sh +1 -1

@@ -41,7 +41,7 @@ diff_cmp () {
 	rm -f "$1.compare" "$2.compare"
 }
 
-test_expect_success 'stash some dirty working directory' '
+setup_stash() {

Style.

	setup_stash () {

but more importantly, is this really setting up "stash"? "setup_stash_test" or something, perhaps?

t/t3903-stash.sh test_expect_success 'stash some dirty working directory' ' · line 62

Quoted 3 lines

t/t3903-stash.sh +2 -0

+test_expect_success 'stash some dirty working directory' '
+	setup_stash
 '

OK.

Quoted 6 lines

t/t3903-stash.sh +6 -0

+test_expect_success 'drop stash reflog updates refs/stash' '
+	git reset --hard &&
+	git rev-parse refs/stash >expect &&
+	echo 9 >file &&
+	git stash &&
+	git stash drop stash@{0} &&

t/t3903-stash.sh test_expect_success 'drop middle stash by index' ' · line 207

Quoted 10 lines

t/t3903-stash.sh +10 -0

+	git rev-parse refs/stash >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success REFFILES 'drop stash reflog updates refs/stash with rewrite' '
+	git init repo &&
+	(
+		cd repo &&
+		setup_stash
+	) &&

Hmph, so this is done inside the subdirectory. The implementation of the helper in this iteration does look cleaner than in the previous iteration.

But these many references to "repo/" and "-C repo" we see below makes me wonder why we do not put the whole thing inside the subshell we started earlier.

i.e.

	git init repo &&
	(
		cd repo &&
		setup_stash_test &&

		echo 9 >file &&
		old=$(git rev-parse stash@{0}) &&
		git stash &&
		new=$(git rev-parse stash@{0}) &&
		...

		test_cmp expect actual
	)

Quoted 24 lines

t/t3903-stash.sh +21 -0

+	echo 9 >repo/file &&
+
+	old_oid="$(git -C repo rev-parse stash@{0})" &&
+	git -C repo stash &&
+	new_oid="$(git -C repo rev-parse stash@{0})" &&
+
+	cat >expect <<-EOF &&
+	$(test_oid zero) $old_oid
+	$old_oid $new_oid
+	EOF
+	cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
+	test_cmp expect actual &&
+
+	git -C repo stash drop stash@{1} &&
+	cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
+	cat >expect <<-EOF &&
+	$(test_oid zero) $new_oid
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'stash pop' '
 	git reset --hard &&
 	git stash pop &&

"John Cai via GitGitGadget" <[email protected]> writes:
[...]

t/t3903-stash.sh test_expect_success 'drop middle stash by index' ' · line 207

Quoted 6 lines

t/t3903-stash.sh +6 -0

+test_expect_success REFFILES 'drop stash reflog updates refs/stash with rewrite' '
+	git init repo &&
+	(
+		cd repo &&
+		setup_stash
+	) &&

Hmph, so this is done inside the subdirectory. The implementation of the helper in this iteration does look cleaner than in the previous iteration.

But these many references to "repo/" and "-C repo" we see below makes me wonder why we do not put the whole thing inside the subshell we started earlier.

i.e.

	git init repo &&
	(
		cd repo &&
		setup_stash_test &&

		echo 9 >file &&
		old=$(git rev-parse stash@{0}) &&
		git stash &&
		new=$(git rev-parse stash@{0}) &&
		...

		test_cmp expect actual
	)

Quoted 24 lines

t/t3903-stash.sh +21 -0

+	echo 9 >repo/file &&
+
+	old_oid="$(git -C repo rev-parse stash@{0})" &&
+	git -C repo stash &&
+	new_oid="$(git -C repo rev-parse stash@{0})" &&
+
+	cat >expect <<-EOF &&
+	$(test_oid zero) $old_oid
+	$old_oid $new_oid
+	EOF
+	cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
+	test_cmp expect actual &&
+
+	git -C repo stash drop stash@{1} &&
+	cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
+	cat >expect <<-EOF &&
+	$(test_oid zero) $new_oid
+	EOF
+	test_cmp expect actual
+'
+
  test_expect_success 'stash pop' '
  	git reset --hard &&
  	git stash pop &&

Quoted 15 lines · Junio C Hamano

"John Cai via GitGitGadget" <[email protected]> writes:
[...]

t/t3903-stash.sh test_expect_success 'drop middle stash by index' ' · line 207

Quoted 6 lines

t/t3903-stash.sh +6 -0

+test_expect_success REFFILES 'drop stash reflog updates refs/stash with rewrite' '
+	git init repo &&
+	(
+		cd repo &&
+		setup_stash
+	) &&

Hmph, so this is done inside the subdirectory. The implementation of the helper in this iteration does look cleaner than in the previous iteration.
But these many references to "repo/" and "-C repo" we see below makes me wonder why we do not put the whole thing inside the subshell we started earlier.
i.e.

Quoted 12 lines

	git init repo &&
	(
		cd repo &&
		setup_stash_test &&
		echo 9 >file &&
		old=$(git rev-parse stash@{0}) &&
		git stash &&
		new=$(git rev-parse stash@{0}) &&
		...
		test_cmp expect actual
	)

I wonder if we could avoid the subshell entirely and avoid relying on REFFILES (assuming we're not trying to test the implementation details of that refs backend) by doing something like

test_expect_success 'drop stash reflog updates refs/stash with rewrite' '

	old=$(git rev-parse stash@{0}) &&
	setup_stash_test &&
	git rev-list -g stash >tmp &&
	sed /$old/d tmp >expect &&
	git rev-list -g stash >actual &&
	test_cmp expect actual

'

Quoted 24 lines

Quoted 24 lines

t/t3903-stash.sh +21 -0

+	echo 9 >repo/file &&
+
+	old_oid="$(git -C repo rev-parse stash@{0})" &&
+	git -C repo stash &&
+	new_oid="$(git -C repo rev-parse stash@{0})" &&
+
+	cat >expect <<-EOF &&
+	$(test_oid zero) $old_oid
+	$old_oid $new_oid
+	EOF
+	cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
+	test_cmp expect actual &&
+
+	git -C repo stash drop stash@{1} &&
+	cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
+	cat >expect <<-EOF &&
+	$(test_oid zero) $new_oid
+	EOF
+	test_cmp expect actual
+'
+
  test_expect_success 'stash pop' '
  	git reset --hard &&
  	git stash pop &&

Quoted 33 lines · Phillip Wood

Quoted 15 lines · Junio C Hamano

"John Cai via GitGitGadget" <[email protected]> writes:
[...]

t/t3903-stash.sh test_expect_success 'drop middle stash by index' ' · line 207

Quoted 6 lines

t/t3903-stash.sh +6 -0

+test_expect_success REFFILES 'drop stash reflog updates refs/stash with rewrite' '
+	git init repo &&
+	(
+		cd repo &&
+		setup_stash
+	) &&

Hmph, so this is done inside the subdirectory. The implementation of the helper in this iteration does look cleaner than in the previous iteration.
But these many references to "repo/" and "-C repo" we see below makes me wonder why we do not put the whole thing inside the subshell we started earlier.
i.e.

Quoted 12 lines

	git init repo &&
	(
		cd repo &&
		setup_stash_test &&
		echo 9 >file &&
		old=$(git rev-parse stash@{0}) &&
		git stash &&
		new=$(git rev-parse stash@{0}) &&
		...
		test_cmp expect actual
	)

I wonder if we could avoid the subshell entirely and avoid relying on REFFILES (assuming we're not trying to test the implementation details of that refs backend) by doing something like

Quoted 8 lines

test_expect_success 'drop stash reflog updates refs/stash with rewrite' '

	old=$(git rev-parse stash@{0}) &&
	setup_stash_test &&
	git rev-list -g stash >tmp &&
	sed /$old/d tmp >expect &&
	git rev-list -g stash >actual &&
	test_cmp expect actual

'

Unless I'm missing something that "rev-list -g" will emit only the RHS of the stash logs, i.e. no "0000..." etc.

And if we only look at that the difference with specifying the flag isn't visible, no?

Quoted 24 lines

Quoted 24 lines

Quoted 24 lines

t/t3903-stash.sh +21 -0

+	echo 9 >repo/file &&
+
+	old_oid="$(git -C repo rev-parse stash@{0})" &&
+	git -C repo stash &&
+	new_oid="$(git -C repo rev-parse stash@{0})" &&
+
+	cat >expect <<-EOF &&
+	$(test_oid zero) $old_oid
+	$old_oid $new_oid
+	EOF
+	cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
+	test_cmp expect actual &&
+
+	git -C repo stash drop stash@{1} &&
+	cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
+	cat >expect <<-EOF &&
+	$(test_oid zero) $new_oid
+	EOF
+	test_cmp expect actual
+'
+
   test_expect_success 'stash pop' '
   	git reset --hard &&
   	git stash pop &&

Quoted 45 lines · Ævar Arnfjörð Bjarmason

Quoted 41 lines · Phillip Wood

Quoted 15 lines · Junio C Hamano

"John Cai via GitGitGadget" <[email protected]> writes:
[...]

t/t3903-stash.sh test_expect_success 'drop middle stash by index' ' · line 207

Quoted 6 lines

t/t3903-stash.sh +6 -0

+test_expect_success REFFILES 'drop stash reflog updates refs/stash with rewrite' '
+	git init repo &&
+	(
+		cd repo &&
+		setup_stash
+	) &&

Hmph, so this is done inside the subdirectory. The implementation of the helper in this iteration does look cleaner than in the previous iteration.
But these many references to "repo/" and "-C repo" we see below makes me wonder why we do not put the whole thing inside the subshell we started earlier.
i.e.

Quoted 12 lines

	git init repo &&
	(
		cd repo &&
		setup_stash_test &&
		echo 9 >file &&
		old=$(git rev-parse stash@{0}) &&
		git stash &&
		new=$(git rev-parse stash@{0}) &&
		...
		test_cmp expect actual
	)

I wonder if we could avoid the subshell entirely and avoid relying on REFFILES (assuming we're not trying to test the implementation details of that refs backend) by doing something like

test_expect_success 'drop stash reflog updates refs/stash with rewrite' '

	old=$(git rev-parse stash@{0}) &&
	setup_stash_test &&
	git rev-list -g stash >tmp &&
	sed /$old/d tmp >expect &&
	git rev-list -g stash >actual &&
	test_cmp expect actual

'

Quoted 5 lines

Unless I'm missing something that "rev-list -g" will emit only the RHS of the stash logs, i.e. no "0000..." etc.

And if we only look at that the difference with specifying the flag isn't visible, no?

Maybe I'm missing what this test is actually needs to do. I thought it just needed to check that the deleted stash is removed from the reflog and the others are unchanged. You're right that it wont show the LHS and if that is important then you need to read the log file directly.

Best Wishes

Phillip

Quoted 27 lines

Quoted 26 lines

Quoted 26 lines

Quoted 26 lines

t/t3903-stash.sh +21 -0

+	echo 9 >repo/file &&
+
+	old_oid="$(git -C repo rev-parse stash@{0})" &&
+	git -C repo stash &&
+	new_oid="$(git -C repo rev-parse stash@{0})" &&
+
+	cat >expect <<-EOF &&
+	$(test_oid zero) $old_oid
+	$old_oid $new_oid
+	EOF
+	cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
+	test_cmp expect actual &&
+
+	git -C repo stash drop stash@{1} &&
+	cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
+	cat >expect <<-EOF &&
+	$(test_oid zero) $new_oid
+	EOF
+	test_cmp expect actual
+'
+
   test_expect_success 'stash pop' '
   	git reset --hard &&
   	git stash pop &&

Hi Phillip,

Quoted 49 lines · Phillip Wood

Quoted 48 lines · Ævar Arnfjörð Bjarmason

Quoted 33 lines · Phillip Wood

Quoted 15 lines · Junio C Hamano

"John Cai via GitGitGadget" <[email protected]> writes:
[...]

t/t3903-stash.sh test_expect_success 'drop middle stash by index' ' · line 207

Quoted 6 lines

t/t3903-stash.sh +6 -0

+test_expect_success REFFILES 'drop stash reflog updates refs/stash with rewrite' '
+	git init repo &&
+	(
+		cd repo &&
+		setup_stash
+	) &&

Hmph, so this is done inside the subdirectory. The implementation of the helper in this iteration does look cleaner than in the previous iteration.
But these many references to "repo/" and "-C repo" we see below makes me wonder why we do not put the whole thing inside the subshell we started earlier.
i.e.

Quoted 12 lines

	git init repo &&
	(
		cd repo &&
		setup_stash_test &&
		echo 9 >file &&
		old=$(git rev-parse stash@{0}) &&
		git stash &&
		new=$(git rev-parse stash@{0}) &&
		...
		test_cmp expect actual
	)

I wonder if we could avoid the subshell entirely and avoid relying on REFFILES (assuming we're not trying to test the implementation details of that refs backend) by doing something like

Quoted 8 lines

test_expect_success 'drop stash reflog updates refs/stash with rewrite' '

	old=$(git rev-parse stash@{0}) &&
	setup_stash_test &&
	git rev-list -g stash >tmp &&
	sed /$old/d tmp >expect &&
	git rev-list -g stash >actual &&
	test_cmp expect actual

'

Unless I'm missing something that "rev-list -g" will emit only the RHS of the stash logs, i.e. no "0000..." etc.

Quoted 4 lines

Quoted 2 lines

And if we only look at that the difference with specifying the flag isn't visible, no?

Maybe I'm missing what this test is actually needs to do. I thought it just needed to check that the deleted stash is removed from the reflog and the others are unchanged. You're right that it wont show the LHS and if that is important then you need to read the log file directly.

We had discussed this briefly in [1], but the --rewrite option for reflog delete will rewrite the LHS, which is not visible to normal ref API users. So the only way to test that this happened is to reach inside of the file.

1. https://lore.kernel.org/git/[email protected]/

Quoted 4 lines

Best Wishes

Phillip

thanks,
John

Quoted 27 lines

Quoted 27 lines

Quoted 26 lines

Quoted 26 lines

Quoted 26 lines

t/t3903-stash.sh +21 -0

+	echo 9 >repo/file &&
+
+	old_oid="$(git -C repo rev-parse stash@{0})" &&
+	git -C repo stash &&
+	new_oid="$(git -C repo rev-parse stash@{0})" &&
+
+	cat >expect <<-EOF &&
+	$(test_oid zero) $old_oid
+	$old_oid $new_oid
+	EOF
+	cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
+	test_cmp expect actual &&
+
+	git -C repo stash drop stash@{1} &&
+	cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
+	cat >expect <<-EOF &&
+	$(test_oid zero) $new_oid
+	EOF
+	test_cmp expect actual
+'
+
    test_expect_success 'stash pop' '
    	git reset --hard &&
    	git stash pop &&

Hi John

Quoted 56 lines · John Cai

Hi Phillip,

Quoted 52 lines · Phillip Wood

Quoted 50 lines · Ævar Arnfjörð Bjarmason

Quoted 33 lines · Phillip Wood

Quoted 15 lines · Junio C Hamano

"John Cai via GitGitGadget" <[email protected]> writes:
[...]

Review

Quoted 6 lines

(quoted diff) +6 -0

+test_expect_success REFFILES 'drop stash reflog updates refs/stash with rewrite' '
+	git init repo &&
+	(
+		cd repo &&
+		setup_stash
+	) &&

Hmph, so this is done inside the subdirectory. The implementation of the helper in this iteration does look cleaner than in the previous iteration.
But these many references to "repo/" and "-C repo" we see below makes me wonder why we do not put the whole thing inside the subshell we started earlier.
i.e.

Quoted 12 lines

	git init repo &&
	(
		cd repo &&
		setup_stash_test &&
		echo 9 >file &&
		old=$(git rev-parse stash@{0}) &&
		git stash &&
		new=$(git rev-parse stash@{0}) &&
		...
		test_cmp expect actual
	)

I wonder if we could avoid the subshell entirely and avoid relying on REFFILES (assuming we're not trying to test the implementation details of that refs backend) by doing something like

Quoted 8 lines

test_expect_success 'drop stash reflog updates refs/stash with rewrite' '

	old=$(git rev-parse stash@{0}) &&
	setup_stash_test &&
	git rev-list -g stash >tmp &&
	sed /$old/d tmp >expect &&
	git rev-list -g stash >actual &&
	test_cmp expect actual

'

Unless I'm missing something that "rev-list -g" will emit only the RHS of the stash logs, i.e. no "0000..." etc.

And if we only look at that the difference with specifying the flag isn't visible, no?

Quoted 5 lines

Quoted 1 line

Maybe I'm missing what this test is actually needs to do. I thought it just needed to check that the deleted stash is removed from the reflog and the others are unchanged. You're right that it wont show the LHS and if that is important then you need to read the log file directly.

We had discussed this briefly in [1], but the --rewrite option for reflog delete will rewrite the LHS, which is not visible to normal ref API users. So the only way to test that this happened is to reach inside of the file.

1. https://lore.kernel.org/git/[email protected]/

Thanks for the pointer, that was useful context that could maybe be added to the commit message to explain why the test needs to check the lhs of the reflog if you reroll.

Best Wishes

Phillip

Quoted 35 lines

Quoted 4 lines

Best Wishes

Phillip

thanks,
John

Quoted 27 lines

Quoted 27 lines

Quoted 26 lines

Quoted 26 lines

Quoted 26 lines

(quoted diff) +21 -0

+	echo 9 >repo/file &&
+
+	old_oid="$(git -C repo rev-parse stash@{0})" &&
+	git -C repo stash &&
+	new_oid="$(git -C repo rev-parse stash@{0})" &&
+
+	cat >expect <<-EOF &&
+	$(test_oid zero) $old_oid
+	$old_oid $new_oid
+	EOF
+	cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
+	test_cmp expect actual &&
+
+	git -C repo stash drop stash@{1} &&
+	cut -d" " -f1-2 repo/.git/logs/refs/stash >actual &&
+	cat >expect <<-EOF &&
+	$(test_oid zero) $new_oid
+	EOF
+	test_cmp expect actual
+'
+
    test_expect_success 'stash pop' '
    	git reset --hard &&
    	git stash pop &&