multi-pack-index: add --no-progress
cover multi-pack-index: add --no-progress
Hello Git contributors!
My name is William Baker and I work at Microsoft. Over the past few years I've worked closely with the Microsoft team contributing to the git ecosystem and I'm excited to start working with the community.
Derrick Stolee helped me pick out my first task, and it's to add support for opting out of the multi-pack-index progress output. The progress feature was introduced in 144d7033 ("multi-pack-index: report progress during 'verify'", 2018-09-13) but it did not include the ability to opt-out. This patch adds the --[no-]progress option to allow callers control whether progress is reported.
This path is very similar to Garima Singh's series for progress reporting in the commit-graph [1].
[1] https://public-inbox.org/git/[email protected]/
I'm looking forward to your review. Thanks! William Baker
William Baker (1):
multi-pack-index: add --no-progress Add --no-progress option to git
multi-pack-index. The progress feature was added in 144d703 ("multi-pack-index: report progress during 'verify'", 2018-09-13) but the ability to opt-out was overlooked.
Documentation/git-multi-pack-index.txt | 6 +++++- builtin/multi-pack-index.c | 14 +++++++++--- midx.c | 30 +++++++++++++++++--------- midx.h | 6 ++++-- t/t5319-multi-pack-index.sh | 30 ++++++++++++++++++++++++++ 5 files changed, 70 insertions(+), 16 deletions(-)
base-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-337%2Fwilbaker%2Fmulti-pack-index-progress-toggle-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-337/wilbaker/multi-pack-index-progress-toggle-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/337
Signature · 1 line
gitgitgadget
PATCH 1/1 multi-pack-index: add --no-progress Add --no-progress option to git multi-pack-index. The progress feature was added in 144d703 ("multi-pack-index: report progress during 'verify'", 2018-09-13) but the ability to opt-out was overlooked.
From: William Baker <[email protected]>
--- Documentation/git-multi-pack-index.txt | 6 +++++- builtin/multi-pack-index.c | 14 +++++++++--- midx.c | 30 +++++++++++++++++--------- midx.h | 6 ++++-- t/t5319-multi-pack-index.sh | 30 ++++++++++++++++++++++++++ 5 files changed, 70 insertions(+), 16 deletions(-)
Documentation/git-multi-pack-index.txt +5 -1
@@ -9,7 +9,7 @@ git-multi-pack-index - Write and verify multi-pack-indexes SYNOPSIS -------- [verse] -'git multi-pack-index' [--object-dir=<dir>] <subcommand> +'git multi-pack-index' [--object-dir=<dir>] <subcommand> [--[no-]progress] DESCRIPTION -----------
@@ -23,6 +23,10 @@ OPTIONS `<dir>/packs/multi-pack-index` for the current MIDX file, and `<dir>/packs` for the pack-files to index. +--[no-]progress:: + Turn progress on/off explicitly. If neither is specified, progress is + shown if standard error is connected to a terminal. + The following subcommands are available: write::
builtin/multi-pack-index.c +11 -3
@@ -6,34 +6,41 @@#include "trace2.h" static char const *const builtin_multi_pack_index_usage[] = { -N_ ("git multi-pack-index [--object-dir=<dir>] (write|verify|expire|repack --batch-size=<size>)" ), +N_ ("git multi-pack-index [--object-dir=<dir>] (write|verify|expire|repack --batch-size=<size>) [--[no-]progress]" ),NULL };static struct opts_multi_pack_index {const char *object_dir;unsigned long batch_size; +int progress; } opts;int cmd_multi_pack_index (int argc,const char **argv,const char *prefix) { +unsigned flags =0 ; +static struct option builtin_multi_pack_index_options[] = {OPT_FILENAME (0 ,"object-dir" , &opts.object_dir,N_ ("object directory containing set of packfile and pack-index pairs" )),OPT_MAGNITUDE (0 ,"batch-size" , &opts.batch_size,N_ ("during repack, collect pack-files of smaller size into a batch that is larger than this size" )), +OPT_BOOL (0 ,"progress" , &opts.progress,N_ ("force progress reporting" )),OPT_END (), };git_config (git_default_config,NULL ); + opts.progress =isatty (2 ); argc =parse_options (argc, argv, prefix, builtin_multi_pack_index_options, builtin_multi_pack_index_usage,0 );if (!opts.object_dir) opts.object_dir =get_object_directory (); +if (opts.progress) + flags |=MIDX_PROGRESS ;if (argc ==0 ) usage_with_options(builtin_multi_pack_index_usage,
@@ -47,14 +54,15 @@ int cmd_multi_pack_index(int argc, const char **argv,trace2_cmd_mode (argv[0 ]);if (!strcmp (argv[0 ],"repack" )) -return midx_repack (the_repository, opts.object_dir, (size_t )opts.batch_size); +return midx_repack (the_repository, opts.object_dir, + (size_t )opts.batch_size, flags);if (opts.batch_size)die (_ ("--batch-size option is only for 'repack' subcommand" ));if (!strcmp (argv[0 ],"write" ))return write_midx_file (opts.object_dir);if (!strcmp (argv[0 ],"verify" )) -return verify_midx_file (the_repository, opts.object_dir); +return verify_midx_file (the_repository, opts.object_dir, flags);if (!strcmp (argv[0 ],"expire" ))return expire_midx_packs (the_repository, opts.object_dir);
midx.c +20 -10
@@ -1077,19 +1077,20 @@ static int compare_pair_pos_vs_id(const void *_a, const void *_b)display_progress (progress, _n); \ }while (0 ) -int verify_midx_file(struct repository *r, const char *object_dir) +int verify_midx_file(struct repository *r, const char *object_dir, unsigned flags) { struct pair_pos_vs_id *pairs = NULL;uint32_t i; -struct progress *progress; +struct progress *progress =NULL ;struct multi_pack_index *m =load_multi_pack_index (object_dir,1 ); verify_midx_error =0 ;if (!m)return 0 ; - progress =start_progress (_ ("Looking for referenced packfiles" ), - m->num_packs); +if (flags &MIDX_PROGRESS ) + progress =start_progress (_ ("Looking for referenced packfiles" ), + m->num_packs);for (i =0 ; i < m->num_packs; i++) {if (prepare_midx_pack (r, m, i))midx_report ("failed to load pack in position %d" , i);
@@ -1107,8 +1108,9 @@ int verify_midx_file(struct repository *r, const char *object_dir) i, oid_fanout1, oid_fanout2, i + 1); } - progress =start_sparse_progress (_ ("Verifying OID order in MIDX" ), - m->num_objects -1 ); +if (flags &MIDX_PROGRESS ) + progress =start_sparse_progress (_ ("Verifying OID order in MIDX" ), + m->num_objects -1 );for (i =0 ; i < m->num_objects -1 ; i++) {struct object_id oid1, oid2;
@@ -1135,13 +1137,15 @@ int verify_midx_file(struct repository *r, const char *object_dir) pairs[i].pack_int_id =nth_midxed_pack_int_id (m, i); } - progress =start_sparse_progress (_ ("Sorting objects by packfile" ), - m->num_objects); +if (flags &MIDX_PROGRESS ) + progress =start_sparse_progress (_ ("Sorting objects by packfile" ), + m->num_objects);display_progress (progress,0 );/* TODO: Measure QSORT() progress */ QSORT (pairs, m->num_objects, compare_pair_pos_vs_id);stop_progress (&progress); - progress =start_sparse_progress (_ ("Verifying object offsets" ), m->num_objects); +if (flags &MIDX_PROGRESS ) + progress =start_sparse_progress (_ ("Verifying object offsets" ), m->num_objects);for (i =0 ; i < m->num_objects; i++) {struct object_id oid;struct pack_entry e;
@@ -1316,7 +1320,7 @@ static int fill_included_packs_batch(struct repository *r,return 0 ; } -int midx_repack (struct repository *r,const char *object_dir,size_t batch_size) +int midx_repack (struct repository *r,const char *object_dir,size_t batch_size,unsigned flags) {int result =0 ;uint32_t i;
@@ -1341,6 +1345,12 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size)strbuf_addstr (&base_name, object_dir);strbuf_addstr (&base_name,"/pack/pack" );argv_array_push (&cmd.args, base_name.buf); + +if (flags &MIDX_PROGRESS ) +argv_array_push (&cmd.args,"--progress" ); +else +argv_array_push (&cmd.args,"-q" ); +strbuf_release (&base_name); cmd.git_cmd =1 ;
midx.h +4 -2
@@ -37,6 +37,8 @@ struct multi_pack_index {char object_dir[FLEX_ARRAY ]; }; +#define MIDX_PROGRESS (1 << 0) +struct multi_pack_index *load_multi_pack_index (const char *object_dir,int local);int prepare_midx_pack (struct repository *r,struct multi_pack_index *m,uint32_t pack_int_id);int bsearch_midx (const struct object_id *oid,struct multi_pack_index *m,uint32_t *result);
@@ -49,9 +51,9 @@ int prepare_multi_pack_index_one(struct repository *r, const char *object_dir, iint write_midx_file (const char *object_dir);void clear_midx_file (struct repository *r); -int verify_midx_file (struct repository *r,const char *object_dir); +int verify_midx_file (struct repository *r,const char *object_dir,unsigned flags);int expire_midx_packs (struct repository *r,const char *object_dir); -int midx_repack (struct repository *r,const char *object_dir,size_t batch_size); +int midx_repack (struct repository *r,const char *object_dir,size_t batch_size,unsigned flags);void close_midx (struct multi_pack_index *m);
t/t5319-multi-pack-index.sh +30 -0
@@ -169,6 +169,21 @@ test_expect_success 'verify multi-pack-index success' ' git multi-pack-index verify --object-dir=$objdir ' +test_expect_success 'verify progress off for redirected stderr' ' + git multi-pack-index verify --object-dir=$objdir 2>err && + test_line_count = 0 err +' + +test_expect_success 'verify force progress on for stderr' ' + git multi-pack-index verify --object-dir=$objdir --progress 2>err && + test_file_not_empty err +' + +test_expect_success 'verify with the --no-progress option' ' + git multi-pack-index verify --object-dir=$objdir --no-progress 2>err && + test_line_count = 0 err +' + # usage: corrupt_midx_and_verify <pos> <data> <objdir> <string> corrupt_midx_and_verify() { POS=$1 &&
@@ -284,6 +299,21 @@ test_expect_success 'git-fsck incorrect offset' ' "git -c core.multipackindex=true fsck" ' +test_expect_success 'repack progress off for redirected stderr' ' + git multi-pack-index repack --object-dir=$objdir 2>err && + test_line_count = 0 err +' + +test_expect_success 'repack force progress on for stderr' ' + git multi-pack-index repack --object-dir=$objdir --progress 2>err && + test_file_not_empty err +' + +test_expect_success 'repack with the --no-progress option' ' + git multi-pack-index repack --object-dir=$objdir --no-progress 2>err && + test_line_count = 0 err +' + test_expect_success 'repack removes multi-pack-index' ' test_path_is_file $objdir/pack/multi-pack-index && GIT_TEST_MULTI_PACK_INDEX=0 git repack -adf &&
Signature · 1 line
gitgitgadget
Quoted 5 lines · William Baker via GitGitGadget
Hello Git contributors!
My name is William Baker and I work at Microsoft. Over the past few years I've worked closely with the Microsoft team contributing to the git ecosystem and I'm excited to start working with the community.
William is being modest. He joined our team from the Windows org due to his extremely impactful work in the VFS for Git client, especially around the file system virtualization areas. While he started with the filesystem, he also delivered important features around managing the very large object store, so he is not new to some of Git's deepest internals at scale.
Thanks,
-Stolee
Re: [PATCH 1/1] multi-pack-index: add --no-progress Add --no-progress option to git multi-pack-index. The progress feature was added in 144d703 ("multi-pack-index: report progress during 'verify'", 2018-09-13) but the ability to opt-out was overlooked.
In reply to William Baker via GitGitGadgetQuoted 3 lines · William Baker via GitGitGadget
[verse]
-'git multi-pack-index' [--object-dir=<dir>] <subcommand>
+'git multi-pack-index' [--object-dir=<dir>] <subcommand> [--[no-]progress]
I am wondering what the reasoning behind having this new one *after* the subcommand while the existing one *before* is. Isn't the --[no-]progress option supported by all subcommands of the multi-pack-index command, just like the --object-dir=<dir> option is?
If there is no reason that explains why one must come before and the other must come after the subcommand, we are then adding another thing the end-users or scriptors need to memorize, which is not ideal.
Documentation/git-multi-pack-index.txt OPTIONS · line 28
Quoted 3 lines
+--[no-]progress::
Documentation/git-multi-pack-index.txt +2
-0+ Turn progress on/off explicitly. If neither is specified, progress is + shown if standard error is connected to a terminal.
Sounds sensible.
builtin/multi-pack-index.c line 11
Quoted 12 lines
builtin/multi-pack-index.c +1
-1@@ -6,34 +6,41 @@#include "trace2.h" static char const *const builtin_multi_pack_index_usage[] = { -N_ ("git multi-pack-index [--object-dir=<dir>] (write|verify|expire|repack --batch-size=<size>)" ), +N_ ("git multi-pack-index [--object-dir=<dir>] (write|verify|expire|repack --batch-size=<size>) [--[no-]progress]" ),NULL };
The same comment as the SYNOPSIS part.
Quoted 9 lines
builtin/multi-pack-index.c +1
-0static struct opts_multi_pack_index {const char *object_dir;unsigned long batch_size; +int progress; } opts;int cmd_multi_pack_index (int argc,const char **argv,const char *prefix) {
builtin/multi-pack-index.c line 31
Quoted 10 lines
builtin/multi-pack-index.c +3
-0+unsigned flags =0 ; +static struct option builtin_multi_pack_index_options[] = {OPT_FILENAME (0 ,"object-dir" , &opts.object_dir,N_ ("object directory containing set of packfile and pack-index pairs" )),OPT_MAGNITUDE (0 ,"batch-size" , &opts.batch_size,N_ ("during repack, collect pack-files of smaller size into a batch that is larger than this size" )), +OPT_BOOL (0 ,"progress" , &opts.progress,N_ ("force progress reporting" )),OPT_END (), };
Seeing that all the options that made me curious (see above) are defined here for all subcommands, I suspect that "technically", all options must come before the <subcommand> (side note: I know parse-options may reorder commands after options, but in the documentation and usage, we strongly discourage users from relying on the reordering and always show "global --options, subcommand, and then subcommand --options" order). I also see in the code that handles opts.batch_size that there is a workaround for this inverted code structure to make sure subcommands other than repack does not allow --batch-size option specified.
Unless and until we get rid of the "git multi-pack-index" as a separate command (side note: when it happens, we;'d call the underlying midx API functions directly from appropriate places in the codepaths like "gc"), we probably would want to correct the use of parse_options() API in the implementation of this command before adding any new option or subcommand.
Quoted 7 lines
builtin/multi-pack-index.c +2
-1@@ -47,14 +54,15 @@ int cmd_multi_pack_index(int argc, const char **argv,trace2_cmd_mode (argv[0 ]);if (!strcmp (argv[0 ],"repack" )) -return midx_repack (the_repository, opts.object_dir, (size_t )opts.batch_size); +return midx_repack (the_repository, opts.object_dir, + (size_t )opts.batch_size, flags);
builtin/multi-pack-index.c int cmd_multi_pack_index(int argc, const char **argv, · line 67
Quoted 10 lines
builtin/multi-pack-index.c +1
-1if (opts.batch_size)die (_ ("--batch-size option is only for 'repack' subcommand" ));if (!strcmp (argv[0 ],"write" ))return write_midx_file (opts.object_dir);if (!strcmp (argv[0 ],"verify" )) -return verify_midx_file (the_repository, opts.object_dir); +return verify_midx_file (the_repository, opts.object_dir, flags);if (!strcmp (argv[0 ],"expire" ))return expire_midx_packs (the_repository, opts.object_dir);
We can see that the new option only affects "verify", even though the SYNOPSIS and usage text pretends that everybody understands and reacts to it. Shouldn't it be documented just like how --batch-size is documented that it is understood only by "repack"?
If the mid-term aspiration of this patch is to later enhance other subcommands to also understand the progress output or verbosity option (and if the excuse given as a response to the above analysis is "this is just a first step, more will come later"), the instead of adding a "unsigned flag" local variable to the function, it would probably make much more sense to
(1) make struct opts_multi_pack_index as a part of the public API between cmd_multi_pack_index() and midx.c and document it in midx.h;
(2) instead of passing opts.object_dir to existing command implementations, pass &opts, the pointer to the whole structure;
(3) add a new field "unsigned progress" to the structure, and teach the command line parser to flip it upon seeing "--[no-]progress".
Re: [PATCH 1/1] multi-pack-index: add --no-progress Add --no-progress option to git multi-pack-index. The progress feature was added in 144d703 ("multi-pack-index: report progress during 'verify'", 2018-09-13) but the ability to opt-out was overlooked.
Hi Junio,
Thanks for the review!
Quoted 9 lines · Junio C Hamano
Quoted 1 line
+'git multi-pack-index' [--object-dir=<dir>] <subcommand> [--[no-]progress]
I am wondering what the reasoning behind having this new one *after* the subcommand while the existing one *before* is. Isn't the --[no-]progress option supported by all subcommands of the multi-pack-index command, just like the --object-dir=<dir> option is?
always show "global --options, subcommand, and then subcommand --options" order).
Thanks for calling this out. I didn't have a specific reason for making this option appear after the subcommand. I tried looking at other commands as examples and I missed that there's a specific ordering based on the type of the option. I will clean this up in the next patch.
Quoted 8 lines
I also see in the code that
handles opts.batch_size that there is a workaround for this inverted code structure to make sure subcommands other than repack does not allow --batch-size option specified.we probably would want to correct the use
ofparse_options()API in the implementation of this command before adding any new option or subcommand.
To confirm I understand, is the recommendation that cmd_multi_pack_index be updated to only parse "batch-size" for the repack subcommand (i.e. use PARSE_OPT_STOP_AT_NON_OPTION to parse all of the common options, and then only parse "batch-size" when the repack subcommand is running)?
Quoted 24 lines
Quoted 7 lines
builtin/multi-pack-index.c +2
-1@@ -47,14 +54,15 @@ int cmd_multi_pack_index(int argc, const char **argv,trace2_cmd_mode (argv[0 ]);if (!strcmp (argv[0 ],"repack" )) -return midx_repack (the_repository, opts.object_dir, (size_t )opts.batch_size); +return midx_repack (the_repository, opts.object_dir, + (size_t )opts.batch_size, flags);builtin/multi-pack-index.c int cmd_multi_pack_index(int argc, const char **argv, · line 67
Quoted 10 lines
builtin/multi-pack-index.c +1
-1if (opts.batch_size)die (_ ("--batch-size option is only for 'repack' subcommand" ));if (!strcmp (argv[0 ],"write" ))return write_midx_file (opts.object_dir);if (!strcmp (argv[0 ],"verify" )) -return verify_midx_file (the_repository, opts.object_dir); +return verify_midx_file (the_repository, opts.object_dir, flags);if (!strcmp (argv[0 ],"expire" ))return expire_midx_packs (the_repository, opts.object_dir);We can see that the new option only affects "verify", even though the SYNOPSIS and usage text pretends that everybody understands and reacts to it. Shouldn't it be documented just like how --batch-size is documented that it is understood only by "repack"?
Quoted 4 lines
If the mid-term aspiration of this patch is to later enhance other subcommands to also understand the progress output or verbosity option (and if the excuse given as a response to the above analysis is "this is just a first step, more will come later")
Yep this was my thinking. Today "repack" and "verify" are the only subcommands that have any progress output but as the other subcommands learn how to provide progress the [--[no-]progress] option can be used to control it.
Quoted 7 lines
instead of adding a "unsigned flag" local variable to the function, it would probably make much more sense to
(1) make struct
opts_multi_pack_indexas a part of the public API betweencmd_multi_pack_index()andmidx.cand document it inmidx.h;
Quoted 7 lines
(2) instead of passing opts.object_dir to existing command implementations, pass &opts, the pointer to the whole structure;
(3) add a new field "unsigned progress" to the structure, and teach the command line parser to flip it upon seeing "--[no-]progress".
Thanks for this suggestion I'll use this approach in the next patch.
One small point to clarify, in the current struct I'm using an int for "progress" because OPT_BOOL expects an int*. Do you have any concerns with keeping "progress" as an int in the public struct, or would you rather cmd_multi_pack_index parse an int internally and use it to populate the public struct?
Thanks!
William
Re: [PATCH 1/1] multi-pack-index: add --no-progress Add --no-progress option to git multi-pack-index. The progress feature was added in 144d703 ("multi-pack-index: report progress during 'verify'", 2018-09-13) but the ability to opt-out was overlooked.
Quoted 5 lines · William Baker
Quoted 4 lines
I also see in the code that
handles opts.batch_size that there is a workaround for this inverted code structure to make sure subcommands other than repack does not allow --batch-size option specified.
Quoted 8 lines
Quoted 3 lines
we probably would want to correct the use
ofparse_options()API in the implementation of this command before adding any new option or subcommand.To confirm I understand, is the recommendation that
cmd_multi_pack_indexbe updated to only parse "batch-size" for the repack subcommand (i.e. usePARSE_OPT_STOP_AT_NON_OPTIONto parse all of the common options, and then only parse "batch-size" when the repack subcommand is running)?
Compare the ways how dispatching and command line option parsing of subcommands in "multi-pack-index" and "commit-graph" are implemented. When a command (e.g. "commit-graph") takes common options and also has subcommands (e.g. "read" and "write") that take different set of options, there is a common options parser in the primary entry point (e.g. "cmd_commit_graph()"), and after dispatching to a chosen subcommand, the implementation of each subcommand (e.g. "graph_read()" and "graph_write()") parses its own options. That's bog-standard way.
cmd_multi_pack_index() "cheats" and does not implement proper subcommand dispatch (it directly makes underlying API calls instead). Started as an isolated experimental command whose existence as a standalone command is solely because it was easier to experiment with (as opposed to being a plumbing command to be used by scripters), it probably was an acceptable trade-off to leave the code in this shape. In the longer run, I suspect we'd rather want to get rid of "git multi-pack-index" as a standalone command and instead make "git gc" and other commands make direct calls to the internal machinery of midx code from strategic places. So in that sense, I am not sure if I should "recommend" fixing the way the subcommand dispatching works in this command, or just accept to keep the ugly technical debt and let it limp along...
Quoted 19 lines
Quoted 3 lines
subcommands to also understand the progress output or verbosity option (and if the excuse given as a response to the above analysis is "this is just a first step, more will come later")
Yep this was my thinking. Today "repack" and "verify" are the only subcommands that have any progress output but as the other subcommands learn how to provide progress the [--[no-]progress] option can be used to control it.
Quoted 11 lines
instead of adding a "unsigned flag" local variable to the function, it would probably make much more sense to
(1) make struct
opts_multi_pack_indexas a part of the public API betweencmd_multi_pack_index()andmidx.cand document it inmidx.h;(2) instead of passing opts.object_dir to existing command implementations, pass &opts, the pointer to the whole structure;
Quoted 5 lines
Quoted 3 lines
(3) add a new field "unsigned progress" to the structure, and teach the command line parser to flip it upon seeing "--[no-]progress".
Thanks for this suggestion I'll use this approach in the next patch.
...but it appears to me that you are more enthused than I am in improving this code, so I probably should actually recommend fixing the main entry point function of this command to imitate the way "commit-graph" implements subcommands and their own set of options ;-)
Re: [PATCH 1/1] multi-pack-index: add --no-progress Add --no-progress option to git multi-pack-index. The progress feature was added in 144d703 ("multi-pack-index: report progress during 'verify'", 2018-09-13) but the ability to opt-out was overlooked.
Quoted 9 lines · Junio C Hamano
Compare the ways how dispatching and command line option parsing of subcommands in "multi-pack-index" and "commit-graph" are implemented. When a command (e.g. "commit-graph") takes common options and also has subcommands (e.g. "read" and "write") that take different set of options, there is a common options parser in the primary entry point (e.g. "
cmd_commit_graph()"), and after dispatching to a chosen subcommand, the implementation of each subcommand (e.g. "graph_read()" and "graph_write()") parses its own options. That's bog-standard way.
Thanks for the pointer to "commit-graph", looking through that code cleared up any questions I had.
After taking another look through the "multi-pack-index" code my plan is to update all of the subcommands to understand [--[no-]progress]. I gave the public struct in midx.h approach a try, but after seeing how that looks I think it would be cleaner to update "write" and "expire" to display progress and have an explicit parameter.
Quoted 11 lines
Started as an isolated experimental command whose existence as a standalone command is solely because it was easier to experiment with (as opposed to being a plumbing command to be used by scripters), it probably was an acceptable trade-off to leave the code in this shape. In the longer run, I suspect we'd rather want to get rid of "git multi-pack-index" as a standalone command and instead make "git gc" and other commands make direct calls to the internal machinery of midx code from strategic places. So in that sense, I am not sure if I should "recommend" fixing the way the subcommand dispatching works in this command, or just accept to keep the ugly technical debt and let it limp along...
Thanks for the background here, it helped with understanding why the multi-pack-index parsing is different than the other commands.
My plan is to include 3 commits in the next (v2) patch series:
1. Adding the new parameters to midx.h/c to control progress
2. Update write/expire to display progress
3. Update the multi-pack-index.c builtin to parse the [--[no-]progress] option and update the tests.
I wasn't thinking that I would adjust the the subcommand dispatching in multi-pack-index.c in this patch series. By updating all of the subcommands to support [--[no-]progress] I should be able to keep the changes to multi-pack-index.c quite small. If you see any potential issues with this approach please let me know.
Thanks,
William
v2 cover multi-pack-index: add --no-progress
In reply to William Baker via GitGitGadgetHello Git contributors!
My name is William Baker and I work at Microsoft. Over the past few years I've worked closely with the Microsoft team contributing to the git ecosystem and I'm excited to start working with the community.
Derrick Stolee helped me pick out my first task, and it's to add support for opting out of the multi-pack-index progress output. The progress feature was introduced in 144d7033 ("multi-pack-index: report progress during 'verify'", 2018-09-13) but it did not include the ability to opt-out. This patch adds the --[no-]progress option to allow callers control whether progress is reported.
This path is very similar to Garima Singh's series for progress reporting in the commit-graph [1].
[1] https://public-inbox.org/git/[email protected]/
I'm looking forward to your review. Thanks! William Baker
William Baker (6):
midx: add MIDX_PROGRESS flag Add the MIDX_PROGRESS flag and update the
write|verify|expire|repack functions in midx.h to accept a flags parameter. The MIDX_PROGRESS flag indicates whether the caller of the function would like progress information to be displayed. This patch only changes the method prototypes and does not change the functionality. The functionality change will be handled by a later patch.
midx: add progress to write_midx_file Add progress to write_midx_file.
Progress is displayed when the MIDX_PROGRESS flag is set.
midx: add progress to expire_midx_packs Add progress to
expire_midx_packs. Progress is displayed when the MIDX_PROGRESS flag is set.
midx: honor the MIDX_PROGRESS flag in verify_midx_file Update
verify_midx_file to only display progress when the MIDX_PROGRESS flag is set.
midx: honor the MIDX_PROGRESS flag in midx_repack Update midx_repack
to only display progress when the MIDX_PROGRESS flag is set.
multi-pack-index: add [--[no-]progress] option. Add the
--[no-]progress option to git multi-pack-index. Pass the MIDX_PROGRESS flag to the subcommand functions when progress should be displayed by multi-pack-index. The progress feature was added to 'verify' in 144d703 ("multi-pack-index: report progress during 'verify'", 2018-09-13) but some subcommands were not updated to display progress, and the ability to opt-out was overlooked.
Documentation/git-multi-pack-index.txt | 6 ++- builtin/multi-pack-index.c | 18 +++++-- builtin/repack.c | 2 +- midx.c | 71 ++++++++++++++++++++------ midx.h | 10 ++-- t/t5319-multi-pack-index.sh | 69 +++++++++++++++++++++++++ 6 files changed, 149 insertions(+), 27 deletions(-)
base-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-337%2Fwilbaker%2Fmulti-pack-index-progress-toggle-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-337/wilbaker/multi-pack-index-progress-toggle-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/337
Range-diff vs v1:
-: ---------- > 1: 6badd9ceaf midx: add MIDX_PROGRESS flag Add the MIDX_PROGRESS flag and update the write|verify|expire|repack functions in midx.h to accept a flags parameter. The MIDX_PROGRESS flag indicates whether the caller of the function would like progress information to be displayed. This patch only changes the method prototypes and does not change the functionality. The functionality change will be handled by a later patch.
-: ---------- > 2: 3bc8677ea7 midx: add progress to write_midx_file Add progress to write_midx_file. Progress is displayed when the MIDX_PROGRESS flag is set.
-: ---------- > 3: 3374574001 midx: add progress to expire_midx_packs Add progress to expire_midx_packs. Progress is displayed when the MIDX_PROGRESS flag is set.
-: ---------- > 4: 29d03771c0 midx: honor the MIDX_PROGRESS flag in verify_midx_file Update verify_midx_file to only display progress when the MIDX_PROGRESS flag is set.
-: ---------- > 5: 57f6742f09 midx: honor the MIDX_PROGRESS flag in midx_repack Update midx_repack to only display progress when the MIDX_PROGRESS flag is set.
1: 0821a8073a ! 6: 5b933ab744 multi-pack-index: add --no-progress Add --no-progress option to git multi-pack-index. The progress feature was added in 144d703 ("multi-pack-index: report progress during 'verify'", 2018-09-13) but the ability to opt-out was overlooked.
@@ -1,10 +1,13 @@
Author: William Baker <[email protected]>
- multi-pack-index: add --no-progress
- Add --no-progress option to git multi-pack-index.
- The progress feature was added in 144d703
+ multi-pack-index: add [--[no-]progress] option.
+ Add the --[no-]progress option to git multi-pack-index.
+ Pass the MIDX_PROGRESS flag to the subcommand functions
+ when progress should be displayed by multi-pack-index.
+ The progress feature was added to 'verify' in 144d703
("multi-pack-index: report progress during 'verify'", 2018-09-13)
- but the ability to opt-out was overlooked.
+ but some subcommands were not updated to display progress, and
+ the ability to opt-out was overlooked.
Signed-off-by: William Baker <[email protected]>
@@ -16,7 +19,7 @@
--------
[verse]
-'git multi-pack-index' [--object-dir=<dir>] <subcommand>
-+'git multi-pack-index' [--object-dir=<dir>] <subcommand> [--[no-]progress]
++'git multi-pack-index' [--object-dir=<dir>] [--[no-]progress] <subcommand>
DESCRIPTION
-----------
@@ -40,7 +43,7 @@
static char const * const builtin_multi_pack_index_usage[] = {
- N_("git multi-pack-index [--object-dir=<dir>] (write|verify|expire|repack --batch-size=<size>)"),
-+ N_("git multi-pack-index [--object-dir=<dir>] (write|verify|expire|repack --batch-size=<size>) [--[no-]progress]"),
++ N_("git multi-pack-index [<options>] (write|verify|expire|repack --batch-size=<size>)"),
NULL
};
@@ -58,11 +61,11 @@
static struct option builtin_multi_pack_index_options[] = {
OPT_FILENAME(0, "object-dir", &opts.object_dir,
N_("object directory containing set of packfile and pack-index pairs")),
++ OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")),
OPT_MAGNITUDE(0, "batch-size", &opts.batch_size,
N_("during repack, collect pack-files of smaller size into a batch that is larger than this size")),
-+ OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")),
OPT_END(),
- };
+@@
git_config(git_default_config, NULL);
@@ -82,131 +85,50 @@
trace2_cmd_mode(argv[0]);
if (!strcmp(argv[0], "repack"))
-- return midx_repack(the_repository, opts.object_dir, (size_t)opts.batch_size);
+- return midx_repack(the_repository, opts.object_dir, (size_t)opts.batch_size, 0);
+ return midx_repack(the_repository, opts.object_dir,
+ (size_t)opts.batch_size, flags);
if (opts.batch_size)
die(_("--batch-size option is only for 'repack' subcommand"));
if (!strcmp(argv[0], "write"))
- return write_midx_file(opts.object_dir);
+- return write_midx_file(opts.object_dir, 0);
++ return write_midx_file(opts.object_dir, flags);
if (!strcmp(argv[0], "verify"))
-- return verify_midx_file(the_repository, opts.object_dir);
+- return verify_midx_file(the_repository, opts.object_dir, 0);
+ return verify_midx_file(the_repository, opts.object_dir, flags);
if (!strcmp(argv[0], "expire"))
- return expire_midx_packs(the_repository, opts.object_dir);
+- return expire_midx_packs(the_repository, opts.object_dir, 0);
++ return expire_midx_packs(the_repository, opts.object_dir, flags);
+ die(_("unrecognized subcommand: %s"), argv[0]);
+ }
- diff --git a/midx.c b/midx.c
- --- a/midx.c
- +++ b/midx.c
-@@
- display_progress(progress, _n); \
- } while (0)
-
--int verify_midx_file(struct repository *r, const char *object_dir)
-+int verify_midx_file(struct repository *r, const char *object_dir, unsigned flags)
- {
- struct pair_pos_vs_id *pairs = NULL;
- uint32_t i;
-- struct progress *progress;
-+ struct progress *progress = NULL;
- struct multi_pack_index *m = load_multi_pack_index(object_dir, 1);
- verify_midx_error = 0;
-
- if (!m)
- return 0;
-
-- progress = start_progress(_("Looking for referenced packfiles"),
-- m->num_packs);
-+ if (flags & MIDX_PROGRESS)
-+ progress = start_progress(_("Looking for referenced packfiles"),
-+ m->num_packs);
- for (i = 0; i < m->num_packs; i++) {
- if (prepare_midx_pack(r, m, i))
- midx_report("failed to load pack in position %d", i);
+ diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
+ --- a/t/t5319-multi-pack-index.sh
+ +++ b/t/t5319-multi-pack-index.sh
@@
- i, oid_fanout1, oid_fanout2, i + 1);
- }
-
-- progress = start_sparse_progress(_("Verifying OID order in MIDX"),
-- m->num_objects - 1);
-+ if (flags & MIDX_PROGRESS)
-+ progress = start_sparse_progress(_("Verifying OID order in MIDX"),
-+ m->num_objects - 1);
- for (i = 0; i < m->num_objects - 1; i++) {
- struct object_id oid1, oid2;
-@@
- pairs[i].pack_int_id = nth_midxed_pack_int_id(m, i);
- }
-
-- progress = start_sparse_progress(_("Sorting objects by packfile"),
-- m->num_objects);
-+ if (flags & MIDX_PROGRESS)
-+ progress = start_sparse_progress(_("Sorting objects by packfile"),
-+ m->num_objects);
- display_progress(progress, 0); /* TODO: Measure QSORT() progress */
- QSORT(pairs, m->num_objects, compare_pair_pos_vs_id);
- stop_progress(&progress);
-
-- progress = start_sparse_progress(_("Verifying object offsets"), m->num_objects);
-+ if (flags & MIDX_PROGRESS)
-+ progress = start_sparse_progress(_("Verifying object offsets"), m->num_objects);
- for (i = 0; i < m->num_objects; i++) {
- struct object_id oid;
- struct pack_entry e;
-@@
- return 0;
- }
+ compare_results_with_midx "two packs"
--int midx_repack(struct repository *r, const char *object_dir, size_t batch_size)
-+int midx_repack(struct repository *r, const char *object_dir, size_t batch_size, unsigned flags)
- {
- int result = 0;
- uint32_t i;
-@@
- strbuf_addstr(&base_name, object_dir);
- strbuf_addstr(&base_name, "/pack/pack");
- argv_array_push(&cmd.args, base_name.buf);
++test_expect_success 'write progress off for redirected stderr' '
++ git multi-pack-index --object-dir=$objdir write 2>err &&
++ test_line_count = 0 err
++'
+
-+ if (flags & MIDX_PROGRESS)
-+ argv_array_push(&cmd.args, "--progress");
-+ else
-+ argv_array_push(&cmd.args, "-q");
++test_expect_success 'write force progress on for stderr' '
++ git multi-pack-index --object-dir=$objdir --progress write 2>err &&
++ test_file_not_empty err
++'
+
- strbuf_release(&base_name);
-
- cmd.git_cmd = 1;
-
- diff --git a/midx.h b/midx.h
- --- a/midx.h
- +++ b/midx.h
-@@
- char object_dir[FLEX_ARRAY];
- };
-
-+#define MIDX_PROGRESS (1 << 0)
++test_expect_success 'write with the --no-progress option' '
++ git multi-pack-index --object-dir=$objdir --no-progress write 2>err &&
++ test_line_count = 0 err
++'
+
- struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local);
- int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, uint32_t pack_int_id);
- int bsearch_midx(const struct object_id *oid, struct multi_pack_index *m, uint32_t *result);
-@@
-
- int write_midx_file(const char *object_dir);
- void clear_midx_file(struct repository *r);
--int verify_midx_file(struct repository *r, const char *object_dir);
-+int verify_midx_file(struct repository *r, const char *object_dir, unsigned flags);
- int expire_midx_packs(struct repository *r, const char *object_dir);
--int midx_repack(struct repository *r, const char *object_dir, size_t batch_size);
-+int midx_repack(struct repository *r, const char *object_dir, size_t batch_size, unsigned flags);
-
- void close_midx(struct multi_pack_index *m);
-
-
- diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
- --- a/t/t5319-multi-pack-index.sh
- +++ b/t/t5319-multi-pack-index.sh
+ test_expect_success 'add more packs' '
+ for j in $(test_seq 11 20)
+ do
@@
git multi-pack-index verify --object-dir=$objdir
'
@@ -234,20 +156,51 @@
'
+test_expect_success 'repack progress off for redirected stderr' '
-+ git multi-pack-index repack --object-dir=$objdir 2>err &&
++ git multi-pack-index --object-dir=$objdir repack 2>err &&
+ test_line_count = 0 err
+'
+
+test_expect_success 'repack force progress on for stderr' '
-+ git multi-pack-index repack --object-dir=$objdir --progress 2>err &&
++ git multi-pack-index --object-dir=$objdir --progress repack 2>err &&
+ test_file_not_empty err
+'
+
+test_expect_success 'repack with the --no-progress option' '
-+ git multi-pack-index repack --object-dir=$objdir --no-progress 2>err &&
++ git multi-pack-index --object-dir=$objdir --no-progress repack 2>err &&
+ test_line_count = 0 err
+'
+
test_expect_success 'repack removes multi-pack-index' '
test_path_is_file $objdir/pack/multi-pack-index &&
GIT_TEST_MULTI_PACK_INDEX=0 git repack -adf &&
+@@
+ )
+ '
+
++test_expect_success 'expire progress off for redirected stderr' '
++ (
++ cd dup &&
++ git multi-pack-index expire 2>err &&
++ test_line_count = 0 err
++ )
++'
++
++test_expect_success 'expire force progress on for stderr' '
++ (
++ cd dup &&
++ git multi-pack-index --progress expire 2>err &&
++ test_file_not_empty err
++ )
++'
++
++test_expect_success 'expire with the --no-progress option' '
++ (
++ cd dup &&
++ git multi-pack-index --no-progress expire 2>err &&
++ test_line_count = 0 err
++ )
++'
++
+ test_expect_success 'expire removes unreferenced packs' '
+ (
+ cd dup &&Signature · 1 line
gitgitgadget
PATCH v2 1/6 midx: add MIDX_PROGRESS flag Add the MIDX_PROGRESS flag and update the write|verify|expire|repack functions in midx.h to accept a flags parameter. The MIDX_PROGRESS flag indicates whether the caller of the function would like progress information to be displayed. This patch only changes the method prototypes and does not change the functionality. The functionality change will be handled by a later patch.
From: William Baker <[email protected]>
--- builtin/multi-pack-index.c | 8 ++++---- builtin/repack.c | 2 +- midx.c | 8 ++++---- midx.h | 10 ++++++---- 4 files changed, 15 insertions(+), 13 deletions(-)
builtin/multi-pack-index.c +4 -4
@@ -47,16 +47,16 @@ int cmd_multi_pack_index(int argc, const char **argv,trace2_cmd_mode (argv[0 ]);if (!strcmp (argv[0 ],"repack" )) -return midx_repack (the_repository, opts.object_dir, (size_t )opts.batch_size); +return midx_repack (the_repository, opts.object_dir, (size_t )opts.batch_size,0 );if (opts.batch_size)die (_ ("--batch-size option is only for 'repack' subcommand" ));if (!strcmp (argv[0 ],"write" )) -return write_midx_file (opts.object_dir); +return write_midx_file (opts.object_dir,0 );if (!strcmp (argv[0 ],"verify" )) -return verify_midx_file (the_repository, opts.object_dir); +return verify_midx_file (the_repository, opts.object_dir,0 );if (!strcmp (argv[0 ],"expire" )) -return expire_midx_packs (the_repository, opts.object_dir); +return expire_midx_packs (the_repository, opts.object_dir,0 );die (_ ("unrecognized subcommand: %s" ), argv[0 ]); }
builtin/repack.c +1 -1
@@ -561,7 +561,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)remove_temporary_files ();if (git_env_bool (GIT_TEST_MULTI_PACK_INDEX ,0 )) -write_midx_file (get_object_directory ()); +write_midx_file (get_object_directory (),0 );string_list_clear (&names,0 );string_list_clear (&rollback,0 );
midx.c +4 -4
@@ -1017,7 +1017,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *return result; } -int write_midx_file (const char *object_dir) +int write_midx_file (const char *object_dir,unsigned flags) {return write_midx_internal (object_dir,NULL ,NULL ); }
@@ -1077,7 +1077,7 @@ static int compare_pair_pos_vs_id(const void *_a, const void *_b)display_progress (progress, _n); \ }while (0 ) -int verify_midx_file(struct repository *r, const char *object_dir) +int verify_midx_file(struct repository *r, const char *object_dir, unsigned flags) { struct pair_pos_vs_id *pairs = NULL;uint32_t i;
@@ -1184,7 +1184,7 @@ int verify_midx_file(struct repository *r, const char *object_dir)return verify_midx_error; } -int expire_midx_packs (struct repository *r,const char *object_dir) +int expire_midx_packs (struct repository *r,const char *object_dir,unsigned flags) {uint32_t i, *count, result =0 ;struct string_list packs_to_drop =STRING_LIST_INIT_DUP ;
@@ -1316,7 +1316,7 @@ static int fill_included_packs_batch(struct repository *r,return 0 ; } -int midx_repack (struct repository *r,const char *object_dir,size_t batch_size) +int midx_repack (struct repository *r,const char *object_dir,size_t batch_size,unsigned flags) {int result =0 ;uint32_t i;
midx.h +6 -4
@@ -37,6 +37,8 @@ struct multi_pack_index {char object_dir[FLEX_ARRAY ]; }; +#define MIDX_PROGRESS (1 << 0) +struct multi_pack_index *load_multi_pack_index (const char *object_dir,int local);int prepare_midx_pack (struct repository *r,struct multi_pack_index *m,uint32_t pack_int_id);int bsearch_midx (const struct object_id *oid,struct multi_pack_index *m,uint32_t *result);
@@ -47,11 +49,11 @@ int fill_midx_entry(struct repository *r, const struct object_id *oid, struct paint midx_contains_pack (struct multi_pack_index *m,const char *idx_or_pack_name);int prepare_multi_pack_index_one (struct repository *r,const char *object_dir,int local); -int write_midx_file (const char *object_dir); +int write_midx_file (const char *object_dir,unsigned flags);void clear_midx_file (struct repository *r); -int verify_midx_file (struct repository *r,const char *object_dir); -int expire_midx_packs (struct repository *r,const char *object_dir); -int midx_repack (struct repository *r,const char *object_dir,size_t batch_size); +int verify_midx_file (struct repository *r,const char *object_dir,unsigned flags); +int expire_midx_packs (struct repository *r,const char *object_dir,unsigned flags); +int midx_repack (struct repository *r,const char *object_dir,size_t batch_size,unsigned flags);void close_midx (struct multi_pack_index *m);
Signature · 1 line
gitgitgadget
PATCH v2 5/6 midx: honor the MIDX_PROGRESS flag in midx_repack Update midx_repack to only display progress when the MIDX_PROGRESS flag is set.
In reply to William Baker via GitGitGadgetFrom: William Baker <[email protected]>
--- midx.c | 6 ++++++ 1 file changed, 6 insertions(+)
midx.c +6 -0
@@ -1374,6 +1374,12 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,strbuf_addstr (&base_name, object_dir);strbuf_addstr (&base_name,"/pack/pack" );argv_array_push (&cmd.args, base_name.buf); + +if (flags &MIDX_PROGRESS ) +argv_array_push (&cmd.args,"--progress" ); +else +argv_array_push (&cmd.args,"-q" ); +strbuf_release (&base_name); cmd.git_cmd =1 ;
Signature · 1 line
gitgitgadget
PATCH v2 4/6 midx: honor the MIDX_PROGRESS flag in verify_midx_file Update verify_midx_file to only display progress when the MIDX_PROGRESS flag is set.
In reply to William Baker via GitGitGadgetFrom: William Baker <[email protected]>
--- midx.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
midx.c +12 -8
@@ -1098,15 +1098,16 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag {struct pair_pos_vs_id *pairs =NULL ;uint32_t i; -struct progress *progress; +struct progress *progress =NULL ;struct multi_pack_index *m =load_multi_pack_index (object_dir,1 ); verify_midx_error =0 ;if (!m)return 0 ; - progress =start_progress (_ ("Looking for referenced packfiles" ), - m->num_packs); +if (flags &MIDX_PROGRESS ) + progress =start_progress (_ ("Looking for referenced packfiles" ), + m->num_packs);for (i =0 ; i < m->num_packs; i++) {if (prepare_midx_pack (r, m, i))midx_report ("failed to load pack in position %d" , i);
@@ -1124,8 +1125,9 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag i, oid_fanout1, oid_fanout2, i + 1); } - progress =start_sparse_progress (_ ("Verifying OID order in MIDX" ), - m->num_objects -1 ); +if (flags &MIDX_PROGRESS ) + progress =start_sparse_progress (_ ("Verifying OID order in multi-pack-index" ), + m->num_objects -1 );for (i =0 ; i < m->num_objects -1 ; i++) {struct object_id oid1, oid2;
@@ -1152,13 +1154,15 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag pairs[i].pack_int_id =nth_midxed_pack_int_id (m, i); } - progress =start_sparse_progress (_ ("Sorting objects by packfile" ), - m->num_objects); +if (flags &MIDX_PROGRESS ) + progress =start_sparse_progress (_ ("Sorting objects by packfile" ), + m->num_objects);display_progress (progress,0 );/* TODO: Measure QSORT() progress */ QSORT (pairs, m->num_objects, compare_pair_pos_vs_id);stop_progress (&progress); - progress =start_sparse_progress (_ ("Verifying object offsets" ), m->num_objects); +if (flags &MIDX_PROGRESS ) + progress =start_sparse_progress (_ ("Verifying object offsets" ), m->num_objects);for (i =0 ; i < m->num_objects; i++) {struct object_id oid;struct pack_entry e;
Signature · 1 line
gitgitgadget
PATCH v2 2/6 midx: add progress to write_midx_file Add progress to write_midx_file. Progress is displayed when the MIDX_PROGRESS flag is set.
In reply to William Baker via GitGitGadgetFrom: William Baker <[email protected]>
--- midx.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-)
midx.c +21 -4
@@ -449,6 +449,8 @@ struct pack_list {uint32_t nr;uint32_t alloc;struct multi_pack_index *m; +struct progress *progress; +uint32_t pack_paths_checked; }; static void add_pack_to_midx(const char *full_path, size_t full_path_len,
@@ -457,6 +459,7 @@ static void add_pack_to_midx(const char *full_path, size_t full_path_len,struct pack_list *packs = (struct pack_list *)data;if (ends_with (file_name,".idx" )) { +display_progress (packs->progress, ++packs->pack_paths_checked);if (packs->m &&midx_contains_pack (packs->m, file_name))return ;
@@ -786,7 +789,7 @@ static size_t write_midx_large_offsets(struct hashfile *f, uint32_t nr_large_off }static int write_midx_internal (const char *object_dir,struct multi_pack_index *m, -struct string_list *packs_to_drop) +struct string_list *packs_to_drop,unsigned flags) {unsigned char cur_chunk, num_chunks =0 ;char *midx_name;
@@ -800,6 +803,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *uint64_t chunk_offsets[MIDX_MAX_CHUNKS +1 ];uint32_t nr_entries, num_large_offsets =0 ;struct pack_midx_entry *entries =NULL ; +struct progress *progress =NULL ;int large_offsets_needed =0 ;int pack_name_concat_len =0 ;int dropped_packs =0 ;
@@ -833,8 +837,15 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index * packs.nr++; } } + + packs.pack_paths_checked =0 ; +if (flags &MIDX_PROGRESS ) + packs.progress =start_progress (_ ("Adding packfiles to multi-pack-index" ),0 ); +else + packs.progress =NULL ;for_each_file_in_pack_dir (object_dir, add_pack_to_midx, &packs); +stop_progress (&packs.progress);if (packs.m && packs.nr == packs.m->num_packs && !packs_to_drop) goto cleanup;
@@ -959,6 +970,9 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index * written +=MIDX_CHUNKLOOKUP_WIDTH ; } +if (flags &MIDX_PROGRESS ) + progress =start_progress (_ ("Writing chunks to multi-pack-index" ), + num_chunks);for (i =0 ; i < num_chunks; i++) {if (written != chunk_offsets[i]) BUG("incorrect chunk offset (%"PRIu64" != %"PRIu64") for chunk id %"PRIx32,
@@ -991,7 +1005,10 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *BUG ("trying to write unknown chunk id %" PRIx32, chunk_ids[i]); } + +display_progress (progress, i +1 ); } +stop_progress (&progress);if (written != chunk_offsets[num_chunks]) BUG("incorrect final offset %"PRIu64" != %"PRIu64,
@@ -1019,7 +1036,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *int write_midx_file (const char *object_dir,unsigned flags) { -return write_midx_internal (object_dir,NULL ,NULL ); +return write_midx_internal (object_dir,NULL ,NULL , flags); }void clear_midx_file (struct repository *r)
@@ -1222,7 +1239,7 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned flafree (count);if (packs_to_drop.nr) - result =write_midx_internal (object_dir, m, &packs_to_drop); + result =write_midx_internal (object_dir, m, &packs_to_drop, flags);string_list_clear (&packs_to_drop,0 );return result;
@@ -1371,7 +1388,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size, goto cleanup; } - result =write_midx_internal (object_dir, m,NULL ); + result =write_midx_internal (object_dir, m,NULL , flags); m =NULL ; cleanup:
Signature · 1 line
gitgitgadget
PATCH v2 3/6 midx: add progress to expire_midx_packs Add progress to expire_midx_packs. Progress is displayed when the MIDX_PROGRESS flag is set.
In reply to William Baker via GitGitGadgetFrom: William Baker <[email protected]>
--- midx.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
midx.c +12 -0
@@ -1206,18 +1206,29 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned flauint32_t i, *count, result =0 ;struct string_list packs_to_drop =STRING_LIST_INIT_DUP ;struct multi_pack_index *m =load_multi_pack_index (object_dir,1 ); +struct progress *progress =NULL ;if (!m)return 0 ; count =xcalloc (m->num_packs,sizeof (uint32_t )); + +if (flags &MIDX_PROGRESS ) + progress =start_progress (_ ("Counting referenced objects" ), + m->num_objects);for (i =0 ; i < m->num_objects; i++) {int pack_int_id =nth_midxed_pack_int_id (m, i); count[pack_int_id]++; +display_progress (progress, i +1 ); } +stop_progress (&progress); +if (flags &MIDX_PROGRESS ) + progress =start_progress (_ ("Finding and deleting unreferenced packfiles" ), + m->num_packs);for (i =0 ; i < m->num_packs; i++) {char *pack_name; +display_progress (progress, i +1 );if (count[i])continue ;
@@ -1235,6 +1246,7 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned flaunlink_pack_path (pack_name,0 );free (pack_name); } +stop_progress (&progress);free (count);
Signature · 1 line
gitgitgadget
PATCH v2 6/6 multi-pack-index: add [--[no-]progress] option. Add the --[no-]progress option to git multi-pack-index. Pass the MIDX_PROGRESS flag to the subcommand functions when progress should be displayed by multi-pack-index. The progress feature was added to 'verify' in 144d703 ("multi-pack-index: report progress during 'verify'", 2018-09-13) but some subcommands were not updated to display progress, and the ability to opt-out was overlooked.
In reply to William Baker via GitGitGadgetFrom: William Baker <[email protected]>
--- Documentation/git-multi-pack-index.txt | 6 ++- builtin/multi-pack-index.c | 18 +++++-- t/t5319-multi-pack-index.sh | 69 ++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 6 deletions(-)
Documentation/git-multi-pack-index.txt +5 -1
@@ -9,7 +9,7 @@ git-multi-pack-index - Write and verify multi-pack-indexes SYNOPSIS -------- [verse] -'git multi-pack-index' [--object-dir=<dir>] <subcommand> +'git multi-pack-index' [--object-dir=<dir>] [--[no-]progress] <subcommand> DESCRIPTION -----------
@@ -23,6 +23,10 @@ OPTIONS `<dir>/packs/multi-pack-index` for the current MIDX file, and `<dir>/packs` for the pack-files to index. +--[no-]progress:: + Turn progress on/off explicitly. If neither is specified, progress is + shown if standard error is connected to a terminal. + The following subcommands are available: write::
builtin/multi-pack-index.c +13 -5
@@ -6,21 +6,25 @@#include "trace2.h" static char const *const builtin_multi_pack_index_usage[] = { -N_ ("git multi-pack-index [--object-dir=<dir>] (write|verify|expire|repack --batch-size=<size>)" ), +N_ ("git multi-pack-index [<options>] (write|verify|expire|repack --batch-size=<size>)" ),NULL };static struct opts_multi_pack_index {const char *object_dir;unsigned long batch_size; +int progress; } opts;int cmd_multi_pack_index (int argc,const char **argv,const char *prefix) { +unsigned flags =0 ; +static struct option builtin_multi_pack_index_options[] = {OPT_FILENAME (0 ,"object-dir" , &opts.object_dir,N_ ("object directory containing set of packfile and pack-index pairs" )), +OPT_BOOL (0 ,"progress" , &opts.progress,N_ ("force progress reporting" )),OPT_MAGNITUDE (0 ,"batch-size" , &opts.batch_size,N_ ("during repack, collect pack-files of smaller size into a batch that is larger than this size" )),OPT_END (),
@@ -28,12 +32,15 @@ int cmd_multi_pack_index(int argc, const char **argv,git_config (git_default_config,NULL ); + opts.progress =isatty (2 ); argc =parse_options (argc, argv, prefix, builtin_multi_pack_index_options, builtin_multi_pack_index_usage,0 );if (!opts.object_dir) opts.object_dir =get_object_directory (); +if (opts.progress) + flags |=MIDX_PROGRESS ;if (argc ==0 ) usage_with_options(builtin_multi_pack_index_usage,
@@ -47,16 +54,17 @@ int cmd_multi_pack_index(int argc, const char **argv,trace2_cmd_mode (argv[0 ]);if (!strcmp (argv[0 ],"repack" )) -return midx_repack (the_repository, opts.object_dir, (size_t )opts.batch_size,0 ); +return midx_repack (the_repository, opts.object_dir, + (size_t )opts.batch_size, flags);if (opts.batch_size)die (_ ("--batch-size option is only for 'repack' subcommand" ));if (!strcmp (argv[0 ],"write" )) -return write_midx_file (opts.object_dir,0 ); +return write_midx_file (opts.object_dir, flags);if (!strcmp (argv[0 ],"verify" )) -return verify_midx_file (the_repository, opts.object_dir,0 ); +return verify_midx_file (the_repository, opts.object_dir, flags);if (!strcmp (argv[0 ],"expire" )) -return expire_midx_packs (the_repository, opts.object_dir,0 ); +return expire_midx_packs (the_repository, opts.object_dir, flags);die (_ ("unrecognized subcommand: %s" ), argv[0 ]); }
t/t5319-multi-pack-index.sh +69 -0
93 lines not on this page · raw message
Signature · 1 line
gitgitgadget
Quoted 17 lines · William Baker via GitGitGadget
William Baker (6):
midx: addMIDX_PROGRESSflag Add theMIDX_PROGRESSflag and update the
write|verify|expire|repack functions inmidx.hto accept a flags parameter. TheMIDX_PROGRESSflag indicates whether the caller of the function would like progress information to be displayed. This patch only changes the method prototypes and does not change the functionality. The functionality change will be handled by a later patch.
midx: add progress towrite_midx_fileAdd progress towrite_midx_file.
Progress is displayed when theMIDX_PROGRESSflag is set.
midx: add progress toexpire_midx_packsAdd progress to
expire_midx_packs. Progress is displayed when theMIDX_PROGRESSflag is set.
midx: honor theMIDX_PROGRESSflag inverify_midx_fileUpdate
verify_midx_fileto only display progress when theMIDX_PROGRESSflag is set.
midx: honor theMIDX_PROGRESSflag inmidx_repackUpdatemidx_repack
Quoted 8 lines
to only display progress when the
MIDX_PROGRESSflag is set.
multi-pack-index: add [--[no-]progress] option. Add the
--[no-]progress option to git multi-pack-index. Pass theMIDX_PROGRESSflag to the subcommand functions when progress should be displayed by multi-pack-index. The progress feature was added to 'verify' in 144d703 ("multi-pack-index: report progress during 'verify'", 2018-09-13) but some subcommands were not updated to display progress, and the ability to opt-out was overlooked.
Do all of these commits have overly long title with no body recorded in the commit objects, or is this just a mail sending program screwing up?
Re: [PATCH v2 1/6] midx: add MIDX_PROGRESS flag Add the MIDX_PROGRESS flag and update the write|verify|expire|repack functions in midx.h to accept a flags parameter. The MIDX_PROGRESS flag indicates whether the caller of the function would like progress information to be displayed. This patch only changes the method prototypes and does not change the functionality. The functionality change will be handled by a later patch.
In reply to William Baker via GitGitGadgetQuoted 22 lines · William Baker via GitGitGadget
From: William Baker <[email protected]>
Signed-off-by William Baker <[email protected]>--- builtin/multi-pack-index.c | 8 ++++---- builtin/repack.c | 2 +- midx.c | 8 ++++---- midx.h | 10 ++++++---- 4 files changed, 15 insertions(+), 13 deletions(-)builtin/multi-pack-index.c +1
-1@@ -47,16 +47,16 @@ int cmd_multi_pack_index(int argc, const char **argv,trace2_cmd_mode (argv[0 ]);if (!strcmp (argv[0 ],"repack" )) -return midx_repack (the_repository, opts.object_dir, (size_t )opts.batch_size); +return midx_repack (the_repository, opts.object_dir, (size_t )opts.batch_size,0 );if (opts.batch_size)die (_ ("--batch-size option is only for 'repack' subcommand" ));
builtin/multi-pack-index.c int cmd_multi_pack_index(int argc, const char **argv, · line 59
Quoted 10 lines
builtin/multi-pack-index.c +3
-3if (!strcmp (argv[0 ],"write" )) -return write_midx_file (opts.object_dir); +return write_midx_file (opts.object_dir,0 );if (!strcmp (argv[0 ],"verify" )) -return verify_midx_file (the_repository, opts.object_dir); +return verify_midx_file (the_repository, opts.object_dir,0 );if (!strcmp (argv[0 ],"expire" )) -return expire_midx_packs (the_repository, opts.object_dir); +return expire_midx_packs (the_repository, opts.object_dir,0 );
Hmm, I actually would have expected to see a new .progress field in the opts structure and these lower-level functions would start taking a pointer to the whole opts structure, instead of just a pointer to a single member opts.object_dir. That way, we can later extend and enrich the interface between this caller and the underlying functions without much patch noise in the dispatch layer (i.e. here).
This step is meant to be just preparing by extending the interface and passing the new argument throughout the callchain, I believe, and it looks correctly done, assuming that we are OK to take this "add a separate 'progress' parameter, when we need more parameters later, the interface will gain more and more parameters" approach.
Re: [PATCH v2 2/6] midx: add progress to write_midx_file Add progress to write_midx_file. Progress is displayed when the MIDX_PROGRESS flag is set.
In reply to William Baker via GitGitGadgetmidx.c struct pack_list { · line 454
Quoted 11 lines · William Baker via GitGitGadget
midx.c +2
-0@@ -449,6 +449,8 @@ struct pack_list {uint32_t nr;uint32_t alloc;struct multi_pack_index *m; +struct progress *progress; +uint32_t pack_paths_checked; };
What is the reason behind u32 here? Do we want to be consistently limited to 4G on all platforms?
I have a feeling that we do not care too deeply all that much for the purpose of progress output, in which case, just an ordinary "unsigned" may be much less misleading.
FWIW, the function that uses this field is display_progress(), which takes uint64_t there.
midx.c static void add_pack_to_midx(const char *full_path, size_t full_path_len, · line 462
Quoted 5 lines
midx.c +1
-0@@ -457,6 +459,7 @@ static void add_pack_to_midx(const char *full_path, size_t full_path_len,struct pack_list *packs = (struct pack_list *)data;if (ends_with (file_name,".idx" )) { +display_progress (packs->progress, ++packs->pack_paths_checked);
OK, "git grep display_progress" tells me that we are expected to pass the value of the counter that is already incremented, so this is also in line with that practice.
midx.c static int write_midx_internal(const char *object_dir, struct multi_pack_index * · line 848
Quoted 7 lines
midx.c +5
-0+if (flags &MIDX_PROGRESS ) + packs.progress =start_progress (_ ("Adding packfiles to multi-pack-index" ),0 ); +else + packs.progress =NULL ;for_each_file_in_pack_dir (object_dir, add_pack_to_midx, &packs); +stop_progress (&packs.progress);
OK.
midx.c static int write_midx_internal(const char *object_dir, struct multi_pack_index * · line 1011
Quoted 9 lines
midx.c +6
-0+if (flags &MIDX_PROGRESS ) + progress =start_progress (_ ("Writing chunks to multi-pack-index" ), + num_chunks); + +display_progress (progress, i +1 ); } +stop_progress (&progress);
OK.
Re: [PATCH v2 5/6] midx: honor the MIDX_PROGRESS flag in midx_repack Update midx_repack to only display progress when the MIDX_PROGRESS flag is set.
In reply to William Baker via GitGitGadgetSteps 4 & 5 look OK.
Re: [PATCH v2 1/6] midx: add MIDX_PROGRESS flag Add the MIDX_PROGRESS flag and update the write|verify|expire|repack functions in midx.h to accept a flags parameter. The MIDX_PROGRESS flag indicates whether the caller of the function would like progress information to be displayed. This patch only changes the method prototypes and does not change the functionality. The functionality change will be handled by a later patch.
In reply to Junio C HamanoQuoted 5 lines · Junio C Hamano
This step is meant to be just preparing by extending the interface and passing the new argument throughout the callchain, I believe, and it looks correctly done, assuming that we are OK to take this "add a separate 'progress' parameter, when we need more parameters later, the interface will gain more and more parameters" approach.
After re-reading the remainder of the series, I think the structure this patch series takes (i.e. adding a new parameter to these callees) is better than the alternative (i.e. making sure everybody takes the pointer to the opts structure).
Thanks. I couldn't review the proposed log messages of these commits, which were unreadable, at all, though.
Hi William, welcome.
Quoted 37 lines · William Baker via GitGitGadget
Hello Git contributors!
My name is William Baker and I work at Microsoft. Over the past few years I've worked closely with the Microsoft team contributing to the git ecosystem and I'm excited to start working with the community.
Derrick Stolee helped me pick out my first task, and it's to add support for opting out of the multi-pack-index progress output. The progress feature was introduced in
144d7033("multi-pack-index: report progress during 'verify'", 2018-09-13) but it did not include the ability to opt-out. This patch adds the --[no-]progress option to allow callers control whether progress is reported.This path is very similar to Garima Singh's series for progress reporting in the commit-graph [1].
[1] https://public-inbox.org/git/[email protected]/
I'm looking forward to your review. Thanks! William Baker
William Baker (6):
midx: addMIDX_PROGRESSflag Add theMIDX_PROGRESSflag and update the
write|verify|expire|repack functions inmidx.hto accept a flags parameter. TheMIDX_PROGRESSflag indicates whether the caller of the function would like progress information to be displayed. This patch only changes the method prototypes and does not change the functionality. The functionality change will be handled by a later patch.
midx: add progress towrite_midx_fileAdd progress towrite_midx_file.
Progress is displayed when theMIDX_PROGRESSflag is set.
midx: add progress toexpire_midx_packsAdd progress to
expire_midx_packs. Progress is displayed when theMIDX_PROGRESSflag is set.
midx: honor theMIDX_PROGRESSflag inverify_midx_fileUpdate
verify_midx_fileto only display progress when theMIDX_PROGRESSflag is set.
midx: honor theMIDX_PROGRESSflag inmidx_repackUpdatemidx_repack
Quoted 8 lines
to only display progress when the
MIDX_PROGRESSflag is set.
multi-pack-index: add [--[no-]progress] option. Add the
--[no-]progress option to git multi-pack-index. Pass theMIDX_PROGRESSflag to the subcommand functions when progress should be displayed by multi-pack-index. The progress feature was added to 'verify' in 144d703 ("multi-pack-index: report progress during 'verify'", 2018-09-13) but some subcommands were not updated to display progress, and the ability to opt-out was overlooked.
These subject lines are a bit long. Has there been some white space damage during preparation, such that the end of line, and following blank line, for a short subject line been compromised?
Normally the 'git log --oneline' should fit within the line, so ~50 chars.
Philip
Quoted 287 lines
Documentation/git-multi-pack-index.txt | 6 ++- builtin/multi-pack-index.c | 18 +++++-- builtin/repack.c | 2 +- midx.c | 71 ++++++++++++++++++++------ midx.h | 10 ++-- t/t5319-multi-pack-index.sh | 69 +++++++++++++++++++++++++ 6 files changed, 149 insertions(+), 27 deletions(-)base-commit:
5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-337%2Fwilbaker%2Fmulti-pack-index-progress-toggle-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-337/wilbaker/multi-pack-index-progress-toggle-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/337Range-diff vs v1:
-: ---------- > 1: 6badd9ceaf midx: add MIDX_PROGRESS flag Add the MIDX_PROGRESS flag and update the write|verify|expire|repack functions in midx.h to accept a flags parameter. The MIDX_PROGRESS flag indicates whether the caller of the function would like progress information to be displayed. This patch only changes the method prototypes and does not change the functionality. The functionality change will be handled by a later patch. -: ---------- > 2: 3bc8677ea7 midx: add progress to write_midx_file Add progress to write_midx_file. Progress is displayed when the MIDX_PROGRESS flag is set. -: ---------- > 3: 3374574001 midx: add progress to expire_midx_packs Add progress to expire_midx_packs. Progress is displayed when the MIDX_PROGRESS flag is set. -: ---------- > 4: 29d03771c0 midx: honor the MIDX_PROGRESS flag in verify_midx_file Update verify_midx_file to only display progress when the MIDX_PROGRESS flag is set. -: ---------- > 5: 57f6742f09 midx: honor the MIDX_PROGRESS flag in midx_repack Update midx_repack to only display progress when the MIDX_PROGRESS flag is set. 1: 0821a8073a ! 6: 5b933ab744 multi-pack-index: add --no-progress Add --no-progress option to git multi-pack-index. The progress feature was added in 144d703 ("multi-pack-index: report progress during 'verify'", 2018-09-13) but the ability to opt-out was overlooked. @@ -1,10 +1,13 @@ Author: William Baker <[email protected]> - multi-pack-index: add --no-progress - Add --no-progress option to git multi-pack-index. - The progress feature was added in 144d703 + multi-pack-index: add [--[no-]progress] option. + Add the --[no-]progress option to git multi-pack-index. + Pass the MIDX_PROGRESS flag to the subcommand functions + when progress should be displayed by multi-pack-index. + The progress feature was added to 'verify' in 144d703 ("multi-pack-index: report progress during 'verify'", 2018-09-13) - but the ability to opt-out was overlooked. + but some subcommands were not updated to display progress, and + the ability to opt-out was overlooked. Signed-off-by: William Baker <[email protected]> @@ -16,7 +19,7 @@ -------- [verse] -'git multi-pack-index' [--object-dir=<dir>] <subcommand> -+'git multi-pack-index' [--object-dir=<dir>] <subcommand> [--[no-]progress] ++'git multi-pack-index' [--object-dir=<dir>] [--[no-]progress] <subcommand> DESCRIPTION ----------- @@ -40,7 +43,7 @@ static char const * const builtin_multi_pack_index_usage[] = { - N_("git multi-pack-index [--object-dir=<dir>] (write|verify|expire|repack --batch-size=<size>)"), -+ N_("git multi-pack-index [--object-dir=<dir>] (write|verify|expire|repack --batch-size=<size>) [--[no-]progress]"), ++ N_("git multi-pack-index [<options>] (write|verify|expire|repack --batch-size=<size>)"), NULL }; @@ -58,11 +61,11 @@ static struct option builtin_multi_pack_index_options[] = { OPT_FILENAME(0, "object-dir", &opts.object_dir, N_("object directory containing set of packfile and pack-index pairs")), ++ OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")), OPT_MAGNITUDE(0, "batch-size", &opts.batch_size, N_("during repack, collect pack-files of smaller size into a batch that is larger than this size")), -+ OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")), OPT_END(), - }; +@@ git_config(git_default_config, NULL); @@ -82,131 +85,50 @@ trace2_cmd_mode(argv[0]); if (!strcmp(argv[0], "repack")) -- return midx_repack(the_repository, opts.object_dir, (size_t)opts.batch_size); +- return midx_repack(the_repository, opts.object_dir, (size_t)opts.batch_size, 0); + return midx_repack(the_repository, opts.object_dir, + (size_t)opts.batch_size, flags); if (opts.batch_size) die(_("--batch-size option is only for 'repack' subcommand")); if (!strcmp(argv[0], "write")) - return write_midx_file(opts.object_dir); +- return write_midx_file(opts.object_dir, 0); ++ return write_midx_file(opts.object_dir, flags); if (!strcmp(argv[0], "verify")) -- return verify_midx_file(the_repository, opts.object_dir); +- return verify_midx_file(the_repository, opts.object_dir, 0); + return verify_midx_file(the_repository, opts.object_dir, flags); if (!strcmp(argv[0], "expire")) - return expire_midx_packs(the_repository, opts.object_dir); +- return expire_midx_packs(the_repository, opts.object_dir, 0); ++ return expire_midx_packs(the_repository, opts.object_dir, flags); + die(_("unrecognized subcommand: %s"), argv[0]); + } - diff --git a/midx.c b/midx.c - --- a/midx.c - +++ b/midx.c -@@ - display_progress(progress, _n); \ - } while (0) - --int verify_midx_file(struct repository *r, const char *object_dir) -+int verify_midx_file(struct repository *r, const char *object_dir, unsigned flags) - { - struct pair_pos_vs_id *pairs = NULL; - uint32_t i; -- struct progress *progress; -+ struct progress *progress = NULL; - struct multi_pack_index *m = load_multi_pack_index(object_dir, 1); - verify_midx_error = 0; - - if (!m) - return 0; - -- progress = start_progress(_("Looking for referenced packfiles"), -- m->num_packs); -+ if (flags & MIDX_PROGRESS) -+ progress = start_progress(_("Looking for referenced packfiles"), -+ m->num_packs); - for (i = 0; i < m->num_packs; i++) { - if (prepare_midx_pack(r, m, i)) - midx_report("failed to load pack in position %d", i); + diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh + --- a/t/t5319-multi-pack-index.sh + +++ b/t/t5319-multi-pack-index.sh @@ - i, oid_fanout1, oid_fanout2, i + 1); - } - -- progress = start_sparse_progress(_("Verifying OID order in MIDX"), -- m->num_objects - 1); -+ if (flags & MIDX_PROGRESS) -+ progress = start_sparse_progress(_("Verifying OID order in MIDX"), -+ m->num_objects - 1); - for (i = 0; i < m->num_objects - 1; i++) { - struct object_id oid1, oid2; -@@ - pairs[i].pack_int_id = nth_midxed_pack_int_id(m, i); - } - -- progress = start_sparse_progress(_("Sorting objects by packfile"), -- m->num_objects); -+ if (flags & MIDX_PROGRESS) -+ progress = start_sparse_progress(_("Sorting objects by packfile"), -+ m->num_objects); - display_progress(progress, 0); /* TODO: Measure QSORT() progress */ - QSORT(pairs, m->num_objects, compare_pair_pos_vs_id); - stop_progress(&progress); - -- progress = start_sparse_progress(_("Verifying object offsets"), m->num_objects); -+ if (flags & MIDX_PROGRESS) -+ progress = start_sparse_progress(_("Verifying object offsets"), m->num_objects); - for (i = 0; i < m->num_objects; i++) { - struct object_id oid; - struct pack_entry e; -@@ - return 0; - } + compare_results_with_midx "two packs" --int midx_repack(struct repository *r, const char *object_dir, size_t batch_size) -+int midx_repack(struct repository *r, const char *object_dir, size_t batch_size, unsigned flags) - { - int result = 0; - uint32_t i; -@@ - strbuf_addstr(&base_name, object_dir); - strbuf_addstr(&base_name, "/pack/pack"); - argv_array_push(&cmd.args, base_name.buf); ++test_expect_success 'write progress off for redirected stderr' ' ++ git multi-pack-index --object-dir=$objdir write 2>err && ++ test_line_count = 0 err ++' + -+ if (flags & MIDX_PROGRESS) -+ argv_array_push(&cmd.args, "--progress"); -+ else -+ argv_array_push(&cmd.args, "-q"); ++test_expect_success 'write force progress on for stderr' ' ++ git multi-pack-index --object-dir=$objdir --progress write 2>err && ++ test_file_not_empty err ++' + - strbuf_release(&base_name); - - cmd.git_cmd = 1; - - diff --git a/midx.h b/midx.h - --- a/midx.h - +++ b/midx.h -@@ - char object_dir[FLEX_ARRAY]; - }; - -+#define MIDX_PROGRESS (1 << 0) ++test_expect_success 'write with the --no-progress option' ' ++ git multi-pack-index --object-dir=$objdir --no-progress write 2>err && ++ test_line_count = 0 err ++' + - struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local); - int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, uint32_t pack_int_id); - int bsearch_midx(const struct object_id *oid, struct multi_pack_index *m, uint32_t *result); -@@ - - int write_midx_file(const char *object_dir); - void clear_midx_file(struct repository *r); --int verify_midx_file(struct repository *r, const char *object_dir); -+int verify_midx_file(struct repository *r, const char *object_dir, unsigned flags); - int expire_midx_packs(struct repository *r, const char *object_dir); --int midx_repack(struct repository *r, const char *object_dir, size_t batch_size); -+int midx_repack(struct repository *r, const char *object_dir, size_t batch_size, unsigned flags); - - void close_midx(struct multi_pack_index *m); - - - diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh - --- a/t/t5319-multi-pack-index.sh - +++ b/t/t5319-multi-pack-index.sh + test_expect_success 'add more packs' ' + for j in $(test_seq 11 20) + do @@ git multi-pack-index verify --object-dir=$objdir ' @@ -234,20 +156,51 @@ ' +test_expect_success 'repack progress off for redirected stderr' ' -+ git multi-pack-index repack --object-dir=$objdir 2>err && ++ git multi-pack-index --object-dir=$objdir repack 2>err && + test_line_count = 0 err +' + +test_expect_success 'repack force progress on for stderr' ' -+ git multi-pack-index repack --object-dir=$objdir --progress 2>err && ++ git multi-pack-index --object-dir=$objdir --progress repack 2>err && + test_file_not_empty err +' + +test_expect_success 'repack with the --no-progress option' ' -+ git multi-pack-index repack --object-dir=$objdir --no-progress 2>err && ++ git multi-pack-index --object-dir=$objdir --no-progress repack 2>err && + test_line_count = 0 err +' + test_expect_success 'repack removes multi-pack-index' ' test_path_is_file $objdir/pack/multi-pack-index && GIT_TEST_MULTI_PACK_INDEX=0 git repack -adf && +@@ + ) + ' + ++test_expect_success 'expire progress off for redirected stderr' ' ++ ( ++ cd dup && ++ git multi-pack-index expire 2>err && ++ test_line_count = 0 err ++ ) ++' ++ ++test_expect_success 'expire force progress on for stderr' ' ++ ( ++ cd dup && ++ git multi-pack-index --progress expire 2>err && ++ test_file_not_empty err ++ ) ++' ++ ++test_expect_success 'expire with the --no-progress option' ' ++ ( ++ cd dup && ++ git multi-pack-index --no-progress expire 2>err && ++ test_line_count = 0 err ++ ) ++' ++ + test_expect_success 'expire removes unreferenced packs' ' + ( + cd dup &&
Quoted 5 lines · Junio C Hamano
Do all of these commits have overly long title with no body recorded in the commit objects, or is this just a mail sending program screwing up?
Sorry about that! With some help from Stolee I found the issue was that I was missing a second line break before the body of the commit.
-William
Quoted 6 lines · Philip Oakley
These subject lines are a bit long. Has there been some white space damage during preparation, such that the end of line, and following blank line, for a short subject line been compromised?
Normally the 'git log --oneline' should fit within the line, so ~50 chars.
Philip
Thanks for the tip regarding 'git log --oneline'! I missed that I need a second end of line before the body of the commit message and I will be sure to include them in future commits.
-William
Re: [PATCH v2 1/6] midx: add MIDX_PROGRESS flag <snip>
In reply to William Baker via GitGitGadgetmidx.h struct multi_pack_index { · line 40
Quoted 9 lines · William Baker via GitGitGadget
midx.h +1
-0@@ -37,6 +37,8 @@ struct multi_pack_index {char object_dir[FLEX_ARRAY ]; }; +#define MIDX_PROGRESS (1 << 0)
Please consider using an enum.
A preprocessor constant is just that: a number. It is shown as a number while debugging, and there is no clear indication what related values belong in the same group (apart from the prefix of the macro name), or what possible values an 'unsigned flags' variable might have (though in this particular case there is only a single possible value...).
An enum, however, is much friendlier to humans when debugging, because even 'gdb' shows the value using the symbolic names, e.g.:
write_commit_graph_reachable (obj_dir=0x9ab870 ".git/objects",
flags=(COMMIT_GRAPH_WRITE_APPEND | COMMIT_GRAPH_WRITE_PROGRESS), split_opts=0x9670e0 <split_opts>) at commit-graph.c:1141
and it's quite clear what values a variable of type 'enum commit_graph_write_flags' can have.
Re: [PATCH v2 2/6] midx: add progress to write_midx_file Add progress to write_midx_file. Progress is displayed when the MIDX_PROGRESS flag is set.
In reply to Junio C HamanoQuoted 15 lines · Junio C Hamano
midx.c struct pack_list { · line 454
Quoted 11 lines
midx.c +2
-0@@ -449,6 +449,8 @@ struct pack_list {uint32_t nr;uint32_t alloc;struct multi_pack_index *m; +struct progress *progress; +uint32_t pack_paths_checked; };What is the reason behind u32 here? Do we want to be consistently limited to 4G on all platforms?
Quoted 3 lines
I have a feeling that we do not care too deeply all that much for the purpose of progress output, in which case, just an ordinary "unsigned" may be much less misleading.
I went with u32 here to match the data type used to track how many entries are in the pack_list ('nr' is a u32).
I could switch to this to an unsigned but we would run the (extremely unlikely) risk of having more than 65k packs on a system where unsigned is 16 bits.
Quoted 2 lines
FWIW, the function that uses this field is
display_progress(), which takesuint64_tthere.
Thanks for pointing this out. Given that display_progress() expects a u64 using that type here for 'pack_paths_checked' could help make things more straightforward.
-William
Re: [PATCH v2 1/6] midx: add MIDX_PROGRESS flag <snip>
In reply to SZEDER GáborQuoted 12 lines · SZEDER Gábor
Quoted 2 lines
+#define
MIDX_PROGRESS(1 << 0)Please consider using an enum.
A preprocessor constant is just that: a number. It is shown as a number while debugging, and there is no clear indication what related values belong in the same group (apart from the prefix of the macro name), or what possible values an 'unsigned flags' variable might have (though in this particular case there is only a single possible value...).
Quoted 2 lines
An enum, however, is much friendlier to humans when debugging, because even 'gdb' shows the value using the symbolic names, e.g.:
Thanks for this suggestion. I agree on the benefits of using an enum here and will make the switch in my next set of changes.
-William
Re: [PATCH v2 1/6] midx: add MIDX_PROGRESS flag <snip>
In reply to SZEDER GáborQuoted 9 lines · SZEDER Gábor
Quoted 8 lines · William Baker via GitGitGadget
midx.h +0
-0@@ -37,6 +37,8 @@ struct multi_pack_index {char object_dir[FLEX_ARRAY ]; };
Quoted 3 lines
midx.h struct multi_pack_index { · line 40
Quoted 1 line
midx.h +1
-0+#define MIDX_PROGRESS (1 << 0)Please consider using an enum.
If they are used by assiging one of their values, definitely a good idea to use an enum. Are debuggers clever enough that they can tell, when they see something like this:
enum gress {
PROGRESS = 1,
REGRESS = 2,
};
void func(enum gress v);
...
void caller(void)
{
func(PROGRESS | REGRESS);
func(PROGRESS + REGRESS);
func(PROGRESS * 3);
}how caller came about to give 3?
Re: [PATCH v2 2/6] midx: add progress to write_midx_file Add progress to write_midx_file. Progress is displayed when the MIDX_PROGRESS flag is set.
In reply to William BakerQuoted 16 lines · William Baker
Quoted 15 lines · Junio C Hamano
midx.c struct pack_list { · line 454
Quoted 11 lines
midx.c +2
-0@@ -449,6 +449,8 @@ struct pack_list {uint32_t nr;uint32_t alloc;struct multi_pack_index *m; +struct progress *progress; +uint32_t pack_paths_checked; };What is the reason behind u32 here? Do we want to be consistently limited to 4G on all platforms?
Quoted 6 lines
Quoted 3 lines
I have a feeling that we do not care too deeply all that much for the purpose of progress output, in which case, just an ordinary "unsigned" may be much less misleading.
I went with u32 here to match the data type used to track how many entries are in the
pack_list('nr' is a u32).
That kind of parallel is valid when you could compare nr with this new thing (or put it differently, the existing nr and this new thing counts the same). Are they both about the number of packs?
Quoted 3 lines
I could switch to this to an unsigned but we would run the (extremely unlikely) risk of having more than 65k packs on a system where unsigned is 16 bits.
Why? If an arch is small enough that the natural integer size is 16-bit, then limiting the total number of packs to 65k sound entirely sensible.
The only reason why you'd want fixed (across platforms and architectures) type is when you want to make sure that a file storing the literal values taken from these fields are portable and everybody is limited the same way. If a platform's natural integer is 64-bit, by artificially limiting the size of this field to u32, you are making disservice to the platform users, and more importantly, you are wasting developers' time by forcing them to wonder if there is a reason behind the choice of u32 (does it really need to be able to store up to 4G, even on a smaller machines? Is it necessary to refuse to store more than 4G? What are the reasons?), like me wondering about these questions and writing them down here.
So, unless there is a reason why this must be of fixed type, I'd say just an unsigned would be the most reasonable choice.
Re: [PATCH v2 2/6] midx: add progress to write_midx_file Add progress to write_midx_file. Progress is displayed when the MIDX_PROGRESS flag is set.
Quoted 15 lines · Junio C Hamano
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
midx.c +0
-0@@ -449,6 +449,8 @@ struct pack_list {uint32_t nr;Quoted 8 lines
Quoted 5 lines
midx.c struct pack_list { · line 454
Quoted 5 lines
midx.c +2
-0uint32_t alloc;struct multi_pack_index *m; +struct progress *progress; +uint32_t pack_paths_checked; };I went with u32 here to match the data type used to track how many entries are in the
pack_list('nr' is a u32).
Quoted 4 lines
That kind of parallel is valid when you could compare nr with this new thing (or put it differently, the existing nr and this new thing counts the same). Are they both about the number of packs?
Both 'nr' and 'pack_paths_checked' are about the number of packs. 'nr' tracks the number of packs in the multi-pack-index and it grows as add_pack_to_midx() finds new packs to add. 'pack_paths_checked' is the number of pack ".idx" files that have been checked by add_pack_to_midx().
Quoted 19 lines
Quoted 3 lines
I could switch to this to an unsigned but we would run the (extremely unlikely) risk of having more than 65k packs on a system where unsigned is 16 bits.
Why? If an arch is small enough that the natural integer size is 16-bit, then limiting the total number of packs to 65k sound entirely sensible.> The only reason why you'd want fixed (across platforms and architectures) type is when you want to make sure that a file storing the literal values taken from these fields are portable and everybody is limited the same way. If a platform's natural integer is 64-bit, by artificially limiting the size of this field to u32, you are making disservice to the platform users, and more importantly, you are wasting developers' time by forcing them to wonder if there is a reason behind the choice of u32 (does it really need to be able to store up to 4G, even on a smaller machines? Is it necessary to refuse to store more than 4G? What are the reasons?), like me wondering about these questions and writing them down here.
Quoted 3 lines
So, unless there is a reason why this must be of fixed type, I'd say just an unsigned would be the most reasonable choice.
I agree that it's best to avoid using a fixed type here unless there's a reason that it must be. Do you think that both 'nr' and 'pack_paths_checked' being about the number of packs is a strong enough reason to use u32 for 'pack_paths_checked'? If not, I will update 'pack_paths_checked' in the next path series to be an unsigned int.
Thanks,
William
Re: [PATCH v2 1/6] midx: add MIDX_PROGRESS flag <snip>
In reply to Junio C HamanoQuoted 26 lines · Junio C Hamano
Quoted 3 lines · SZEDER Gábor
Quoted 1 line
+#define
MIDX_PROGRESS(1 << 0)Please consider using an enum.
If they are used by assiging one of their values, definitely a good idea to use an enum. Are debuggers clever enough that they can tell, when they see something like this:
enum gress { PROGRESS = 1, REGRESS = 2, }; void func(enum gress v); ... void caller(void) { func(PROGRESS | REGRESS); func(PROGRESS + REGRESS); func(PROGRESS * 3); }
Quoted 2 lines
how caller came about to give 3?
My debugger was not smart enough to figure out what flags were combined to give the value of 3 in this example.
I saw that the code base is currently a mix of #define and enums when it comes to flags (e.g. dir_struct.flags and rebase_options.flags are both enums) and so using one here would not be something new stylistically.
Although my debugger might not be the smartest, I haven't noticed any downsides to switching this to an enum.
Thanks,
William
Re: [PATCH v2 1/6] midx: add MIDX_PROGRESS flag <snip>
Quoted 3 lines · William Baker
I saw that the code base is currently a mix of #define and enums when it comes to flags (e.g. dir_struct.flags and rebase_options.flags are both enums) and so using one here would not be something new stylistically.
Yes. But it is a different matter to spread a bad practice to parts of the code that haven't been contaminated by it.
Re: [PATCH v2 1/6] midx: add MIDX_PROGRESS flag <snip>
In reply to William BakerQuoted 2 lines · William Baker
Although my debugger might not be the smartest, I haven't noticed any downsides to switching this to an enum.
Well, if you write
enum { BIT_0 = 1, BIT_1 = 2, BIT_3 = 4 } var;it's pretty much a promise that the normal value for the var is one of these listed values to your readers. But bit flags are meant to be used combined (after all, they are cheaper alternative for 1-bit wide bitfields in a structure), so it is misleading to use enum as such.
Re: [PATCH v2 1/6] midx: add MIDX_PROGRESS flag <snip>
Quoted 11 lines · Junio C Hamano
Quoted 2 lines · William Baker
Although my debugger might not be the smartest, I haven't noticed any downsides to switching this to an enum.
Well, if you write
enum { BIT_0 = 1, BIT_1 = 2, BIT_3 = 4 } var;it's pretty much a promise that the normal value for the var is one of these listed values to your readers.
... that is why compilers give a warning when you write
switch (var) {
case ...:
...
}and do not have case arms for all the declared enum values without having the 'default' arm.
Re: [PATCH v2 1/6] midx: add MIDX_PROGRESS flag <snip>
On 10/2/19 12:04 AM, Junio C Hamano wrote>>
Quoted 9 lines
Quoted 6 lines
Well, if you write
enum { BIT_0 = 1, BIT_1 = 2, BIT_3 = 4 } var;it's pretty much a promise that the normal value for the var is one of these listed values to your readers.
... that is why compilers give a warning when you write
Quoted 8 lines
switch (var) { case ...: ... }and do not have case arms for all the declared enum values without having the 'default' arm.
Thanks for all of the details and feedback here. I can leave the flag as-is (and not switch to enum).
Thanks,
William
v3 cover multi-pack-index: add --no-progress
In reply to William Baker via GitGitGadgetHello Git contributors,
This is the third iteration of changes for adding support for --[no-]progress to multi-pack-index, and it includes the following updates for the feedback I received on v2:
* Fixed commit message formatting
* Updated 'pack_paths_checked' from u32 to unsigned
Thanks, William
William Baker (6):
midx: add MIDX_PROGRESS flag
midx: add progress to write_midx_file
midx: add progress to expire_midx_packs
midx: honor the MIDX_PROGRESS flag in verify_midx_file
midx: honor the MIDX_PROGRESS flag in midx_repack
multi-pack-index: add [--[no-]progress] option.
Documentation/git-multi-pack-index.txt | 6 ++- builtin/multi-pack-index.c | 18 +++++-- builtin/repack.c | 2 +- midx.c | 71 ++++++++++++++++++++------ midx.h | 10 ++-- t/t5319-multi-pack-index.sh | 69 +++++++++++++++++++++++++ 6 files changed, 149 insertions(+), 27 deletions(-)
base-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-337%2Fwilbaker%2Fmulti-pack-index-progress-toggle-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-337/wilbaker/multi-pack-index-progress-toggle-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/337
Range-diff vs v2:
1: 6badd9ceaf ! 1: cd041107de midx: add MIDX_PROGRESS flag Add the MIDX_PROGRESS flag and update the write|verify|expire|repack functions in midx.h to accept a flags parameter. The MIDX_PROGRESS flag indicates whether the caller of the function would like progress information to be displayed. This patch only changes the method prototypes and does not change the functionality. The functionality change will be handled by a later patch.
@@ -1,6 +1,7 @@
Author: William Baker <[email protected]>
midx: add MIDX_PROGRESS flag
+
Add the MIDX_PROGRESS flag and update the
write|verify|expire|repack functions in midx.h
to accept a flags parameter. The MIDX_PROGRESS
2: 3bc8677ea7 ! 2: 0117f55c9d midx: add progress to write_midx_file Add progress to write_midx_file. Progress is displayed when the MIDX_PROGRESS flag is set.
@@ -1,6 +1,7 @@
Author: William Baker <[email protected]>
midx: add progress to write_midx_file
+
Add progress to write_midx_file. Progress is displayed
when the MIDX_PROGRESS flag is set.
@@ -14,7 +15,7 @@
uint32_t alloc;
struct multi_pack_index *m;
+ struct progress *progress;
-+ uint32_t pack_paths_checked;
++ unsigned pack_paths_checked;
};
static void add_pack_to_midx(const char *full_path, size_t full_path_len, 3: 3374574001 ! 3: d741dc60bc midx: add progress to expire_midx_packs Add progress to expire_midx_packs. Progress is displayed when the MIDX_PROGRESS flag is set.
@@ -1,6 +1,7 @@
Author: William Baker <[email protected]>
midx: add progress to expire_midx_packs
+
Add progress to expire_midx_packs. Progress is displayed when the MIDX_PROGRESS flag is set.
4: 29d03771c0 ! 4: f208c09639 midx: honor the MIDX_PROGRESS flag in verify_midx_file Update verify_midx_file to only display progress when the MIDX_PROGRESS flag is set.
@@ -1,6 +1,7 @@
Author: William Baker <[email protected]>
midx: honor the MIDX_PROGRESS flag in verify_midx_file
+
Update verify_midx_file to only display progress when the MIDX_PROGRESS flag is set.
5: 57f6742f09 ! 5: 7566090769 midx: honor the MIDX_PROGRESS flag in midx_repack Update midx_repack to only display progress when the MIDX_PROGRESS flag is set.
@@ -1,6 +1,7 @@
Author: William Baker <[email protected]>
midx: honor the MIDX_PROGRESS flag in midx_repack
+
Update midx_repack to only display progress when the MIDX_PROGRESS flag is set.
6: 5b933ab744 ! 6: a3c50948d9 multi-pack-index: add [--[no-]progress] option. Add the --[no-]progress option to git multi-pack-index. Pass the MIDX_PROGRESS flag to the subcommand functions when progress should be displayed by multi-pack-index. The progress feature was added to 'verify' in 144d703 ("multi-pack-index: report progress during 'verify'", 2018-09-13) but some subcommands were not updated to display progress, and the ability to opt-out was overlooked.
@@ -1,6 +1,7 @@
Author: William Baker <[email protected]>
multi-pack-index: add [--[no-]progress] option.
+
Add the --[no-]progress option to git multi-pack-index. Pass the MIDX_PROGRESS flag to the subcommand functions when progress should be displayed by multi-pack-index.
Signature · 1 line
gitgitgadget
PATCH v3 1/6 midx: add MIDX_PROGRESS flag
From: William Baker <[email protected]>
Add the MIDX_PROGRESS flag and update the write|verify|expire|repack functions in midx.h to accept a flags parameter. The MIDX_PROGRESS flag indicates whether the caller of the function would like progress information to be displayed. This patch only changes the method prototypes and does not change the functionality. The functionality change will be handled by a later patch.
--- builtin/multi-pack-index.c | 8 ++++---- builtin/repack.c | 2 +- midx.c | 8 ++++---- midx.h | 10 ++++++---- 4 files changed, 15 insertions(+), 13 deletions(-)
builtin/multi-pack-index.c +4 -4
@@ -47,16 +47,16 @@ int cmd_multi_pack_index(int argc, const char **argv,trace2_cmd_mode (argv[0 ]);if (!strcmp (argv[0 ],"repack" )) -return midx_repack (the_repository, opts.object_dir, (size_t )opts.batch_size); +return midx_repack (the_repository, opts.object_dir, (size_t )opts.batch_size,0 );if (opts.batch_size)die (_ ("--batch-size option is only for 'repack' subcommand" ));if (!strcmp (argv[0 ],"write" )) -return write_midx_file (opts.object_dir); +return write_midx_file (opts.object_dir,0 );if (!strcmp (argv[0 ],"verify" )) -return verify_midx_file (the_repository, opts.object_dir); +return verify_midx_file (the_repository, opts.object_dir,0 );if (!strcmp (argv[0 ],"expire" )) -return expire_midx_packs (the_repository, opts.object_dir); +return expire_midx_packs (the_repository, opts.object_dir,0 );die (_ ("unrecognized subcommand: %s" ), argv[0 ]); }
builtin/repack.c +1 -1
@@ -561,7 +561,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)remove_temporary_files ();if (git_env_bool (GIT_TEST_MULTI_PACK_INDEX ,0 )) -write_midx_file (get_object_directory ()); +write_midx_file (get_object_directory (),0 );string_list_clear (&names,0 );string_list_clear (&rollback,0 );
midx.c +4 -4
@@ -1017,7 +1017,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *return result; } -int write_midx_file (const char *object_dir) +int write_midx_file (const char *object_dir,unsigned flags) {return write_midx_internal (object_dir,NULL ,NULL ); }
@@ -1077,7 +1077,7 @@ static int compare_pair_pos_vs_id(const void *_a, const void *_b)display_progress (progress, _n); \ }while (0 ) -int verify_midx_file(struct repository *r, const char *object_dir) +int verify_midx_file(struct repository *r, const char *object_dir, unsigned flags) { struct pair_pos_vs_id *pairs = NULL;uint32_t i;
@@ -1184,7 +1184,7 @@ int verify_midx_file(struct repository *r, const char *object_dir)return verify_midx_error; } -int expire_midx_packs (struct repository *r,const char *object_dir) +int expire_midx_packs (struct repository *r,const char *object_dir,unsigned flags) {uint32_t i, *count, result =0 ;struct string_list packs_to_drop =STRING_LIST_INIT_DUP ;
@@ -1316,7 +1316,7 @@ static int fill_included_packs_batch(struct repository *r,return 0 ; } -int midx_repack (struct repository *r,const char *object_dir,size_t batch_size) +int midx_repack (struct repository *r,const char *object_dir,size_t batch_size,unsigned flags) {int result =0 ;uint32_t i;
midx.h +6 -4
@@ -37,6 +37,8 @@ struct multi_pack_index {char object_dir[FLEX_ARRAY ]; }; +#define MIDX_PROGRESS (1 << 0) +struct multi_pack_index *load_multi_pack_index (const char *object_dir,int local);int prepare_midx_pack (struct repository *r,struct multi_pack_index *m,uint32_t pack_int_id);int bsearch_midx (const struct object_id *oid,struct multi_pack_index *m,uint32_t *result);
@@ -47,11 +49,11 @@ int fill_midx_entry(struct repository *r, const struct object_id *oid, struct paint midx_contains_pack (struct multi_pack_index *m,const char *idx_or_pack_name);int prepare_multi_pack_index_one (struct repository *r,const char *object_dir,int local); -int write_midx_file (const char *object_dir); +int write_midx_file (const char *object_dir,unsigned flags);void clear_midx_file (struct repository *r); -int verify_midx_file (struct repository *r,const char *object_dir); -int expire_midx_packs (struct repository *r,const char *object_dir); -int midx_repack (struct repository *r,const char *object_dir,size_t batch_size); +int verify_midx_file (struct repository *r,const char *object_dir,unsigned flags); +int expire_midx_packs (struct repository *r,const char *object_dir,unsigned flags); +int midx_repack (struct repository *r,const char *object_dir,size_t batch_size,unsigned flags);void close_midx (struct multi_pack_index *m);
Signature · 1 line
gitgitgadget
PATCH v3 3/6 midx: add progress to expire_midx_packs
In reply to William Baker via GitGitGadgetFrom: William Baker <[email protected]>
Add progress to expire_midx_packs. Progress is
displayed when the MIDX_PROGRESS flag is set.
--- midx.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
midx.c +12 -0
@@ -1206,18 +1206,29 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned flauint32_t i, *count, result =0 ;struct string_list packs_to_drop =STRING_LIST_INIT_DUP ;struct multi_pack_index *m =load_multi_pack_index (object_dir,1 ); +struct progress *progress =NULL ;if (!m)return 0 ; count =xcalloc (m->num_packs,sizeof (uint32_t )); + +if (flags &MIDX_PROGRESS ) + progress =start_progress (_ ("Counting referenced objects" ), + m->num_objects);for (i =0 ; i < m->num_objects; i++) {int pack_int_id =nth_midxed_pack_int_id (m, i); count[pack_int_id]++; +display_progress (progress, i +1 ); } +stop_progress (&progress); +if (flags &MIDX_PROGRESS ) + progress =start_progress (_ ("Finding and deleting unreferenced packfiles" ), + m->num_packs);for (i =0 ; i < m->num_packs; i++) {char *pack_name; +display_progress (progress, i +1 );if (count[i])continue ;
@@ -1235,6 +1246,7 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned flaunlink_pack_path (pack_name,0 );free (pack_name); } +stop_progress (&progress);free (count);
Signature · 1 line
gitgitgadget
PATCH v3 2/6 midx: add progress to write_midx_file
In reply to William Baker via GitGitGadgetFrom: William Baker <[email protected]>
Add progress to write_midx_file. Progress is displayed when the MIDX_PROGRESS flag is set.
--- midx.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-)
midx.c +21 -4
@@ -449,6 +449,8 @@ struct pack_list {uint32_t nr;uint32_t alloc;struct multi_pack_index *m; +struct progress *progress; +unsigned pack_paths_checked; }; static void add_pack_to_midx(const char *full_path, size_t full_path_len,
@@ -457,6 +459,7 @@ static void add_pack_to_midx(const char *full_path, size_t full_path_len,struct pack_list *packs = (struct pack_list *)data;if (ends_with (file_name,".idx" )) { +display_progress (packs->progress, ++packs->pack_paths_checked);if (packs->m &&midx_contains_pack (packs->m, file_name))return ;
@@ -786,7 +789,7 @@ static size_t write_midx_large_offsets(struct hashfile *f, uint32_t nr_large_off }static int write_midx_internal (const char *object_dir,struct multi_pack_index *m, -struct string_list *packs_to_drop) +struct string_list *packs_to_drop,unsigned flags) {unsigned char cur_chunk, num_chunks =0 ;char *midx_name;
@@ -800,6 +803,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *uint64_t chunk_offsets[MIDX_MAX_CHUNKS +1 ];uint32_t nr_entries, num_large_offsets =0 ;struct pack_midx_entry *entries =NULL ; +struct progress *progress =NULL ;int large_offsets_needed =0 ;int pack_name_concat_len =0 ;int dropped_packs =0 ;
@@ -833,8 +837,15 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index * packs.nr++; } } + + packs.pack_paths_checked =0 ; +if (flags &MIDX_PROGRESS ) + packs.progress =start_progress (_ ("Adding packfiles to multi-pack-index" ),0 ); +else + packs.progress =NULL ;for_each_file_in_pack_dir (object_dir, add_pack_to_midx, &packs); +stop_progress (&packs.progress);if (packs.m && packs.nr == packs.m->num_packs && !packs_to_drop) goto cleanup;
@@ -959,6 +970,9 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index * written +=MIDX_CHUNKLOOKUP_WIDTH ; } +if (flags &MIDX_PROGRESS ) + progress =start_progress (_ ("Writing chunks to multi-pack-index" ), + num_chunks);for (i =0 ; i < num_chunks; i++) {if (written != chunk_offsets[i]) BUG("incorrect chunk offset (%"PRIu64" != %"PRIu64") for chunk id %"PRIx32,
@@ -991,7 +1005,10 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *BUG ("trying to write unknown chunk id %" PRIx32, chunk_ids[i]); } + +display_progress (progress, i +1 ); } +stop_progress (&progress);if (written != chunk_offsets[num_chunks]) BUG("incorrect final offset %"PRIu64" != %"PRIu64,
@@ -1019,7 +1036,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *int write_midx_file (const char *object_dir,unsigned flags) { -return write_midx_internal (object_dir,NULL ,NULL ); +return write_midx_internal (object_dir,NULL ,NULL , flags); }void clear_midx_file (struct repository *r)
@@ -1222,7 +1239,7 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned flafree (count);if (packs_to_drop.nr) - result =write_midx_internal (object_dir, m, &packs_to_drop); + result =write_midx_internal (object_dir, m, &packs_to_drop, flags);string_list_clear (&packs_to_drop,0 );return result;
@@ -1371,7 +1388,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size, goto cleanup; } - result =write_midx_internal (object_dir, m,NULL ); + result =write_midx_internal (object_dir, m,NULL , flags); m =NULL ; cleanup:
Signature · 1 line
gitgitgadget
PATCH v3 5/6 midx: honor the MIDX_PROGRESS flag in midx_repack
In reply to William Baker via GitGitGadgetFrom: William Baker <[email protected]>
Update midx_repack to only display progress when
the MIDX_PROGRESS flag is set.
--- midx.c | 6 ++++++ 1 file changed, 6 insertions(+)
midx.c +6 -0
@@ -1374,6 +1374,12 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,strbuf_addstr (&base_name, object_dir);strbuf_addstr (&base_name,"/pack/pack" );argv_array_push (&cmd.args, base_name.buf); + +if (flags &MIDX_PROGRESS ) +argv_array_push (&cmd.args,"--progress" ); +else +argv_array_push (&cmd.args,"-q" ); +strbuf_release (&base_name); cmd.git_cmd =1 ;
Signature · 1 line
gitgitgadget
PATCH v3 4/6 midx: honor the MIDX_PROGRESS flag in verify_midx_file
In reply to William Baker via GitGitGadgetFrom: William Baker <[email protected]>
Update verify_midx_file to only display progress when the MIDX_PROGRESS flag is set.
--- midx.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
midx.c +12 -8
@@ -1098,15 +1098,16 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag {struct pair_pos_vs_id *pairs =NULL ;uint32_t i; -struct progress *progress; +struct progress *progress =NULL ;struct multi_pack_index *m =load_multi_pack_index (object_dir,1 ); verify_midx_error =0 ;if (!m)return 0 ; - progress =start_progress (_ ("Looking for referenced packfiles" ), - m->num_packs); +if (flags &MIDX_PROGRESS ) + progress =start_progress (_ ("Looking for referenced packfiles" ), + m->num_packs);for (i =0 ; i < m->num_packs; i++) {if (prepare_midx_pack (r, m, i))midx_report ("failed to load pack in position %d" , i);
@@ -1124,8 +1125,9 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag i, oid_fanout1, oid_fanout2, i + 1); } - progress =start_sparse_progress (_ ("Verifying OID order in MIDX" ), - m->num_objects -1 ); +if (flags &MIDX_PROGRESS ) + progress =start_sparse_progress (_ ("Verifying OID order in multi-pack-index" ), + m->num_objects -1 );for (i =0 ; i < m->num_objects -1 ; i++) {struct object_id oid1, oid2;
@@ -1152,13 +1154,15 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag pairs[i].pack_int_id =nth_midxed_pack_int_id (m, i); } - progress =start_sparse_progress (_ ("Sorting objects by packfile" ), - m->num_objects); +if (flags &MIDX_PROGRESS ) + progress =start_sparse_progress (_ ("Sorting objects by packfile" ), + m->num_objects);display_progress (progress,0 );/* TODO: Measure QSORT() progress */ QSORT (pairs, m->num_objects, compare_pair_pos_vs_id);stop_progress (&progress); - progress =start_sparse_progress (_ ("Verifying object offsets" ), m->num_objects); +if (flags &MIDX_PROGRESS ) + progress =start_sparse_progress (_ ("Verifying object offsets" ), m->num_objects);for (i =0 ; i < m->num_objects; i++) {struct object_id oid;struct pack_entry e;
Signature · 1 line
gitgitgadget
PATCH v3 6/6 multi-pack-index: add [--[no-]progress] option.
In reply to William Baker via GitGitGadgetFrom: William Baker <[email protected]>
Add the --[no-]progress option to git multi-pack-index. Pass the MIDX_PROGRESS flag to the subcommand functions when progress should be displayed by multi-pack-index. The progress feature was added to 'verify' in 144d703 ("multi-pack-index: report progress during 'verify'", 2018-09-13) but some subcommands were not updated to display progress, and the ability to opt-out was overlooked.
--- Documentation/git-multi-pack-index.txt | 6 ++- builtin/multi-pack-index.c | 18 +++++-- t/t5319-multi-pack-index.sh | 69 ++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 6 deletions(-)
Documentation/git-multi-pack-index.txt +5 -1
@@ -9,7 +9,7 @@ git-multi-pack-index - Write and verify multi-pack-indexes SYNOPSIS -------- [verse] -'git multi-pack-index' [--object-dir=<dir>] <subcommand> +'git multi-pack-index' [--object-dir=<dir>] [--[no-]progress] <subcommand> DESCRIPTION -----------
@@ -23,6 +23,10 @@ OPTIONS `<dir>/packs/multi-pack-index` for the current MIDX file, and `<dir>/packs` for the pack-files to index. +--[no-]progress:: + Turn progress on/off explicitly. If neither is specified, progress is + shown if standard error is connected to a terminal. + The following subcommands are available: write::
builtin/multi-pack-index.c +13 -5
@@ -6,21 +6,25 @@#include "trace2.h" static char const *const builtin_multi_pack_index_usage[] = { -N_ ("git multi-pack-index [--object-dir=<dir>] (write|verify|expire|repack --batch-size=<size>)" ), +N_ ("git multi-pack-index [<options>] (write|verify|expire|repack --batch-size=<size>)" ),NULL };static struct opts_multi_pack_index {const char *object_dir;unsigned long batch_size; +int progress; } opts;int cmd_multi_pack_index (int argc,const char **argv,const char *prefix) { +unsigned flags =0 ; +static struct option builtin_multi_pack_index_options[] = {OPT_FILENAME (0 ,"object-dir" , &opts.object_dir,N_ ("object directory containing set of packfile and pack-index pairs" )), +OPT_BOOL (0 ,"progress" , &opts.progress,N_ ("force progress reporting" )),OPT_MAGNITUDE (0 ,"batch-size" , &opts.batch_size,N_ ("during repack, collect pack-files of smaller size into a batch that is larger than this size" )),OPT_END (),
@@ -28,12 +32,15 @@ int cmd_multi_pack_index(int argc, const char **argv,git_config (git_default_config,NULL ); + opts.progress =isatty (2 ); argc =parse_options (argc, argv, prefix, builtin_multi_pack_index_options, builtin_multi_pack_index_usage,0 );if (!opts.object_dir) opts.object_dir =get_object_directory (); +if (opts.progress) + flags |=MIDX_PROGRESS ;if (argc ==0 ) usage_with_options(builtin_multi_pack_index_usage,
@@ -47,16 +54,17 @@ int cmd_multi_pack_index(int argc, const char **argv,trace2_cmd_mode (argv[0 ]);if (!strcmp (argv[0 ],"repack" )) -return midx_repack (the_repository, opts.object_dir, (size_t )opts.batch_size,0 ); +return midx_repack (the_repository, opts.object_dir, + (size_t )opts.batch_size, flags);if (opts.batch_size)die (_ ("--batch-size option is only for 'repack' subcommand" ));if (!strcmp (argv[0 ],"write" )) -return write_midx_file (opts.object_dir,0 ); +return write_midx_file (opts.object_dir, flags);if (!strcmp (argv[0 ],"verify" )) -return verify_midx_file (the_repository, opts.object_dir,0 ); +return verify_midx_file (the_repository, opts.object_dir, flags);if (!strcmp (argv[0 ],"expire" )) -return expire_midx_packs (the_repository, opts.object_dir,0 ); +return expire_midx_packs (the_repository, opts.object_dir, flags);die (_ ("unrecognized subcommand: %s" ), argv[0 ]); }
t/t5319-multi-pack-index.sh +69 -0
93 lines not on this page · raw message
Signature · 1 line
gitgitgadget
Quoted 8 lines · William Baker via GitGitGadget
This is the third iteration of changes for adding support for --[no-]progress to multi-pack-index, and it includes the following updates for the feedback I received on v2:
* Fixed commit message formatting
* Updated 'pack_paths_checked' from u32 to unsignedThanks, William
Thanks, will take a look.
Re: [PATCH v2 1/6] midx: add MIDX_PROGRESS flag <snip>
In reply to Junio C HamanoQuoted 7 lines · Junio C Hamano
Quoted 2 lines · William Baker
Although my debugger might not be the smartest, I haven't noticed any downsides to switching this to an enum.
Well, if you write
Quoted 7 lines
enum { BIT_0 = 1, BIT_1 = 2, BIT_3 = 4 } var;it's pretty much a promise that the normal value for the var is one of these listed values to your readers. But bit flags are meant to be used combined (after all, they are cheaper alternative for 1-bit wide bitfields in a structure), so it is misleading to use enum as such.
Having the combination of enum constants with power-of-two values is not misleading, but rather an idiom.
Back when debugging the racy split index issues I only saw gibberish like this:
(gdb) p ce->ce_flags
$2 = 469762048
With an enum I now have this instead:
(gdb) p ce->ce_flags
$2 = (CE_MATCHED | CE_UPDATE_IN_BASE | CE_STRIP_NAME)
The latter is about as many times more readable as the int value of that 'ce_flags'.
The sooner everyone gets on board with this the better.
Re: [PATCH v2 1/6] midx: add MIDX_PROGRESS flag <snip>
In reply to Junio C HamanoQuoted 35 lines · Junio C Hamano
Quoted 9 lines · SZEDER Gábor
Quoted 8 lines · William Baker via GitGitGadget
midx.h +0
-0@@ -37,6 +37,8 @@ struct multi_pack_index {char object_dir[FLEX_ARRAY ]; };Quoted 3 lines
midx.h struct multi_pack_index { · line 40
Quoted 1 line
midx.h +1
-0+#define MIDX_PROGRESS (1 << 0)Please consider using an enum.
If they are used by assiging one of their values, definitely a good idea to use an enum. Are debuggers clever enough that they can tell, when they see something like this:
enum gress { PROGRESS = 1, REGRESS = 2, }; void func(enum gress v); ... void caller(void) { func(PROGRESS | REGRESS); func(PROGRESS + REGRESS); func(PROGRESS * 3); }
Quoted 1 line
how caller came about to give 3?
No, they tend to show (PROGRESS | REGRESS), at least both gdb and lldb do. If the enum has only constants with power-of-two values, then that is the right way to write it, and the other two are asking for trouble (e.g. after someone changes the value of REGRESS from 2 to 8, 'PROGRESS * 3' will mean something different).
Re: [PATCH v2 1/6] midx: add MIDX_PROGRESS flag <snip>
Quoted 9 lines · SZEDER Gábor
Quoted 6 lines
func(PROGRESS | REGRESS); func(PROGRESS + REGRESS); func(PROGRESS * 3); }how caller came about to give 3?
No, they tend to show (PROGRESS | REGRESS), at least both gdb and lldb do.
OK.
Quoted 2 lines
If the enum has only constants with power-of-two values, then that
is the right way to write it, and the other two are asking for trouble
If the programmer and the debugger knows the constants are used to represent bits that can be or'ed together, what you say is correct, but that is entirely irrelevant.
What I was worried about is that the constants that are used to represent something that are *NOT* set of bits (hence "PROGRESS * 3" may be perfectly a reasonable thing for such an application) may be mistaken by an overly clever debugger and "3" may end up getting shown as "PROGRESS | REGRESS". When there are only two constants (PROGRESS=1 and REGRESS=2), we humans nor debuggers can tell if that is to represent two bits that can be or'ed together, or it is an enumeration.
Until we gain the third constant, that is, at which time the third one may likely be 3 (if enumeration) or 4 (if bits).
Re: [PATCH v2 1/6] midx: add MIDX_PROGRESS flag <snip>
Quoted 30 lines · Junio C Hamano
Quoted 9 lines · SZEDER Gábor
Quoted 6 lines
func(PROGRESS | REGRESS); func(PROGRESS + REGRESS); func(PROGRESS * 3); }how caller came about to give 3?
No, they tend to show (PROGRESS | REGRESS), at least both gdb and lldb do.
OK.
Quoted 2 lines
If the enum has only constants with power-of-two values, then that
is the right way to write it, and the other two are asking for troubleIf the programmer and the debugger knows the constants are used to represent bits that can be or'ed together, what you say is correct, but that is entirely irrelevant.
What I was worried about is that the constants that are used to represent something that are *NOT* set of bits (hence "PROGRESS * 3" may be perfectly a reasonable thing for such an application) may be mistaken by an overly clever debugger and "3" may end up getting shown as "PROGRESS | REGRESS". When there are only two constants (PROGRESS=1 and REGRESS=2), we humans nor debuggers can tell if that is to represent two bits that can be or'ed together, or it is an enumeration.
Quoted 3 lines
Until we gain the third constant, that is, at which time the third one may likely be 3 (if enumeration) or 4 (if bits).
I tried to see how lldb would handle the "PROGRESS * 3" scenario but I was unable to get lldb to display the "PROGRESS | REGRESS" format even when ORing the flags:
(lldb) l 399
399 enum test_flags {
400 TEST_FLAG_1 = 1 << 0,
401 TEST_FLAG_2 = 1 << 1,
402 };
403
404 enum test_flags flags_1 = TEST_FLAG_1;
405 enum test_flags flags_2 = TEST_FLAG_2;
406 enum test_flags flags_both = TEST_FLAG_1 | TEST_FLAG_2;
407
408 if (flags_1 || flags_2 || flags_both)
(lldb) p flags_1
(test_flags) $0 = TEST_FLAG_1
(lldb) p flags_2
(test_flags) $1 = TEST_FLAG_2
(lldb) p flags_both
(test_flags) $2 = 3
(lldb) fr v flags_both
(test_flags) flags_both = 3
(lldb) fr v --format enum flags_both
(test_flags) flags_both = 3
(lldb) version
lldb-902.0.79.7
Swift-4.1
Is there something that needs to be adjusted in the config or with --format to display "TEST_FLAG_1 | TEST_FLAG_2" in this example?
Thanks,
William
Re: [PATCH v2 1/6] midx: add MIDX_PROGRESS flag <snip>
Quoted 42 lines · William Baker
Quoted 11 lines · Junio C Hamano
Quoted 9 lines · SZEDER Gábor
Quoted 6 lines
func(PROGRESS | REGRESS); func(PROGRESS + REGRESS); func(PROGRESS * 3); }how caller came about to give 3?
No, they tend to show (PROGRESS | REGRESS), at least both gdb and lldb do.
I tried to see how lldb would handle the "PROGRESS * 3" scenario but I was unable to get lldb to display the "PROGRESS | REGRESS" format even when ORing the flags:
(lldb) l 399
399 enum test_flags { 400 TEST_FLAG_1 = 1 << 0, 401 TEST_FLAG_2 = 1 << 1, 402 }; 403 404 enum test_flags flags_1 = TEST_FLAG_1; 405 enum test_flags flags_2 = TEST_FLAG_2; 406 enum test_flags flags_both = TEST_FLAG_1 | TEST_FLAG_2; 407 408 if (flags_1 || flags_2 || flags_both)(lldb) p
flags_1
(test_flags) $0 =TEST_FLAG_1
(lldb) pflags_2
(test_flags) $1 =TEST_FLAG_2
(lldb) pflags_both
(test_flags) $2 = 3
(lldb) fr vflags_both
(test_flags)flags_both= 3
(lldb) fr v --format enumflags_both
(test_flags)flags_both= 3
(lldb) version
lldb-902.0.79.7
Swift-4.1
Quoted 2 lines
Is there something that needs to be adjusted in the config or with --format to display "
TEST_FLAG_1|TEST_FLAG_2" in this example?
I'm afraid you are right and lldb doesn't do this, at least not by default, what a pity. I tried different versions of gdb and lldb, distro-shipped older ones and some more recent in containers a couple of days ago; now I tried it again and see the same behavior as you, so I must have misremembered. Sorry for the confusion.
gdb still does the user-friendly thing, though.
Re: [PATCH v2 1/6] midx: add MIDX_PROGRESS flag <snip>
In reply to Junio C HamanoQuoted 11 lines · Junio C Hamano
Quoted 9 lines · SZEDER Gábor
Quoted 6 lines
func(PROGRESS | REGRESS); func(PROGRESS + REGRESS); func(PROGRESS * 3); }how caller came about to give 3?
No, they tend to show (PROGRESS | REGRESS), at least both gdb and lldb do.
I was wrong here, gdb does this, but lldb, unfortunately, doesn't; see my other reply in this thread.
Quoted 5 lines
OK.
Quoted 2 lines
If the enum has only constants with power-of-two values, then that
is the right way to write it, and the other two are asking for trouble
Quoted 7 lines
If the programmer and the debugger knows the constants are used to represent bits that can be or'ed together, what you say is correct, but that is entirely irrelevant.
What I was worried about is that the constants that are used to represent something that are *NOT* set of bits (hence "PROGRESS * 3" may be perfectly a reasonable thing for such an application)
I don't really see how that could be reasonable, it's prone to break when changing the values of the enum constants.
Quoted 9 lines
may be
mistaken by an overly clever debugger and "3" may end up getting shown as "PROGRESS | REGRESS". When there are only two constants (PROGRESS=1 and REGRESS=2), we humans nor debuggers can tell if that is to represent two bits that can be or'ed together, or it is an enumeration.Until we gain the third constant, that is, at which time the third one may likely be 3 (if enumeration) or 4 (if bits).
Humans benefit from context: they understand the name of the enum type (e.g. does it end with "_flags"?), the name of the enum constants, and the comment above the enum's definition (if any), and can then infer whether those constants represent OR-able bits or not. If they can't find this out, then that enum is poorly named and/or documented, which should be fixed. As for the patch that I originally commented on, I would expect the enum to be called e.g. 'midx_flags', and thus already with that single constant in there it'll be clear that it is intended as a collection of related OR-able bits.
As for the debugger, if it sees a variable of an enum type whose value doesn't match any of the enum constants, then there are basically three possibilities:
- All constants in that enum have power-of-two values. In this case it's reasonable from the debugger to assume that those constants are OR'ed together, and is extremely helpful to display the value that way.
- The constants are just a set of values (1, 2, 3, 42, etc). In this case the variable shouldn't have a value that doesn't match one of the constants in the first place, and I would first suspect that the program might be buggy.
- A "mostly" power-of-two enum might contain shorthand constants for
combinations of a set of other constants, e.g.:
enum flags {
BIT0 = (1 << 0),
BIT1 = (1 << 1),
BIT2 = (1 << 2),
FIRST_TWO = (BIT0 | BIT1),
};
enum flags f0 = BIT0;
enum flags f1 = BIT0 | BIT1;
enum flags f2 = BIT0 | BIT2;
enum flags f3 = BIT0 | BIT1 | BIT2;
In this case, sadly, gdb shows only matching constants:
(gdb) p f0
$1 = BIT0
(gdb) p f1
$2 = FIRST_TWO
(gdb) p f2
$3 = 5
(gdb) p f3
$4 = 7Re: [PATCH v2 1/6] midx: add MIDX_PROGRESS flag <snip>
Quoted 64 lines · SZEDER Gábor
Quoted 2 lines
Quoted 2 lines
No, they tend to show (PROGRESS | REGRESS), at least both gdb and lldb do.
I was wrong here, gdb does this, but lldb, unfortunately, doesn't; see my other reply in this thread.
Quoted 3 lines
What I was worried about is that the constants that are used to represent something that are *NOT* set of bits (hence "PROGRESS * 3" may be perfectly a reasonable thing for such an application)
I don't really see how that could be reasonable, it's prone to break when changing the values of the enum constants.
Quoted 9 lines
may be
mistaken by an overly clever debugger and "3" may end up getting shown as "PROGRESS | REGRESS". When there are only two constants (PROGRESS=1 and REGRESS=2), we humans nor debuggers can tell if that is to represent two bits that can be or'ed together, or it is an enumeration.Until we gain the third constant, that is, at which time the third one may likely be 3 (if enumeration) or 4 (if bits).
Humans benefit from context: they understand the name of the enum type (e.g. does it end with "
_flags"?), the name of the enum constants, and the comment above the enum's definition (if any), and can then infer whether those constants represent OR-able bits or not. If they can't find this out, then that enum is poorly named and/or documented, which should be fixed. As for the patch that I originally commented on, I would expect the enum to be called e.g. 'midx_flags', and thus already with that single constant in there it'll be clear that it is intended as a collection of related OR-able bits.As for the debugger, if it sees a variable of an enum type whose value doesn't match any of the enum constants, then there are basically three possibilities:
- All constants in that enum have power-of-two values. In this case it's reasonable from the debugger to assume that those constants are OR'ed together, and is extremely helpful to display the value that way.
- The constants are just a set of values (1, 2, 3, 42, etc). In this case the variable shouldn't have a value that doesn't match one of the constants in the first place, and I would first suspect that the program might be buggy.
- A "mostly" power-of-two enum might contain shorthand constants for
combinations of a set of other constants, e.g.: enum flags { BIT0 = (1 << 0), BIT1 = (1 << 1), BIT2 = (1 << 2), FIRST_TWO = (BIT0 | BIT1), }; enum flags f0 = BIT0; enum flags f1 = BIT0 | BIT1; enum flags f2 = BIT0 | BIT2; enum flags f3 = BIT0 | BIT1 | BIT2; In this case, sadly, gdb shows only matching constants:
Quoted 9 lines
(gdb) p f0
$1 = BIT0
(gdb) p f1
$2 = FIRST_TWO
(gdb) p f2
$3 = 5
(gdb) p f3
$4 = 7I believe this is the last open thread/discussion on the "midx: add MIDX_PROGRESS flag" patch series.
A quick summary of where the discussion stands:
- The patch series currently uses #define for the new MIDX_PROGRESS flag.
- The git codebase uses both #defines and enums for flags.
- The debugger always shows the enum value's name if there is a match, if values are OR-ed together there a few possibilities (see discussion above and earlier in the thread).
- There are concerns regarding misinterpreting constants that are not a set of bits (e.g. "PROGRESS * 3").
Are there any other details I can provide that would help in reaching a conclusion as to how to proceed?
At this time there are no other MIDX_* flags and so there's always the option to revisit the best way to handle multiple MIDX_* flags if/when additional flags are added.
Thanks,
William
Re: [PATCH v2 1/6] midx: add MIDX_PROGRESS flag <snip>
Quoted 3 lines · William Baker
At this time there are no other MIDX_* flags and so there's always the option to revisit the best way to handle multiple MIDX_* flags if/when additional flags are added.
I do not care too deeply either way, but if you wrote it in one way, how about completing the series without changing it in the middle, and leave the clean-ups to a follow-up series (if needed)?
Re: [PATCH v2 1/6] midx: add MIDX_PROGRESS flag <snip>
Quoted 7 lines · Junio C Hamano
Quoted 3 lines
At this time there are no other MIDX_* flags and so there's always the option to revisit the best way to handle multiple MIDX_* flags if/when additional flags are added.
I do not care too deeply either way, but if you wrote it in one way, how about completing the series without changing it in the middle, and leave the clean-ups to a follow-up series (if needed)?
That plan sounds good to me. The most recent series (v3) should be ready to go, I don't believe there is any outstanding feedback to address.
Thanks,
William
Re: [PATCH v2 1/6] midx: add MIDX_PROGRESS flag <snip>
On 10/16/19 12:48 PM, William Baker wrote:>> I do not care too deeply either way, but if you wrote it in one way,
Quoted 6 lines
Quoted 2 lines
how about completing the series without changing it in the middle, and leave the clean-ups to a follow-up series (if needed)?
That plan sounds good to me. The most recent series (v3) should be ready to go, I don't believe there is any outstanding feedback to address.
Follow-up question on this patch series.
I noticed there is a branch named 'wb/midx-progress' in
https://github.com/gitster/git but it does not appear to have the commits from this patch series and I have not seen it mentioned in "What's cooking in git.git" emails.
Is there anything else I should do to get these changes picked up in 'pu'?
Thanks again,
William
v4 cover multi-pack-index: add --no-progress
In reply to William Baker via GitGitGadgetHello Git contributors,
This is the fourth iteration of changes for adding support for --[no-]progress to multi-pack-index.
I'm resubmitting the series after a discussion regarding 'MIDX_PROGRESS' that concluded with a decision to stick with the original changes in the v3 patch.
Thanks, William
William Baker (6):
midx: add MIDX_PROGRESS flag
midx: add progress to write_midx_file
midx: add progress to expire_midx_packs
midx: honor the MIDX_PROGRESS flag in verify_midx_file
midx: honor the MIDX_PROGRESS flag in midx_repack
multi-pack-index: add [--[no-]progress] option.
Documentation/git-multi-pack-index.txt | 6 ++- builtin/multi-pack-index.c | 18 +++++-- builtin/repack.c | 2 +- midx.c | 71 ++++++++++++++++++++------ midx.h | 10 ++-- t/t5319-multi-pack-index.sh | 69 +++++++++++++++++++++++++ 6 files changed, 149 insertions(+), 27 deletions(-)
base-commit: d966095db01190a2196e31195ea6fa0c722aa732
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-337%2Fwilbaker%2Fmulti-pack-index-progress-toggle-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-337/wilbaker/multi-pack-index-progress-toggle-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/337
Range-diff vs v3:
1: cd041107de = 1: e7b1cc633b midx: add MIDX_PROGRESS flag 2: 0117f55c9d = 2: 37768f5886 midx: add progress to write_midx_file 3: d741dc60bc = 3: 4ac9527d98 midx: add progress to expire_midx_packs 4: f208c09639 = 4: 3468acf39a midx: honor the MIDX_PROGRESS flag in verify_midx_file 5: 7566090769 = 5: d6ae4e1c54 midx: honor the MIDX_PROGRESS flag in midx_repack 6: a3c50948d9 = 6: ea4f869780 multi-pack-index: add [--[no-]progress] option.
Signature · 1 line
gitgitgadget
PATCH v4 3/6 midx: add progress to expire_midx_packs
From: William Baker <[email protected]>
Add progress to expire_midx_packs. Progress is
displayed when the MIDX_PROGRESS flag is set.
--- midx.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
midx.c +12 -0
@@ -1205,18 +1205,29 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned flauint32_t i, *count, result =0 ;struct string_list packs_to_drop =STRING_LIST_INIT_DUP ;struct multi_pack_index *m =load_multi_pack_index (object_dir,1 ); +struct progress *progress =NULL ;if (!m)return 0 ; count =xcalloc (m->num_packs,sizeof (uint32_t )); + +if (flags &MIDX_PROGRESS ) + progress =start_progress (_ ("Counting referenced objects" ), + m->num_objects);for (i =0 ; i < m->num_objects; i++) {int pack_int_id =nth_midxed_pack_int_id (m, i); count[pack_int_id]++; +display_progress (progress, i +1 ); } +stop_progress (&progress); +if (flags &MIDX_PROGRESS ) + progress =start_progress (_ ("Finding and deleting unreferenced packfiles" ), + m->num_packs);for (i =0 ; i < m->num_packs; i++) {char *pack_name; +display_progress (progress, i +1 );if (count[i])continue ;
@@ -1234,6 +1245,7 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned flaunlink_pack_path (pack_name,0 );free (pack_name); } +stop_progress (&progress);free (count);
Signature · 1 line
gitgitgadget
PATCH v4 1/6 midx: add MIDX_PROGRESS flag
In reply to William Baker via GitGitGadgetFrom: William Baker <[email protected]>
Add the MIDX_PROGRESS flag and update the write|verify|expire|repack functions in midx.h to accept a flags parameter. The MIDX_PROGRESS flag indicates whether the caller of the function would like progress information to be displayed. This patch only changes the method prototypes and does not change the functionality. The functionality change will be handled by a later patch.
--- builtin/multi-pack-index.c | 8 ++++---- builtin/repack.c | 2 +- midx.c | 8 ++++---- midx.h | 10 ++++++---- 4 files changed, 15 insertions(+), 13 deletions(-)
builtin/multi-pack-index.c +4 -4
@@ -47,16 +47,16 @@ int cmd_multi_pack_index(int argc, const char **argv,trace2_cmd_mode (argv[0 ]);if (!strcmp (argv[0 ],"repack" )) -return midx_repack (the_repository, opts.object_dir, (size_t )opts.batch_size); +return midx_repack (the_repository, opts.object_dir, (size_t )opts.batch_size,0 );if (opts.batch_size)die (_ ("--batch-size option is only for 'repack' subcommand" ));if (!strcmp (argv[0 ],"write" )) -return write_midx_file (opts.object_dir); +return write_midx_file (opts.object_dir,0 );if (!strcmp (argv[0 ],"verify" )) -return verify_midx_file (the_repository, opts.object_dir); +return verify_midx_file (the_repository, opts.object_dir,0 );if (!strcmp (argv[0 ],"expire" )) -return expire_midx_packs (the_repository, opts.object_dir); +return expire_midx_packs (the_repository, opts.object_dir,0 );die (_ ("unrecognized subcommand: %s" ), argv[0 ]); }
builtin/repack.c +1 -1
@@ -562,7 +562,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)remove_temporary_files ();if (git_env_bool (GIT_TEST_MULTI_PACK_INDEX ,0 )) -write_midx_file (get_object_directory ()); +write_midx_file (get_object_directory (),0 );string_list_clear (&names,0 );string_list_clear (&rollback,0 );
midx.c +4 -4
@@ -1016,7 +1016,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *return result; } -int write_midx_file (const char *object_dir) +int write_midx_file (const char *object_dir,unsigned flags) {return write_midx_internal (object_dir,NULL ,NULL ); }
@@ -1076,7 +1076,7 @@ static int compare_pair_pos_vs_id(const void *_a, const void *_b)display_progress (progress, _n); \ }while (0 ) -int verify_midx_file(struct repository *r, const char *object_dir) +int verify_midx_file(struct repository *r, const char *object_dir, unsigned flags) { struct pair_pos_vs_id *pairs = NULL;uint32_t i;
@@ -1183,7 +1183,7 @@ int verify_midx_file(struct repository *r, const char *object_dir)return verify_midx_error; } -int expire_midx_packs (struct repository *r,const char *object_dir) +int expire_midx_packs (struct repository *r,const char *object_dir,unsigned flags) {uint32_t i, *count, result =0 ;struct string_list packs_to_drop =STRING_LIST_INIT_DUP ;
@@ -1315,7 +1315,7 @@ static int fill_included_packs_batch(struct repository *r,return 0 ; } -int midx_repack (struct repository *r,const char *object_dir,size_t batch_size) +int midx_repack (struct repository *r,const char *object_dir,size_t batch_size,unsigned flags) {int result =0 ;uint32_t i;
midx.h +6 -4
@@ -37,6 +37,8 @@ struct multi_pack_index {char object_dir[FLEX_ARRAY ]; }; +#define MIDX_PROGRESS (1 << 0) +struct multi_pack_index *load_multi_pack_index (const char *object_dir,int local);int prepare_midx_pack (struct repository *r,struct multi_pack_index *m,uint32_t pack_int_id);int bsearch_midx (const struct object_id *oid,struct multi_pack_index *m,uint32_t *result);
@@ -47,11 +49,11 @@ int fill_midx_entry(struct repository *r, const struct object_id *oid, struct paint midx_contains_pack (struct multi_pack_index *m,const char *idx_or_pack_name);int prepare_multi_pack_index_one (struct repository *r,const char *object_dir,int local); -int write_midx_file (const char *object_dir); +int write_midx_file (const char *object_dir,unsigned flags);void clear_midx_file (struct repository *r); -int verify_midx_file (struct repository *r,const char *object_dir); -int expire_midx_packs (struct repository *r,const char *object_dir); -int midx_repack (struct repository *r,const char *object_dir,size_t batch_size); +int verify_midx_file (struct repository *r,const char *object_dir,unsigned flags); +int expire_midx_packs (struct repository *r,const char *object_dir,unsigned flags); +int midx_repack (struct repository *r,const char *object_dir,size_t batch_size,unsigned flags);void close_midx (struct multi_pack_index *m);
Signature · 1 line
gitgitgadget
PATCH v4 5/6 midx: honor the MIDX_PROGRESS flag in midx_repack
In reply to William Baker via GitGitGadgetFrom: William Baker <[email protected]>
Update midx_repack to only display progress when
the MIDX_PROGRESS flag is set.
--- midx.c | 6 ++++++ 1 file changed, 6 insertions(+)
midx.c +6 -0
@@ -1373,6 +1373,12 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,strbuf_addstr (&base_name, object_dir);strbuf_addstr (&base_name,"/pack/pack" );argv_array_push (&cmd.args, base_name.buf); + +if (flags &MIDX_PROGRESS ) +argv_array_push (&cmd.args,"--progress" ); +else +argv_array_push (&cmd.args,"-q" ); +strbuf_release (&base_name); cmd.git_cmd =1 ;
Signature · 1 line
gitgitgadget
PATCH v4 4/6 midx: honor the MIDX_PROGRESS flag in verify_midx_file
In reply to William Baker via GitGitGadgetFrom: William Baker <[email protected]>
Update verify_midx_file to only display progress when the MIDX_PROGRESS flag is set.
--- midx.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
midx.c +12 -8
@@ -1097,15 +1097,16 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag {struct pair_pos_vs_id *pairs =NULL ;uint32_t i; -struct progress *progress; +struct progress *progress =NULL ;struct multi_pack_index *m =load_multi_pack_index (object_dir,1 ); verify_midx_error =0 ;if (!m)return 0 ; - progress =start_progress (_ ("Looking for referenced packfiles" ), - m->num_packs); +if (flags &MIDX_PROGRESS ) + progress =start_progress (_ ("Looking for referenced packfiles" ), + m->num_packs);for (i =0 ; i < m->num_packs; i++) {if (prepare_midx_pack (r, m, i))midx_report ("failed to load pack in position %d" , i);
@@ -1123,8 +1124,9 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag i, oid_fanout1, oid_fanout2, i + 1); } - progress =start_sparse_progress (_ ("Verifying OID order in MIDX" ), - m->num_objects -1 ); +if (flags &MIDX_PROGRESS ) + progress =start_sparse_progress (_ ("Verifying OID order in multi-pack-index" ), + m->num_objects -1 );for (i =0 ; i < m->num_objects -1 ; i++) {struct object_id oid1, oid2;
@@ -1151,13 +1153,15 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag pairs[i].pack_int_id =nth_midxed_pack_int_id (m, i); } - progress =start_sparse_progress (_ ("Sorting objects by packfile" ), - m->num_objects); +if (flags &MIDX_PROGRESS ) + progress =start_sparse_progress (_ ("Sorting objects by packfile" ), + m->num_objects);display_progress (progress,0 );/* TODO: Measure QSORT() progress */ QSORT (pairs, m->num_objects, compare_pair_pos_vs_id);stop_progress (&progress); - progress =start_sparse_progress (_ ("Verifying object offsets" ), m->num_objects); +if (flags &MIDX_PROGRESS ) + progress =start_sparse_progress (_ ("Verifying object offsets" ), m->num_objects);for (i =0 ; i < m->num_objects; i++) {struct object_id oid;struct pack_entry e;
Signature · 1 line
gitgitgadget
PATCH v4 6/6 multi-pack-index: add [--[no-]progress] option.
In reply to William Baker via GitGitGadgetFrom: William Baker <[email protected]>
Add the --[no-]progress option to git multi-pack-index. Pass the MIDX_PROGRESS flag to the subcommand functions when progress should be displayed by multi-pack-index. The progress feature was added to 'verify' in 144d703 ("multi-pack-index: report progress during 'verify'", 2018-09-13) but some subcommands were not updated to display progress, and the ability to opt-out was overlooked.
--- Documentation/git-multi-pack-index.txt | 6 ++- builtin/multi-pack-index.c | 18 +++++-- t/t5319-multi-pack-index.sh | 69 ++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 6 deletions(-)
Documentation/git-multi-pack-index.txt +5 -1
@@ -9,7 +9,7 @@ git-multi-pack-index - Write and verify multi-pack-indexes SYNOPSIS -------- [verse] -'git multi-pack-index' [--object-dir=<dir>] <subcommand> +'git multi-pack-index' [--object-dir=<dir>] [--[no-]progress] <subcommand> DESCRIPTION -----------
@@ -23,6 +23,10 @@ OPTIONS `<dir>/packs/multi-pack-index` for the current MIDX file, and `<dir>/packs` for the pack-files to index. +--[no-]progress:: + Turn progress on/off explicitly. If neither is specified, progress is + shown if standard error is connected to a terminal. + The following subcommands are available: write::
builtin/multi-pack-index.c +13 -5
@@ -6,21 +6,25 @@#include "trace2.h" static char const *const builtin_multi_pack_index_usage[] = { -N_ ("git multi-pack-index [--object-dir=<dir>] (write|verify|expire|repack --batch-size=<size>)" ), +N_ ("git multi-pack-index [<options>] (write|verify|expire|repack --batch-size=<size>)" ),NULL };static struct opts_multi_pack_index {const char *object_dir;unsigned long batch_size; +int progress; } opts;int cmd_multi_pack_index (int argc,const char **argv,const char *prefix) { +unsigned flags =0 ; +static struct option builtin_multi_pack_index_options[] = {OPT_FILENAME (0 ,"object-dir" , &opts.object_dir,N_ ("object directory containing set of packfile and pack-index pairs" )), +OPT_BOOL (0 ,"progress" , &opts.progress,N_ ("force progress reporting" )),OPT_MAGNITUDE (0 ,"batch-size" , &opts.batch_size,N_ ("during repack, collect pack-files of smaller size into a batch that is larger than this size" )),OPT_END (),
@@ -28,12 +32,15 @@ int cmd_multi_pack_index(int argc, const char **argv,git_config (git_default_config,NULL ); + opts.progress =isatty (2 ); argc =parse_options (argc, argv, prefix, builtin_multi_pack_index_options, builtin_multi_pack_index_usage,0 );if (!opts.object_dir) opts.object_dir =get_object_directory (); +if (opts.progress) + flags |=MIDX_PROGRESS ;if (argc ==0 ) usage_with_options(builtin_multi_pack_index_usage,
@@ -47,16 +54,17 @@ int cmd_multi_pack_index(int argc, const char **argv,trace2_cmd_mode (argv[0 ]);if (!strcmp (argv[0 ],"repack" )) -return midx_repack (the_repository, opts.object_dir, (size_t )opts.batch_size,0 ); +return midx_repack (the_repository, opts.object_dir, + (size_t )opts.batch_size, flags);if (opts.batch_size)die (_ ("--batch-size option is only for 'repack' subcommand" ));if (!strcmp (argv[0 ],"write" )) -return write_midx_file (opts.object_dir,0 ); +return write_midx_file (opts.object_dir, flags);if (!strcmp (argv[0 ],"verify" )) -return verify_midx_file (the_repository, opts.object_dir,0 ); +return verify_midx_file (the_repository, opts.object_dir, flags);if (!strcmp (argv[0 ],"expire" )) -return expire_midx_packs (the_repository, opts.object_dir,0 ); +return expire_midx_packs (the_repository, opts.object_dir, flags);die (_ ("unrecognized subcommand: %s" ), argv[0 ]); }
t/t5319-multi-pack-index.sh +69 -0
93 lines not on this page · raw message
Signature · 1 line
gitgitgadget
PATCH v4 2/6 midx: add progress to write_midx_file
In reply to William Baker via GitGitGadgetFrom: William Baker <[email protected]>
Add progress to write_midx_file. Progress is displayed when the MIDX_PROGRESS flag is set.
--- midx.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-)
midx.c +21 -4
@@ -448,6 +448,8 @@ struct pack_list {uint32_t nr;uint32_t alloc;struct multi_pack_index *m; +struct progress *progress; +unsigned pack_paths_checked; }; static void add_pack_to_midx(const char *full_path, size_t full_path_len,
@@ -456,6 +458,7 @@ static void add_pack_to_midx(const char *full_path, size_t full_path_len,struct pack_list *packs = (struct pack_list *)data;if (ends_with (file_name,".idx" )) { +display_progress (packs->progress, ++packs->pack_paths_checked);if (packs->m &&midx_contains_pack (packs->m, file_name))return ;
@@ -785,7 +788,7 @@ static size_t write_midx_large_offsets(struct hashfile *f, uint32_t nr_large_off }static int write_midx_internal (const char *object_dir,struct multi_pack_index *m, -struct string_list *packs_to_drop) +struct string_list *packs_to_drop,unsigned flags) {unsigned char cur_chunk, num_chunks =0 ;char *midx_name;
@@ -799,6 +802,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *uint64_t chunk_offsets[MIDX_MAX_CHUNKS +1 ];uint32_t nr_entries, num_large_offsets =0 ;struct pack_midx_entry *entries =NULL ; +struct progress *progress =NULL ;int large_offsets_needed =0 ;int pack_name_concat_len =0 ;int dropped_packs =0 ;
@@ -832,8 +836,15 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index * packs.nr++; } } + + packs.pack_paths_checked =0 ; +if (flags &MIDX_PROGRESS ) + packs.progress =start_progress (_ ("Adding packfiles to multi-pack-index" ),0 ); +else + packs.progress =NULL ;for_each_file_in_pack_dir (object_dir, add_pack_to_midx, &packs); +stop_progress (&packs.progress);if (packs.m && packs.nr == packs.m->num_packs && !packs_to_drop) goto cleanup;
@@ -958,6 +969,9 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index * written +=MIDX_CHUNKLOOKUP_WIDTH ; } +if (flags &MIDX_PROGRESS ) + progress =start_progress (_ ("Writing chunks to multi-pack-index" ), + num_chunks);for (i =0 ; i < num_chunks; i++) {if (written != chunk_offsets[i]) BUG("incorrect chunk offset (%"PRIu64" != %"PRIu64") for chunk id %"PRIx32,
@@ -990,7 +1004,10 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *BUG ("trying to write unknown chunk id %" PRIx32, chunk_ids[i]); } + +display_progress (progress, i +1 ); } +stop_progress (&progress);if (written != chunk_offsets[num_chunks]) BUG("incorrect final offset %"PRIu64" != %"PRIu64,
@@ -1018,7 +1035,7 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *int write_midx_file (const char *object_dir,unsigned flags) { -return write_midx_internal (object_dir,NULL ,NULL ); +return write_midx_internal (object_dir,NULL ,NULL , flags); }void clear_midx_file (struct repository *r)
@@ -1221,7 +1238,7 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned flafree (count);if (packs_to_drop.nr) - result =write_midx_internal (object_dir, m, &packs_to_drop); + result =write_midx_internal (object_dir, m, &packs_to_drop, flags);string_list_clear (&packs_to_drop,0 );return result;
@@ -1370,7 +1387,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size, goto cleanup; } - result =write_midx_internal (object_dir, m,NULL ); + result =write_midx_internal (object_dir, m,NULL , flags); m =NULL ; cleanup:
Signature · 1 line
gitgitgadget