| Message ID | abcc7fb7312b349562fe6d13e2250496e107c9ed.1613422804.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | midx: split out sub-commands | expand |
On Mon, Feb 15 2021, Taylor Blau wrote: > @@ -31,15 +30,14 @@ int cmd_multi_pack_index(int argc, const char **argv, > > git_config(git_default_config, NULL); > > - opts.progress = isatty(2); > + if (isatty(2)) > + opts.flags |= MIDX_PROGRESS; > 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) > - opts.flags |= MIDX_PROGRESS; Funnily enough we could also just do: opts.flags = isatty(2); Since there's a grand total of one flag it knows about, and MIDX_PROGRESS is defined as 1. Not the problem of this series really, just a nit: In efbc3aee08d (midx: add MIDX_PROGRESS flag, 2019-10-21) we added this flag, and around the same time the similar commit-graph code got refactored to have an enum of flags in 5af80394521 (commit-graph: collapse parameters into flags, 2019-06-12). I prefer the commit-graph way of having a clean boundary between the two a bit more, and then just setting a flag based on an OPT_BOOL...
On Mon, Feb 15, 2021 at 10:39:16PM +0100, Ævar Arnfjörð Bjarmason wrote: > > On Mon, Feb 15 2021, Taylor Blau wrote: > > > @@ -31,15 +30,14 @@ int cmd_multi_pack_index(int argc, const char **argv, > > > > git_config(git_default_config, NULL); > > > > - opts.progress = isatty(2); > > + if (isatty(2)) > > + opts.flags |= MIDX_PROGRESS; > > 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) > > - opts.flags |= MIDX_PROGRESS; > > > Funnily enough we could also just do: > > opts.flags = isatty(2); > > Since there's a grand total of one flag it knows about, and > MIDX_PROGRESS is defined as 1. :-). I have a handful of branches that add some new flags (including the original series I sent down-thread), so I'm not sure that I'm in favor of this (admittedly cute) hack. > Not the problem of this series really, just a nit: In efbc3aee08d (midx: > add MIDX_PROGRESS flag, 2019-10-21) we added this flag, and around the > same time the similar commit-graph code got refactored to have an enum > of flags in 5af80394521 (commit-graph: collapse parameters into flags, > 2019-06-12). Hmm. I don't really have a strong opinion either way. I'd like to avoid steering too far away from my original goal of multi-pack reverse indexes, at least for now... > I prefer the commit-graph way of having a clean boundary between the two > a bit more, and then just setting a flag based on an OPT_BOOL... Me too. But if you can part ways with it, it cuts down on the code duplication (since callers in the sub-commands don't have to set that bit on the flags themselves). OTOH, we could keep half of this change and store the flags in the options structure in addition to the progress field, then set the appropriate bit in "flags" in cmd_builtin_multi_pack_index(). But I think at that point you're already sharing the flags field everywhere, so you're just as well off to have something like what's written in this patch here. I don't have strong feelings either way. Thanks, Taylor
On 2/15/2021 4:45 PM, Taylor Blau wrote: > On Mon, Feb 15, 2021 at 10:39:16PM +0100, Ævar Arnfjörð Bjarmason wrote: >> Funnily enough we could also just do: >> >> opts.flags = isatty(2); >> >> Since there's a grand total of one flag it knows about, and >> MIDX_PROGRESS is defined as 1. > > :-). I have a handful of branches that add some new flags (including the > original series I sent down-thread), so I'm not sure that I'm in favor > of this (admittedly cute) hack. It's also _wrong_ if the user passes in '--progress' but redirects stderr to a file. I don't know why someone would want to do that, but they could, and we honor that throughout all commands. Thanks, -Stolee
diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c index 4a0ddb06c4..c70f020d8f 100644 --- a/builtin/multi-pack-index.c +++ b/builtin/multi-pack-index.c @@ -13,7 +13,6 @@ static char const * const builtin_multi_pack_index_usage[] = { static struct opts_multi_pack_index { const char *object_dir; unsigned long batch_size; - int progress; unsigned flags; } opts; @@ -23,7 +22,7 @@ int cmd_multi_pack_index(int argc, const char **argv, 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_BIT(0, "progress", &opts.flags, N_("force progress reporting"), MIDX_PROGRESS), 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(), @@ -31,15 +30,14 @@ int cmd_multi_pack_index(int argc, const char **argv, git_config(git_default_config, NULL); - opts.progress = isatty(2); + if (isatty(2)) + opts.flags |= MIDX_PROGRESS; 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) - opts.flags |= MIDX_PROGRESS; if (argc == 0) usage_with_options(builtin_multi_pack_index_usage,
Now that there is a shared 'flags' member in the options structure, there is no need to keep track of whether to force progress or not, since ultimately the decision of whether or not to show a progress meter is controlled by a bit in the flags member. Manipulate that bit directly, and drop the now-unnecessary 'progress' field while we're at it. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- builtin/multi-pack-index.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)