git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: William Baker <williamtbakeremail@gmail.com>
To: Junio C Hamano <gitster@pobox.com>,
	William Baker via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, stolee@gmail.com, jeffhost@microsoft.com,
	William Baker <William.Baker@microsoft.com>
Subject: 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.
Date: Fri, 13 Sep 2019 11:45:29 -0700	[thread overview]
Message-ID: <24c1a9aa-c83b-a984-8821-ecc51a4bc0e2@gmail.com> (raw)
In-Reply-To: <xmqqlfutfewv.fsf@gitster-ct.c.googlers.com>

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

  reply	other threads:[~2019-09-13 18:45 UTC|newest]

Thread overview: 58+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
2019-09-13 20:26       ` Junio C Hamano
2019-09-16 19:49         ` William Baker
2019-09-11 20:30 ` [PATCH 0/1] multi-pack-index: add --no-progress Derrick Stolee
2019-09-20 16:53 ` [PATCH v2 0/6] " William Baker via GitGitGadget
2019-09-20 16:53   ` [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 William Baker via GitGitGadget
2019-09-20 20:01     ` Junio C Hamano
2019-09-20 20:16       ` Junio C Hamano
2019-09-21 12:11     ` [PATCH v2 1/6] midx: add MIDX_PROGRESS flag <snip> SZEDER Gábor
2019-09-23 21:55       ` William Baker
2019-09-28  3:40       ` Junio C Hamano
2019-09-30 17:01         ` William Baker
2019-10-02  5:38           ` Junio C Hamano
2019-10-02  5:43           ` Junio C Hamano
2019-10-02  7:04             ` Junio C Hamano
2019-10-02 15:56               ` William Baker
2019-10-07 17:12             ` SZEDER Gábor
2019-10-07 17:29         ` SZEDER Gábor
2019-10-08  4:30           ` Junio C Hamano
2019-10-08 16:24             ` William Baker
2019-10-09  0:16               ` SZEDER Gábor
2019-10-09  1:32             ` SZEDER Gábor
2019-10-15 20:00               ` William Baker
2019-10-16  2:09                 ` Junio C Hamano
2019-10-16 19:48                   ` William Baker
2019-10-18 21:35                     ` William Baker
2019-09-20 16:53   ` [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 William Baker via GitGitGadget
2019-09-20 20:10     ` Junio C Hamano
2019-09-23 21:12       ` William Baker
2019-09-28  3:49         ` Junio C Hamano
2019-09-30 16:36           ` William Baker
2019-09-20 16:53   ` [PATCH v2 3/6] midx: add progress to expire_midx_packs Add progress to expire_midx_packs. " William Baker via GitGitGadget
2019-09-20 16:53   ` [PATCH v2 5/6] midx: honor the MIDX_PROGRESS flag in midx_repack Update midx_repack to only display progress " William Baker via GitGitGadget
2019-09-20 20:12     ` Junio C Hamano
2019-09-20 16:53   ` [PATCH v2 4/6] midx: honor the MIDX_PROGRESS flag in verify_midx_file Update verify_midx_file " William Baker via GitGitGadget
2019-09-20 16:53   ` [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 William Baker via GitGitGadget
2019-09-20 19:54   ` [PATCH v2 0/6] multi-pack-index: add --no-progress Junio C Hamano
2019-09-20 20:33     ` William Baker
2019-09-20 20:23   ` Philip Oakley
2019-09-20 20:39     ` William Baker
2019-10-03 17:53   ` [PATCH v3 " William Baker via GitGitGadget
2019-10-03 17:53     ` [PATCH v3 1/6] midx: add MIDX_PROGRESS flag William Baker via GitGitGadget
2019-10-03 17:53     ` [PATCH v3 2/6] midx: add progress to write_midx_file William Baker via GitGitGadget
2019-10-03 17:53     ` [PATCH v3 3/6] midx: add progress to expire_midx_packs William Baker via GitGitGadget
2019-10-03 17:53     ` [PATCH v3 4/6] midx: honor the MIDX_PROGRESS flag in verify_midx_file William Baker via GitGitGadget
2019-10-03 17:53     ` [PATCH v3 5/6] midx: honor the MIDX_PROGRESS flag in midx_repack William Baker via GitGitGadget
2019-10-03 17:53     ` [PATCH v3 6/6] multi-pack-index: add [--[no-]progress] option William Baker via GitGitGadget
2019-10-03 22:46     ` [PATCH v3 0/6] multi-pack-index: add --no-progress Junio C Hamano
2019-10-21 18:39     ` [PATCH v4 " William Baker via GitGitGadget
2019-10-21 18:39       ` [PATCH v4 1/6] midx: add MIDX_PROGRESS flag William Baker via GitGitGadget
2019-10-21 18:39       ` [PATCH v4 2/6] midx: add progress to write_midx_file William Baker via GitGitGadget
2019-10-21 18:40       ` [PATCH v4 3/6] midx: add progress to expire_midx_packs William Baker via GitGitGadget
2019-10-21 18:40       ` [PATCH v4 4/6] midx: honor the MIDX_PROGRESS flag in verify_midx_file William Baker via GitGitGadget
2019-10-21 18:40       ` [PATCH v4 5/6] midx: honor the MIDX_PROGRESS flag in midx_repack William Baker via GitGitGadget
2019-10-21 18:40       ` [PATCH v4 6/6] multi-pack-index: add [--[no-]progress] option William Baker via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=24c1a9aa-c83b-a984-8821-ecc51a4bc0e2@gmail.com \
    --to=williamtbakeremail@gmail.com \
    --cc=William.Baker@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=jeffhost@microsoft.com \
    --cc=stolee@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).