git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 3/9] maintenance: add loose-objects task

From
Derrick Stolee <stolee@gmail.com>
Date
Aug 14, 2020, 01:46 UTC
Message-ID
<0fd9d838-657f-448a-f09b-6e431aebdf69@gmail.com>
In-Reply-To
<20200812231053.GI2965447@google.com>
On 8/12/2020 7:10 PM, Emily Shaffer wrote:
Show 25 quoted lines
> On Thu, Aug 06, 2020 at 04:30:18PM +0000, Derrick Stolee via GitGitGadget wrote:
> 
>> diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt
>> index bb0d5eded4..898aff4726 100644
>> --- a/Documentation/git-maintenance.txt
>> +++ b/Documentation/git-maintenance.txt
>> @@ -80,6 +80,17 @@ gc::
>>  	It can also be disruptive in some situations, as it deletes stale
>>  	data.
>>  
>> +loose-objects::
>> +	The `loose-objects` job cleans up loose objects and places them into
>> +	pack-files. In order to prevent race conditions with concurrent Git
>> +	commands, it follows a two-step process. First, it deletes any loose
>> +	objects that already exist in a pack-file; concurrent Git processes
>> +	will examine the pack-file for the object data instead of the loose
>> +	object. Second, it creates a new pack-file (starting with "loose-")
>> +	containing a batch of loose objects. The batch size is limited to 50
>> +	thousand objects to prevent the job from taking too long on a
>> +	repository with many loose objects.
> 
> [emily] What's not said is what happens to loose-* packfile. Does this
> get repacked and disappear, is it treated differently, etc?
> [jonathantan] This is treated the same as any other pack. Is there a
> reason to call it loose-*?

Auditing, mostly. When working with repositories that have multiple pack-files as a rule, it is helpful to know what "kind" of pack-file they are. I almost added a "repacked-" prefix to the packs produced by 'git multi-pack-index repack'. It helps to distinguish "reprocessed" data from "I got this from a 'clone' or 'fetch'."

Of course, as these pack-files are repacked later, the prefixes will get dropped. The name does give us some help when performing diagnostics on a misbehaving repository. A pack directory filled with many "loose-*.pack" files indicates that the 'loose-objects' step was run many times without either 'gc' or 'incremental-repack' coalescing those packs with delta compression.

> [jrnieder] It seems like unreachable objects will get stuck going in and
> out of this pack, right? unreachable loose obj -> loose-*.pack ->
> repack, unreachable becomes loose -> repeat

This is a good point. This task would probably interact poorly with the 'gc' task in that regard. They are not intended to both be enabled at the same time, but it might be good to point that out directly in the documentation.

How does this sound?
 The `gc` task writes unreachable objects as loose objects to
 be cleaned up by a later step only if they are not re-added to
 a pack-file; for this reason it is not advisable to enable both
 the `loose-objects` and `gc` tasks at the same time.
Show 11 quoted lines
>> +static int prune_packed(struct maintenance_opts *opts)
>> +{
>> +	struct child_process child = CHILD_PROCESS_INIT;
>> +
>> +	child.git_cmd = 1;
>> +	strvec_push(&child.args, "prune-packed");
>> +
>> +	if (opts->quiet)
>> +		strvec_push(&child.args, "--quiet");
>> +
>> +	return !!run_command(&child);
Show 9 quoted lines
> [emily] Why not report the error code here to the caller? Is there a
> path to notify user of errors that require user intervention?
> [jrnieder] Some errors might be expected and some might not, so should
> we handle them differently?
> [emily] Probably makes more sense to revisit this in the far future when
> we teach 'git maintenance' to send us emails and flashing lights when
> our jobs fail, instead of worrying about it now :)
> [jrnieder] Imagine if we got exit 127 because the usage we are using is
> wrong - in that case, for example, we would want to BUG()

I was previously returning the exact error code, but received feedback that these internal methods should have simpler return values [1].

