git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Taylor Blau <me@ttaylorr.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Johannes.Schindelin@gmx.de,
	sandals@crustytoothpaste.net, steadmon@google.com,
	jrnieder@gmail.com, peff@peff.net, congdanhqx@gmail.com,
	phillip.wood123@gmail.com, emilyshaffer@google.com,
	sluongng@gmail.com, jonathantanmy@google.com,
	Derrick Stolee <derrickstolee@github.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH v2 03/18] maintenance: replace run_auto_gc()
Date: Fri, 24 Jul 2020 21:33:17 -0400	[thread overview]
Message-ID: <20200725013317.GB35171@syl.lan> (raw)
In-Reply-To: <xmqqblk6q8ws.fsf@gitster.c.googlers.com>

On Thu, Jul 23, 2020 at 01:21:55PM -0700, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +--[no-]maintenance::
> >  --[no-]auto-gc::
> > -	Run `git gc --auto` at the end to perform garbage collection
> > -	if needed. This is enabled by default.
> > +	Run `git maintenance run --auto` at the end to perform garbage
> > +	collection if needed. This is enabled by default.
>
> Shouldn't the new synonym be called --auto-maintenance or an
> abbreviation thereof?  It is not like we will run the full
> maintenance suite when "--no-maintenance" is omitted, which
> certainly is not the impression we want to give our readers.
>
> >  These objects may be removed by normal Git operations (such as `git commit`)
> > -which automatically call `git gc --auto`. (See linkgit:git-gc[1].)
> > -If these objects are removed and were referenced by the cloned repository,
> > -then the cloned repository will become corrupt.
> > +which automatically call `git maintenance run --auto` and `git gc --auto`.
>
> Hmph.  Perhaps the picture may change in the end of the series but I
> got an impression that "gc --auto" would eventually become just part
> of "maintenance --auto" and the users won't have to be even aware of
> its existence?  Wouldn't we rather want to say something like
>
> 	--[no-]auto-maintenance::
> 	--[no-]auto-gc::
>                 Run `git maintenance run --auto` at the end to perform
>                 garbage collection if needed (`--[no-]auto-gc` is a
>                 synonym).  This is enabled by default.
>
> > diff --git a/builtin/fetch.c b/builtin/fetch.c
> > index 82ac4be8a5..49a4d727d4 100644
> > --- a/builtin/fetch.c
> > +++ b/builtin/fetch.c
> > @@ -196,8 +196,10 @@ static struct option builtin_fetch_options[] = {
> >  	OPT_STRING_LIST(0, "negotiation-tip", &negotiation_tip, N_("revision"),
> >  			N_("report that we have only objects reachable from this object")),
> >  	OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),
>
> > +	OPT_BOOL(0, "maintenance", &enable_auto_gc,
> > +		 N_("run 'maintenance --auto' after fetching")),
> >  	OPT_BOOL(0, "auto-gc", &enable_auto_gc,
> > +		 N_("run 'maintenance --auto' after fetching")),
>
> OK, so this is truly a backward-compatible synonym at this point.

I wouldn't be opposed to making the 'auto-gc' option an
'OPT_HIDDEN_BOOL', but I realize that users may not want to move as
quickly as that. Perhaps we should visit this in a couple of releases
(or perhaps you are getting to it in a later patch that I haven't read
yet).

