git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] let feature.experimental imply gc.cruftPacks=true
@ 2022-08-03 20:57 Emily Shaffer
  2022-08-03 20:57 ` [PATCH 1/2] gc: add tests for --cruft and friends Emily Shaffer
  2022-08-03 20:57 ` [PATCH 2/2] config: let feature.experimental imply gc.cruftPacks=true Emily Shaffer
  0 siblings, 2 replies; 9+ messages in thread
From: Emily Shaffer @ 2022-08-03 20:57 UTC (permalink / raw)
  To: git
  Cc: Emily Shaffer, Derrick Stolee, Jonathan Nieder,
	Ævar Arnfjörð Bjarmason

Cruft packs seem to save us disk space during garbage collection. There
may still be some interesting races around mtime, but as I understand
it, those are not particularly worse or better with or without cruft
packs; so this setting primarily works to save us disk space. At least
at Google, we see concerns around loose object explosion during gc
fairly often, so this is a welcome change and we've turned it on for all
Googlers. Because we did so, I wonder if it's going to make sense to for
gc.cruftPacks to default to true in the future, with or without mtime
race fixes. With that in mind, I think it's a good idea to get any folks
who may be testing for us with feature.experimental to also try out
cruft packs and complain to us if there is an issue.

In Monday's standup there was some discussion around remaining concerns
with mtime and cruft packs. As I understood it at least, it seems there
can be a loss of information around mtimes when switching between cruft
repacks and no-cruft repacks, for example:

 ab/cdef is unreachable and has mtime <last week>
 'git gc --cruft'
 packs/<cruft>.pack contains abcdef
 packs/<cruft>.mtimes says abcdef has mtime <last week>
 'git gc' (no cruft)
 ab/cdef is unreachable and has mtime <now>

I could have misunderstood it. But it seems to be an issue primarily
around switching between crufty and non-crufty gc.

Read the full conversation around mtime races:
https://colabti.org/irclogger/irclogger_log/git-devel?date=2022-08-01

As for the series, it's only a two-patch series because I noticed while
trying to add tests for feature.experimental => gc.cruftPacks=true that
there weren't any tests around gc.cruftPacks to begin with. It's
possible that there are too many tests in patch 1 - gc --cruft is tested
in t5329 'expiring cruft objects with git gc' - but it looked like the
content of the test was different, in that t5329's test checks to make
sure the cruft pack and associated metadata are deleted during
expiration, but the one I added checks that the cruft pack is generated
during a 'gc --cruft' which isn't ready to expire yet.

 - Emily

Emily Shaffer (2):
  gc: add tests for --cruft and friends
  config: let feature.experimental imply gc.cruftPacks=true

 Documentation/config/feature.txt |  2 +
 builtin/gc.c                     |  6 +++
 t/t6500-gc.sh                    | 71 ++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+)

-- 
2.37.1.455.g008518b4e5-goog


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-08-04 16:10 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-03 20:57 [PATCH 0/2] let feature.experimental imply gc.cruftPacks=true Emily Shaffer
2022-08-03 20:57 ` [PATCH 1/2] gc: add tests for --cruft and friends Emily Shaffer
2022-08-03 21:56   ` Junio C Hamano
2022-08-04  7:48   ` Ævar Arnfjörð Bjarmason
2022-08-04 16:09     ` Junio C Hamano
2022-08-03 20:57 ` [PATCH 2/2] config: let feature.experimental imply gc.cruftPacks=true Emily Shaffer
2022-08-03 22:05   ` Junio C Hamano
2022-08-04 13:05   ` Derrick Stolee
2022-08-04 16:10     ` Junio C Hamano

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