git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Taylor Blau <me@ttaylorr.com>
To: Derrick Stolee <derrickstolee@github.com>
Cc: git@vger.kernel.org, Jeff King <peff@peff.net>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH] builtin/pack-objects.c: introduce `pack.extraCruftTips`
Date: Tue, 2 May 2023 20:06:52 -0400	[thread overview]
Message-ID: <ZFGlnB5foNSRFmmw@nand.local> (raw)
In-Reply-To: <97d84abc-9b25-9d43-503e-264b33c223d4@github.com>

On Wed, Apr 26, 2023 at 06:52:26AM -0400, Derrick Stolee wrote:
> You corrected me that we _are_ updating mtimes, so my understanding was
> incorrect and this follow-up is incorrect. Thanks.
>
> Is it possible to demonstrate this mtime-updating in a test?

It is, though I think that this might be out of scope for the
`pack.extraCruftTips` feature, and instead is more directly related to
the core machinery behind cruft packs. We have fairly thorough tests
throughout t5329 that cover this behavior implicitly, but let me know if
you think there is anything missing there.

> >>> +	if (progress)
> >>> +		progress_state = start_progress(_("Enumerating extra cruft tips"), 0);
> >>
> >> Should we be including two progress counts? or would it be sufficient
> >> to say "traversing extra cruft objects" and include both of these steps
> >> in that one progress mode?
> >
> > I may be missing something, but there are already two progress meters
> > here: one (above) for enumerating the list of extra cruft tips, which
> > increments each time we read a new line that refers to an extant object,
> > and another (below) for the number of objects that we visit along the
> > second walk.
>
> I meant: why two counts? Should we instead only have one?

Ah, thanks for helping my understanding. We have two counts to match the
behavior when generating cruft packs with pruning, where one counter
tracks the number of tips we add to the traversal, and the second
counter tracks the number of objects that we actually visit along the
traversal (and thus end up in the cruft pack).

Since we have to do an identical traversal here to ensure reachability
closure (where possible) over the extra cruft tips, I added a second
counter to track that progress.

> >> Could we store this commit to make sure it is removed from the
> >> repository later?
> >
> > Possibly, though I think we already have good coverage of this in other
> > tests. So I'd be equally happy to assert on the exact contents of the
> > cruft pack (which wouldn't include the above commit), but I'm happy to
> > assert on it more directly if you feel strongly.
>
> This is a case of me thinking "can we test the test?" to make sure
> the test is doing everything we think it is doing...

Good call, and doing so isn't too bad, either:

--- 8< ---
diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh
index 0ac2f515b8..3ad16a9fca 100755
--- a/t/t5329-pack-objects-cruft.sh
+++ b/t/t5329-pack-objects-cruft.sh
@@ -814,8 +814,16 @@ test_expect_success 'additional cruft tips may be specified via pack.extraCruftT
 		cut -d" " -f2 cruft | sort >cruft.actual &&

 		git rev-list --objects --no-object-names $cruft_new >cruft.raw &&
+		cp cruft.expect cruft.old &&
 		sort cruft.raw >cruft.expect &&
-		test_cmp cruft.expect cruft.actual
+		test_cmp cruft.expect cruft.actual &&
+
+		# ensure objects which are no longer in the cruft pack were
+		# removed from the repository
+		for object in $(comm -13 cruft.expect cruft.old)
+		do
+			test_must_fail git cat-file -t $object || return 1
+		done
 	)
 '
--- >8 ---

> > +		# Ensure that the "old" objects are removed after
> > +		# dropping the pack.extraCruftTips hook.
> > +		git config --unset pack.extraCruftTips &&
> > +		git repack --cruft --cruft-expiration=now -d &&
> > +
> > +		mtimes="$(ls .git/objects/pack/pack-*.mtimes)" &&
> > +		git show-index <${mtimes%.mtimes}.idx >cruft &&
> > +		cut -d" " -f2 cruft | sort >cruft.actual &&
> > +
> > +		git rev-list --objects --no-object-names $cruft_new >cruft.raw &&
>
> I like how $cruft_new is still kept because its mtime is still
> "later than now".

Thanks. I added this object to make sure that we weren't testing trivial
behavior, such as removing all objects regardless of whether or not they
appear as extra cruft tips.

Thanks,
Taylor

  reply	other threads:[~2023-05-03  0:06 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-04-20 17:27 [PATCH] builtin/pack-objects.c: introduce `pack.extraCruftTips` Taylor Blau
2023-04-20 18:12 ` Junio C Hamano
2023-04-20 19:30   ` Taylor Blau
2023-04-20 19:52     ` Junio C Hamano
2023-04-20 20:48       ` Taylor Blau
2023-04-21  0:10 ` Chris Torek
2023-04-21  2:14   ` Taylor Blau
2023-04-25 19:42 ` Derrick Stolee
2023-04-25 21:25   ` Taylor Blau
2023-04-26 10:52     ` Derrick Stolee
2023-05-03  0:06       ` Taylor Blau [this message]
2023-05-03  0:09 ` [PATCH v2] " Taylor Blau
2023-05-03 14:01   ` Derrick Stolee
2023-05-03 19:59   ` Jeff King
2023-05-03 21:22     ` Taylor Blau
2023-05-05 21:23       ` Jeff King
2023-05-06  0:06         ` Taylor Blau
2023-05-06  0:14           ` Taylor Blau
2023-05-03 21:28     ` Taylor Blau
2023-05-05 21:26       ` Jeff King
2023-05-05 22:13         ` Jeff King
2023-05-06  0:13           ` Taylor Blau
2023-05-06  0:20             ` Taylor Blau
2023-05-06  2:12             ` Jeff King
2023-05-03 22:05 ` [PATCH v3] " Taylor Blau
2023-05-03 23:18   ` Junio C Hamano
2023-05-03 23:42     ` Junio C Hamano
2023-05-03 23:48       ` Taylor Blau
2023-05-03 23:50       ` Taylor Blau
2023-05-05 21:39     ` Jeff King
2023-05-05 22:19   ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZFGlnB5foNSRFmmw@nand.local \
    --to=me@ttaylorr.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).