@matheusbsbs / matheus-bagliano / commits / 702d1b9583

pack-objects: respect --local/--honor-pack-keep/--incremental when bitmap is in use

Since 6b8fda2d (pack-objects: use bitmaps when packing objects) there are two codepaths in pack-objects: with & without using bitmap reachability index. However add_object_entry_from_bitmap(), despite its non-bitmapped counterpart add_object_entry(), in no way does check for whether --local or --honor-pack-keep or --incremental should be respected. In non-bitmapped codepath this is handled in want_object_in_pack(), but bitmapped codepath has simply no such checking at all. The bitmapped codepath however was allowing to pass in all those options and with bitmap indices still being used under such conditions - potentially giving wrong output (e.g. including objects from non-local or .keep'ed pack). We can easily fix this by noting the following: when an object comes to add_object_entry_from_bitmap() it can come for two reasons: 1. entries coming from main pack covered by bitmap index, and 2. object coming from, possibly alternate, loose or other packs. "2" can be already handled by want_object_in_pack() and to cover "1" we can teach want_object_in_pack() to expect that *found_pack can be non-NULL, meaning calling client already found object's pack entry. In want_object_in_pack() we care to start the checks from already found pack, if we have one, this way determining the answer right away in case neither --local nor --honour-pack-keep are active. In particular, as p5310-pack-bitmaps.sh shows (3 consecutive runs), we do not do harm to served-with-bitmap clones performance-wise: Test 56dfeb62 this tree ----------------------------------------------------------------- 5310.2: repack to disk 9.08(8.20+0.25) 9.09(8.14+0.32) +0.1% 5310.3: simulated clone 1.92(2.12+0.08) 1.93(2.12+0.09) +0.5% 5310.4: simulated fetch 0.82(1.07+0.04) 0.82(1.06+0.04) +0.0% 5310.6: partial bitmap 1.96(2.42+0.13) 1.95(2.40+0.15) -0.5% Test 56dfeb62 this tree ----------------------------------------------------------------- 5310.2: repack to disk 9.11(8.16+0.32) 9.11(8.19+0.28) +0.0% 5310.3: simulated clone 1.93(2.14+0.07) 1.92(2.11+0.10) -0.5% 5310.4: simulated fetch 0.82(1.06+0.04) 0.82(1.04+0.05) +0.0% 5310.6: partial bitmap 1.95(2.38+0.16) 1.94(2.39+0.14) -0.5% Test 56dfeb62 this tree ----------------------------------------------------------------- 5310.2: repack to disk 9.13(8.17+0.31) 9.07(8.13+0.28) -0.7% 5310.3: simulated clone 1.92(2.13+0.07) 1.91(2.12+0.06) -0.5% 5310.4: simulated fetch 0.82(1.08+0.03) 0.82(1.08+0.03) +0.0% 5310.6: partial bitmap 1.96(2.43+0.14) 1.96(2.42+0.14) +0.0% with delta timings showing they are all within noise from run to run. In the general case we do not want to call find_pack_entry_one() more than once, because it is expensive. This patch splits the loop in want_object_in_pack() into two parts: finding the object and seeing if it impacts our choice to include it in the pack. We may call the inexpensive want_found_object() twice, but we will never call find_pack_entry_one() if we do not need to. I appreciate help and discussing this change with Junio C Hamano and Jeff King. Signed-off-by: Kirill Smelkov <kirr@nexedi.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Kirill Smelkov committed Sep 10, 2016 at 18:01 UTC 702d1b95831c1a34a217ad63b14895226962955a
2 files changed +156 -33
builtin/pack-objects.c
+64 -33
@@ -944,13 +944,48 @@ static int have_duplicate_entry(const unsigned char *sha1,
944 return 1;
945 }
946
947 +static int want_found_object(int exclude, struct packed_git *p)
948 +{
949 + if (exclude)
950 + return 1;
951 + if (incremental)
952 + return 0;
953 +
954 + /*
955 + * When asked to do --local (do not include an object that appears in a
956 + * pack we borrow from elsewhere) or --honor-pack-keep (do not include
957 + * an object that appears in a pack marked with .keep), finding a pack
958 + * that matches the criteria is sufficient for us to decide to omit it.
959 + * However, even if this pack does not satisfy the criteria, we need to
960 + * make sure no copy of this object appears in _any_ pack that makes us
961 + * to omit the object, so we need to check all the packs.
962 + *
963 + * We can however first check whether these options can possible matter;
964 + * if they do not matter we know we want the object in generated pack.
965 + * Otherwise, we signal "-1" at the end to tell the caller that we do
966 + * not know either way, and it needs to check more packs.
967 + */
968 + if (!ignore_packed_keep &&
969 + (!local || !have_non_local_packs))
970 + return 1;
971 +
972 + if (local && !p->pack_local)
973 + return 0;
974 + if (ignore_packed_keep && p->pack_local && p->pack_keep)
975 + return 0;
976 +
977 + /* we don't know yet; keep looking for more packs */
978 + return -1;
979 +}
980 +
981 /*
982 * Check whether we want the object in the pack (e.g., we do not want
983 * objects found in non-local stores if the "--local" option was used).
984 *
951 - * As a side effect of this check, we will find the packed version of this
952 - * object, if any. We therefore pass out the pack information to avoid having
953 - * to look it up again later.
985 + * If the caller already knows an existing pack it wants to take the object
986 + * from, that is passed in *found_pack and *found_offset; otherwise this
987 + * function finds if there is any pack that has the object and returns the pack
988 + * and its offset in these variables.
989 */
990 static int want_object_in_pack(const unsigned char *sha1,
991 int exclude,
@@ -958,15 +993,30 @@ static int want_object_in_pack(const unsigned char *sha1,
993 off_t *found_offset)
994 {
995 struct packed_git *p;
996 + int want;
997
998 if (!exclude && local && has_loose_object_nonlocal(sha1))
999 return 0;
1000
965 - *found_pack = NULL;
966 - *found_offset = 0;
1001 + /*
1002 + * If we already know the pack object lives in, start checks from that
1003 + * pack - in the usual case when neither --local was given nor .keep files
1004 + * are present we will determine the answer right now.
1005 + */
1006 + if (*found_pack) {
1007 + want = want_found_object(exclude, *found_pack);
1008 + if (want != -1)
1009 + return want;
1010 + }
1011
1012 for (p = packed_git; p; p = p->next) {
969 - off_t offset = find_pack_entry_one(sha1, p);
1013 + off_t offset;
1014 +
1015 + if (p == *found_pack)
1016 + offset = *found_offset;
1017 + else
1018 + offset = find_pack_entry_one(sha1, p);
1019 +
1020 if (offset) {
1021 if (!*found_pack) {
1022 if (!is_pack_valid(p))
@@ -974,31 +1024,9 @@ static int want_object_in_pack(const unsigned char *sha1,
1024 *found_offset = offset;
1025 *found_pack = p;
1026 }
977 - if (exclude)
978 - return 1;
979 - if (incremental)
980 - return 0;
981 -
982 - /*
983 - * When asked to do --local (do not include an
984 - * object that appears in a pack we borrow
985 - * from elsewhere) or --honor-pack-keep (do not
986 - * include an object that appears in a pack marked
987 - * with .keep), we need to make sure no copy of this
988 - * object come from in _any_ pack that causes us to
989 - * omit it, and need to complete this loop. When
990 - * neither option is in effect, we know the object
991 - * we just found is going to be packed, so break
992 - * out of the loop to return 1 now.
993 - */
994 - if (!ignore_packed_keep &&
995 - (!local || !have_non_local_packs))
996 - break;
997 -
998 - if (local && !p->pack_local)
999 - return 0;
1000 - if (ignore_packed_keep && p->pack_local && p->pack_keep)
1001 - return 0;
1027 + want = want_found_object(exclude, p);
1028 + if (want != -1)
1029 + return want;
1030 }
1031 }
1032
@@ -1039,8 +1067,8 @@ static const char no_closure_warning[] = N_(
1067 static int add_object_entry(const unsigned char *sha1, enum object_type type,
1068 const char *name, int exclude)
1069 {
1042 - struct packed_git *found_pack;
1043 - off_t found_offset;
1070 + struct packed_git *found_pack = NULL;
1071 + off_t found_offset = 0;
1072 uint32_t index_pos;
1073
1074 if (have_duplicate_entry(sha1, exclude, &index_pos))
@@ -1073,6 +1101,9 @@ static int add_object_entry_from_bitmap(const unsigned char *sha1,
1101 if (have_duplicate_entry(sha1, 0, &index_pos))
1102 return 0;
1103
1104 + if (!want_object_in_pack(sha1, 0, &pack, &offset))
1105 + return 0;
1106 +
1107 create_object_entry(sha1, type, name_hash, 0, 0, index_pos, pack, offset);
1108
1109 display_progress(progress_state, nr_result);
t/t5310-pack-bitmaps.sh
+92
@@ -7,6 +7,18 @@ objpath () {
7 echo ".git/objects/$(echo "$1" | sed -e 's|\(..\)|\1/|')"
8 }
9
10 +# show objects present in pack ($1 should be associated *.idx)
11 +list_packed_objects () {
12 + git show-index <"$1" | cut -d' ' -f2
13 +}
14 +
15 +# has_any pattern-file content-file
16 +# tests whether content-file has any entry from pattern-file with entries being
17 +# whole lines.
18 +has_any () {
19 + grep -Ff "$1" "$2"
20 +}
21 +
22 test_expect_success 'setup repo with moderate-sized history' '
23 for i in $(test_seq 1 10); do
24 test_commit $i
@@ -16,6 +28,7 @@ test_expect_success 'setup repo with moderate-sized history' '
28 test_commit side-$i
29 done &&
30 git checkout master &&
31 + bitmaptip=$(git rev-parse master) &&
32 blob=$(echo tagged-blob | git hash-object -w --stdin) &&
33 git tag tagged-blob $blob &&
34 git config repack.writebitmaps true &&
@@ -118,6 +131,71 @@ test_expect_success 'incremental repack can disable bitmaps' '
131 git repack -d --no-write-bitmap-index
132 '
133
134 +test_expect_success 'pack-objects respects --local (non-local loose)' '
135 + git init --bare alt.git &&
136 + echo $(pwd)/alt.git/objects >.git/objects/info/alternates &&
137 + echo content1 >file1 &&
138 + # non-local loose object which is not present in bitmapped pack
139 + altblob=$(GIT_DIR=alt.git git hash-object -w file1) &&
140 + # non-local loose object which is also present in bitmapped pack
141 + git cat-file blob $blob | GIT_DIR=alt.git git hash-object -w --stdin &&
142 + git add file1 &&
143 + test_tick &&
144 + git commit -m commit_file1 &&
145 + echo HEAD | git pack-objects --local --stdout --revs >1.pack &&
146 + git index-pack 1.pack &&
147 + list_packed_objects 1.idx >1.objects &&
148 + printf "%s\n" "$altblob" "$blob" >nonlocal-loose &&
149 + ! has_any nonlocal-loose 1.objects
150 +'
151 +
152 +test_expect_success 'pack-objects respects --honor-pack-keep (local non-bitmapped pack)' '
153 + echo content2 >file2 &&
154 + blob2=$(git hash-object -w file2) &&
155 + git add file2 &&
156 + test_tick &&
157 + git commit -m commit_file2 &&
158 + printf "%s\n" "$blob2" "$bitmaptip" >keepobjects &&
159 + pack2=$(git pack-objects pack2 <keepobjects) &&
160 + mv pack2-$pack2.* .git/objects/pack/ &&
161 + >.git/objects/pack/pack2-$pack2.keep &&
162 + rm $(objpath $blob2) &&
163 + echo HEAD | git pack-objects --honor-pack-keep --stdout --revs >2a.pack &&
164 + git index-pack 2a.pack &&
165 + list_packed_objects 2a.idx >2a.objects &&
166 + ! has_any keepobjects 2a.objects
167 +'
168 +
169 +test_expect_success 'pack-objects respects --local (non-local pack)' '
170 + mv .git/objects/pack/pack2-$pack2.* alt.git/objects/pack/ &&
171 + echo HEAD | git pack-objects --local --stdout --revs >2b.pack &&
172 + git index-pack 2b.pack &&
173 + list_packed_objects 2b.idx >2b.objects &&
174 + ! has_any keepobjects 2b.objects
175 +'
176 +
177 +test_expect_success 'pack-objects respects --honor-pack-keep (local bitmapped pack)' '
178 + ls .git/objects/pack/ | grep bitmap >output &&
179 + test_line_count = 1 output &&
180 + packbitmap=$(basename $(cat output) .bitmap) &&
181 + list_packed_objects .git/objects/pack/$packbitmap.idx >packbitmap.objects &&
182 + test_when_finished "rm -f .git/objects/pack/$packbitmap.keep" &&
183 + >.git/objects/pack/$packbitmap.keep &&
184 + echo HEAD | git pack-objects --honor-pack-keep --stdout --revs >3a.pack &&
185 + git index-pack 3a.pack &&
186 + list_packed_objects 3a.idx >3a.objects &&
187 + ! has_any packbitmap.objects 3a.objects
188 +'
189 +
190 +test_expect_success 'pack-objects respects --local (non-local bitmapped pack)' '
191 + mv .git/objects/pack/$packbitmap.* alt.git/objects/pack/ &&
192 + test_when_finished "mv alt.git/objects/pack/$packbitmap.* .git/objects/pack/" &&
193 + echo HEAD | git pack-objects --local --stdout --revs >3b.pack &&
194 + git index-pack 3b.pack &&
195 + list_packed_objects 3b.idx >3b.objects &&
196 + ! has_any packbitmap.objects 3b.objects
197 +'
198 +
199 test_expect_success 'full repack, reusing previous bitmaps' '
200 git repack -ad &&
201 ls .git/objects/pack/ | grep bitmap >output &&
@@ -143,6 +221,20 @@ test_expect_success 'create objects for missing-HAVE tests' '
221 EOF
222 '
223
224 +test_expect_success 'pack-objects respects --incremental' '
225 + cat >revs2 <<-EOF &&
226 + HEAD
227 + $commit
228 + EOF
229 + git pack-objects --incremental --stdout --revs <revs2 >4.pack &&
230 + git index-pack 4.pack &&
231 + list_packed_objects 4.idx >4.objects &&
232 + test_line_count = 4 4.objects &&
233 + git rev-list --objects $commit >revlist &&
234 + cut -d" " -f1 revlist |sort >objects &&
235 + test_cmp 4.objects objects
236 +'
237 +
238 test_expect_success 'pack with missing blob' '
239 rm $(objpath $blob) &&
240 git pack-objects --stdout --revs <revs >/dev/null