> > diff --git a/run-command.c b/run-command.c
> > index 9b3a57d1e3..82ad241638 100644
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -1865,14 +1865,17 @@ int run_processes_parallel_tr2(int n, get_next_task_fn get_next_task,
> >  	return result;
> >  }
> >
> > -int run_auto_gc(int quiet)
> > +int run_auto_maintenance(int quiet)
> >  {
> >  	struct argv_array argv_gc_auto = ARGV_ARRAY_INIT;
> >  	int status;
> >
> > -	argv_array_pushl(&argv_gc_auto, "gc", "--auto", NULL);
> > +	argv_array_pushl(&argv_gc_auto, "maintenance", "run", "--auto", NULL);
> >  	if (quiet)
> >  		argv_array_push(&argv_gc_auto, "--quiet");
> > +	else
> > +		argv_array_push(&argv_gc_auto, "--no-quiet");
> > +
> >  	status = run_command_v_opt(argv_gc_auto.argv, RUN_GIT_CMD);
> >  	argv_array_clear(&argv_gc_auto);
> >  	return status;
>
> Don't we want to replace all _gc_ with _maintenance_ in this
> function?  I think the first business before we can do so would be
> to rethink if spelling out "maintenance" fully in code is a good
> idea in the first space.  It would make names for variables,
> structures and fields unnecessarily long without contributing to
> ease of understanding an iota, and a easy-to-remember short-form or
> an abbreviation may be needed.  Using a short-form/abbreviation
> wouldn't worsen the end-user experience, and not the developer
> experience for that matter.
>
> If we choose "gc" as the short-hand, most of the change in this step
> would become unnecessary.  I also do not mind if we some other words
> or word-fragment (perhaps "maint"???) is chosen.

Yeah, writing out 'maintenance' every time in the code and in
command-line arguments is kind of a mouthful. I'm more willing to accept
that '--maintenance' is something that users would write or script
around, but 'maint' makes sense to me as a shorthand in the code.

I could go either way on calling the command-line flag '--maint',
though.

>
> > diff --git a/run-command.h b/run-command.h
> > index 191dfcdafe..d9a800e700 100644
> > --- a/run-command.h
> > +++ b/run-command.h
> > @@ -221,7 +221,7 @@ int run_hook_ve(const char *const *env, const char *name, va_list args);
> >  /*
> >   * Trigger an auto-gc
> >   */
> > -int run_auto_gc(int quiet);
> > +int run_auto_maintenance(int quiet);
> >
> >  #define RUN_COMMAND_NO_STDIN 1
> >  #define RUN_GIT_CMD	     2	/*If this is to be git sub-command */
> > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> > index a66dbe0bde..9850ecde5d 100755
> > --- a/t/t5510-fetch.sh
> > +++ b/t/t5510-fetch.sh
> > @@ -919,7 +919,7 @@ test_expect_success 'fetching with auto-gc does not lock up' '
> >  		git config fetch.unpackLimit 1 &&
> >  		git config gc.autoPackLimit 1 &&
> >  		git config gc.autoDetach false &&
> > -		GIT_ASK_YESNO="$D/askyesno" git fetch >fetch.out 2>&1 &&
> > +		GIT_ASK_YESNO="$D/askyesno" git fetch --verbose >fetch.out 2>&1 &&
> >  		test_i18ngrep "Auto packing the repository" fetch.out &&
> >  		! grep "Should I try again" fetch.out
> >  	)

Thanks,
Taylor

  reply	other threads:[~2020-07-25  1:33 UTC|newest]

