git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
* [PATCH 0/1] multi-pack-index: add --no-progress
@ 2019-09-11 15:37 William Baker via GitGitGadget
  2019-09-11 15:37 ` [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 William Baker via GitGitGadget
  2019-09-11 20:30 ` [PATCH 0/1] multi-pack-index: add --no-progress Derrick Stolee
  0 siblings, 2 replies; 6+ messages in thread
From: William Baker via GitGitGadget @ 2019-09-11 15:37 UTC (permalink / raw)
  To: git; +Cc: williamtbakeremail, stolee, jeffhost, Junio C Hamano

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/pull.315.git.gitgitgadget@gmail.com/

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
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [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.
  2019-09-11 15:37 [PATCH 0/1] multi-pack-index: add --no-progress William Baker via GitGitGadget
@ 2019-09-11 15:37 ` William Baker via GitGitGadget
  2019-09-12 20:17   ` Junio C Hamano
  2019-09-11 20:30 ` [PATCH 0/1] multi-pack-index: add --no-progress Derrick Stolee
  1 sibling, 1 reply; 6+ messages in thread
From: William Baker via GitGitGadget @ 2019-09-11 15:37 UTC (permalink / raw)
  To: git; +Cc: williamtbakeremail, stolee, jeffhost, Junio C Hamano, William Baker

From: William Baker <William.Baker@microsoft.com>

Signed-off-by: William Baker <William.Baker@microsoft.com>
---
 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(-)

diff --git a/Documentation/git-multi-pack-index.txt b/Documentation/git-multi-pack-index.txt
index 233b2b7862..19a5a42dc0 100644
--- a/Documentation/git-multi-pack-index.txt
+++ b/Documentation/git-multi-pack-index.txt
@@ -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::
diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
index b1ea1a6aa1..f8b2a74179 100644
--- a/builtin/multi-pack-index.c
+++ b/builtin/multi-pack-index.c
@@ -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);
 
diff --git a/midx.c b/midx.c
index d649644420..b1a9ad9e5b 100644
--- a/midx.c
+++ b/midx.c
@@ -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;
diff --git a/midx.h b/midx.h
index f0ae656b5d..88abc85bc3 100644
--- a/midx.h
+++ b/midx.h
@@ -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, i
 
 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
index c72ca04399..cdc09b85f1 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -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 &&
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 0/1] multi-pack-index: add --no-progress
  2019-09-11 15:37 [PATCH 0/1] multi-pack-index: add --no-progress William Baker via GitGitGadget
  2019-09-11 15:37 ` [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 William Baker via GitGitGadget
@ 2019-09-11 20:30 ` Derrick Stolee
  1 sibling, 0 replies; 6+ messages in thread
From: Derrick Stolee @ 2019-09-11 20:30 UTC (permalink / raw)
  To: William Baker via GitGitGadget, git
  Cc: williamtbakeremail, jeffhost, Junio C Hamano

On 9/11/2019 11:37 AM, William Baker via GitGitGadget wrote:
> 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 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* 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.
  2019-09-11 15:37 ` [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 William Baker via GitGitGadget
@ 2019-09-12 20:17   ` Junio C Hamano
  2019-09-13 18:45     ` William Baker
  0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2019-09-12 20:17 UTC (permalink / raw)
  To: William Baker via GitGitGadget
  Cc: git, williamtbakeremail, stolee, jeffhost, William Baker

"William Baker via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  [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.

> +--[no-]progress::
> +	Turn progress on/off explicitly. If neither is specified, progress is 
> +	shown if standard error is connected to a terminal.

Sounds sensible.

> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
> index b1ea1a6aa1..f8b2a74179 100644
> --- a/builtin/multi-pack-index.c
> +++ b/builtin/multi-pack-index.c
> @@ -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.

>  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(),
>  	};

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.

> @@ -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);

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".

^ permalink raw reply	[flat|nested] 6+ messages in thread

* 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.
  2019-09-12 20:17   ` Junio C Hamano
@ 2019-09-13 18:45     ` William Baker
  2019-09-13 20:26       ` Junio C Hamano
  0 siblings, 1 reply; 6+ messages in thread
From: William Baker @ 2019-09-13 18:45 UTC (permalink / raw)
  To: Junio C Hamano, William Baker via GitGitGadget
  Cc: git, stolee, jeffhost, William Baker

Hi Junio,

Thanks for the review!

On 9/12/19 1:17 PM, Junio C Hamano wrote:

>> +'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. 

> 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
> of parse_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)?


>> @@ -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);
> 

> 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")

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. 

> 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".
> 

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* 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.
  2019-09-13 18:45     ` William Baker
@ 2019-09-13 20:26       ` Junio C Hamano
  0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2019-09-13 20:26 UTC (permalink / raw)
  To: William Baker
  Cc: William Baker via GitGitGadget, git, stolee, jeffhost, William Baker

William Baker <williamtbakeremail@gmail.com> writes:

>> 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
>> of parse_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)?

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...

>> 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. 
>
>> 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".
>> 
>
> 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
;-)


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-11 15:37 [PATCH 0/1] multi-pack-index: add --no-progress William Baker via GitGitGadget
2019-09-11 15:37 ` [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 William Baker via GitGitGadget
2019-09-12 20:17   ` Junio C Hamano
2019-09-13 18:45     ` William Baker
2019-09-13 20:26       ` Junio C Hamano
2019-09-11 20:30 ` [PATCH 0/1] multi-pack-index: add --no-progress Derrick Stolee

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox