Skip to content

Commit c08589e

Browse files
jonathantanmygitster
authored andcommitted
index-pack: repack local links into promisor packs
Teach index-pack to, when processing the objects in a pack with --promisor specified on the CLI, repack local objects (and the local objects that they refer to, recursively) referenced by these objects into promisor packs. This prevents the situation in which, when fetching from a promisor remote, we end up with promisor objects (newly fetched) referring to non-promisor objects (locally created prior to the fetch). This situation may arise if the client had previously pushed objects to the remote, for example. One issue that arises in this situation is that, if the non-promisor objects become inaccessible except through promisor objects (for example, if the branch pointing to them has moved to point to the promisor object that refers to them), then GC will garbage collect them. There are other ways to solve this, but the simplest seems to be to enforce the invariant that we don't have promisor objects referring to non-promisor objects. This repacking is done from index-pack to minimize the performance impact. During a fetch, the only time most objects are fully inflated in memory is when their object ID is computed, so we also scan the objects (to see which objects they refer to) during this time. Also to minimize the performance impact, an object is calculated to be local if it's a loose object or present in a non-promisor pack. (If it's also in a promisor pack or referred to by an object in a promisor pack, it is technically already a promisor object. But a misidentification of a promisor object as a non-promisor object is relatively benign here - we will thus repack that promisor object into a promisor pack, duplicating it in the object store, but there is no correctness issue, just an issue of inefficiency.) Signed-off-by: Jonathan Tan <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent d9e24ce commit c08589e

File tree

4 files changed

+172
-2
lines changed

4 files changed

+172
-2
lines changed

Documentation/git-index-pack.txt

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,11 @@ include::object-format-disclaimer.txt[]
139139
written. If a `<message>` is provided, then that content will be
140140
written to the .promisor file for future reference. See
141141
link:technical/partial-clone.html[partial clone] for more information.
142+
+
143+
Also, if there are objects in the given pack that references non-promisor
144+
objects (in the repo), repacks those non-promisor objects into a promisor
145+
pack. This avoids a situation in which a repo has non-promisor objects that are
146+
accessible through promisor objects.
142147

143148
NOTES
144149
-----

builtin/index-pack.c

Lines changed: 109 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include "csum-file.h"
1010
#include "blob.h"
1111
#include "commit.h"
12+
#include "tag.h"
1213
#include "tree.h"
1314
#include "progress.h"
1415
#include "fsck.h"
@@ -20,9 +21,14 @@
2021
#include "object-file.h"
2122
#include "object-store-ll.h"
2223
#include "oid-array.h"
24+
#include "oidset.h"
25+
#include "path.h"
2326
#include "replace-object.h"
27+
#include "tree-walk.h"
2428
#include "promisor-remote.h"
29+
#include "run-command.h"
2530
#include "setup.h"
31+
#include "strvec.h"
2632

2733
static const char index_pack_usage[] =
2834
"git index-pack [-v] [-o <index-file>] [--keep | --keep=<msg>] [--[no-]rev-index] [--verify] [--strict[=<msg-id>=<severity>...]] [--fsck-objects[=<msg-id>=<severity>...]] (<pack-file> | --stdin [--fix-thin] [<pack-file>])";
@@ -148,6 +154,13 @@ static uint32_t input_crc32;
148154
static int input_fd, output_fd;
149155
static const char *curr_pack;
150156

157+
/*
158+
* local_links is guarded by read_mutex, and record_local_links is read-only in
159+
* a thread.
160+
*/
161+
static struct oidset local_links = OIDSET_INIT;
162+
static int record_local_links;
163+
151164
static struct thread_local *thread_data;
152165
static int nr_dispatched;
153166
static int threads_active;
@@ -799,6 +812,44 @@ static int check_collison(struct object_entry *entry)
799812
return 0;
800813
}
801814

815+
static void record_if_local_object(const struct object_id *oid)
816+
{
817+
struct object_info info = OBJECT_INFO_INIT;
818+
if (oid_object_info_extended(the_repository, oid, &info, 0))
819+
/* Missing; assume it is a promisor object */
820+
return;
821+
if (info.whence == OI_PACKED && info.u.packed.pack->pack_promisor)
822+
return;
823+
oidset_insert(&local_links, oid);
824+
}
825+
826+
static void do_record_local_links(struct object *obj)
827+
{
828+
if (obj->type == OBJ_TREE) {
829+
struct tree *tree = (struct tree *)obj;
830+
struct tree_desc desc;
831+
struct name_entry entry;
832+
if (init_tree_desc_gently(&desc, &tree->object.oid,
833+
tree->buffer, tree->size, 0))
834+
/*
835+
* Error messages are given when packs are
836+
* verified, so do not print any here.
837+
*/
838+
return;
839+
while (tree_entry_gently(&desc, &entry))
840+
record_if_local_object(&entry.oid);
841+
} else if (obj->type == OBJ_COMMIT) {
842+
struct commit *commit = (struct commit *) obj;
843+
struct commit_list *parents = commit->parents;
844+
845+
for (; parents; parents = parents->next)
846+
record_if_local_object(&parents->item->object.oid);
847+
} else if (obj->type == OBJ_TAG) {
848+
struct tag *tag = (struct tag *) obj;
849+
record_if_local_object(get_tagged_oid(tag));
850+
}
851+
}
852+
802853
static void sha1_object(const void *data, struct object_entry *obj_entry,
803854
unsigned long size, enum object_type type,
804855
const struct object_id *oid)
@@ -845,7 +896,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,
845896
free(has_data);
846897
}
847898

