From 3f75a6e5b44d179dbfa99622a8a19d4bd4e04d8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Tue, 29 Nov 2022 13:21:17 +0100 Subject: [PATCH 1/5] t5317: stop losing return codes of git ls-files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fb2d0db502 (test-lib-functions: add parsing helpers for ls-files and ls-tree, 2022-04-04) not only started to use helper functions, it also started to pipe the output of git ls-files into them directly, without using a temporary file. No explanation was given. This causes the return code of that git command to be ignored. Revert that part of the change, use temporary files and check the return code of git ls-files again. Suggested-by: Ævar Arnfjörð Bjarmason Signed-off-by: René Scharfe Signed-off-by: Junio C Hamano --- t/t5317-pack-objects-filter-objects.sh | 52 ++++++++++++++------------ 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh index bb633c9b09..82a22ecaa5 100755 --- a/t/t5317-pack-objects-filter-objects.sh +++ b/t/t5317-pack-objects-filter-objects.sh @@ -24,8 +24,9 @@ parse_verify_pack_blob_oid () { } test_expect_success 'verify blob count in normal packfile' ' - git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 | - test_parse_ls_files_stage_oids | + git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 \ + >ls_files_result && + test_parse_ls_files_stage_oids expected && git -C r1 pack-objects --revs --stdout >all.pack <<-EOF && @@ -123,8 +124,8 @@ test_expect_success 'setup r2' ' ' test_expect_success 'verify blob count in normal packfile' ' - git -C r2 ls-files -s large.1000 large.10000 | - test_parse_ls_files_stage_oids | + git -C r2 ls-files -s large.1000 large.10000 >ls_files_result && + test_parse_ls_files_stage_oids expected && git -C r2 pack-objects --revs --stdout >all.pack <<-EOF && @@ -161,8 +162,8 @@ test_expect_success 'verify blob:limit=1000' ' ' test_expect_success 'verify blob:limit=1001' ' - git -C r2 ls-files -s large.1000 | - test_parse_ls_files_stage_oids | + git -C r2 ls-files -s large.1000 >ls_files_result && + test_parse_ls_files_stage_oids expected && git -C r2 pack-objects --revs --stdout --filter=blob:limit=1001 >filter.pack <<-EOF && @@ -179,8 +180,8 @@ test_expect_success 'verify blob:limit=1001' ' ' test_expect_success 'verify blob:limit=10001' ' - git -C r2 ls-files -s large.1000 large.10000 | - test_parse_ls_files_stage_oids | + git -C r2 ls-files -s large.1000 large.10000 >ls_files_result && + test_parse_ls_files_stage_oids expected && git -C r2 pack-objects --revs --stdout --filter=blob:limit=10001 >filter.pack <<-EOF && @@ -197,8 +198,8 @@ test_expect_success 'verify blob:limit=10001' ' ' test_expect_success 'verify blob:limit=1k' ' - git -C r2 ls-files -s large.1000 | - test_parse_ls_files_stage_oids | + git -C r2 ls-files -s large.1000 >ls_files_result && + test_parse_ls_files_stage_oids expected && git -C r2 pack-objects --revs --stdout --filter=blob:limit=1k >filter.pack <<-EOF && @@ -215,8 +216,8 @@ test_expect_success 'verify blob:limit=1k' ' ' test_expect_success 'verify explicitly specifying oversized blob in input' ' - git -C r2 ls-files -s large.1000 large.10000 | - test_parse_ls_files_stage_oids | + git -C r2 ls-files -s large.1000 large.10000 >ls_files_result && + test_parse_ls_files_stage_oids expected && echo HEAD >objects && @@ -233,8 +234,8 @@ test_expect_success 'verify explicitly specifying oversized blob in input' ' ' test_expect_success 'verify blob:limit=1m' ' - git -C r2 ls-files -s large.1000 large.10000 | - test_parse_ls_files_stage_oids | + git -C r2 ls-files -s large.1000 large.10000 >ls_files_result && + test_parse_ls_files_stage_oids expected && git -C r2 pack-objects --revs --stdout --filter=blob:limit=1m >filter.pack <<-EOF && @@ -289,8 +290,9 @@ test_expect_success 'setup r3' ' ' test_expect_success 'verify blob count in normal packfile' ' - git -C r3 ls-files -s sparse1 sparse2 dir1/sparse1 dir1/sparse2 | - test_parse_ls_files_stage_oids | + git -C r3 ls-files -s sparse1 sparse2 dir1/sparse1 dir1/sparse2 \ + >ls_files_result && + test_parse_ls_files_stage_oids expected && git -C r3 pack-objects --revs --stdout >all.pack <<-EOF && @@ -341,8 +343,9 @@ test_expect_success 'setup r4' ' ' test_expect_success 'verify blob count in normal packfile' ' - git -C r4 ls-files -s pattern sparse1 sparse2 dir1/sparse1 dir1/sparse2 | - test_parse_ls_files_stage_oids | + git -C r4 ls-files -s pattern sparse1 sparse2 dir1/sparse1 dir1/sparse2 \ + >ls_files_result && + test_parse_ls_files_stage_oids expected && git -C r4 pack-objects --revs --stdout >all.pack <<-EOF && @@ -359,8 +362,8 @@ test_expect_success 'verify blob count in normal packfile' ' ' test_expect_success 'verify sparse:oid=OID' ' - git -C r4 ls-files -s dir1/sparse1 dir1/sparse2 | - test_parse_ls_files_stage_oids | + git -C r4 ls-files -s dir1/sparse1 dir1/sparse2 >ls_files_result && + test_parse_ls_files_stage_oids expected && git -C r4 ls-files -s pattern >staged && @@ -379,8 +382,8 @@ test_expect_success 'verify sparse:oid=OID' ' ' test_expect_success 'verify sparse:oid=oid-ish' ' - git -C r4 ls-files -s dir1/sparse1 dir1/sparse2 | - test_parse_ls_files_stage_oids | + git -C r4 ls-files -s dir1/sparse1 dir1/sparse2 >ls_files_result && + test_parse_ls_files_stage_oids expected && git -C r4 pack-objects --revs --stdout --filter=sparse:oid=main:pattern >filter.pack <<-EOF && @@ -400,8 +403,9 @@ test_expect_success 'verify sparse:oid=oid-ish' ' # This models previously omitted objects that we did not receive. test_expect_success 'setup r1 - delete loose blobs' ' - git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 | - test_parse_ls_files_stage_oids | + git -C r1 ls-files -s file.1 file.2 file.3 file.4 file.5 \ + >ls_files_result && + test_parse_ls_files_stage_oids expected && for id in `cat expected | sed "s|..|&/|"` From f00d81153336e1d560288c5f698aada3ca507dba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Tue, 29 Nov 2022 13:22:26 +0100 Subject: [PATCH 2/5] t5317: demonstrate failure to handle multiple --filter options MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit git pack-objects should accept multiple --filter options as documented in Documentation/rev-list-options.txt, but currently the last one wins. Show that using tests with multiple blob size limits Signed-off-by: René Scharfe Signed-off-by: Junio C Hamano --- t/t5317-pack-objects-filter-objects.sh | 38 ++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh index 82a22ecaa5..25faebaada 100755 --- a/t/t5317-pack-objects-filter-objects.sh +++ b/t/t5317-pack-objects-filter-objects.sh @@ -265,6 +265,44 @@ test_expect_success 'verify normal and blob:limit packfiles have same commits/tr test_cmp expected observed ' +test_expect_failure 'verify small limit and big limit results in small limit' ' + git -C r2 ls-files -s large.1000 >ls_files_result && + test_parse_ls_files_stage_oids expected && + + git -C r2 pack-objects --revs --stdout --filter=blob:limit=1001 \ + --filter=blob:limit=10001 >filter.pack <<-EOF && + HEAD + EOF + git -C r2 index-pack ../filter.pack && + + git -C r2 verify-pack -v ../filter.pack >verify_result && + grep blob verify_result | + parse_verify_pack_blob_oid | + sort >observed && + + test_cmp expected observed +' + +test_expect_success 'verify big limit and small limit results in small limit' ' + git -C r2 ls-files -s large.1000 >ls_files_result && + test_parse_ls_files_stage_oids expected && + + git -C r2 pack-objects --revs --stdout --filter=blob:limit=10001 \ + --filter=blob:limit=1001 >filter.pack <<-EOF && + HEAD + EOF + git -C r2 index-pack ../filter.pack && + + git -C r2 verify-pack -v ../filter.pack >verify_result && + grep blob verify_result | + parse_verify_pack_blob_oid | + sort >observed && + + test_cmp expected observed +' + # Test sparse:path= filter. # !!!! # NOTE: sparse:path filter support has been dropped for security reasons, From 825babe5d5c720cb196f4f12c8eeb15fe00e95a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Tue, 29 Nov 2022 13:23:53 +0100 Subject: [PATCH 3/5] pack-objects: fix handling of multiple --filter options MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since 5cb28270a1 (pack-objects: lazily set up "struct rev_info", don't leak, 2022-03-28) --filter options given to git pack-objects overrule earlier ones, letting only the leftmost win and leaking the memory allocated for earlier ones. Fix that by only initializing the rev_info struct once. Signed-off-by: René Scharfe Signed-off-by: Junio C Hamano --- builtin/pack-objects.c | 3 ++- t/t5317-pack-objects-filter-objects.sh | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 573d0b20b7..c702c09dd4 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -4158,7 +4158,8 @@ static struct list_objects_filter_options *po_filter_revs_init(void *value) { struct po_filter_data *data = value; - repo_init_revisions(the_repository, &data->revs, NULL); + if (!data->have_revs) + repo_init_revisions(the_repository, &data->revs, NULL); data->have_revs = 1; return &data->revs.filter; diff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh index 25faebaada..5b707d911b 100755 --- a/t/t5317-pack-objects-filter-objects.sh +++ b/t/t5317-pack-objects-filter-objects.sh @@ -265,7 +265,7 @@ test_expect_success 'verify normal and blob:limit packfiles have same commits/tr test_cmp expected observed ' -test_expect_failure 'verify small limit and big limit results in small limit' ' +test_expect_success 'verify small limit and big limit results in small limit' ' git -C r2 ls-files -s large.1000 >ls_files_result && test_parse_ls_files_stage_oids expected && From 0d5448a554723acfb0d8bbb131e0583504dc7230 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Tue, 29 Nov 2022 13:25:05 +0100 Subject: [PATCH 4/5] pack-objects: simplify --filter handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pack-objects uses OPT_PARSE_LIST_OBJECTS_FILTER_INIT() to initialize the a rev_info struct lazily before populating its filter member using the --filter option values. It tracks whether the initialization is needed using the .have_revs member of the callback data. There is a better way: Use a stand-alone list_objects_filter_options struct and build a rev_info struct with its .filter member after option parsing. This allows using the simpler OPT_PARSE_LIST_OBJECTS_FILTER() and getting rid of the extra callback mechanism. Even simpler would be using a struct rev_info as before 5cb28270a1 (pack-objects: lazily set up "struct rev_info", don't leak, 2022-03-28), but that would expose a memory leak caused by repo_init_revisions() followed by release_revisions() without a setup_revisions() call in between. Using list_objects_filter_options also allows pushing the rev_info struct into get_object_list(), where it arguably belongs. Either way, this is all left for later. Helped-by: Ævar Arnfjörð Bjarmason Signed-off-by: René Scharfe Signed-off-by: Junio C Hamano --- builtin/pack-objects.c | 28 ++++++---------------------- 1 file changed, 6 insertions(+), 22 deletions(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index c702c09dd4..2193f80b89 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -4149,22 +4149,6 @@ static int option_parse_cruft_expiration(const struct option *opt, return 0; } -struct po_filter_data { - unsigned have_revs:1; - struct rev_info revs; -}; - -static struct list_objects_filter_options *po_filter_revs_init(void *value) -{ - struct po_filter_data *data = value; - - if (!data->have_revs) - repo_init_revisions(the_repository, &data->revs, NULL); - data->have_revs = 1; - - return &data->revs.filter; -} - int cmd_pack_objects(int argc, const char **argv, const char *prefix) { int use_internal_rev_list = 0; @@ -4175,7 +4159,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) int rev_list_index = 0; int stdin_packs = 0; struct string_list keep_pack_list = STRING_LIST_INIT_NODUP; - struct po_filter_data pfd = { .have_revs = 0 }; + struct list_objects_filter_options filter_options = + LIST_OBJECTS_FILTER_INIT; struct option pack_objects_options[] = { OPT_SET_INT('q', "quiet", &progress, @@ -4266,7 +4251,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) &write_bitmap_index, N_("write a bitmap index if possible"), WRITE_BITMAP_QUIET, PARSE_OPT_HIDDEN), - OPT_PARSE_LIST_OBJECTS_FILTER_INIT(&pfd, po_filter_revs_init), + OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options), OPT_CALLBACK_F(0, "missing", NULL, N_("action"), N_("handling for missing objects"), PARSE_OPT_NONEG, option_parse_missing_action), @@ -4386,7 +4371,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) if (!rev_list_all || !rev_list_reflog || !rev_list_index) unpack_unreachable_expiration = 0; - if (pfd.have_revs && pfd.revs.filter.choice) { + if (filter_options.choice) { if (!pack_to_stdout) die(_("cannot use --filter without --stdout")); if (stdin_packs) @@ -4473,13 +4458,11 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) read_cruft_objects(); } else if (!use_internal_rev_list) { read_object_list_from_stdin(); - } else if (pfd.have_revs) { - get_object_list(&pfd.revs, rp.nr, rp.v); - release_revisions(&pfd.revs); } else { struct rev_info revs; repo_init_revisions(the_repository, &revs, NULL); + list_objects_filter_copy(&revs.filter, &filter_options); get_object_list(&revs, rp.nr, rp.v); release_revisions(&revs); } @@ -4514,6 +4497,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) reuse_packfile_objects); cleanup: + list_objects_filter_release(&filter_options); strvec_clear(&rp); return 0; From d4f7036887f54184d666a1096ee96dfb2e9ad881 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Tue, 29 Nov 2022 13:26:44 +0100 Subject: [PATCH 5/5] list-objects-filter: remove OPT_PARSE_LIST_OBJECTS_FILTER_INIT() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OPT_PARSE_LIST_OBJECTS_FILTER_INIT() with a non-NULL second argument passes a function pointer via an object pointer, which is undefined. It may work fine on platforms that implement C99 extension J.5.7 (Function pointer casts). Remove the unused macro and avoid the dependency on that extension. Signed-off-by: René Scharfe Signed-off-by: Junio C Hamano --- list-objects-filter-options.c | 4 ---- list-objects-filter-options.h | 18 ++---------------- 2 files changed, 2 insertions(+), 20 deletions(-) diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c index 5339660238..ee01bcd2cc 100644 --- a/list-objects-filter-options.c +++ b/list-objects-filter-options.c @@ -290,10 +290,6 @@ int opt_parse_list_objects_filter(const struct option *opt, const char *arg, int unset) { struct list_objects_filter_options *filter_options = opt->value; - opt_lof_init init = (opt_lof_init)opt->defval; - - if (init) - filter_options = init(opt->value); if (unset || !arg) list_objects_filter_set_no_filter(filter_options); diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h index 7eeadab2dd..1fe393f447 100644 --- a/list-objects-filter-options.h +++ b/list-objects-filter-options.h @@ -111,27 +111,13 @@ void parse_list_objects_filter( * The opt->value to opt_parse_list_objects_filter() is either a * "struct list_objects_filter_option *" when using * OPT_PARSE_LIST_OBJECTS_FILTER(). - * - * Or, if using no "struct option" field is used by the callback, - * except the "defval" which is expected to be an "opt_lof_init" - * function, which is called with the "opt->value" and must return a - * pointer to the ""struct list_objects_filter_option *" to be used. - * - * The OPT_PARSE_LIST_OBJECTS_FILTER_INIT() can be used e.g. the - * "struct list_objects_filter_option" is embedded in a "struct - * rev_info", which the "defval" could be tasked with lazily - * initializing. See cmd_pack_objects() for an example. */ int opt_parse_list_objects_filter(const struct option *opt, const char *arg, int unset); -typedef struct list_objects_filter_options *(*opt_lof_init)(void *); -#define OPT_PARSE_LIST_OBJECTS_FILTER_INIT(fo, init) \ - { OPTION_CALLBACK, 0, "filter", (fo), N_("args"), \ - N_("object filtering"), 0, opt_parse_list_objects_filter, \ - (intptr_t)(init) } #define OPT_PARSE_LIST_OBJECTS_FILTER(fo) \ - OPT_PARSE_LIST_OBJECTS_FILTER_INIT((fo), NULL) + OPT_CALLBACK(0, "filter", (fo), N_("args"), \ + N_("object filtering"), opt_parse_list_objects_filter) /* * Translates abbreviated numbers in the filter's filter_spec into their