{"thread":{"id":"55419","subject":"[PATCH 1/3] pack-bitmap: add 'test_bitmap_commits()' helper","startedAt":"2021-04-01T01:32:45Z","lastAt":"2021-05-27T12:52:22Z","messageCount":13,"participants":["Taylor Blau","Derrick Stolee","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"420764","messageId":"053ba4b8319cae9fc804b5f9ce452be1bb16e1da.1617240723.git.me@ttaylorr.com","threadId":"55419","inReplyTo":"cover.1617240723.git.me@ttaylorr.com","subject":"[PATCH 1/3] pack-bitmap: add 'test_bitmap_commits()' helper","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-04-01T01:32:07Z","receivedAt":"2021-04-01T01:32:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The next patch will add a 'bitmap' test-tool which prints the list of\ncommits that have bitmaps computed.\n\nThe test helper could implement this itself, but it would need access to\nthe 'bitmaps' field of the 'pack_bitmap' struct. To avoid exposing this\nprivate detail, implement the entirety of the helper behind a\ntest_bitmap_commits() function in pack-bitmap.c.\n\nThere is some precedence for this with test_bitmap_walk() which is used\nto implement the '--test-bitmap' flag in 'git rev-list' (and is also\nimplemented in pack-bitmap.c).\n\nA caller will be added in the next patch.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n pack-bitmap.c | 18 ++++++++++++++++++\n pack-bitmap.h |  1 +\n 2 files changed, 19 insertions(+)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 1ebe0c8162..7554510b14 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1350,6 +1350,24 @@ void test_bitmap_walk(struct rev_info *revs)\n \tfree_bitmap_index(bitmap_git);\n }\n \n+int test_bitmap_commits(struct repository *r)\n+{\n+\tstruct bitmap_index *bitmap_git = prepare_bitmap_git(r);\n+\tstruct object_id oid;\n+\tMAYBE_UNUSED void *value;\n+\n+\tif (!bitmap_git)\n+\t\tdie(\"failed to load bitmap indexes\");\n+\n+\tkh_foreach(bitmap_git->bitmaps, oid, value, {\n+\t\tprintf(\"%s\\n\", oid_to_hex(&oid));\n+\t});\n+\n+\tfree_bitmap_index(bitmap_git);\n+\n+\treturn 0;\n+}\n+\n int rebuild_bitmap(const uint32_t *reposition,\n \t\t   struct ewah_bitmap *source,\n \t\t   struct bitmap *dest)\ndiff --git a/pack-bitmap.h b/pack-bitmap.h\nindex 36d99930d8..c3cdd80756 100644\n--- a/pack-bitmap.h\n+++ b/pack-bitmap.h\n@@ -49,6 +49,7 @@ void traverse_bitmap_commit_list(struct bitmap_index *,\n \t\t\t\t struct rev_info *revs,\n \t\t\t\t show_reachable_fn show_reachable);\n void test_bitmap_walk(struct rev_info *revs);\n+int test_bitmap_commits(struct repository *r);\n struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs,\n \t\t\t\t\t struct list_objects_filter_options *filter);\n int reuse_partial_packfile_from_bitmap(struct bitmap_index *,\n-- \n2.31.1.163.ga65ce7f831\n\n"},{"id":"420765","messageId":"cover.1617240723.git.me@ttaylorr.com","threadId":"55419","inReplyTo":null,"subject":"[PATCH 0/3] pack-objects: introduce pack.preferBitmapTips","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-04-01T01:32:03Z","receivedAt":"2021-04-01T01:32:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This series adds a new multi-valued configuration, pack.preferBitmapTips\nto allow callers to specify a list of reference prefixes which should\nmark their tips with the NEEDS_BITMAP flag.\n\nThis has a similar effect as setting pack.islandCore: it indicates to\nthe bitmap selection code that any commit object with that flag should\nget bitmap coverage (it doesn't necessarily imply that it *will*, see\nthe third patch for details of why).\n\nThis is more fall-out from the multi-pack bitmaps topic. Since we\ngenerate MIDX bitmaps after a geometric repack, any delta-islands\nconfiguration only takes effect on objects in the newly-created pack.\nAny commits which weren't in that pack may not get the same coverage they\notherwise would have if repacking all-into-one with the same\ndelta-islands settings.\n\nSo, this config was designed to suggest which commits should get marked\nwith NEEDS_BITMAP when the MIDX bitmap code does its own traversal to\nfigure out which reachable objects are in the MIDX. But it's useful for\nsingle-pack bitmaps, too, if you're using them without delta islands.\n\n(Like all of the MIDX bitmap topics, we have been running this code at\nGitHub without any issue for a few weeks.)\n\nThanks in advance for your review.\n\nTaylor Blau (3):\n  pack-bitmap: add 'test_bitmap_commits()' helper\n  t/helper/test-bitmap.c: initial commit\n  builtin/pack-objects.c: respect 'pack.preferBitmapTips'\n\n Documentation/config/pack.txt | 15 ++++++++++++++\n Makefile                      |  1 +\n builtin/pack-objects.c        | 34 +++++++++++++++++++++++++++++++\n pack-bitmap.c                 | 24 ++++++++++++++++++++++\n pack-bitmap.h                 |  4 ++++\n t/helper/test-bitmap.c        | 24 ++++++++++++++++++++++\n t/helper/test-tool.c          |  1 +\n t/helper/test-tool.h          |  1 +\n t/t5310-pack-bitmaps.sh       | 38 +++++++++++++++++++++++++++++++++++\n 9 files changed, 142 insertions(+)\n create mode 100644 t/helper/test-bitmap.c\n\n-- \n2.31.1.163.ga65ce7f831\n"},{"id":"420766","messageId":"ec974bb0c85bcde00d45e983d701c538488550a6.1617240723.git.me@ttaylorr.com","threadId":"55419","inReplyTo":"cover.1617240723.git.me@ttaylorr.com","subject":"[PATCH 2/3] t/helper/test-bitmap.c: initial commit","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-04-01T01:32:11Z","receivedAt":"2021-04-01T01:33:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Add a new 'bitmap' test-tool which can be used to list the commits that\nhave received bitmaps.\n\nIn theory, a determined tester could run 'git rev-list --test-bitmap\n<commit>' to check if '<commit>' received a bitmap or not, since\n'--test-bitmap' exits with a non-zero code when it can't find the\nrequested commit.\n\nBut this is a dubious behavior to rely on, since arguably 'git\nrev-list' could continue its object walk outside of which commits are\ncovered by bitmaps.\n\nThis will be used to test the behavior of 'pack.preferBitmapTips', which\nwill be added in the following patch.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Makefile               |  1 +\n t/helper/test-bitmap.c | 24 ++++++++++++++++++++++++\n t/helper/test-tool.c   |  1 +\n t/helper/test-tool.h   |  1 +\n 4 files changed, 27 insertions(+)\n create mode 100644 t/helper/test-bitmap.c\n\ndiff --git a/Makefile b/Makefile\nindex 55c8035fa8..89b30495eb 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -693,6 +693,7 @@ X =\n PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))\n \n TEST_BUILTINS_OBJS += test-advise.o\n+TEST_BUILTINS_OBJS += test-bitmap.o\n TEST_BUILTINS_OBJS += test-bloom.o\n TEST_BUILTINS_OBJS += test-chmtime.o\n TEST_BUILTINS_OBJS += test-config.o\ndiff --git a/t/helper/test-bitmap.c b/t/helper/test-bitmap.c\nnew file mode 100644\nindex 0000000000..134a1e9d76\n--- /dev/null\n+++ b/t/helper/test-bitmap.c\n@@ -0,0 +1,24 @@\n+#include \"test-tool.h\"\n+#include \"cache.h\"\n+#include \"pack-bitmap.h\"\n+\n+static int bitmap_list_commits(void)\n+{\n+\treturn test_bitmap_commits(the_repository);\n+}\n+\n+int cmd__bitmap(int argc, const char **argv)\n+{\n+\tsetup_git_directory();\n+\n+\tif (argc != 2)\n+\t\tgoto usage;\n+\n+\tif (!strcmp(argv[1], \"list-commits\"))\n+\t\treturn bitmap_list_commits();\n+\n+usage:\n+\tusage(\"\\ttest-tool bitmap list-commits\");\n+\n+\treturn -1;\n+}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex f97cd9f48a..a48bd44116 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -15,6 +15,7 @@ struct test_cmd {\n \n static struct test_cmd cmds[] = {\n \t{ \"advise\", cmd__advise_if_enabled },\n+\t{ \"bitmap\", cmd__bitmap },\n \t{ \"bloom\", cmd__bloom },\n \t{ \"chmtime\", cmd__chmtime },\n \t{ \"config\", cmd__config },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex 28072c0ad5..563fe1b030 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -5,6 +5,7 @@\n #include \"git-compat-util.h\"\n \n int cmd__advise_if_enabled(int argc, const char **argv);\n+int cmd__bitmap(int argc, const char **argv);\n int cmd__bloom(int argc, const char **argv);\n int cmd__chmtime(int argc, const char **argv);\n int cmd__config(int argc, const char **argv);\n-- \n2.31.1.163.ga65ce7f831\n\n"},{"id":"420767","messageId":"5e10199dae5e342c0482497b038b3299a8ed046b.1617240723.git.me@ttaylorr.com","threadId":"55419","inReplyTo":"cover.1617240723.git.me@ttaylorr.com","subject":"[PATCH 3/3] builtin/pack-objects.c: respect 'pack.preferBitmapTips'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-04-01T01:32:14Z","receivedAt":"2021-04-01T01:33:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When writing a new pack with a bitmap, it is sometimes convenient to\nindicate some reference prefixes which should receive priority when\nselecting which commits to receive bitmaps.\n\nA truly motivated caller could accomplish this by setting\n'pack.islandCore', (since all commits in the core island are similarly\nmarked as preferred) but this requires callers to opt into using delta\nislands, which they may or may not want to do.\n\nIntroduce a new multi-valued configuration, 'pack.preferBitmapTips' to\nallow callers to specify a list of reference prefixes. All references\nwhich have a prefix contained in 'pack.preferBitmapTips' will mark their\ntips as \"preferred\" in the same way as commits are marked as preferred\nfor selection by 'pack.islandCore'.\n\nThe choice of the verb \"prefer\" is intentional: marking the NEEDS_BITMAP\nflag on an object does *not* guarantee that that object will receive a\nbitmap. It merely guarantees that that commit will receive a bitmap over\nany *other* commit in the same window by bitmap_writer_select_commits().\n\nThe test this patch adds reflects this quirk, too. It only tests that\na commit (which didn't receive bitmaps by default) is selected for\nbitmaps after changing the value of 'pack.preferBitmapTips' to include\nit. Other commits may lose their bitmaps as a byproduct of how the\nselection process works (bitmap_writer_select_commits() ignores the\nremainder of a window after seeing a commit with the NEEDS_BITMAP flag).\n\nThis configuration will aide in selecting important references for\nmulti-pack bitmaps, since they do not respect the same pack.islandCore\nconfiguration. (They could, but doing so may be confusing, since it is\npacks--not bitmaps--which are influenced by the delta-islands\nconfiguration).\n\nIn a fork network repository (one which lists all forks of a given\nrepository as remotes), for example, it is useful to set\npack.preferBitmapTips to 'refs/remotes/<root>/heads' and\n'refs/remotes/<root>/tags', where '<root>' is an opaque identifier\nreferring to the repository which is at the base of the fork chain.\n\nSuggested-by: Jeff King <peff@peff.net>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/config/pack.txt | 15 ++++++++++++++\n builtin/pack-objects.c        | 34 +++++++++++++++++++++++++++++++\n pack-bitmap.c                 |  6 ++++++\n pack-bitmap.h                 |  3 +++\n t/t5310-pack-bitmaps.sh       | 38 +++++++++++++++++++++++++++++++++++\n 5 files changed, 96 insertions(+)\n\ndiff --git a/Documentation/config/pack.txt b/Documentation/config/pack.txt\nindex 3da4ea98e2..c0844d8d8e 100644\n--- a/Documentation/config/pack.txt\n+++ b/Documentation/config/pack.txt\n@@ -122,6 +122,21 @@ pack.useSparse::\n \tcommits contain certain types of direct renames. Default is\n \t`true`.\n \n+pack.preferBitmapTips::\n+\tWhen selecting which commits will receive bitmaps, prefer a\n+\tcommit at the tip of any reference that is a suffix of any value\n+\tof this configuration over any other commits in the \"selection\n+\twindow\".\n++\n+Note that setting this configuration to `refs/foo` does not mean that\n+the commits at the tips of `refs/foo/bar` and `refs/foo/baz` will\n+necessarily be selected. This is because commits are selected for\n+bitmaps from within a series of windows of variable length.\n++\n+If a commit at the tip of any reference which is a suffix of any value\n+of this configuration is seen in a window, it is immediately given\n+preference over any other commit in that window.\n+\n pack.writeBitmaps (deprecated)::\n \tThis is a deprecated synonym for `repack.writeBitmaps`.\n \ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 525c2d8552..a1e33d7507 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3547,6 +3547,37 @@ static void record_recent_commit(struct commit *commit, void *data)\n \toid_array_append(&recent_objects, &commit->object.oid);\n }\n \n+static int mark_bitmap_preferred_tip(const char *refname,\n+\t\t\t\t     const struct object_id *oid, int flags,\n+\t\t\t\t     void *_data)\n+{\n+\tstruct object_id peeled;\n+\tstruct object *object;\n+\n+\tif (!peel_iterated_oid(oid, &peeled))\n+\t\toid = &peeled;\n+\n+\tobject = parse_object_or_die(oid, refname);\n+\tif (object->type == OBJ_COMMIT)\n+\t\tobject->flags |= NEEDS_BITMAP;\n+\n+\treturn 0;\n+}\n+\n+static void mark_bitmap_preferred_tips(void)\n+{\n+\tstruct string_list_item *item;\n+\tconst struct string_list *preferred_tips;\n+\n+\tpreferred_tips = bitmap_preferred_tips(the_repository);\n+\tif (!preferred_tips)\n+\t\treturn;\n+\n+\tfor_each_string_list_item(item, preferred_tips) {\n+\t\tfor_each_ref_in(item->string, mark_bitmap_preferred_tip, NULL);\n+\t}\n+}\n+\n static void get_object_list(int ac, const char **av)\n {\n \tstruct rev_info revs;\n@@ -3601,6 +3632,9 @@ static void get_object_list(int ac, const char **av)\n \tif (use_delta_islands)\n \t\tload_delta_islands(the_repository, progress);\n \n+\tif (write_bitmap_index)\n+\t\tmark_bitmap_preferred_tips();\n+\n \tif (prepare_revision_walk(&revs))\n \t\tdie(_(\"revision walk setup failed\"));\n \tmark_edges_uninteresting(&revs, show_edge, sparse);\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 7554510b14..bfe2943a9b 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -13,6 +13,7 @@\n #include \"repository.h\"\n #include \"object-store.h\"\n #include \"list-objects-filter-options.h\"\n+#include \"config.h\"\n \n /*\n  * An entry on the bitmap index, representing the bitmap for a given\n@@ -1529,3 +1530,8 @@ off_t get_disk_usage_from_bitmap(struct bitmap_index *bitmap_git,\n \n \treturn total;\n }\n+\n+const struct string_list *bitmap_preferred_tips(struct repository *r)\n+{\n+\treturn repo_config_get_value_multi(r, \"pack.preferbitmaptips\");\n+}\ndiff --git a/pack-bitmap.h b/pack-bitmap.h\nindex c3cdd80756..78f2b3ff79 100644\n--- a/pack-bitmap.h\n+++ b/pack-bitmap.h\n@@ -5,6 +5,7 @@\n #include \"khash.h\"\n #include \"pack.h\"\n #include \"pack-objects.h\"\n+#include \"string-list.h\"\n \n struct commit;\n struct repository;\n@@ -91,4 +92,6 @@ void bitmap_writer_finish(struct pack_idx_entry **index,\n \t\t\t  const char *filename,\n \t\t\t  uint16_t options);\n \n+const struct string_list *bitmap_preferred_tips(struct repository *r);\n+\n #endif\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex 40b9f63244..f53efc8229 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -554,4 +554,42 @@ test_expect_success 'fetch with bitmaps can reuse old base' '\n \t)\n '\n \n+test_expect_success 'pack.preferBitmapTips' '\n+\tgit init repo &&\n+\ttest_when_finished \"rm -fr repo\" &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\t# create enough commits that not all are receive bitmap\n+\t\t# coverage even if they are all at the tip of some reference.\n+\t\ttest_commit_bulk --message=\"%s\" 103 &&\n+\n+\t\tgit rev-list HEAD >commits.raw &&\n+\t\tsort <commits.raw >commits &&\n+\n+\t\tgit log --format=\"create refs/tags/%s %H\" HEAD >refs &&\n+\t\tgit update-ref --stdin <refs &&\n+\n+\t\tgit repack -adb &&\n+\t\ttest-tool bitmap list-commits | sort >bitmaps &&\n+\n+\t\t# remember which commits did not receive bitmaps\n+\t\tcomm -13 bitmaps commits >before &&\n+\t\ttest_file_not_empty before &&\n+\n+\t\t# mark the commits which did not receive bitmaps as preferred,\n+\t\t# and generate the bitmap again\n+\t\tperl -pe \"s{^}{create refs/tags/include/$. }\" <before |\n+\t\t\tgit update-ref --stdin &&\n+\t\tgit -c pack.preferBitmapTips=refs/tags/include repack -adb &&\n+\n+\t\t# finally, check that the commit(s) without bitmap coverage\n+\t\t# are not the same ones as before\n+\t\ttest-tool bitmap list-commits | sort >bitmaps &&\n+\t\tcomm -13 bitmaps commits >after &&\n+\n+\t\t! test_cmp before after\n+\t)\n+'\n+\n test_done\n-- \n2.31.1.163.ga65ce7f831\n"},{"id":"420806","messageId":"924b5815-f7c6-7451-92e5-19bb936c54b1@gmail.com","threadId":"55419","inReplyTo":"5e10199dae5e342c0482497b038b3299a8ed046b.1617240723.git.me@ttaylorr.com","subject":"Re: [PATCH 3/3] builtin/pack-objects.c: respect 'pack.preferBitmapTips'","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-04-01T13:05:55Z","receivedAt":"2021-04-01T17:43:24Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/31/2021 9:32 PM, Taylor Blau wrote:\n> When writing a new pack with a bitmap, it is sometimes convenient to\n> indicate some reference prefixes which should receive priority when\n> selecting which commits to receive bitmaps.\n> \n> A truly motivated caller could accomplish this by setting\n> 'pack.islandCore', (since all commits in the core island are similarly\n> marked as preferred) but this requires callers to opt into using delta\n> islands, which they may or may not want to do.\n> \n> Introduce a new multi-valued configuration, 'pack.preferBitmapTips' to\n> allow callers to specify a list of reference prefixes. All references\n> which have a prefix contained in 'pack.preferBitmapTips' will mark their\n> tips as \"preferred\" in the same way as commits are marked as preferred\n> for selection by 'pack.islandCore'.\n\nI almost hit send on an email saying \"this doesn't seem to work with\nprefixes\" but I have discovered the subtle reason why. I'll point it\nout below.\n\n> +static int mark_bitmap_preferred_tip(const char *refname,\n> +\t\t\t\t     const struct object_id *oid, int flags,\n> +\t\t\t\t     void *_data)\n> +{\n> +\tstruct object_id peeled;\n> +\tstruct object *object;\n> +\n> +\tif (!peel_iterated_oid(oid, &peeled))\n> +\t\toid = &peeled;\n> +\n> +\tobject = parse_object_or_die(oid, refname);\n> +\tif (object->type == OBJ_COMMIT)\n> +\t\tobject->flags |= NEEDS_BITMAP;\n> +\n> +\treturn 0;\n> +}\n\nThis takes a refname and peels it (in case of a tag) but doesn't\ndo any prefix matching.\n\n> +static void mark_bitmap_preferred_tips(void)\n> +{\n> +\tstruct string_list_item *item;\n> +\tconst struct string_list *preferred_tips;\n> +\n> +\tpreferred_tips = bitmap_preferred_tips(the_repository);\n> +\tif (!preferred_tips)\n> +\t\treturn;\n> +\n> +\tfor_each_string_list_item(item, preferred_tips) {\n> +\t\tfor_each_ref_in(item->string, mark_bitmap_preferred_tip, NULL);\n> +\t}\n> +}\n\nThis iterates on the list returned by bitmap_preferred_tips(), but\nspecifically for_each_ref_in() does the prefix matching for us. I\nmissed that point on my first read.\n\n> +const struct string_list *bitmap_preferred_tips(struct repository *r)\n> +{\n> +\treturn repo_config_get_value_multi(r, \"pack.preferbitmaptips\");\n> +}\n\nAnd this just returns the config values, which are fed into\nfor_each_ref_in().\n\n> +test_expect_success 'pack.preferBitmapTips' '\n> +\tgit init repo &&\n> +\ttest_when_finished \"rm -fr repo\" &&\n> +\t(\n> +\t\tcd repo &&\n> +\n> +\t\t# create enough commits that not all are receive bitmap\n> +\t\t# coverage even if they are all at the tip of some reference.\n> +\t\ttest_commit_bulk --message=\"%s\" 103 &&\n> +\n> +\t\tgit rev-list HEAD >commits.raw &&\n> +\t\tsort <commits.raw >commits &&\n> +\n> +\t\tgit log --format=\"create refs/tags/%s %H\" HEAD >refs &&\n> +\t\tgit update-ref --stdin <refs &&\n> +\n> +\t\tgit repack -adb &&\n> +\t\ttest-tool bitmap list-commits | sort >bitmaps &&\n> +\n> +\t\t# remember which commits did not receive bitmaps\n> +\t\tcomm -13 bitmaps commits >before &&\n> +\t\ttest_file_not_empty before &&\n> +\n> +\t\t# mark the commits which did not receive bitmaps as preferred,\n> +\t\t# and generate the bitmap again\n> +\t\tperl -pe \"s{^}{create refs/tags/include/$. }\" <before |\n> +\t\t\tgit update-ref --stdin &&\n> +\t\tgit -c pack.preferBitmapTips=refs/tags/include repack -adb &&\n\nAnd this test is a clever way of creating a new ref category to\ncheck the prefix matching. Since you created refs pointing to\nevery commit earlier, this verifies that the config is the reason\nthey are picked now. Good.\n\nSorry for all of my thinking out loud. These are good patches and\nI have no recommended changes.\n\nThanks,\n-Stolee\n"},{"id":"425579","messageId":"87h7ipmjkw.fsf@evledraar.gmail.com","threadId":"55419","inReplyTo":"ec974bb0c85bcde00d45e983d701c538488550a6.1617240723.git.me@ttaylorr.com","subject":"SunCC doesn't compile v2.32.0-rc* anymore (was \"Re: [PATCH 2/3] t/helper/test-bitmap.c: initial commit\")","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-26T18:30:49Z","receivedAt":"2021-05-26T18:33:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Mar 31 2021, Taylor Blau wrote:\n\n> Add a new 'bitmap' test-tool which can be used to list the commits that\n> have received bitmaps.\n> [...]\n> @@ -0,0 +1,24 @@\n> +#include \"test-tool.h\"\n> +#include \"cache.h\"\n> +#include \"pack-bitmap.h\"\n\nSince this commit SunCC (on Solaris) refuses to compile git, a new bug\nin v2.32.0-rc*.\n\nIt's because it can't find the oe_get_size_slow symbol, you include\npack-bitmap.h, which in turn includes pack-objects.h. That file\nreferences oe_get_size_slow, but it's only defined in\nbuiltin/pack-objects.c.\n\nThat looseness of definitions far pre-dates v2.32.0, but I suspect we\ngot away with it due to builtins including everything (or something)\nanyway, and that this is the first test-tool usage.\n"},{"id":"425582","messageId":"YK6W+q1BhC1ZiB6H@nand.local","threadId":"55419","inReplyTo":"87h7ipmjkw.fsf@evledraar.gmail.com","subject":"Re: SunCC doesn't compile v2.32.0-rc* anymore (was \"Re: [PATCH 2/3] t/helper/test-bitmap.c: initial commit\")","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-05-26T18:44:10Z","receivedAt":"2021-05-26T18:44:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, May 26, 2021 at 08:30:49PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n> On Wed, Mar 31 2021, Taylor Blau wrote:\n>\n> > Add a new 'bitmap' test-tool which can be used to list the commits that\n> > have received bitmaps.\n> > [...]\n> > @@ -0,0 +1,24 @@\n> > +#include \"test-tool.h\"\n> > +#include \"cache.h\"\n> > +#include \"pack-bitmap.h\"\n>\n> Since this commit SunCC (on Solaris) refuses to compile git, a new bug\n> in v2.32.0-rc*.\n>\n> It's because it can't find the oe_get_size_slow symbol, you include\n> pack-bitmap.h, which in turn includes pack-objects.h. That file\n> references oe_get_size_slow, but it's only defined in\n> builtin/pack-objects.c.\n\nI'm not sure I understand. pack-objects.h has a declaration of that\nmethod, but the implementation is in builtin/pack-objects.c. That should\nbe fine, but I don't know about how SunCC works.\n\nWhat needs to be changed here?\n\nThanks,\nTaylor\n"},{"id":"425594","messageId":"878s41m5nc.fsf@evledraar.gmail.com","threadId":"55419","inReplyTo":"YK6W+q1BhC1ZiB6H@nand.local","subject":"Re: SunCC doesn't compile v2.32.0-rc* anymore (was \"Re: [PATCH 2/3] t/helper/test-bitmap.c: initial commit\")","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-26T21:10:23Z","receivedAt":"2021-05-26T23:34:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, May 26 2021, Taylor Blau wrote:\n\n> On Wed, May 26, 2021 at 08:30:49PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>>\n>> On Wed, Mar 31 2021, Taylor Blau wrote:\n>>\n>> > Add a new 'bitmap' test-tool which can be used to list the commits that\n>> > have received bitmaps.\n>> > [...]\n>> > @@ -0,0 +1,24 @@\n>> > +#include \"test-tool.h\"\n>> > +#include \"cache.h\"\n>> > +#include \"pack-bitmap.h\"\n>>\n>> Since this commit SunCC (on Solaris) refuses to compile git, a new bug\n>> in v2.32.0-rc*.\n>>\n>> It's because it can't find the oe_get_size_slow symbol, you include\n>> pack-bitmap.h, which in turn includes pack-objects.h. That file\n>> references oe_get_size_slow, but it's only defined in\n>> builtin/pack-objects.c.\n>\n> I'm not sure I understand. pack-objects.h has a declaration of that\n> method, but the implementation is in builtin/pack-objects.c. That should\n> be fine, but I don't know about how SunCC works.\n>\n> What needs to be changed here?\n\nI think that oe_get_size_slow needs to be in libgit as long as\npack-objects.h defines other (inline) functions that reference it, or\nmaybe most of that stuff can just be moved to builtin/pack-objects.h?\n\nThis obviously not OK patch makes things \"ok\" again under SunCC:\n    \n    diff --git a/Makefile b/Makefile\n    index c3565fc0f8f..c2b05aeddac 100644\n    --- a/Makefile\n    +++ b/Makefile\n    @@ -1186,7 +1186,7 @@ THIRD_PARTY_SOURCES += compat/regex/%\n     THIRD_PARTY_SOURCES += sha1collisiondetection/%\n     THIRD_PARTY_SOURCES += sha1dc/%\n    \n    -GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB)\n    +GITLIBS = builtin/pack-objects.o common-main.o $(LIB_FILE) $(XDIFF_LIB)\n     EXTLIBS =\n    \n     GIT_USER_AGENT = git/$(GIT_VERSION)\n\nI.e. the problem is that in the final program we're linking there's\nreferences to this function we can't find:\n    \n    Undefined                       first referenced\n     symbol                             in file\n    oe_get_size_slow                    t/helper/test-bitmap.o\n    ld: fatal: symbol referencing errors. No output written to t/helper/test-tool\n\nI think e.g. the GNU toolchain and others are probably OK with it\nbecause they do some analysis to figure out that none of those inline\nfunctions that need oe_get_size_slow are themselves needed. SunCC (or\nSolaris's) linker seems to be more eager.\n\nIt seems to me that the best way to solve this is something like the\nbelow code-move-only patch of just moving this stuff only used by\nbuiltin/pack-objects.c to that file. This also fixes the build on\nSolaris.\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 6ded130e45b..70072b07b6b 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -37,6 +37,100 @@\n #include \"shallow.h\"\n #include \"promisor-remote.h\"\n \n+/*\n+ * Objects we are going to pack are collected in the `to_pack` structure.\n+ * It contains an array (dynamically expanded) of the object data, and a map\n+ * that can resolve SHA1s to their position in the array.\n+ */\n+static struct packing_data to_pack;\n+\n+/*\n+ * Return the size of the object without doing any delta\n+ * reconstruction (so non-deltas are true object sizes, but deltas\n+ * return the size of the delta data).\n+ */\n+unsigned long oe_get_size_slow(struct packing_data *pack,\n+\t\t\t       const struct object_entry *e);\n+unsigned long oe_get_size_slow(struct packing_data *pack,\n+\t\t\t       const struct object_entry *e)\n+{\n+\tstruct packed_git *p;\n+\tstruct pack_window *w_curs;\n+\tunsigned char *buf;\n+\tenum object_type type;\n+\tunsigned long used, avail, size;\n+\n+\tif (e->type_ != OBJ_OFS_DELTA && e->type_ != OBJ_REF_DELTA) {\n+\t\tpacking_data_lock(&to_pack);\n+\t\tif (oid_object_info(the_repository, &e->idx.oid, &size) < 0)\n+\t\t\tdie(_(\"unable to get size of %s\"),\n+\t\t\t    oid_to_hex(&e->idx.oid));\n+\t\tpacking_data_unlock(&to_pack);\n+\t\treturn size;\n+\t}\n+\n+\tp = oe_in_pack(pack, e);\n+\tif (!p)\n+\t\tBUG(\"when e->type is a delta, it must belong to a pack\");\n+\n+\tpacking_data_lock(&to_pack);\n+\tw_curs = NULL;\n+\tbuf = use_pack(p, &w_curs, e->in_pack_offset, &avail);\n+\tused = unpack_object_header_buffer(buf, avail, &type, &size);\n+\tif (used == 0)\n+\t\tdie(_(\"unable to parse object header of %s\"),\n+\t\t    oid_to_hex(&e->idx.oid));\n+\n+\tunuse_pack(&w_curs);\n+\tpacking_data_unlock(&to_pack);\n+\treturn size;\n+}\n+\n+static inline unsigned long oe_size(struct packing_data *pack,\n+\t\t\t\t    const struct object_entry *e)\n+{\n+\tif (e->size_valid)\n+\t\treturn e->size_;\n+\n+\treturn oe_get_size_slow(pack, e);\n+}\n+\n+static inline int oe_size_less_than(struct packing_data *pack,\n+\t\t\t\t    const struct object_entry *lhs,\n+\t\t\t\t    unsigned long rhs)\n+{\n+\tif (lhs->size_valid)\n+\t\treturn lhs->size_ < rhs;\n+\tif (rhs < pack->oe_size_limit) /* rhs < 2^x <= lhs ? */\n+\t\treturn 0;\n+\treturn oe_get_size_slow(pack, lhs) < rhs;\n+}\n+\n+static inline int oe_size_greater_than(struct packing_data *pack,\n+\t\t\t\t       const struct object_entry *lhs,\n+\t\t\t\t       unsigned long rhs)\n+{\n+\tif (lhs->size_valid)\n+\t\treturn lhs->size_ > rhs;\n+\tif (rhs < pack->oe_size_limit) /* rhs < 2^x <= lhs ? */\n+\t\treturn 1;\n+\treturn oe_get_size_slow(pack, lhs) > rhs;\n+}\n+\n+static inline void oe_set_size(struct packing_data *pack,\n+\t\t\t       struct object_entry *e,\n+\t\t\t       unsigned long size)\n+{\n+\tif (size < pack->oe_size_limit) {\n+\t\te->size_ = size;\n+\t\te->size_valid = 1;\n+\t} else {\n+\t\te->size_valid = 0;\n+\t\tif (oe_get_size_slow(pack, e) != size)\n+\t\t\tBUG(\"'size' is supposed to be the object size!\");\n+\t}\n+}\n+\n #define IN_PACK(obj) oe_in_pack(&to_pack, obj)\n #define SIZE(obj) oe_size(&to_pack, obj)\n #define SET_SIZE(obj,size) oe_set_size(&to_pack, obj, size)\n@@ -56,13 +150,6 @@ static const char *pack_usage[] = {\n \tNULL\n };\n \n-/*\n- * Objects we are going to pack are collected in the `to_pack` structure.\n- * It contains an array (dynamically expanded) of the object data, and a map\n- * that can resolve SHA1s to their position in the array.\n- */\n-static struct packing_data to_pack;\n-\n static struct pack_idx_entry **written_list;\n static uint32_t nr_result, nr_written, nr_seen;\n static struct bitmap_index *bitmap_git;\n@@ -2231,46 +2318,6 @@ static pthread_mutex_t progress_mutex;\n  * progress_mutex for protection.\n  */\n \n-/*\n- * Return the size of the object without doing any delta\n- * reconstruction (so non-deltas are true object sizes, but deltas\n- * return the size of the delta data).\n- */\n-unsigned long oe_get_size_slow(struct packing_data *pack,\n-\t\t\t       const struct object_entry *e)\n-{\n-\tstruct packed_git *p;\n-\tstruct pack_window *w_curs;\n-\tunsigned char *buf;\n-\tenum object_type type;\n-\tunsigned long used, avail, size;\n-\n-\tif (e->type_ != OBJ_OFS_DELTA && e->type_ != OBJ_REF_DELTA) {\n-\t\tpacking_data_lock(&to_pack);\n-\t\tif (oid_object_info(the_repository, &e->idx.oid, &size) < 0)\n-\t\t\tdie(_(\"unable to get size of %s\"),\n-\t\t\t    oid_to_hex(&e->idx.oid));\n-\t\tpacking_data_unlock(&to_pack);\n-\t\treturn size;\n-\t}\n-\n-\tp = oe_in_pack(pack, e);\n-\tif (!p)\n-\t\tBUG(\"when e->type is a delta, it must belong to a pack\");\n-\n-\tpacking_data_lock(&to_pack);\n-\tw_curs = NULL;\n-\tbuf = use_pack(p, &w_curs, e->in_pack_offset, &avail);\n-\tused = unpack_object_header_buffer(buf, avail, &type, &size);\n-\tif (used == 0)\n-\t\tdie(_(\"unable to parse object header of %s\"),\n-\t\t    oid_to_hex(&e->idx.oid));\n-\n-\tunuse_pack(&w_curs);\n-\tpacking_data_unlock(&to_pack);\n-\treturn size;\n-}\n-\n static int try_delta(struct unpacked *trg, struct unpacked *src,\n \t\t     unsigned max_depth, unsigned long *mem_usage)\n {\ndiff --git a/pack-objects.h b/pack-objects.h\nindex 9d88e3e518f..0c4d5d475f6 100644\n--- a/pack-objects.h\n+++ b/pack-objects.h\n@@ -332,52 +332,6 @@ static inline void oe_set_delta_sibling(struct packing_data *pack,\n \t\te->delta_sibling_idx = 0;\n }\n \n-unsigned long oe_get_size_slow(struct packing_data *pack,\n-\t\t\t       const struct object_entry *e);\n-static inline unsigned long oe_size(struct packing_data *pack,\n-\t\t\t\t    const struct object_entry *e)\n-{\n-\tif (e->size_valid)\n-\t\treturn e->size_;\n-\n-\treturn oe_get_size_slow(pack, e);\n-}\n-\n-static inline int oe_size_less_than(struct packing_data *pack,\n-\t\t\t\t    const struct object_entry *lhs,\n-\t\t\t\t    unsigned long rhs)\n-{\n-\tif (lhs->size_valid)\n-\t\treturn lhs->size_ < rhs;\n-\tif (rhs < pack->oe_size_limit) /* rhs < 2^x <= lhs ? */\n-\t\treturn 0;\n-\treturn oe_get_size_slow(pack, lhs) < rhs;\n-}\n-\n-static inline int oe_size_greater_than(struct packing_data *pack,\n-\t\t\t\t       const struct object_entry *lhs,\n-\t\t\t\t       unsigned long rhs)\n-{\n-\tif (lhs->size_valid)\n-\t\treturn lhs->size_ > rhs;\n-\tif (rhs < pack->oe_size_limit) /* rhs < 2^x <= lhs ? */\n-\t\treturn 1;\n-\treturn oe_get_size_slow(pack, lhs) > rhs;\n-}\n-\n-static inline void oe_set_size(struct packing_data *pack,\n-\t\t\t       struct object_entry *e,\n-\t\t\t       unsigned long size)\n-{\n-\tif (size < pack->oe_size_limit) {\n-\t\te->size_ = size;\n-\t\te->size_valid = 1;\n-\t} else {\n-\t\te->size_valid = 0;\n-\t\tif (oe_get_size_slow(pack, e) != size)\n-\t\t\tBUG(\"'size' is supposed to be the object size!\");\n-\t}\n-}\n \n static inline unsigned long oe_delta_size(struct packing_data *pack,\n \t\t\t\t\t  const struct object_entry *e)\n\n"},{"id":"425643","messageId":"patch-1.1-d085d59a417-20210527T005213Z-avarab@gmail.com","threadId":"55419","inReplyTo":"878s41m5nc.fsf@evledraar.gmail.com","subject":"[PATCH] pack-objects: move builtin-only code to its own header","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-27T00:52:51Z","receivedAt":"2021-05-27T00:58:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move the code that's only used in builtin/pack-objects.c to a new\nbuiltin/pack-objects.h header and out of pack-objects.h.\n\nThis fixes an issue where Solaris's SunCC hasn't been able to compile\ngit since 483fa7f42d9 (t/helper/test-bitmap.c: initial commit,\n2021-03-31).\n\nThe real origin of that issue is that in 898eba5e630 (pack-objects:\nrefer to delta objects by index instead of pointer, 2018-04-14)\nutility functions only needed by builtin/pack-objects.c were added to\npack-objects.h. Since then the header has been used in a few other\nplaces, but 483fa7f42d9 was the first time it was used by test helper.\n\nSince Solaris is stricter about linking and the oe_get_size_slow()\nfunction lives in builtin/pack-objects.c the build started failing\nwith:\n\n    Undefined                       first referenced\n     symbol                             in file\n    oe_get_size_slow                    t/helper/test-bitmap.o\n    ld: fatal: symbol referencing errors. No output written to t/helper/test-tool\n\nOn other platforms this is presumably OK because the compiler and/or\nlinker detects that the \"static inline\" functions that reference\noe_get_size_slow() aren't used.\n\nLet's solve this by moving the relevant code to\nbuiltin/pack-objects.c. This is almost entirely a code-only move, but\nbecause of the early macro definitions in that file referencing some\nof these inline functions we need to move the definition of \"static\nstruct packing_data to_pack\" earlier, and declare these inline\nfunctions above the macros.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/pack-objects.c | 174 +++++++++++++++++++++++++++++++++++++++--\n pack-objects.h         | 159 -------------------------------------\n 2 files changed, 167 insertions(+), 166 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 6ded130e45b..de00adbb9e0 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -37,6 +37,134 @@\n #include \"shallow.h\"\n #include \"promisor-remote.h\"\n \n+/*\n+ * Objects we are going to pack are collected in the `to_pack` structure.\n+ * It contains an array (dynamically expanded) of the object data, and a map\n+ * that can resolve SHA1s to their position in the array.\n+ */\n+static struct packing_data to_pack;\n+\n+static inline struct object_entry *oe_delta(\n+\t\tconst struct packing_data *pack,\n+\t\tconst struct object_entry *e)\n+{\n+\tif (!e->delta_idx)\n+\t\treturn NULL;\n+\tif (e->ext_base)\n+\t\treturn &pack->ext_bases[e->delta_idx - 1];\n+\telse\n+\t\treturn &pack->objects[e->delta_idx - 1];\n+}\n+\n+static inline unsigned long oe_delta_size(struct packing_data *pack,\n+\t\t\t\t\t  const struct object_entry *e)\n+{\n+\tif (e->delta_size_valid)\n+\t\treturn e->delta_size_;\n+\n+\t/*\n+\t * pack->delta_size[] can't be NULL because oe_set_delta_size()\n+\t * must have been called when a new delta is saved with\n+\t * oe_set_delta().\n+\t * If oe_delta() returns NULL (i.e. default state, which means\n+\t * delta_size_valid is also false), then the caller must never\n+\t * call oe_delta_size().\n+\t */\n+\treturn pack->delta_size[e - pack->objects];\n+}\n+\n+unsigned long oe_get_size_slow(struct packing_data *pack,\n+\t\t\t       const struct object_entry *e);\n+\n+static inline unsigned long oe_size(struct packing_data *pack,\n+\t\t\t\t    const struct object_entry *e)\n+{\n+\tif (e->size_valid)\n+\t\treturn e->size_;\n+\n+\treturn oe_get_size_slow(pack, e);\n+}\n+\n+static inline void oe_set_delta(struct packing_data *pack,\n+\t\t\t\tstruct object_entry *e,\n+\t\t\t\tstruct object_entry *delta)\n+{\n+\tif (delta)\n+\t\te->delta_idx = (delta - pack->objects) + 1;\n+\telse\n+\t\te->delta_idx = 0;\n+}\n+\n+static inline struct object_entry *oe_delta_sibling(\n+\t\tconst struct packing_data *pack,\n+\t\tconst struct object_entry *e)\n+{\n+\tif (e->delta_sibling_idx)\n+\t\treturn &pack->objects[e->delta_sibling_idx - 1];\n+\treturn NULL;\n+}\n+\n+static inline struct object_entry *oe_delta_child(\n+\t\tconst struct packing_data *pack,\n+\t\tconst struct object_entry *e)\n+{\n+\tif (e->delta_child_idx)\n+\t\treturn &pack->objects[e->delta_child_idx - 1];\n+\treturn NULL;\n+}\n+\n+static inline void oe_set_delta_child(struct packing_data *pack,\n+\t\t\t\t      struct object_entry *e,\n+\t\t\t\t      struct object_entry *delta)\n+{\n+\tif (delta)\n+\t\te->delta_child_idx = (delta - pack->objects) + 1;\n+\telse\n+\t\te->delta_child_idx = 0;\n+}\n+\n+static inline void oe_set_delta_sibling(struct packing_data *pack,\n+\t\t\t\t\tstruct object_entry *e,\n+\t\t\t\t\tstruct object_entry *delta)\n+{\n+\tif (delta)\n+\t\te->delta_sibling_idx = (delta - pack->objects) + 1;\n+\telse\n+\t\te->delta_sibling_idx = 0;\n+}\n+\n+static inline void oe_set_size(struct packing_data *pack,\n+\t\t\t       struct object_entry *e,\n+\t\t\t       unsigned long size)\n+{\n+\tif (size < pack->oe_size_limit) {\n+\t\te->size_ = size;\n+\t\te->size_valid = 1;\n+\t} else {\n+\t\te->size_valid = 0;\n+\t\tif (oe_get_size_slow(pack, e) != size)\n+\t\t\tBUG(\"'size' is supposed to be the object size!\");\n+\t}\n+}\n+\n+static inline void oe_set_delta_size(struct packing_data *pack,\n+\t\t\t\t     struct object_entry *e,\n+\t\t\t\t     unsigned long size)\n+{\n+\tif (size < pack->oe_delta_size_limit) {\n+\t\te->delta_size_ = size;\n+\t\te->delta_size_valid = 1;\n+\t} else {\n+\t\tpacking_data_lock(pack);\n+\t\tif (!pack->delta_size)\n+\t\t\tALLOC_ARRAY(pack->delta_size, pack->nr_alloc);\n+\t\tpacking_data_unlock(pack);\n+\n+\t\tpack->delta_size[e - pack->objects] = size;\n+\t\te->delta_size_valid = 0;\n+\t}\n+}\n+\n #define IN_PACK(obj) oe_in_pack(&to_pack, obj)\n #define SIZE(obj) oe_size(&to_pack, obj)\n #define SET_SIZE(obj,size) oe_set_size(&to_pack, obj, size)\n@@ -56,13 +184,6 @@ static const char *pack_usage[] = {\n \tNULL\n };\n \n-/*\n- * Objects we are going to pack are collected in the `to_pack` structure.\n- * It contains an array (dynamically expanded) of the object data, and a map\n- * that can resolve SHA1s to their position in the array.\n- */\n-static struct packing_data to_pack;\n-\n static struct pack_idx_entry **written_list;\n static uint32_t nr_result, nr_written, nr_seen;\n static struct bitmap_index *bitmap_git;\n@@ -301,6 +422,17 @@ static void copy_pack_data(struct hashfile *f,\n \t}\n }\n \n+static inline int oe_size_greater_than(struct packing_data *pack,\n+\t\t\t\t       const struct object_entry *lhs,\n+\t\t\t\t       unsigned long rhs)\n+{\n+\tif (lhs->size_valid)\n+\t\treturn lhs->size_ > rhs;\n+\tif (rhs < pack->oe_size_limit) /* rhs < 2^x <= lhs ? */\n+\t\treturn 1;\n+\treturn oe_get_size_slow(pack, lhs) > rhs;\n+}\n+\n /* Return 0 if we will bust the pack-size limit */\n static unsigned long write_no_reuse_object(struct hashfile *f, struct object_entry *entry,\n \t\t\t\t\t   unsigned long limit, int usable_delta)\n@@ -642,6 +774,14 @@ static int mark_tagged(const char *path, const struct object_id *oid, int flag,\n \treturn 0;\n }\n \n+static inline unsigned char oe_layer(struct packing_data *pack,\n+\t\t\t\t     struct object_entry *e)\n+{\n+\tif (!pack->layer)\n+\t\treturn 0;\n+\treturn pack->layer[e - pack->objects];\n+}\n+\n static inline void add_to_write_order(struct object_entry **wo,\n \t\t\t       unsigned int *endp,\n \t\t\t       struct object_entry *e)\n@@ -2231,6 +2371,26 @@ static pthread_mutex_t progress_mutex;\n  * progress_mutex for protection.\n  */\n \n+static inline int oe_size_less_than(struct packing_data *pack,\n+\t\t\t\t    const struct object_entry *lhs,\n+\t\t\t\t    unsigned long rhs)\n+{\n+\tif (lhs->size_valid)\n+\t\treturn lhs->size_ < rhs;\n+\tif (rhs < pack->oe_size_limit) /* rhs < 2^x <= lhs ? */\n+\t\treturn 0;\n+\treturn oe_get_size_slow(pack, lhs) < rhs;\n+}\n+\n+static inline void oe_set_tree_depth(struct packing_data *pack,\n+\t\t\t\t     struct object_entry *e,\n+\t\t\t\t     unsigned int tree_depth)\n+{\n+\tif (!pack->tree_depth)\n+\t\tCALLOC_ARRAY(pack->tree_depth, pack->nr_alloc);\n+\tpack->tree_depth[e - pack->objects] = tree_depth;\n+}\n+\n /*\n  * Return the size of the object without doing any delta\n  * reconstruction (so non-deltas are true object sizes, but deltas\ndiff --git a/pack-objects.h b/pack-objects.h\nindex 9d88e3e518f..dca2351ef94 100644\n--- a/pack-objects.h\n+++ b/pack-objects.h\n@@ -268,152 +268,10 @@ static inline void oe_set_in_pack(struct packing_data *pack,\n \tpack->in_pack[e - pack->objects] = p;\n }\n \n-static inline struct object_entry *oe_delta(\n-\t\tconst struct packing_data *pack,\n-\t\tconst struct object_entry *e)\n-{\n-\tif (!e->delta_idx)\n-\t\treturn NULL;\n-\tif (e->ext_base)\n-\t\treturn &pack->ext_bases[e->delta_idx - 1];\n-\telse\n-\t\treturn &pack->objects[e->delta_idx - 1];\n-}\n-\n-static inline void oe_set_delta(struct packing_data *pack,\n-\t\t\t\tstruct object_entry *e,\n-\t\t\t\tstruct object_entry *delta)\n-{\n-\tif (delta)\n-\t\te->delta_idx = (delta - pack->objects) + 1;\n-\telse\n-\t\te->delta_idx = 0;\n-}\n-\n void oe_set_delta_ext(struct packing_data *pack,\n \t\t      struct object_entry *e,\n \t\t      const struct object_id *oid);\n \n-static inline struct object_entry *oe_delta_child(\n-\t\tconst struct packing_data *pack,\n-\t\tconst struct object_entry *e)\n-{\n-\tif (e->delta_child_idx)\n-\t\treturn &pack->objects[e->delta_child_idx - 1];\n-\treturn NULL;\n-}\n-\n-static inline void oe_set_delta_child(struct packing_data *pack,\n-\t\t\t\t      struct object_entry *e,\n-\t\t\t\t      struct object_entry *delta)\n-{\n-\tif (delta)\n-\t\te->delta_child_idx = (delta - pack->objects) + 1;\n-\telse\n-\t\te->delta_child_idx = 0;\n-}\n-\n-static inline struct object_entry *oe_delta_sibling(\n-\t\tconst struct packing_data *pack,\n-\t\tconst struct object_entry *e)\n-{\n-\tif (e->delta_sibling_idx)\n-\t\treturn &pack->objects[e->delta_sibling_idx - 1];\n-\treturn NULL;\n-}\n-\n-static inline void oe_set_delta_sibling(struct packing_data *pack,\n-\t\t\t\t\tstruct object_entry *e,\n-\t\t\t\t\tstruct object_entry *delta)\n-{\n-\tif (delta)\n-\t\te->delta_sibling_idx = (delta - pack->objects) + 1;\n-\telse\n-\t\te->delta_sibling_idx = 0;\n-}\n-\n-unsigned long oe_get_size_slow(struct packing_data *pack,\n-\t\t\t       const struct object_entry *e);\n-static inline unsigned long oe_size(struct packing_data *pack,\n-\t\t\t\t    const struct object_entry *e)\n-{\n-\tif (e->size_valid)\n-\t\treturn e->size_;\n-\n-\treturn oe_get_size_slow(pack, e);\n-}\n-\n-static inline int oe_size_less_than(struct packing_data *pack,\n-\t\t\t\t    const struct object_entry *lhs,\n-\t\t\t\t    unsigned long rhs)\n-{\n-\tif (lhs->size_valid)\n-\t\treturn lhs->size_ < rhs;\n-\tif (rhs < pack->oe_size_limit) /* rhs < 2^x <= lhs ? */\n-\t\treturn 0;\n-\treturn oe_get_size_slow(pack, lhs) < rhs;\n-}\n-\n-static inline int oe_size_greater_than(struct packing_data *pack,\n-\t\t\t\t       const struct object_entry *lhs,\n-\t\t\t\t       unsigned long rhs)\n-{\n-\tif (lhs->size_valid)\n-\t\treturn lhs->size_ > rhs;\n-\tif (rhs < pack->oe_size_limit) /* rhs < 2^x <= lhs ? */\n-\t\treturn 1;\n-\treturn oe_get_size_slow(pack, lhs) > rhs;\n-}\n-\n-static inline void oe_set_size(struct packing_data *pack,\n-\t\t\t       struct object_entry *e,\n-\t\t\t       unsigned long size)\n-{\n-\tif (size < pack->oe_size_limit) {\n-\t\te->size_ = size;\n-\t\te->size_valid = 1;\n-\t} else {\n-\t\te->size_valid = 0;\n-\t\tif (oe_get_size_slow(pack, e) != size)\n-\t\t\tBUG(\"'size' is supposed to be the object size!\");\n-\t}\n-}\n-\n-static inline unsigned long oe_delta_size(struct packing_data *pack,\n-\t\t\t\t\t  const struct object_entry *e)\n-{\n-\tif (e->delta_size_valid)\n-\t\treturn e->delta_size_;\n-\n-\t/*\n-\t * pack->delta_size[] can't be NULL because oe_set_delta_size()\n-\t * must have been called when a new delta is saved with\n-\t * oe_set_delta().\n-\t * If oe_delta() returns NULL (i.e. default state, which means\n-\t * delta_size_valid is also false), then the caller must never\n-\t * call oe_delta_size().\n-\t */\n-\treturn pack->delta_size[e - pack->objects];\n-}\n-\n-static inline void oe_set_delta_size(struct packing_data *pack,\n-\t\t\t\t     struct object_entry *e,\n-\t\t\t\t     unsigned long size)\n-{\n-\tif (size < pack->oe_delta_size_limit) {\n-\t\te->delta_size_ = size;\n-\t\te->delta_size_valid = 1;\n-\t} else {\n-\t\tpacking_data_lock(pack);\n-\t\tif (!pack->delta_size)\n-\t\t\tALLOC_ARRAY(pack->delta_size, pack->nr_alloc);\n-\t\tpacking_data_unlock(pack);\n-\n-\t\tpack->delta_size[e - pack->objects] = size;\n-\t\te->delta_size_valid = 0;\n-\t}\n-}\n-\n static inline unsigned int oe_tree_depth(struct packing_data *pack,\n \t\t\t\t\t struct object_entry *e)\n {\n@@ -422,23 +280,6 @@ static inline unsigned int oe_tree_depth(struct packing_data *pack,\n \treturn pack->tree_depth[e - pack->objects];\n }\n \n-static inline void oe_set_tree_depth(struct packing_data *pack,\n-\t\t\t\t     struct object_entry *e,\n-\t\t\t\t     unsigned int tree_depth)\n-{\n-\tif (!pack->tree_depth)\n-\t\tCALLOC_ARRAY(pack->tree_depth, pack->nr_alloc);\n-\tpack->tree_depth[e - pack->objects] = tree_depth;\n-}\n-\n-static inline unsigned char oe_layer(struct packing_data *pack,\n-\t\t\t\t     struct object_entry *e)\n-{\n-\tif (!pack->layer)\n-\t\treturn 0;\n-\treturn pack->layer[e - pack->objects];\n-}\n-\n static inline void oe_set_layer(struct packing_data *pack,\n \t\t\t\tstruct object_entry *e,\n \t\t\t\tunsigned char layer)\n-- \n2.32.0.rc1.413.g73297d74493\n\n"},{"id":"425645","messageId":"xmqqr1ht9d5n.fsf@gitster.g","threadId":"55419","inReplyTo":"patch-1.1-d085d59a417-20210527T005213Z-avarab@gmail.com","subject":"Re: [PATCH] pack-objects: move builtin-only code to its own header","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-27T01:30:12Z","receivedAt":"2021-05-27T01:30:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Move the code that's only used in builtin/pack-objects.c to a new\n> builtin/pack-objects.h header and out of pack-objects.h.\n>\n> This fixes an issue where Solaris's SunCC hasn't been able to compile\n> git since 483fa7f42d9 (t/helper/test-bitmap.c: initial commit,\n> 2021-03-31).\n>\n> The real origin of that issue is that in 898eba5e630 (pack-objects:\n> refer to delta objects by index instead of pointer, 2018-04-14)\n> utility functions only needed by builtin/pack-objects.c were added to\n> pack-objects.h. Since then the header has been used in a few other\n> places, but 483fa7f42d9 was the first time it was used by test helper.\n\nHmph.  Its good that removing these functions that should not be\ncalled by other people from *.h and move them to their only user\nregardless of the Solaris issue.  Sounds good.\n"},{"id":"425648","messageId":"xmqqeedsaoh2.fsf@gitster.g","threadId":"55419","inReplyTo":"patch-1.1-d085d59a417-20210527T005213Z-avarab@gmail.com","subject":"Re: [PATCH] pack-objects: move builtin-only code to its own header","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-27T02:40:25Z","receivedAt":"2021-05-27T02:40:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Move the code that's only used in builtin/pack-objects.c to a new\n> builtin/pack-objects.h header and out of pack-objects.h.\n> ...\n>  builtin/pack-objects.c | 174 +++++++++++++++++++++++++++++++++++++++--\n>  pack-objects.h         | 159 -------------------------------------\n>  2 files changed, 167 insertions(+), 166 deletions(-)\n\nNeither the title or the description reflects the reality, though.\n\nmove builtin-only \"static inline\" code from the header to its sole\nconsumer?\n"},{"id":"425650","messageId":"xmqqa6ogamwb.fsf@gitster.g","threadId":"55419","inReplyTo":"patch-1.1-d085d59a417-20210527T005213Z-avarab@gmail.com","subject":"Re: [PATCH] pack-objects: move builtin-only code to its own header","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-27T03:14:28Z","receivedAt":"2021-05-27T03:14:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Move the code that's only used in builtin/pack-objects.c to a new\n> builtin/pack-objects.h header and out of pack-objects.h.\n\nI've amended the early part of the proposed log message in\npreparation for merging it to 'next' and then later down to\n'master'.\n\nHere is what the result looks like.\n\nThanks.\n\n\nAuthor: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nDate:   Thu May 27 02:52:51 2021 +0200\n\n    pack-objects: move static inline from a header to its sole consumer\n    \n    Move the code that is only used in builtin/pack-objects.c out of\n    pack-objects.h.\n    \n    This fixes an issue where Solaris's SunCC hasn't been able to compile\n    git since 483fa7f42d9 (t/helper/test-bitmap.c: initial commit,\n    2021-03-31).\n    \n    The real origin of that issue is that in 898eba5e630 (pack-objects:\n    refer to delta objects by index instead of pointer, 2018-04-14)\n    utility functions only needed by builtin/pack-objects.c were added to\n    pack-objects.h. Since then the header has been used in a few other\n    places, but 483fa7f42d9 was the first time it was used by test helper.\n    \n    Since Solaris is stricter about linking and the oe_get_size_slow()\n    function lives in builtin/pack-objects.c the build started failing\n    with:\n    \n        Undefined                       first referenced\n         symbol                             in file\n        oe_get_size_slow                    t/helper/test-bitmap.o\n        ld: fatal: symbol referencing errors. No output written to t/helper/test-tool\n    \n    On other platforms this is presumably OK because the compiler and/or\n    linker detects that the \"static inline\" functions that reference\n    oe_get_size_slow() aren't used.\n    \n    Let's solve this by moving the relevant code from pack-objects.h to\n    builtin/pack-objects.c. This is almost entirely a code-only move, but\n    because of the early macro definitions in that file referencing some\n    of these inline functions we need to move the definition of \"static\n    struct packing_data to_pack\" earlier, and declare these inline\n    functions above the macros.\n    \n    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n"},{"id":"425672","messageId":"87o8cwl4p4.fsf@evledraar.gmail.com","threadId":"55419","inReplyTo":"xmqqa6ogamwb.fsf@gitster.g","subject":"Re: [PATCH] pack-objects: move builtin-only code to its own header","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-05-27T12:51:39Z","receivedAt":"2021-05-27T12:52:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, May 27 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>\n>> Move the code that's only used in builtin/pack-objects.c to a new\n>> builtin/pack-objects.h header and out of pack-objects.h.\n>\n> I've amended the early part of the proposed log message in\n> preparation for merging it to 'next' and then later down to\n> 'master'.\n>\n> Here is what the result looks like.\n>\n> Thanks.\n>\n>\n> Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> Date:   Thu May 27 02:52:51 2021 +0200\n>\n>     pack-objects: move static inline from a header to its sole consumer\n>     \n>     Move the code that is only used in builtin/pack-objects.c out of\n>     pack-objects.h.\n>     \n>     This fixes an issue where Solaris's SunCC hasn't been able to compile\n>     git since 483fa7f42d9 (t/helper/test-bitmap.c: initial commit,\n>     2021-03-31).\n>     \n>     The real origin of that issue is that in 898eba5e630 (pack-objects:\n>     refer to delta objects by index instead of pointer, 2018-04-14)\n>     utility functions only needed by builtin/pack-objects.c were added to\n>     pack-objects.h. Since then the header has been used in a few other\n>     places, but 483fa7f42d9 was the first time it was used by test helper.\n>     \n>     Since Solaris is stricter about linking and the oe_get_size_slow()\n>     function lives in builtin/pack-objects.c the build started failing\n>     with:\n>     \n>         Undefined                       first referenced\n>          symbol                             in file\n>         oe_get_size_slow                    t/helper/test-bitmap.o\n>         ld: fatal: symbol referencing errors. No output written to t/helper/test-tool\n>     \n>     On other platforms this is presumably OK because the compiler and/or\n>     linker detects that the \"static inline\" functions that reference\n>     oe_get_size_slow() aren't used.\n>     \n>     Let's solve this by moving the relevant code from pack-objects.h to\n>     builtin/pack-objects.c. This is almost entirely a code-only move, but\n>     because of the early macro definitions in that file referencing some\n>     of these inline functions we need to move the definition of \"static\n>     struct packing_data to_pack\" earlier, and declare these inline\n>     functions above the macros.\n>     \n>     Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>     Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\nLooks good to me for what it's worth, and I see you merged this down\nalready, thanks! Git builds again on the Solaris boxes now.\n"}]}