848-
if (strict || do_fsck_object) {
899+
if (strict || do_fsck_object || record_local_links) {
849900
read_lock();
850901
if (type == OBJ_BLOB) {
851902
struct blob *blob = lookup_blob(the_repository, oid);
@@ -877,6 +928,8 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,
877928
die(_("fsck error in packed object"));
878929
if (strict && fsck_walk(obj, NULL, &fsck_options))
879930
die(_("Not all child objects of %s are reachable"), oid_to_hex(&obj->oid));
931+
if (record_local_links)
932+
do_record_local_links(obj);
880933

881934
if (obj->type == OBJ_TREE) {
882935
struct tree *item = (struct tree *) obj;
@@ -1719,6 +1772,58 @@ static void show_pack_info(int stat_only)
17191772
free(chain_histogram);
17201773
}
17211774

1775+
static void repack_local_links(void)
1776+
{
1777+
struct child_process cmd = CHILD_PROCESS_INIT;
1778+
FILE *out;
1779+
struct strbuf line = STRBUF_INIT;
1780+
struct oidset_iter iter;
1781+
struct object_id *oid;
1782+
char *base_name;
1783+
1784+
if (!oidset_size(&local_links))
1785+
return;
1786+
1787+
base_name = mkpathdup("%s/pack/pack", repo_get_object_directory(the_repository));
1788+
1789+
strvec_push(&cmd.args, "pack-objects");
1790+
strvec_push(&cmd.args, "--exclude-promisor-objects-best-effort");
1791+
strvec_push(&cmd.args, base_name);
1792+
cmd.git_cmd = 1;
1793+
cmd.in = -1;
1794+
cmd.out = -1;
1795+
if (start_command(&cmd))
1796+
die(_("could not start pack-objects to repack local links"));
1797+
1798+
oidset_iter_init(&local_links, &iter);
1799+
while ((oid = oidset_iter_next(&iter))) {
1800+
if (write_in_full(cmd.in, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||
1801+
write_in_full(cmd.in, "\n", 1) < 0)
1802+
die(_("failed to feed local object to pack-objects"));
1803+
}
1804+
close(cmd.in);
1805+
1806+
out = xfdopen(cmd.out, "r");
1807+
while (strbuf_getline_lf(&line, out) != EOF) {
1808+
unsigned char binary[GIT_MAX_RAWSZ];
1809+
if (line.len != the_hash_algo->hexsz ||
1810+
!hex_to_bytes(binary, line.buf, line.len))
1811+
die(_("index-pack: Expecting full hex object ID lines only from pack-objects."));
1812+
1813+
/*
1814+
* pack-objects creates the .pack and .idx files, but not the
1815+
* .promisor file. Create the .promisor file, which is empty.
1816+
*/
1817+
write_special_file("promisor", "", NULL, binary, NULL);
1818+
}
1819+
1820+
fclose(out);
1821+
if (finish_command(&cmd))
1822+
die(_("could not finish pack-objects to repack local links"));
1823+
strbuf_release(&line);
1824+
free(base_name);
1825+
}
1826+
17221827
int cmd_index_pack(int argc,
17231828
const char **argv,
17241829
const char *prefix,
@@ -1794,7 +1899,7 @@ int cmd_index_pack(int argc,
17941899
} else if (skip_to_optional_arg(arg, "--keep", &keep_msg)) {
17951900
; /* nothing to do */
17961901
} else if (skip_to_optional_arg(arg, "--promisor", &promisor_msg)) {
1797-
; /* already parsed */
1902+
record_local_links = 1;
17981903
} else if (starts_with(arg, "--threads=")) {
17991904
char *end;
18001905
nr_threads = strtoul(arg+10, &end, 0);
@@ -1970,6 +2075,8 @@ int cmd_index_pack(int argc,
19702075
free((void *) curr_index);
19712076
free(curr_rev_index);
19722077

2078+
repack_local_links();
2079+
19732080
/*
19742081
* Let the caller know this pack is not self contained
19752082
*/

builtin/pack-objects.c

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -239,6 +239,7 @@ static enum {
239239
static uint16_t write_bitmap_options = BITMAP_OPT_HASH_CACHE;
240240

241241
static int exclude_promisor_objects;
242+
static int exclude_promisor_objects_best_effort;
242243

243244
static int use_delta_islands;
244245

@@ -4312,6 +4313,18 @@ static int option_parse_cruft_expiration(const struct option *opt UNUSED,
43124313
return 0;
43134314
}
43144315

4316+
static int is_not_in_promisor_pack_obj(struct object *obj, void *data UNUSED)
4317+
{
4318+
struct object_info info = OBJECT_INFO_INIT;
4319+
if (oid_object_info_extended(the_repository, &obj->oid, &info, 0))
4320+
BUG("should_include_obj should only be called on existing objects");
4321+
return info.whence != OI_PACKED || !info.u.packed.pack->pack_promisor;
4322+
}
4323+
4324+
static int is_not_in_promisor_pack(struct commit *commit, void *data) {
4325+
return is_not_in_promisor_pack_obj((struct object *) commit, data);
4326+
}
4327+
43154328
int cmd_pack_objects(int argc,
43164329
const char **argv,
43174330
const char *prefix,
@@ -4424,6 +4437,9 @@ int cmd_pack_objects(int argc,
44244437
option_parse_missing_action),
44254438
OPT_BOOL(0, "exclude-promisor-objects", &exclude_promisor_objects,
44264439
N_("do not pack objects in promisor packfiles")),
4440+
OPT_BOOL(0, "exclude-promisor-objects-best-effort",
4441+
&exclude_promisor_objects_best_effort,
4442+
N_("implies --missing=allow-any")),
44274443
OPT_BOOL(0, "delta-islands", &use_delta_islands,
44284444
N_("respect islands during delta compression")),
44294445
OPT_STRING_LIST(0, "uri-protocol", &uri_protocols,
@@ -4504,10 +4520,18 @@ int cmd_pack_objects(int argc,
45044520
strvec_push(&rp, "--unpacked");
45054521
}
45064522

4523+
if (exclude_promisor_objects && exclude_promisor_objects_best_effort)
4524+
die(_("options '%s' and '%s' cannot be used together"),
4525+
"--exclude-promisor-objects", "--exclude-promisor-objects-best-effort");
45074526
if (exclude_promisor_objects) {
45084527
use_internal_rev_list = 1;
45094528
fetch_if_missing = 0;
45104529
strvec_push(&rp, "--exclude-promisor-objects");
4530+
} else if (exclude_promisor_objects_best_effort) {
4531+
use_internal_rev_list = 1;
4532+
fetch_if_missing = 0;
4533+
option_parse_missing_action(NULL, "allow-any", 0);
4534+
/* revs configured below */
45114535
}
45124536
if (unpack_unreachable || keep_unreachable || pack_loose_unreachable)
45134537
use_internal_rev_list = 1;
@@ -4627,6 +4651,10 @@ int cmd_pack_objects(int argc,
46274651

46284652
repo_init_revisions(the_repository, &revs, NULL);
46294653
list_objects_filter_copy(&revs.filter, &filter_options);
4654+
if (exclude_promisor_objects_best_effort) {
4655+
revs.include_check = is_not_in_promisor_pack;
4656+
revs.include_check_obj = is_not_in_promisor_pack_obj;
4657+
}
46304658
get_object_list(&revs, rp.nr, rp.v);
46314659
release_revisions(&revs);
46324660
}

t/t5616-partial-clone.sh

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -694,6 +694,36 @@ test_expect_success 'lazy-fetch in submodule succeeds' '
694694
git -C client restore --recurse-submodules --source=HEAD^ :/
695695
'
696696

697+
test_expect_success 'after fetching descendants of non-promisor commits, gc works' '
698+
# Setup
699+
git init full &&
700+
git -C full config uploadpack.allowfilter 1 &&
701+
git -C full config uploadpack.allowanysha1inwant 1 &&
702+
touch full/foo &&
703+
git -C full add foo &&
704+
git -C full commit -m "commit 1" &&
705+
git -C full checkout --detach &&
706+
707+
# Partial clone and push commit to remote
708+
git clone "file://$(pwd)/full" --filter=blob:none partial &&
709+
echo "hello" > partial/foo &&
710+
git -C partial commit -a -m "commit 2" &&
711+
git -C partial push &&
712+
713+
# gc in partial repo
714+
git -C partial gc --prune=now &&
715+
716+
# Create another commit in normal repo
717+
git -C full checkout main &&
718+
echo " world" >> full/foo &&
719+
git -C full commit -a -m "commit 3" &&
720+
721+
# Pull from remote in partial repo, and run gc again
722+
git -C partial pull &&
723+
git -C partial gc --prune=now
724+
'
725+
726+
697727
. "$TEST_DIRECTORY"/lib-httpd.sh
698728
start_httpd
699729

0 commit comments

Comments
 (0)