Skip to content

Commit 089a0e4

Browse files
gitstergitster
committed
Merge branch 'ds/fix-thin-fix' into jch
"git index-pack --fix-thin" used to abort to prevent a cycle in delta chains from forming in a corner case even when there is no such cycle. Comments? * ds/fix-thin-fix: index-pack: allow revisiting REF_DELTA chains t5309: create failing test for 'git index-pack' test-tool: add pack-deltas helper
2 parents 10caa4a + d8d5c2d commit 089a0e4

File tree

7 files changed

+211
-28
lines changed

7 files changed

+211
-28
lines changed

‎Makefile‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -819,6 +819,7 @@ TEST_BUILTINS_OBJS += test-mergesort.o
819819
TEST_BUILTINS_OBJS += test-mktemp.o
820820
TEST_BUILTINS_OBJS += test-name-hash.o
821821
TEST_BUILTINS_OBJS += test-online-cpus.o
822+
TEST_BUILTINS_OBJS += test-pack-deltas.o
822823
TEST_BUILTINS_OBJS += test-pack-mtimes.o
823824
TEST_BUILTINS_OBJS += test-parse-options.o
824825
TEST_BUILTINS_OBJS += test-parse-pathspec-file.o

‎builtin/index-pack.c‎

Lines changed: 32 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1109,8 +1109,8 @@ static void *threaded_second_pass(void *data)
11091109
set_thread_data(data);
11101110
for (;;) {
11111111
struct base_data *parent = NULL;
1112-
struct object_entry *child_obj;
1113-
struct base_data *child;
1112+
struct object_entry *child_obj = NULL;
1113+
struct base_data *child = NULL;
11141114

11151115
counter_lock();
11161116
display_progress(progress, nr_resolved_deltas);
@@ -1137,15 +1137,18 @@ static void *threaded_second_pass(void *data)
11371137
parent = list_first_entry(&work_head, struct base_data,
11381138
list);
11391139

1140-
if (parent->ref_first <= parent->ref_last) {
1140+
while (parent->ref_first <= parent->ref_last) {
11411141
int offset = ref_deltas[parent->ref_first++].obj_no;
11421142
child_obj = objects + offset;
1143-
if (child_obj->real_type != OBJ_REF_DELTA)
1144-
die("REF_DELTA at offset %"PRIuMAX" already resolved (duplicate base %s?)",
1145-
(uintmax_t) child_obj->idx.offset,
1146-
oid_to_hex(&parent->obj->idx.oid));
1143+
if (child_obj->real_type != OBJ_REF_DELTA) {
1144+
child_obj = NULL;
1145+
continue;
1146+
}
11471147
child_obj->real_type = parent->obj->real_type;
1148-
} else {
1148+
break;
1149+
}
1150+
1151+
if (!child_obj && parent->ofs_first <= parent->ofs_last) {
11491152
child_obj = objects +
11501153
ofs_deltas[parent->ofs_first++].obj_no;
11511154
assert(child_obj->real_type == OBJ_OFS_DELTA);
@@ -1178,29 +1181,32 @@ static void *threaded_second_pass(void *data)
11781181
}
11791182
work_unlock();
11801183

1181-
if (parent) {
1182-
child = resolve_delta(child_obj, parent);
1183-
if (!child->children_remaining)
1184-
FREE_AND_NULL(child->data);
1185-
} else {
1186-
child = make_base(child_obj, NULL);
1187-
if (child->children_remaining) {
1188-
/*
1189-
* Since this child has its own delta children,
1190-
* we will need this data in the future.
1191-
* Inflate now so that future iterations will
1192-
* have access to this object's data while
1193-
* outside the work mutex.
1194-
*/
1195-
child->data = get_data_from_pack(child_obj);
1196-
child->size = child_obj->size;
1184+
if (child_obj) {
1185+
if (parent) {
1186+
child = resolve_delta(child_obj, parent);
1187+
if (!child->children_remaining)
1188+
FREE_AND_NULL(child->data);
1189+
} else{
1190+
child = make_base(child_obj, NULL);
1191+
if (child->children_remaining) {
1192+
/*
1193+
* Since this child has its own delta children,
1194+
* we will need this data in the future.
1195+
* Inflate now so that future iterations will
1196+
* have access to this object's data while
1197+
* outside the work mutex.
1198+
*/
1199+
child->data = get_data_from_pack(child_obj);
1200+
child->size = child_obj->size;
1201+
}
11971202
}
11981203
}
11991204

12001205
work_lock();
12011206
if (parent)
12021207
parent->retain_data--;
1203-
if (child->data) {
1208+
1209+
if (child && child->data) {
12041210
/*
12051211
* This child has its own children, so add it to
12061212
* work_head.
@@ -1209,7 +1215,7 @@ static void *threaded_second_pass(void *data)
12091215
base_cache_used += child->size;
12101216
prune_base_data(NULL);
12111217
free_base_data(child);
1212-
} else {
1218+
} else if (child) {
12131219
/*
12141220
* This child does not have its own children. It may be
12151221
* the last descendant of its ancestors; free those

‎t/helper/meson.build‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ test_tool_sources = [
3636
'test-mktemp.c',
3737
'test-name-hash.c',
3838
'test-online-cpus.c',
39+
'test-pack-deltas.c',
3940
'test-pack-mtimes.c',
4041
'test-parse-options.c',
4142
'test-parse-pathspec-file.c',

‎t/helper/test-pack-deltas.c‎

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
#define USE_THE_REPOSITORY_VARIABLE
2+
3+
#include "test-tool.h"
4+
#include "git-compat-util.h"
5+
#include "delta.h"
6+
#include "git-zlib.h"
7+
#include "hash.h"
8+
#include "hex.h"
9+
#include "pack.h"
10+
#include "pack-objects.h"
11+
#include "setup.h"
12+
#include "strbuf.h"
13+
#include "string-list.h"
14+
15+
static const char usage_str[] = "test-tool pack-deltas <n>";
16+
17+
static unsigned long do_compress(void **pptr, unsigned long size)
18+
{
19+
git_zstream stream;
20+
void *in, *out;
21+
unsigned long maxsize;
22+
23+
git_deflate_init(&stream, 1);
24+
maxsize = git_deflate_bound(&stream, size);
25+
26+
in = *pptr;
27+
out = xmalloc(maxsize);
28+
*pptr = out;
29+
30+
stream.next_in = in;
31+
stream.avail_in = size;
32+
stream.next_out = out;
33+
stream.avail_out = maxsize;
34+
while (git_deflate(&stream, Z_FINISH) == Z_OK)
35+
; /* nothing */
36+
git_deflate_end(&stream);
37+
38+
free(in);
39+
return stream.total_out;
40+
}
41+
42+
static void write_ref_delta(struct hashfile *f,
43+
struct object_id *oid,
44+
struct object_id *base)
45+
{
46+
unsigned char header[MAX_PACK_OBJECT_HEADER];
47+
unsigned long size, base_size, delta_size, compressed_size, hdrlen;
48+
enum object_type type;
49+
void *base_buf, *delta_buf;
50+
void *buf = repo_read_object_file(the_repository,
51+
oid, &type,
52+
&size);
53+
54+
if (!buf)
55+
die("unable to read %s", oid_to_hex(oid));
56+
57+
base_buf = repo_read_object_file(the_repository,
58+
base, &type,
59+
&base_size);
60+
61+
if (!base_buf)
62+
die("unable to read %s", oid_to_hex(base));
63+
64+
delta_buf = diff_delta(base_buf, base_size,
65+
buf, size, &delta_size, 0);
66+
67+
compressed_size = do_compress(&delta_buf, delta_size);
68+
69+
hdrlen = encode_in_pack_object_header(header, sizeof(header),
70+
OBJ_REF_DELTA, delta_size);
71+
hashwrite(f, header, hdrlen);
72+
hashwrite(f, base->hash, the_repository->hash_algo->rawsz);
73+
hashwrite(f, delta_buf, compressed_size);
74+
75+
free(buf);
76+
free(base_buf);
77+
free(delta_buf);
78+
}
79+
80+
int cmd__pack_deltas(int argc, const char **argv)
81+
{
82+
int N;
83+
struct hashfile *f;
84+
struct strbuf line = STRBUF_INIT;
85+
86+
if (argc != 2) {
87+
usage(usage_str);
88+
return -1;
89+
}
90+
91+
N = atoi(argv[1]);
92+
93+
setup_git_directory();
94+
95+
f = hashfd(the_repository->hash_algo, 1, "<stdout>");
96+
write_pack_header(f, N);
97+
98+
/* Read each line from stdin into 'line' */
99+
while (strbuf_getline_lf(&line, stdin) != EOF) {
100+
const char *type_str, *content_oid_str, *base_oid_str = NULL;
101+
struct object_id content_oid, base_oid;
102+
struct string_list items = STRING_LIST_INIT_NODUP;
103+
/*
104+
* Tokenize into two or three parts:
105+
* 1. REF_DELTA, OFS_DELTA, or FULL.
106+
* 2. The object ID for the content object.
107+
* 3. The object ID for the base object (optional).
108+
*/
109+
if (string_list_split_in_place(&items, line.buf, " ", 3) < 0)
110+
die("invalid input format: %s", line.buf);
111+
112+
if (items.nr < 2)
113+
die("invalid input format: %s", line.buf);
114+
115+
type_str = items.items[0].string;
116+
content_oid_str = items.items[1].string;
117+
118+
if (get_oid_hex(content_oid_str, &content_oid))
119+
die("invalid object: %s", content_oid_str);
120+
if (items.nr >= 3) {
121+
base_oid_str = items.items[2].string;
122+
if (get_oid_hex(base_oid_str, &base_oid))
123+
die("invalid object: %s", base_oid_str);
124+
}
125+
string_list_clear(&items, 0);
126+
127+
if (!strcmp(type_str, "REF_DELTA"))
128+
write_ref_delta(f, &content_oid, &base_oid);
129+
else if (!strcmp(type_str, "OFS_DELTA"))
130+
die("OFS_DELTA not implemented");
131+
else if (!strcmp(type_str, "FULL"))
132+
die("FULL not implemented");
133+
else
134+
die("unknown pack type: %s", type_str);
135+
}
136+
137+
finalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK,
138+
CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE);
139+
strbuf_release(&line);
140+
return 0;
141+
}

‎t/helper/test-tool.c‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ static struct test_cmd cmds[] = {
4646
{ "mktemp", cmd__mktemp },
4747
{ "name-hash", cmd__name_hash },
4848
{ "online-cpus", cmd__online_cpus },
49+
{ "pack-deltas", cmd__pack_deltas },
4950
{ "pack-mtimes", cmd__pack_mtimes },
5051
{ "parse-options", cmd__parse_options },
5152
{ "parse-options-flags", cmd__parse_options_flags },

‎t/helper/test-tool.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ int cmd__mergesort(int argc, const char **argv);
3939
int cmd__mktemp(int argc, const char **argv);
4040
int cmd__name_hash(int argc, const char **argv);
4141
int cmd__online_cpus(int argc, const char **argv);
42+
int cmd__pack_deltas(int argc, const char **argv);
4243
int cmd__pack_mtimes(int argc, const char **argv);
4344
int cmd__parse_options(int argc, const char **argv);
4445
int cmd__parse_options_flags(int argc, const char **argv);

‎t/t5309-pack-delta-cycles.sh‎

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,10 @@ test_expect_success 'index-pack detects REF_DELTA cycles' '
6060
test_expect_success 'failover to an object in another pack' '
6161
clear_packs &&
6262
git index-pack --stdin <ab.pack &&
63-
test_must_fail git index-pack --stdin --fix-thin <cycle.pack
63+
64+
# This cycle does not fail since the existence of A & B in
65+
# the repo allows us to resolve the cycle.
66+
git index-pack --stdin --fix-thin <cycle.pack
6467
'
6568

6669
test_expect_success 'failover to a duplicate object in the same pack' '
@@ -72,7 +75,36 @@ test_expect_success 'failover to a duplicate object in the same pack' '
7275
pack_obj $A
7376
} >recoverable.pack &&
7477
pack_trailer recoverable.pack &&
75-
test_must_fail git index-pack --fix-thin --stdin <recoverable.pack
78+
79+
# This cycle does not fail since the existence of a full copy
80+
# of A in the pack allows us to resolve the cycle.
81+
git index-pack --fix-thin --stdin <recoverable.pack
82+
'
83+
84+
test_expect_success 'index-pack works with thin pack A->B->C with B on disk' '
85+
git init server &&
86+
(
87+
cd server &&
88+
test_commit_bulk 4
89+
) &&
90+
91+
A=$(git -C server rev-parse HEAD^{tree}) &&
92+
B=$(git -C server rev-parse HEAD~1^{tree}) &&
93+
C=$(git -C server rev-parse HEAD~2^{tree}) &&
94+
git -C server reset --hard HEAD~1 &&
95+
96+
cat >in <<-EOF &&
97+
REF_DELTA $A $B
98+
REF_DELTA $B $C
99+
EOF
100+
101+
test-tool -C server pack-deltas 2 <in >thin.pack &&
102+
103+
git clone "file://$(pwd)/server" client &&
104+
(
105+
cd client &&
106+
git index-pack --fix-thin --stdin <../thin.pack
107+
)
76108
'
77109

78110
test_done

0 commit comments

Comments
 (0)