[1] https://lore.kernel.org/git/xmqq4kpyq8wh.fsf@gitster.c.googlers.com/
Show 5 quoted lines
>> +static int write_loose_object_to_stdin(const struct object_id *oid,
>> +				       const char *path,
>> +				       void *data)
>> +{
>> +	struct write_loose_object_data *d = (struct write_loose_object_data *)data;
> [jrnieder] Since we are enlightened C developers, not C++ developers,
> you can skip the explicit cast.

Hm. I definitely _could_ but don't really want to. Perhaps I'm just being pedantic, but I was just talking to someone today off-list that they were disappointed that they couldn't compile Git in a C++ compiler mainly because of these kinds of casting issues.

I was surprised the word "cast" does not appear anywhere in the
CodingGuidelines, because if this is a convention we want to follow,
then I have definitely been violating it for a *while*.
Show 6 quoted lines
>> +++ b/t/t7900-maintenance.sh
>> @@ -83,4 +83,43 @@ test_expect_success 'prefetch multiple remotes' '
>>  	git log prefetch/remote2/two
>>  '
>>  
>> +test_expect_success 'loose-objects task' '
> [jrnieder] Unrelated to this change, this test makes me sad that we
> don't have better low-level test helpers to enable this kind of testing.> Makes me wish for something better :)

Do you mean you wish we had helpers for getting a repo into a certain state of its object database? Or somehow mocking the object database in-memory to provide more unit-level testing? I don't understand what "something better" looks like.

Show 39 quoted lines
>> +	# Repack everything so we know the state of the object dir
>> +	git repack -adk &&
>> +
>> +	# Hack to stop maintenance from running during "git commit"
>> +	echo in use >.git/objects/maintenance.lock &&
>> +
>> +	# Assuming that "git commit" creates at least one loose object
>> +	test_commit create-loose-object &&
>> +	rm .git/objects/maintenance.lock &&
>> +
>> +	ls .git/objects >obj-dir-before &&
>> +	test_file_not_empty obj-dir-before &&
>> +	ls .git/objects/pack/*.pack >packs-before &&
>> +	test_line_count = 1 packs-before &&
>> +
>> +	# The first run creates a pack-file
>> +	# but does not delete loose objects.
>> +	git maintenance run --task=loose-objects &&
>> +	ls .git/objects >obj-dir-between &&
>> +	test_cmp obj-dir-before obj-dir-between &&
>> +	ls .git/objects/pack/*.pack >packs-between &&
>> +	test_line_count = 2 packs-between &&
>> +	ls .git/objects/pack/loose-*.pack >loose-packs &&
>> +	test_line_count = 1 loose-packs &&
>> +
>> +	# The second run deletes loose objects
>> +	# but does not create a pack-file.
>> +	git maintenance run --task=loose-objects &&
>> +	ls .git/objects >obj-dir-after &&
>> +	cat >expect <<-\EOF &&
>> +	info
>> +	pack
>> +	EOF
>> +	test_cmp expect obj-dir-after &&
>> +	ls .git/objects/pack/*.pack >packs-after &&
>> +	test_cmp packs-between packs-after
>> +'
>> +
>
Previous: Emily ShafferNext: Derrick Stolee via GitGitGadget
Message 12 of 66 in “Maintenance II: prefetch, loose-objects, incremental-repack tasks”
  1. 0/9 Maintenance II: prefetch, loose-objects, incremental-repack tasksDerrick Stolee via GitGitGadget, Aug 6, 2020
  2. 8/9 maintenance: auto-size incremental-repack batchDerrick Stolee via GitGitGadget, Aug 6, 2020
  3. Son Luong NgocAug 6, 2020
  4. Derrick StoleeAug 6, 2020
  5. 6/9 midx: use start_delayed_progress()Derrick Stolee via GitGitGadget, Aug 6, 2020
  6. 9/9 maintenance: add incremental-repack auto conditionDerrick Stolee via GitGitGadget, Aug 6, 2020
  7. 7/9 maintenance: add incremental-repack taskDerrick Stolee via GitGitGadget, Aug 6, 2020
  8. 5/9 midx: enable core.multiPackIndex by defaultDerrick Stolee via GitGitGadget, Aug 6, 2020
  9. 4/9 maintenance: create auto condition for loose-objectsDerrick Stolee via GitGitGadget, Aug 6, 2020
  10. 3/9 maintenance: add loose-objects taskDerrick Stolee via GitGitGadget, Aug 6, 2020
  11. Emily ShafferAug 12, 2020
  12. Derrick StoleeAug 14, 2020
  13. 2/9 maintenance: add prefetch taskDerrick Stolee via GitGitGadget, Aug 6, 2020
  14. Emily ShafferAug 12, 2020
  15. Derrick StoleeAug 14, 2020
  16. 1/9 fetch: optionally allow disabling FETCH_HEAD updateJunio C Hamano via GitGitGadget, Aug 6, 2020
  17. Emily ShafferAug 12, 2020
  18. Junio C HamanoAug 13, 2020
  19. Jonathan NiederAug 13, 2020
  20. fetch: optionally allow disabling FETCH_HEAD updateJunio C Hamano, Aug 13, 2020
  21. Derrick StoleeAug 14, 2020
  22. Junio C HamanoAug 14, 2020
  23. 0/9 Maintenance II: prefetch, loose-objects, incremental-repack tasksDerrick Stolee via GitGitGadget, Aug 18, 2020
  24. 1/9 fetch: optionally allow disabling FETCH_HEAD updateJunio C Hamano via GitGitGadget, Aug 18, 2020
  25. 3/9 maintenance: add loose-objects taskDerrick Stolee via GitGitGadget, Aug 18, 2020
  26. 5/9 midx: enable core.multiPackIndex by defaultDerrick Stolee via GitGitGadget, Aug 18, 2020
  27. 2/9 maintenance: add prefetch taskDerrick Stolee via GitGitGadget, Aug 18, 2020
  28. 8/9 maintenance: auto-size incremental-repack batchDerrick Stolee via GitGitGadget, Aug 18, 2020
  29. 9/9 maintenance: add incremental-repack auto conditionDerrick Stolee via GitGitGadget, Aug 18, 2020
  30. 7/9 maintenance: add incremental-repack taskDerrick Stolee via GitGitGadget, Aug 18, 2020
  31. 6/9 midx: use start_delayed_progress()Derrick Stolee via GitGitGadget, Aug 18, 2020
  32. 4/9 maintenance: create auto condition for loose-objectsDerrick Stolee via GitGitGadget, Aug 18, 2020
  33. 0/8 Maintenance II: prefetch, loose-objects, incremental-repack tasksDerrick Stolee via GitGitGadget, Aug 25, 2020
  34. 1/8 maintenance: add prefetch taskDerrick Stolee via GitGitGadget, Aug 25, 2020
  35. Jonathan TanSep 22, 2020
  36. 2/8 maintenance: add loose-objects taskDerrick Stolee via GitGitGadget, Aug 25, 2020
  37. Jonathan TanSep 22, 2020
  38. Derrick StoleeSep 24, 2020
  39. 3/8 maintenance: create auto condition for loose-objectsDerrick Stolee via GitGitGadget, Aug 25, 2020
  40. Jonathan TanSep 22, 2020
  41. Derrick StoleeSep 24, 2020
  42. 4/8 midx: enable core.multiPackIndex by defaultDerrick Stolee via GitGitGadget, Aug 25, 2020
  43. Jonathan TanSep 22, 2020
  44. Derrick StoleeSep 24, 2020
  45. 5/8 midx: use start_delayed_progress()Derrick Stolee via GitGitGadget, Aug 25, 2020
  46. 6/8 maintenance: add incremental-repack taskDerrick Stolee via GitGitGadget, Aug 25, 2020
  47. Jonathan TanSep 22, 2020
  48. Derrick StoleeSep 24, 2020
  49. Jonathan TanSep 24, 2020
  50. 8/8 maintenance: add incremental-repack auto conditionDerrick Stolee via GitGitGadget, Aug 25, 2020
  51. Jonathan TanSep 22, 2020
  52. 7/8 maintenance: auto-size incremental-repack batchDerrick Stolee via GitGitGadget, Aug 25, 2020
  53. Junio C HamanoAug 25, 2020
  54. Son Luong NgocAug 26, 2020
  55. Derrick StoleeAug 26, 2020
  56. 0/8 Maintenance II: prefetch, loose-objects, incremental-repack tasksDerrick Stolee via GitGitGadget, Sep 25, 2020
  57. 1/8 maintenance: add prefetch taskDerrick Stolee via GitGitGadget, Sep 25, 2020
  58. 3/8 maintenance: create auto condition for loose-objectsDerrick Stolee via GitGitGadget, Sep 25, 2020
  59. Junio C HamanoSep 25, 2020
  60. Derrick StoleeSep 25, 2020
  61. 4/8 midx: enable core.multiPackIndex by defaultDerrick Stolee via GitGitGadget, Sep 25, 2020
  62. 2/8 maintenance: add loose-objects taskDerrick Stolee via GitGitGadget, Sep 25, 2020
  63. 5/8 midx: use start_delayed_progress()Derrick Stolee via GitGitGadget, Sep 25, 2020
  64. 7/8 maintenance: auto-size incremental-repack batchDerrick Stolee via GitGitGadget, Sep 25, 2020
  65. 8/8 maintenance: add incremental-repack auto conditionDerrick Stolee via GitGitGadget, Sep 25, 2020
  66. 6/8 maintenance: add incremental-repack taskDerrick Stolee via GitGitGadget, Sep 25, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.