Thread overview: 164+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-07 14:21 [PATCH 00/21] Maintenance builtin, allowing 'gc --auto' customization Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 01/21] gc: use the_repository less often Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 02/21] gc: use repository in too_many_loose_objects() Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 03/21] gc: use repo config Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 04/21] gc: drop the_repository in log location Derrick Stolee via GitGitGadget
2020-07-09  2:22   ` Jonathan Tan
2020-07-09 11:13     ` Derrick Stolee
2020-07-07 14:21 ` [PATCH 05/21] maintenance: create basic maintenance runner Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 06/21] maintenance: add --quiet option Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 07/21] maintenance: replace run_auto_gc() Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 08/21] maintenance: initialize task array and hashmap Derrick Stolee via GitGitGadget
2020-07-09  2:25   ` Jonathan Tan
2020-07-09 13:15     ` Derrick Stolee
2020-07-09 13:51       ` Junio C Hamano
2020-07-07 14:21 ` [PATCH 09/21] maintenance: add commit-graph task Derrick Stolee via GitGitGadget
2020-07-09  2:29   ` Jonathan Tan
2020-07-09 11:14     ` Derrick Stolee
2020-07-09 22:52       ` Jeff King
2020-07-09 23:41         ` Derrick Stolee
2020-07-07 14:21 ` [PATCH 10/21] maintenance: add --task option Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 11/21] maintenance: take a lock on the objects directory Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 12/21] maintenance: add fetch task Derrick Stolee via GitGitGadget
2020-07-09  2:35   ` Jonathan Tan
2020-07-07 14:21 ` [PATCH 13/21] maintenance: add loose-objects task Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 14/21] maintenance: add pack-files task Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 15/21] maintenance: auto-size pack-files batch Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 16/21] maintenance: create maintenance.<task>.enabled config Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 17/21] maintenance: use pointers to check --auto Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 18/21] maintenance: add auto condition for commit-graph task Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 19/21] maintenance: create auto condition for loose-objects Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 20/21] maintenance: add pack-files auto condition Derrick Stolee via GitGitGadget
2020-07-07 14:21 ` [PATCH 21/21] midx: use start_delayed_progress() Derrick Stolee via GitGitGadget
2020-07-08 23:57 ` [PATCH 00/21] Maintenance builtin, allowing 'gc --auto' customization Emily Shaffer
2020-07-09 11:21   ` Derrick Stolee
2020-07-09 12:43     ` Derrick Stolee
2020-07-09 23:16       ` Jeff King
2020-07-09 23:45         ` Derrick Stolee
2020-07-10 18:46           ` Emily Shaffer
2020-07-10 19:30             ` Son Luong Ngoc
2020-07-09 14:05     ` Junio C Hamano
2020-07-09 15:54       ` Derrick Stolee
2020-07-09 16:26         ` Junio C Hamano
2020-07-09 16:56           ` Derrick Stolee
2020-07-23 17:56 ` [PATCH v2 00/18] " Derrick Stolee via GitGitGadget
2020-07-23 17:56   ` [PATCH v2 01/18] maintenance: create basic maintenance runner Derrick Stolee via GitGitGadget
2020-07-25  1:26     ` Taylor Blau
2020-07-25  1:47     ` Đoàn Trần Công Danh
2020-07-29 22:19     ` Jonathan Nieder
2020-07-30 13:12       ` Derrick Stolee
2020-07-31  0:30         ` Jonathan Nieder
2020-08-03 17:37           ` Derrick Stolee
2020-08-03 17:46             ` Jonathan Nieder
2020-08-03 22:46               ` Taylor Blau
2020-08-03 23:01                 ` Jonathan Nieder
2020-08-03 23:08                   ` Taylor Blau
2020-08-03 23:17                     ` Jonathan Nieder
2020-08-04  0:07                       ` Junio C Hamano
2020-08-04 13:32                       ` Derrick Stolee
2020-08-04 14:42                         ` Jonathan Nieder
2020-08-04 16:32                           ` Derrick Stolee
2020-08-04 17:02                             ` Jonathan Nieder
2020-08-04 17:51                               ` Derrick Stolee
2020-08-05 15:02               ` Derrick Stolee
2020-07-31 16:40         ` Jonathan Nieder
2020-08-03 23:52         ` Jonathan Nieder
2020-07-23 17:56   ` [PATCH v2 02/18] maintenance: add --quiet option Derrick Stolee via GitGitGadget
2020-07-23 17:56   ` [PATCH v2 03/18] maintenance: replace run_auto_gc() Derrick Stolee via GitGitGadget
2020-07-23 20:21     ` Junio C Hamano
2020-07-25  1:33       ` Taylor Blau [this message]
2020-07-30 13:29       ` Derrick Stolee
2020-07-30 13:31         ` Derrick Stolee
2020-07-30 19:00           ` Eric Sunshine
2020-07-30 20:21             ` Derrick Stolee
2020-07-23 17:56   ` [PATCH v2 04/18] maintenance: initialize task array Derrick Stolee via GitGitGadget
2020-07-23 19:57     ` Junio C Hamano
2020-07-24 12:23       ` Derrick Stolee
2020-07-24 12:51         ` Derrick Stolee
2020-07-24 19:39           ` Junio C Hamano
2020-07-25  1:46           ` Taylor Blau
2020-07-29 22:19     ` Emily Shaffer
2020-07-23 17:56   ` [PATCH v2 05/18] maintenance: add commit-graph task Derrick Stolee via GitGitGadget
2020-07-23 20:22     ` Junio C Hamano
2020-07-24 13:09       ` Derrick Stolee
2020-07-24 19:47         ` Junio C Hamano
2020-07-25  1:52           ` Taylor Blau
2020-07-30 13:59             ` Derrick Stolee
2020-07-29  0:22     ` Jeff King
2020-07-23 17:56   ` [PATCH v2 06/18] maintenance: add --task option Derrick Stolee via GitGitGadget
2020-07-23 20:21     ` Junio C Hamano
2020-07-23 22:18       ` Junio C Hamano
2020-07-24 13:36       ` Derrick Stolee
2020-07-24 19:50         ` Junio C Hamano
2020-07-23 17:56   ` [PATCH v2 07/18] maintenance: take a lock on the objects directory Derrick Stolee via GitGitGadget
2020-07-23 17:56   ` [PATCH v2 08/18] maintenance: add prefetch task Derrick Stolee via GitGitGadget
2020-07-23 20:53     ` Junio C Hamano
2020-07-24 14:25       ` Derrick Stolee
2020-07-24 20:47         ` Junio C Hamano
2020-07-25  1:37     ` Đoàn Trần Công Danh
2020-07-25  1:48       ` Junio C Hamano
2020-07-27 14:07         ` Derrick Stolee
2020-07-27 16:13           ` Junio C Hamano
2020-07-27 18:27             ` Derrick Stolee
2020-07-28 16:37               ` [PATCH v2] fetch: optionally allow disabling FETCH_HEAD update Junio C Hamano
2020-07-29  9:12                 ` Phillip Wood
2020-07-29  9:17                   ` Phillip Wood
2020-07-30 15:17                 ` Derrick Stolee
2020-07-23 17:56   ` [PATCH v2 09/18] maintenance: add loose-objects task Derrick Stolee via GitGitGadget
2020-07-23 20:59     ` Junio C Hamano
2020-07-24 14:50       ` Derrick Stolee
2020-07-24 19:57         ` Junio C Hamano
2020-07-29 22:21     ` Emily Shaffer
2020-07-30 15:38       ` Derrick Stolee
2020-07-23 17:56   ` [PATCH v2 10/18] maintenance: add incremental-repack task Derrick Stolee via GitGitGadget
2020-07-23 22:00     ` Junio C Hamano
2020-07-24 15:03       ` Derrick Stolee
2020-07-29 22:22     ` Emily Shaffer
2020-07-23 17:56   ` [PATCH v2 11/18] maintenance: auto-size incremental-repack batch Derrick Stolee via GitGitGadget
2020-07-23 22:15     ` Junio C Hamano
2020-07-23 23:09       ` Eric Sunshine
2020-07-23 23:24         ` Junio C Hamano
2020-07-24 16:09           ` Derrick Stolee
2020-07-24 19:51       ` Derrick Stolee
2020-07-24 20:17         ` Junio C Hamano
2020-07-29 22:23     ` Emily Shaffer
2020-07-30 16:57       ` Derrick Stolee
2020-07-30 19:02         ` Derrick Stolee
2020-07-30 19:24           ` Chris Torek
2020-08-05 12:37           ` Đoàn Trần Công Danh
2020-08-06 13:54             ` Derrick Stolee
2020-07-23 17:56   ` [PATCH v2 12/18] maintenance: create maintenance.<task>.enabled config Derrick Stolee via GitGitGadget
2020-07-23 17:56   ` [PATCH v2 13/18] maintenance: use pointers to check --auto Derrick Stolee via GitGitGadget
2020-07-23 17:56   ` [PATCH v2 14/18] maintenance: add auto condition for commit-graph task Derrick Stolee via GitGitGadget
2020-07-23 17:56   ` [PATCH v2 15/18] maintenance: create auto condition for loose-objects Derrick Stolee via GitGitGadget
2020-07-23 17:56   ` [PATCH v2 16/18] maintenance: add incremental-repack auto condition Derrick Stolee via GitGitGadget
2020-07-23 17:56   ` [PATCH v2 17/18] midx: use start_delayed_progress() Derrick Stolee via GitGitGadget
2020-07-23 17:56   ` [PATCH v2 18/18] maintenance: add trace2 regions for task execution Derrick Stolee via GitGitGadget
2020-07-29 22:03   ` [PATCH v2 00/18] Maintenance builtin, allowing 'gc --auto' customization Emily Shaffer
2020-07-30 22:24   ` [PATCH v3 00/20] " Derrick Stolee via GitGitGadget
2020-07-30 22:24     ` [PATCH v3 01/20] maintenance: create basic maintenance runner Derrick Stolee via GitGitGadget
2020-07-30 22:24     ` [PATCH v3 02/20] maintenance: add --quiet option Derrick Stolee via GitGitGadget
2020-07-30 22:24     ` [PATCH v3 03/20] maintenance: replace run_auto_gc() Derrick Stolee via GitGitGadget
2020-07-30 22:24     ` [PATCH v3 04/20] maintenance: initialize task array Derrick Stolee via GitGitGadget
2020-07-30 22:24     ` [PATCH v3 05/20] maintenance: add commit-graph task Derrick Stolee via GitGitGadget
2020-07-30 22:24     ` [PATCH v3 06/20] maintenance: add --task option Derrick Stolee via GitGitGadget
2020-07-30 22:24     ` [PATCH v3 07/20] maintenance: take a lock on the objects directory Derrick Stolee via GitGitGadget
2020-07-30 22:24     ` [PATCH v3 08/20] fetch: optionally allow disabling FETCH_HEAD update Junio C Hamano via GitGitGadget
2020-07-30 22:24     ` [PATCH v3 09/20] maintenance: add prefetch task Derrick Stolee via GitGitGadget
2020-07-30 22:24     ` [PATCH v3 10/20] maintenance: add loose-objects task Derrick Stolee via GitGitGadget
2020-07-30 22:24     ` [PATCH v3 11/20] midx: enable core.multiPackIndex by default Derrick Stolee via GitGitGadget
2020-07-30 22:24     ` [PATCH v3 12/20] maintenance: add incremental-repack task Derrick Stolee via GitGitGadget
2020-07-30 22:24     ` [PATCH v3 13/20] maintenance: auto-size incremental-repack batch Derrick Stolee via GitGitGadget
2020-07-30 23:36       ` Chris Torek
2020-08-03 17:43         ` Derrick Stolee
2020-07-30 22:24     ` [PATCH v3 14/20] maintenance: create maintenance.<task>.enabled config Derrick Stolee via GitGitGadget
2020-07-30 22:24     ` [PATCH v3 15/20] maintenance: use pointers to check --auto Derrick Stolee via GitGitGadget
2020-07-30 22:24     ` [PATCH v3 16/20] maintenance: add auto condition for commit-graph task Derrick Stolee via GitGitGadget
2020-07-30 22:24     ` [PATCH v3 17/20] maintenance: create auto condition for loose-objects Derrick Stolee via GitGitGadget
2020-07-30 22:24     ` [PATCH v3 18/20] maintenance: add incremental-repack auto condition Derrick Stolee via GitGitGadget
2020-07-30 22:24     ` [PATCH v3 19/20] midx: use start_delayed_progress() Derrick Stolee via GitGitGadget
2020-07-30 22:24     ` [PATCH v3 20/20] maintenance: add trace2 regions for task execution Derrick Stolee via GitGitGadget
2020-07-30 23:06     ` [PATCH v3 00/20] Maintenance builtin, allowing 'gc --auto' customization Junio C Hamano
2020-07-30 23:31     ` Junio C Hamano
2020-07-31  2:58       ` Junio C Hamano
2020-08-06 17:58     ` Derrick Stolee

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=20200725013317.GB35171@syl.lan \
    --to=me@ttaylorr.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=congdanhqx@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=dstolee@microsoft.com \
    --cc=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=jonathantanmy@google.com \
    --cc=jrnieder@gmail.com \
    --cc=peff@peff.net \
    --cc=phillip.wood123@gmail.com \
    --cc=sandals@crustytoothpaste.net \
    --cc=sluongng@gmail.com \
    --cc=steadmon@google.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).